From f16f41c5b3cf2345abc7bd4d5e951a43479ce4a7 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Mon, 25 Sep 2023 17:56:03 -0400 Subject: [PATCH 1/8] fs: improve `readFileSync` with file descriptors PR-URL: https://github.com/nodejs/node/pull/49691 Reviewed-By: Stephen Belanger --- benchmark/fs/readFileSync.js | 18 +++++++++++--- lib/fs.js | 6 ++--- lib/internal/fs/sync.js | 6 +++-- src/node_file.cc | 47 ++++++++++++++++++++++-------------- 4 files changed, 50 insertions(+), 27 deletions(-) diff --git a/benchmark/fs/readFileSync.js b/benchmark/fs/readFileSync.js index b81bdce8f27f69..800ab31450f43a 100644 --- a/benchmark/fs/readFileSync.js +++ b/benchmark/fs/readFileSync.js @@ -6,12 +6,21 @@ const fs = require('fs'); const bench = common.createBenchmark(main, { encoding: ['undefined', 'utf8'], path: ['existing', 'non-existing'], - n: [60e1], + hasFileDescriptor: ['true', 'false'], + n: [1e4], }); -function main({ n, encoding, path }) { +function main({ n, encoding, path, hasFileDescriptor }) { const enc = encoding === 'undefined' ? undefined : encoding; - const file = path === 'existing' ? __filename : '/tmp/not-found'; + let file; + let shouldClose = false; + + if (hasFileDescriptor === 'true') { + shouldClose = path === 'existing'; + file = path === 'existing' ? fs.openSync(__filename) : -1; + } else { + file = path === 'existing' ? __filename : '/tmp/not-found'; + } bench.start(); for (let i = 0; i < n; ++i) { try { @@ -21,4 +30,7 @@ function main({ n, encoding, path }) { } } bench.end(n); + if (shouldClose) { + fs.closeSync(file); + } } diff --git a/lib/fs.js b/lib/fs.js index 015e424efbdf4c..9b6b61dc8efd42 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -437,13 +437,11 @@ function tryReadSync(fd, isUserFd, buffer, pos, len) { function readFileSync(path, options) { options = getOptions(options, { flag: 'r' }); - const isUserFd = isFd(path); // File descriptor ownership - - // TODO(@anonrig): Do not handle file descriptor ownership for now. - if (!isUserFd && (options.encoding === 'utf8' || options.encoding === 'utf-8')) { + if (options.encoding === 'utf8' || options.encoding === 'utf-8') { return syncFs.readFileUtf8(path, options.flag); } + const isUserFd = isFd(path); // File descriptor ownership const fd = isUserFd ? path : fs.openSync(path, options.flag, 0o666); const stats = tryStatSync(fd, isUserFd); diff --git a/lib/internal/fs/sync.js b/lib/internal/fs/sync.js index 7fd356833b11a2..0d4ba90150e186 100644 --- a/lib/internal/fs/sync.js +++ b/lib/internal/fs/sync.js @@ -9,7 +9,7 @@ const { getStatFsFromBinding, getValidatedFd, } = require('internal/fs/utils'); -const { parseFileMode } = require('internal/validators'); +const { parseFileMode, isInt32 } = require('internal/validators'); const binding = internalBinding('fs'); @@ -19,7 +19,9 @@ const binding = internalBinding('fs'); * @return {string} */ function readFileUtf8(path, flag) { - path = pathModule.toNamespacedPath(getValidatedPath(path)); + if (!isInt32(path)) { + path = pathModule.toNamespacedPath(getValidatedPath(path)); + } return binding.readFileUtf8(path, stringToFlags(flag)); } diff --git a/src/node_file.cc b/src/node_file.cc index a19c4fa5bb7893..b76eb385295836 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -2155,7 +2155,7 @@ static void OpenSync(const FunctionCallbackInfo& args) { uv_fs_t req; auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); }); FS_SYNC_TRACE_BEGIN(open); - int err = uv_fs_open(nullptr, &req, *path, flags, mode, nullptr); + auto err = uv_fs_open(nullptr, &req, *path, flags, mode, nullptr); FS_SYNC_TRACE_END(open); if (err < 0) { return env->ThrowUVException(err, "open", nullptr, path.out()); @@ -2546,30 +2546,41 @@ static void ReadFileUtf8(const FunctionCallbackInfo& args) { CHECK_GE(args.Length(), 2); - BufferValue path(env->isolate(), args[0]); - CHECK_NOT_NULL(*path); - CHECK(args[1]->IsInt32()); const int flags = args[1].As()->Value(); - if (CheckOpenPermissions(env, path, flags).IsNothing()) return; - + uv_file file; uv_fs_t req; - auto defer_req_cleanup = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); }); - FS_SYNC_TRACE_BEGIN(open); - uv_file file = uv_fs_open(nullptr, &req, *path, flags, 438, nullptr); - FS_SYNC_TRACE_END(open); - if (req.result < 0) { - // req will be cleaned up by scope leave. - return env->ThrowUVException(req.result, "open", nullptr, path.out()); + bool is_fd = args[0]->IsInt32(); + + // Check for file descriptor + if (is_fd) { + file = args[0].As()->Value(); + } else { + BufferValue path(env->isolate(), args[0]); + CHECK_NOT_NULL(*path); + if (CheckOpenPermissions(env, path, flags).IsNothing()) return; + + FS_SYNC_TRACE_BEGIN(open); + file = uv_fs_open(nullptr, &req, *path, flags, O_RDONLY, nullptr); + FS_SYNC_TRACE_END(open); + if (req.result < 0) { + uv_fs_req_cleanup(&req); + // req will be cleaned up by scope leave. + return env->ThrowUVException(req.result, "open", nullptr, path.out()); + } } - auto defer_close = OnScopeLeave([file]() { - uv_fs_t close_req; - CHECK_EQ(0, uv_fs_close(nullptr, &close_req, file, nullptr)); - uv_fs_req_cleanup(&close_req); + auto defer_close = OnScopeLeave([file, is_fd, &req]() { + if (!is_fd) { + FS_SYNC_TRACE_BEGIN(close); + CHECK_EQ(0, uv_fs_close(nullptr, &req, file, nullptr)); + FS_SYNC_TRACE_END(close); + } + uv_fs_req_cleanup(&req); }); + std::string result{}; char buffer[8192]; uv_buf_t buf = uv_buf_init(buffer, sizeof(buffer)); @@ -2580,7 +2591,7 @@ static void ReadFileUtf8(const FunctionCallbackInfo& args) { if (req.result < 0) { FS_SYNC_TRACE_END(read); // req will be cleaned up by scope leave. - return env->ThrowUVException(req.result, "read", nullptr, path.out()); + return env->ThrowUVException(req.result, "read", nullptr); } if (r <= 0) { break; From 31e727db7e0f57bd71d35ce8c1cf378bab309b0f Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Tue, 26 Sep 2023 15:56:54 +0300 Subject: [PATCH 2/8] tools: support updating @reporters/github manually MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/49871 Refs: https://github.com/nodejs/node/commit/fbe28e2f1da0a66a78164201048c0c60615bd0c8 Reviewed-By: Marco Ippolito Reviewed-By: Michaël Zasso Reviewed-By: Chemi Atlow --- .github/workflows/tools.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/tools.yml b/.github/workflows/tools.yml index 880586e2879cbe..187352cc542561 100644 --- a/.github/workflows/tools.yml +++ b/.github/workflows/tools.yml @@ -23,6 +23,7 @@ on: - corepack - doc - eslint + - github_reporter - googletest - histogram - icu From c829c03df2451212d1d6d28e867579e5d5e4a06e Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 26 Sep 2023 11:40:17 -0400 Subject: [PATCH 3/8] url: improve invalid url performance PR-URL: https://github.com/nodejs/node/pull/49692 Reviewed-By: Matteo Collina Reviewed-By: Robert Nagy Reviewed-By: Benjamin Gruenbaum --- benchmark/url/whatwg-url-validity.js | 23 +++++++++++ lib/internal/errors.js | 7 +++- lib/internal/url.js | 8 +--- src/env_properties.h | 1 + src/node_url.cc | 38 +++++++++++++++++-- src/node_url.h | 3 ++ test/parallel/test-url-null-char.js | 2 +- .../test-whatwg-url-custom-parsing.js | 2 +- 8 files changed, 70 insertions(+), 14 deletions(-) create mode 100644 benchmark/url/whatwg-url-validity.js diff --git a/benchmark/url/whatwg-url-validity.js b/benchmark/url/whatwg-url-validity.js new file mode 100644 index 00000000000000..6ba22336408fa1 --- /dev/null +++ b/benchmark/url/whatwg-url-validity.js @@ -0,0 +1,23 @@ +'use strict'; +const common = require('../common.js'); +const url = require('url'); +const URL = url.URL; + +const bench = common.createBenchmark(main, { + type: ['valid', 'invalid'], + e: [1e5], +}); + +// This benchmark is used to compare the `Invalid URL` path of the URL parser +function main({ type, e }) { + const url = type === 'valid' ? 'https://www.nodejs.org' : 'www.nodejs.org'; + bench.start(); + for (let i = 0; i < e; i++) { + try { + new URL(url); + } catch { + // do nothing + } + } + bench.end(e); +} diff --git a/lib/internal/errors.js b/lib/internal/errors.js index a8c2a9ea15db04..4e332e1ce18d16 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1370,8 +1370,13 @@ E('ERR_INVALID_SYNC_FORK_INPUT', E('ERR_INVALID_THIS', 'Value of "this" must be of type %s', TypeError); E('ERR_INVALID_TUPLE', '%s must be an iterable %s tuple', TypeError); E('ERR_INVALID_URI', 'URI malformed', URIError); -E('ERR_INVALID_URL', function(input) { +E('ERR_INVALID_URL', function(input, base = null) { this.input = input; + + if (base != null) { + this.base = base; + } + // Don't include URL in message. // (See https://github.com/nodejs/node/pull/38614) return 'Invalid URL'; diff --git a/lib/internal/url.js b/lib/internal/url.js index 92d5e84e87c71f..13a9e287ffc47a 100644 --- a/lib/internal/url.js +++ b/lib/internal/url.js @@ -780,13 +780,7 @@ class URL { base = `${base}`; } - const href = bindingUrl.parse(input, base); - - if (!href) { - throw new ERR_INVALID_URL(input); - } - - this.#updateContext(href); + this.#updateContext(bindingUrl.parse(input, base)); } [inspect.custom](depth, opts) { diff --git a/src/env_properties.h b/src/env_properties.h index db471d4efb8d1d..8214dba4e59c7d 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -57,6 +57,7 @@ V(args_string, "args") \ V(asn1curve_string, "asn1Curve") \ V(async_ids_stack_string, "async_ids_stack") \ + V(base_string, "base") \ V(bits_string, "bits") \ V(block_list_string, "blockList") \ V(buffer_string, "buffer") \ diff --git a/src/node_url.cc b/src/node_url.cc index 666492ca47cee3..89fcfec20f5685 100644 --- a/src/node_url.cc +++ b/src/node_url.cc @@ -227,6 +227,35 @@ void BindingData::Format(const FunctionCallbackInfo& args) { .ToLocalChecked()); } +void BindingData::ThrowInvalidURL(node::Environment* env, + std::string_view input, + std::optional base) { + Local err = ERR_INVALID_URL(env->isolate(), "Invalid URL"); + DCHECK(err->IsObject()); + + auto err_object = err.As(); + + USE(err_object->Set(env->context(), + env->input_string(), + v8::String::NewFromUtf8(env->isolate(), + input.data(), + v8::NewStringType::kNormal, + input.size()) + .ToLocalChecked())); + + if (base.has_value()) { + USE(err_object->Set(env->context(), + env->base_string(), + v8::String::NewFromUtf8(env->isolate(), + base.value().c_str(), + v8::NewStringType::kNormal, + base.value().size()) + .ToLocalChecked())); + } + + env->isolate()->ThrowException(err); +} + void BindingData::Parse(const FunctionCallbackInfo& args) { CHECK_GE(args.Length(), 1); CHECK(args[0]->IsString()); // input @@ -235,15 +264,16 @@ void BindingData::Parse(const FunctionCallbackInfo& args) { Realm* realm = Realm::GetCurrent(args); BindingData* binding_data = realm->GetBindingData(); Isolate* isolate = realm->isolate(); + std::optional base_{}; Utf8Value input(isolate, args[0]); ada::result base; ada::url_aggregator* base_pointer = nullptr; if (args[1]->IsString()) { - base = - ada::parse(Utf8Value(isolate, args[1]).ToString()); + base_ = Utf8Value(isolate, args[1]).ToString(); + base = ada::parse(*base_); if (!base) { - return args.GetReturnValue().Set(false); + return ThrowInvalidURL(realm->env(), input.ToStringView(), base_); } base_pointer = &base.value(); } @@ -251,7 +281,7 @@ void BindingData::Parse(const FunctionCallbackInfo& args) { ada::parse(input.ToStringView(), base_pointer); if (!out) { - return args.GetReturnValue().Set(false); + return ThrowInvalidURL(realm->env(), input.ToStringView(), base_); } binding_data->UpdateComponents(out->get_components(), out->type); diff --git a/src/node_url.h b/src/node_url.h index c485caa2eb0343..f3aa136a5b538d 100644 --- a/src/node_url.h +++ b/src/node_url.h @@ -76,6 +76,9 @@ class BindingData : public SnapshotableObject { const ada::scheme::type type); static v8::CFunction fast_can_parse_methods_[]; + static void ThrowInvalidURL(Environment* env, + std::string_view input, + std::optional base); }; std::string FromFilePath(const std::string_view file_path); diff --git a/test/parallel/test-url-null-char.js b/test/parallel/test-url-null-char.js index 468080844d534b..d81cbcfb6648d8 100644 --- a/test/parallel/test-url-null-char.js +++ b/test/parallel/test-url-null-char.js @@ -4,5 +4,5 @@ const assert = require('assert'); assert.throws( () => { new URL('a\0b'); }, - { input: 'a\0b' } + { code: 'ERR_INVALID_URL', input: 'a\0b' } ); diff --git a/test/parallel/test-whatwg-url-custom-parsing.js b/test/parallel/test-whatwg-url-custom-parsing.js index 905028fee3812c..cdeda59eec0c98 100644 --- a/test/parallel/test-whatwg-url-custom-parsing.js +++ b/test/parallel/test-whatwg-url-custom-parsing.js @@ -55,7 +55,7 @@ for (const test of failureTests) { () => new URL(test.input, test.base), (error) => { assert.throws(() => { throw error; }, expectedError); - assert.strictEqual(`${error}`, 'TypeError [ERR_INVALID_URL]: Invalid URL'); + assert.strictEqual(`${error}`, 'TypeError: Invalid URL'); assert.strictEqual(error.message, 'Invalid URL'); return true; }); From 67546529c6e8181aa3f93aca5cbbada29bfd29e9 Mon Sep 17 00:00:00 2001 From: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Tue, 26 Sep 2023 22:19:08 +0300 Subject: [PATCH 4/8] stream: use bitmap in writable state PR-URL: https://github.com/nodejs/node/pull/49834 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Robert Nagy Reviewed-By: Luigi Pinca Reviewed-By: Yagiz Nizipli --- lib/internal/streams/writable.js | 337 ++++++++++++++++++------------- 1 file changed, 195 insertions(+), 142 deletions(-) diff --git a/lib/internal/streams/writable.js b/lib/internal/streams/writable.js index 718d4e48478677..595aadc23c8bec 100644 --- a/lib/internal/streams/writable.js +++ b/lib/internal/streams/writable.js @@ -74,6 +74,110 @@ function nop() {} const kOnFinished = Symbol('kOnFinished'); +const kObjectMode = 1 << 0; +const kEnded = 1 << 1; +const kConstructed = 1 << 2; +const kSync = 1 << 3; +const kErrorEmitted = 1 << 4; +const kEmitClose = 1 << 5; +const kAutoDestroy = 1 << 6; +const kDestroyed = 1 << 7; +const kClosed = 1 << 8; +const kCloseEmitted = 1 << 9; +const kFinalCalled = 1 << 10; +const kNeedDrain = 1 << 11; +const kEnding = 1 << 12; +const kFinished = 1 << 13; +const kDecodeStrings = 1 << 14; +const kWriting = 1 << 15; +const kBufferProcessing = 1 << 16; +const kPrefinished = 1 << 17; +const kAllBuffers = 1 << 18; +const kAllNoop = 1 << 19; + +// TODO(benjamingr) it is likely slower to do it this way than with free functions +function makeBitMapDescriptor(bit) { + return { + enumerable: false, + get() { return (this.state & bit) !== 0; }, + set(value) { + if (value) this.state |= bit; + else this.state &= ~bit; + }, + }; +} +ObjectDefineProperties(WritableState.prototype, { + // Object stream flag to indicate whether or not this stream + // contains buffers or objects. + objectMode: makeBitMapDescriptor(kObjectMode), + + // if _final has been called. + finalCalled: makeBitMapDescriptor(kFinalCalled), + + // drain event flag. + needDrain: makeBitMapDescriptor(kNeedDrain), + + // At the start of calling end() + ending: makeBitMapDescriptor(kEnding), + + // When end() has been called, and returned. + ended: makeBitMapDescriptor(kEnded), + + // When 'finish' is emitted. + finished: makeBitMapDescriptor(kFinished), + + // Has it been destroyed. + destroyed: makeBitMapDescriptor(kDestroyed), + + // Should we decode strings into buffers before passing to _write? + // this is here so that some node-core streams can optimize string + // handling at a lower level. + decodeStrings: makeBitMapDescriptor(kDecodeStrings), + + // A flag to see when we're in the middle of a write. + writing: makeBitMapDescriptor(kWriting), + + // A flag to be able to tell if the onwrite cb is called immediately, + // or on a later tick. We set this to true at first, because any + // actions that shouldn't happen until "later" should generally also + // not happen before the first write call. + sync: makeBitMapDescriptor(kSync), + + // A flag to know if we're processing previously buffered items, which + // may call the _write() callback in the same tick, so that we don't + // end up in an overlapped onwrite situation. + bufferProcessing: makeBitMapDescriptor(kBufferProcessing), + + // Stream is still being constructed and cannot be + // destroyed until construction finished or failed. + // Async construction is opt in, therefore we start as + // constructed. + constructed: makeBitMapDescriptor(kConstructed), + + // Emit prefinish if the only thing we're waiting for is _write cbs + // This is relevant for synchronous Transform streams. + prefinished: makeBitMapDescriptor(kPrefinished), + + // True if the error was already emitted and should not be thrown again. + errorEmitted: makeBitMapDescriptor(kErrorEmitted), + + // Should close be emitted on destroy. Defaults to true. + emitClose: makeBitMapDescriptor(kEmitClose), + + // Should .destroy() be called after 'finish' (and potentially 'end'). + autoDestroy: makeBitMapDescriptor(kAutoDestroy), + + // Indicates whether the stream has finished destroying. + closed: makeBitMapDescriptor(kClosed), + + // True if close has been emitted or would have been emitted + // depending on emitClose. + closeEmitted: makeBitMapDescriptor(kCloseEmitted), + + allBuffers: makeBitMapDescriptor(kAllBuffers), + allNoop: makeBitMapDescriptor(kAllNoop), +}); + function WritableState(options, stream, isDuplex) { // Duplex streams are both readable and writable, but share // the same options object. @@ -83,13 +187,12 @@ function WritableState(options, stream, isDuplex) { if (typeof isDuplex !== 'boolean') isDuplex = stream instanceof Stream.Duplex; - // Object stream flag to indicate whether or not this stream - // contains buffers or objects. - this.objectMode = !!(options && options.objectMode); + // Bit map field to store WritableState more effciently with 1 bit per field + // instead of a V8 slot per field. + this.state = kSync | kConstructed | kEmitClose | kAutoDestroy; - if (isDuplex) - this.objectMode = this.objectMode || - !!(options && options.writableObjectMode); + if (options && options.objectMode) this.state |= kObjectMode; + if (isDuplex && options && options.writableObjectMode) this.state |= kObjectMode; // The point at which write() starts returning false // Note: 0 is a valid value, means that we always return false if @@ -98,26 +201,13 @@ function WritableState(options, stream, isDuplex) { getHighWaterMark(this, options, 'writableHighWaterMark', isDuplex) : getDefaultHighWaterMark(false); - // if _final has been called. - this.finalCalled = false; + if (!options || options.decodeStrings !== false) this.state |= kDecodeStrings; - // drain event flag. - this.needDrain = false; - // At the start of calling end() - this.ending = false; - // When end() has been called, and returned. - this.ended = false; - // When 'finish' is emitted. - this.finished = false; + // Should close be emitted on destroy. Defaults to true. + if (options && options.emitClose === false) this.state &= ~kEmitClose; - // Has it been destroyed - this.destroyed = false; - - // Should we decode strings into buffers before passing to _write? - // this is here so that some node-core streams can optimize string - // handling at a lower level. - const noDecode = !!(options && options.decodeStrings === false); - this.decodeStrings = !noDecode; + // Should .destroy() be called after 'end' (and potentially 'finish'). + if (options && options.autoDestroy === false) this.state &= ~kAutoDestroy; // Crypto is kind of old and crusty. Historically, its default string // encoding is 'binary' so we have to make this configurable. @@ -136,23 +226,9 @@ function WritableState(options, stream, isDuplex) { // socket or file. this.length = 0; - // A flag to see when we're in the middle of a write. - this.writing = false; - // When true all writes will be buffered until .uncork() call. this.corked = 0; - // A flag to be able to tell if the onwrite cb is called immediately, - // or on a later tick. We set this to true at first, because any - // actions that shouldn't happen until "later" should generally also - // not happen before the first write call. - this.sync = true; - - // A flag to know if we're processing previously buffered items, which - // may call the _write() callback in the same tick, so that we don't - // end up in an overlapped onwrite situation. - this.bufferProcessing = false; - // The callback that's passed to _write(chunk, cb). this.onwrite = onwrite.bind(undefined, stream); @@ -172,45 +248,18 @@ function WritableState(options, stream, isDuplex) { // this must be 0 before 'finish' can be emitted. this.pendingcb = 0; - // Stream is still being constructed and cannot be - // destroyed until construction finished or failed. - // Async construction is opt in, therefore we start as - // constructed. - this.constructed = true; - - // Emit prefinish if the only thing we're waiting for is _write cbs - // This is relevant for synchronous Transform streams. - this.prefinished = false; - - // True if the error was already emitted and should not be thrown again. - this.errorEmitted = false; - - // Should close be emitted on destroy. Defaults to true. - this.emitClose = !options || options.emitClose !== false; - - // Should .destroy() be called after 'finish' (and potentially 'end'). - this.autoDestroy = !options || options.autoDestroy !== false; - // Indicates whether the stream has errored. When true all write() calls // should return false. This is needed since when autoDestroy // is disabled we need a way to tell whether the stream has failed. this.errored = null; - // Indicates whether the stream has finished destroying. - this.closed = false; - - // True if close has been emitted or would have been emitted - // depending on emitClose. - this.closeEmitted = false; - this[kOnFinished] = []; } function resetBuffer(state) { state.buffered = []; state.bufferedIndex = 0; - state.allBuffers = true; - state.allNoop = true; + state.state |= kAllBuffers | kAllNoop; } WritableState.prototype.getBuffer = function getBuffer() { @@ -307,9 +356,9 @@ function _write(stream, chunk, encoding, cb) { if (chunk === null) { throw new ERR_STREAM_NULL_VALUES(); - } else if (!state.objectMode) { + } else if ((state.state & kObjectMode) === 0) { if (typeof chunk === 'string') { - if (state.decodeStrings !== false) { + if ((state.state & kDecodeStrings) !== 0) { chunk = Buffer.from(chunk, encoding); encoding = 'buffer'; } @@ -325,9 +374,9 @@ function _write(stream, chunk, encoding, cb) { } let err; - if (state.ending) { + if ((state.state & kEnding) !== 0) { err = new ERR_STREAM_WRITE_AFTER_END(); - } else if (state.destroyed) { + } else if ((state.state & kDestroyed) !== 0) { err = new ERR_STREAM_DESTROYED('write'); } @@ -354,7 +403,7 @@ Writable.prototype.uncork = function() { if (state.corked) { state.corked--; - if (!state.writing) + if ((state.state & kWriting) === 0) clearBuffer(this, state); } }; @@ -373,7 +422,7 @@ Writable.prototype.setDefaultEncoding = function setDefaultEncoding(encoding) { // in the queue, and wait our turn. Otherwise, call _write // If we return false, then we need a drain event, so set that flag. function writeOrBuffer(stream, state, chunk, encoding, callback) { - const len = state.objectMode ? 1 : chunk.length; + const len = (state.state & kObjectMode) !== 0 ? 1 : chunk.length; state.length += len; @@ -381,42 +430,40 @@ function writeOrBuffer(stream, state, chunk, encoding, callback) { const ret = state.length < state.highWaterMark; // We must ensure that previous needDrain will not be reset to false. if (!ret) - state.needDrain = true; + state.state |= kNeedDrain; - if (state.writing || state.corked || state.errored || !state.constructed) { + if ((state.state & kWriting) !== 0 || state.corked || state.errored || (state.state & kConstructed) === 0) { state.buffered.push({ chunk, encoding, callback }); - if (state.allBuffers && encoding !== 'buffer') { - state.allBuffers = false; + if ((state.state & kAllBuffers) !== 0 && encoding !== 'buffer') { + state.state &= ~kAllBuffers; } - if (state.allNoop && callback !== nop) { - state.allNoop = false; + if ((state.state & kAllNoop) !== 0 && callback !== nop) { + state.state &= ~kAllNoop; } } else { state.writelen = len; state.writecb = callback; - state.writing = true; - state.sync = true; + state.state |= kWriting | kSync; stream._write(chunk, encoding, state.onwrite); - state.sync = false; + state.state &= ~kSync; } // Return false if errored or destroyed in order to break // any synchronous while(stream.write(data)) loops. - return ret && !state.errored && !state.destroyed; + return ret && !state.errored && (state.state & kDestroyed) === 0; } function doWrite(stream, state, writev, len, chunk, encoding, cb) { state.writelen = len; state.writecb = cb; - state.writing = true; - state.sync = true; - if (state.destroyed) + state.state |= kWriting | kSync; + if ((state.state & kDestroyed) !== 0) state.onwrite(new ERR_STREAM_DESTROYED('write')); else if (writev) stream._writev(chunk, state.onwrite); else stream._write(chunk, encoding, state.onwrite); - state.sync = false; + state.state &= ~kSync; } function onwriteError(stream, state, er, cb) { @@ -434,7 +481,7 @@ function onwriteError(stream, state, er, cb) { function onwrite(stream, er) { const state = stream._writableState; - const sync = state.sync; + const sync = (state.state & kSync) !== 0; const cb = state.writecb; if (typeof cb !== 'function') { @@ -442,7 +489,7 @@ function onwrite(stream, er) { return; } - state.writing = false; + state.state &= ~kWriting; state.writecb = null; state.length -= state.writelen; state.writelen = 0; @@ -495,10 +542,9 @@ function afterWriteTick({ stream, state, count, cb }) { } function afterWrite(stream, state, count, cb) { - const needDrain = !state.ending && !stream.destroyed && state.length === 0 && - state.needDrain; + const needDrain = (state.state & (kEnding | kNeedDrain)) === kNeedDrain && !stream.destroyed && state.length === 0; if (needDrain) { - state.needDrain = false; + state.state &= ~kNeedDrain; stream.emit('drain'); } @@ -507,7 +553,7 @@ function afterWrite(stream, state, count, cb) { cb(null); } - if (state.destroyed) { + if ((state.state & kDestroyed) !== 0) { errorBuffer(state); } @@ -516,13 +562,13 @@ function afterWrite(stream, state, count, cb) { // If there's something in the buffer waiting, then invoke callbacks. function errorBuffer(state) { - if (state.writing) { + if ((state.state & kWriting) !== 0) { return; } for (let n = state.bufferedIndex; n < state.buffered.length; ++n) { const { chunk, callback } = state.buffered[n]; - const len = state.objectMode ? 1 : chunk.length; + const len = (state.state & kObjectMode) !== 0 ? 1 : chunk.length; state.length -= len; callback(state.errored ?? new ERR_STREAM_DESTROYED('write')); } @@ -538,13 +584,13 @@ function errorBuffer(state) { // If there's something in the buffer waiting, then process it. function clearBuffer(stream, state) { if (state.corked || - state.bufferProcessing || - state.destroyed || - !state.constructed) { + (state.state & (kDestroyed | kBufferProcessing)) !== 0 || + (state.state & kConstructed) === 0) { return; } - const { buffered, bufferedIndex, objectMode } = state; + const objectMode = (state.state & kObjectMode) !== 0; + const { buffered, bufferedIndex } = state; const bufferedLength = buffered.length - bufferedIndex; if (!bufferedLength) { @@ -553,20 +599,20 @@ function clearBuffer(stream, state) { let i = bufferedIndex; - state.bufferProcessing = true; + state.state |= kBufferProcessing; if (bufferedLength > 1 && stream._writev) { state.pendingcb -= bufferedLength - 1; - const callback = state.allNoop ? nop : (err) => { + const callback = (state.state & kAllNoop) !== 0 ? nop : (err) => { for (let n = i; n < buffered.length; ++n) { buffered[n].callback(err); } }; // Make a copy of `buffered` if it's going to be used by `callback` above, // since `doWrite` will mutate the array. - const chunks = state.allNoop && i === 0 ? + const chunks = (state.state & kAllNoop) !== 0 && i === 0 ? buffered : ArrayPrototypeSlice(buffered, i); - chunks.allBuffers = state.allBuffers; + chunks.allBuffers = (state.state & kAllBuffers) !== 0; doWrite(stream, state, true, state.length, chunks, '', callback); @@ -577,7 +623,7 @@ function clearBuffer(stream, state) { buffered[i++] = null; const len = objectMode ? 1 : chunk.length; doWrite(stream, state, false, len, chunk, encoding, callback); - } while (i < buffered.length && !state.writing); + } while (i < buffered.length && (state.state & kWriting) === 0); if (i === buffered.length) { resetBuffer(state); @@ -588,7 +634,7 @@ function clearBuffer(stream, state) { state.bufferedIndex = i; } } - state.bufferProcessing = false; + state.state &= ~kBufferProcessing; } Writable.prototype._write = function(chunk, encoding, cb) { @@ -630,26 +676,26 @@ Writable.prototype.end = function(chunk, encoding, cb) { if (err) { // Do nothing... - } else if (!state.errored && !state.ending) { + } else if (!state.errored && (state.state & kEnding) === 0) { // This is forgiving in terms of unnecessary calls to end() and can hide // logic errors. However, usually such errors are harmless and causing a // hard error can be disproportionately destructive. It is not always // trivial for the user to determine whether end() needs to be called // or not. - state.ending = true; + state.state |= kEnding; finishMaybe(this, state, true); - state.ended = true; - } else if (state.finished) { + state.state |= kEnded; + } else if ((state.state & kFinished) !== 0) { err = new ERR_STREAM_ALREADY_FINISHED('end'); - } else if (state.destroyed) { + } else if ((state.state & kDestroyed) !== 0) { err = new ERR_STREAM_DESTROYED('end'); } if (typeof cb === 'function') { if (err) { process.nextTick(cb, err); - } else if (state.finished) { + } else if ((state.state & kFinished) !== 0) { process.nextTick(cb, null); } else { state[kOnFinished].push(cb); @@ -660,16 +706,20 @@ Writable.prototype.end = function(chunk, encoding, cb) { }; function needFinish(state) { - return (state.ending && - !state.destroyed && - state.constructed && + return ( + // State is ended && constructed but not destroyed, finished, writing, errorEmitted or closedEmitted + (state.state & ( + kEnding | + kDestroyed | + kConstructed | + kFinished | + kWriting | + kErrorEmitted | + kCloseEmitted + )) === (kEnding | kConstructed) && state.length === 0 && !state.errored && - state.buffered.length === 0 && - !state.finished && - !state.writing && - !state.errorEmitted && - !state.closeEmitted); + state.buffered.length === 0); } function callFinal(stream, state) { @@ -688,9 +738,9 @@ function callFinal(stream, state) { for (let i = 0; i < onfinishCallbacks.length; i++) { onfinishCallbacks[i](err); } - errorOrDestroy(stream, err, state.sync); + errorOrDestroy(stream, err, (state.state & kSync) !== 0); } else if (needFinish(state)) { - state.prefinished = true; + state.state |= kPrefinished; stream.emit('prefinish'); // Backwards compat. Don't check state.sync here. // Some streams assume 'finish' will be emitted @@ -700,7 +750,7 @@ function callFinal(stream, state) { } } - state.sync = true; + state.state |= kSync; state.pendingcb++; try { @@ -709,16 +759,16 @@ function callFinal(stream, state) { onFinish(err); } - state.sync = false; + state.state &= ~kSync; } function prefinish(stream, state) { - if (!state.prefinished && !state.finalCalled) { - if (typeof stream._final === 'function' && !state.destroyed) { - state.finalCalled = true; + if ((state.state & (kPrefinished | kFinalCalled)) === 0) { + if (typeof stream._final === 'function' && (state.state & kDestroyed) === 0) { + state.state |= kFinalCalled; callFinal(stream, state); } else { - state.prefinished = true; + state.state |= kPrefinished; stream.emit('prefinish'); } } @@ -747,7 +797,7 @@ function finishMaybe(stream, state, sync) { function finish(stream, state) { state.pendingcb--; - state.finished = true; + state.state |= kFinished; const onfinishCallbacks = state[kOnFinished].splice(0); for (let i = 0; i < onfinishCallbacks.length; i++) { @@ -756,7 +806,7 @@ function finish(stream, state) { stream.emit('finish'); - if (state.autoDestroy) { + if ((state.state & kAutoDestroy) !== 0) { // In case of duplex streams we need a way to detect // if the readable side is ready for autoDestroy as well. const rState = stream._readableState; @@ -777,20 +827,21 @@ ObjectDefineProperties(Writable.prototype, { closed: { __proto__: null, get() { - return this._writableState ? this._writableState.closed : false; + return this._writableState ? (this._writableState.state & kClosed) !== 0 : false; }, }, destroyed: { __proto__: null, get() { - return this._writableState ? this._writableState.destroyed : false; + return this._writableState ? (this._writableState.state & kDestroyed) !== 0 : false; }, set(value) { // Backward compatibility, the user is explicitly managing destroyed. - if (this._writableState) { - this._writableState.destroyed = value; - } + if (!this._writableState) return; + + if (value) this._writableState.state |= kDestroyed; + else this._writableState.state &= ~kDestroyed; }, }, @@ -802,8 +853,8 @@ ObjectDefineProperties(Writable.prototype, { // where the writable side was disabled upon construction. // Compat. The user might manually disable writable side through // deprecated setter. - return !!w && w.writable !== false && !w.destroyed && !w.errored && - !w.ending && !w.ended; + return !!w && w.writable !== false && !w.errored && + (w.state & (kEnding | kEnded | kDestroyed)) === 0; }, set(val) { // Backwards compatible. @@ -816,14 +867,14 @@ ObjectDefineProperties(Writable.prototype, { writableFinished: { __proto__: null, get() { - return this._writableState ? this._writableState.finished : false; + return this._writableState ? (this._writableState.state & kFinished) !== 0 : false; }, }, writableObjectMode: { __proto__: null, get() { - return this._writableState ? this._writableState.objectMode : false; + return this._writableState ? (this._writableState.state & kObjectMode) !== 0 : false; }, }, @@ -837,7 +888,7 @@ ObjectDefineProperties(Writable.prototype, { writableEnded: { __proto__: null, get() { - return this._writableState ? this._writableState.ending : false; + return this._writableState ? (this._writableState.state & kEnding) !== 0 : false; }, }, @@ -846,7 +897,9 @@ ObjectDefineProperties(Writable.prototype, { get() { const wState = this._writableState; if (!wState) return false; - return !wState.destroyed && !wState.ending && wState.needDrain; + + // !destroyed && !ending && needDrain + return (wState.state & (kDestroyed | kEnding | kNeedDrain)) === kNeedDrain; }, }, @@ -885,8 +938,8 @@ ObjectDefineProperties(Writable.prototype, { get: function() { return !!( this._writableState.writable !== false && - (this._writableState.destroyed || this._writableState.errored) && - !this._writableState.finished + ((this._writableState.state & kDestroyed) !== 0 || this._writableState.errored) && + (this._writableState.state & kFinished) === 0 ); }, }, @@ -897,7 +950,7 @@ Writable.prototype.destroy = function(err, cb) { const state = this._writableState; // Invoke pending callbacks. - if (!state.destroyed && + if ((state.state & kDestroyed) === 0 && (state.bufferedIndex < state.buffered.length || state[kOnFinished].length)) { process.nextTick(errorBuffer, state); From fef7927cc3a7a3fb81c355301aada4a679c5661c Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Wed, 27 Sep 2023 02:01:14 +0300 Subject: [PATCH 5/8] test: deflake test-runner-output PR-URL: https://github.com/nodejs/node/pull/49878 Fixes: https://github.com/nodejs/node/issues/49853 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Colin Ihrig Reviewed-By: Luigi Pinca Reviewed-By: Geoffrey Booth Reviewed-By: Chemi Atlow --- test/fixtures/test-runner/output/abort.snapshot | 16 ++++++++-------- .../test-runner/output/abort_hooks.snapshot | 4 ++-- .../test-runner/output/abort_suite.snapshot | 8 ++++---- .../test-runner/output/arbitrary-output.snapshot | 6 +++--- .../test-runner/output/describe_it.snapshot | 4 ++-- test/fixtures/test-runner/output/hooks.snapshot | 4 ++-- .../output/unresolved_promise.snapshot | 2 +- test/parallel/test-runner-output.mjs | 3 --- 8 files changed, 22 insertions(+), 25 deletions(-) diff --git a/test/fixtures/test-runner/output/abort.snapshot b/test/fixtures/test-runner/output/abort.snapshot index ceca09da14bfb1..1b758a2314c486 100644 --- a/test/fixtures/test-runner/output/abort.snapshot +++ b/test/fixtures/test-runner/output/abort.snapshot @@ -32,7 +32,7 @@ TAP version 13 # Subtest: not ok 2 not ok 6 - not ok 2 --- - duration_ms: ZERO + duration_ms: * location: '/test/fixtures/test-runner/output/abort.js:(LINE):7' failureType: 'cancelledByParent' error: 'test did not finish before its parent and was cancelled' @@ -41,7 +41,7 @@ TAP version 13 # Subtest: not ok 3 not ok 7 - not ok 3 --- - duration_ms: ZERO + duration_ms: * location: '/test/fixtures/test-runner/output/abort.js:(LINE):7' failureType: 'testAborted' error: 'This operation was aborted' @@ -62,7 +62,7 @@ TAP version 13 # Subtest: not ok 4 not ok 8 - not ok 4 --- - duration_ms: ZERO + duration_ms: * location: '/test/fixtures/test-runner/output/abort.js:(LINE):7' failureType: 'testAborted' error: 'This operation was aborted' @@ -83,7 +83,7 @@ TAP version 13 # Subtest: not ok 5 not ok 9 - not ok 5 --- - duration_ms: ZERO + duration_ms: * location: '/test/fixtures/test-runner/output/abort.js:(LINE):7' failureType: 'testAborted' error: 'This operation was aborted' @@ -169,7 +169,7 @@ not ok 2 - promise abort signal # Subtest: not ok 2 not ok 6 - not ok 2 --- - duration_ms: ZERO + duration_ms: * location: '/test/fixtures/test-runner/output/abort.js:(LINE):5' failureType: 'cancelledByParent' error: 'test did not finish before its parent and was cancelled' @@ -178,7 +178,7 @@ not ok 2 - promise abort signal # Subtest: not ok 3 not ok 7 - not ok 3 --- - duration_ms: ZERO + duration_ms: * location: '/test/fixtures/test-runner/output/abort.js:(LINE):5' failureType: 'testAborted' error: 'This operation was aborted' @@ -199,7 +199,7 @@ not ok 2 - promise abort signal # Subtest: not ok 4 not ok 8 - not ok 4 --- - duration_ms: ZERO + duration_ms: * location: '/test/fixtures/test-runner/output/abort.js:(LINE):5' failureType: 'testAborted' error: 'This operation was aborted' @@ -220,7 +220,7 @@ not ok 2 - promise abort signal # Subtest: not ok 5 not ok 9 - not ok 5 --- - duration_ms: ZERO + duration_ms: * location: '/test/fixtures/test-runner/output/abort.js:(LINE):5' failureType: 'testAborted' error: 'This operation was aborted' diff --git a/test/fixtures/test-runner/output/abort_hooks.snapshot b/test/fixtures/test-runner/output/abort_hooks.snapshot index d0b567bb6a22cd..278b5e5fd36ca5 100644 --- a/test/fixtures/test-runner/output/abort_hooks.snapshot +++ b/test/fixtures/test-runner/output/abort_hooks.snapshot @@ -11,7 +11,7 @@ TAP version 13 # Subtest: test 1 not ok 1 - test 1 --- - duration_ms: ZERO + duration_ms: * location: '/test/fixtures/test-runner/output/abort_hooks.js:(LINE):3' failureType: 'cancelledByParent' error: 'test did not finish before its parent and was cancelled' @@ -20,7 +20,7 @@ TAP version 13 # Subtest: test 2 not ok 2 - test 2 --- - duration_ms: ZERO + duration_ms: * location: '/test/fixtures/test-runner/output/abort_hooks.js:(LINE):3' failureType: 'cancelledByParent' error: 'test did not finish before its parent and was cancelled' diff --git a/test/fixtures/test-runner/output/abort_suite.snapshot b/test/fixtures/test-runner/output/abort_suite.snapshot index e7e8c4f4e2360f..30d48d236ff4a5 100644 --- a/test/fixtures/test-runner/output/abort_suite.snapshot +++ b/test/fixtures/test-runner/output/abort_suite.snapshot @@ -32,7 +32,7 @@ TAP version 13 # Subtest: not ok 2 not ok 6 - not ok 2 --- - duration_ms: ZERO + duration_ms: * location: '/test/fixtures/test-runner/output/abort_suite.js:(LINE):3' failureType: 'cancelledByParent' error: 'test did not finish before its parent and was cancelled' @@ -41,7 +41,7 @@ TAP version 13 # Subtest: not ok 3 not ok 7 - not ok 3 --- - duration_ms: ZERO + duration_ms: * location: '/test/fixtures/test-runner/output/abort_suite.js:(LINE):3' failureType: 'testAborted' error: 'This operation was aborted' @@ -62,7 +62,7 @@ TAP version 13 # Subtest: not ok 4 not ok 8 - not ok 4 --- - duration_ms: ZERO + duration_ms: * location: '/test/fixtures/test-runner/output/abort_suite.js:(LINE):3' failureType: 'testAborted' error: 'This operation was aborted' @@ -83,7 +83,7 @@ TAP version 13 # Subtest: not ok 5 not ok 9 - not ok 5 --- - duration_ms: ZERO + duration_ms: * location: '/test/fixtures/test-runner/output/abort_suite.js:(LINE):3' failureType: 'testAborted' error: 'This operation was aborted' diff --git a/test/fixtures/test-runner/output/arbitrary-output.snapshot b/test/fixtures/test-runner/output/arbitrary-output.snapshot index 2389096398cd09..601aaa42f3c74a 100644 --- a/test/fixtures/test-runner/output/arbitrary-output.snapshot +++ b/test/fixtures/test-runner/output/arbitrary-output.snapshot @@ -1,17 +1,17 @@ TAP version 13 ok 1 - test --- - duration_ms: ZERO + duration_ms: * ... # arbitrary - pre ok 2 - test --- - duration_ms: ZERO + duration_ms: * ... # arbitrary - mid ok 3 - test --- - duration_ms: ZERO + duration_ms: * ... # arbitrary - post 1..3 diff --git a/test/fixtures/test-runner/output/describe_it.snapshot b/test/fixtures/test-runner/output/describe_it.snapshot index be345f11575c8d..1d4f7853ead0d1 100644 --- a/test/fixtures/test-runner/output/describe_it.snapshot +++ b/test/fixtures/test-runner/output/describe_it.snapshot @@ -513,7 +513,7 @@ not ok 51 - subtest sync throw fails # Subtest: should not run not ok 1 - should not run --- - duration_ms: ZERO + duration_ms: * location: '/test/fixtures/test-runner/output/describe_it.js:(LINE):3' failureType: 'cancelledByParent' error: 'test did not finish before its parent and was cancelled' @@ -544,7 +544,7 @@ not ok 52 - describe sync throw fails # Subtest: should not run not ok 1 - should not run --- - duration_ms: ZERO + duration_ms: * location: '/test/fixtures/test-runner/output/describe_it.js:(LINE):3' failureType: 'cancelledByParent' error: 'test did not finish before its parent and was cancelled' diff --git a/test/fixtures/test-runner/output/hooks.snapshot b/test/fixtures/test-runner/output/hooks.snapshot index 5afe398ed3d0ea..6cf29612c535cb 100644 --- a/test/fixtures/test-runner/output/hooks.snapshot +++ b/test/fixtures/test-runner/output/hooks.snapshot @@ -37,7 +37,7 @@ ok 1 - describe hooks # Subtest: 1 not ok 1 - 1 --- - duration_ms: ZERO + duration_ms: * location: '/test/fixtures/test-runner/output/hooks.js:(LINE):3' failureType: 'cancelledByParent' error: 'test did not finish before its parent and was cancelled' @@ -46,7 +46,7 @@ ok 1 - describe hooks # Subtest: 2 not ok 2 - 2 --- - duration_ms: ZERO + duration_ms: * location: '/test/fixtures/test-runner/output/hooks.js:(LINE):3' failureType: 'cancelledByParent' error: 'test did not finish before its parent and was cancelled' diff --git a/test/fixtures/test-runner/output/unresolved_promise.snapshot b/test/fixtures/test-runner/output/unresolved_promise.snapshot index 839ec311a65e04..0090885468c338 100644 --- a/test/fixtures/test-runner/output/unresolved_promise.snapshot +++ b/test/fixtures/test-runner/output/unresolved_promise.snapshot @@ -18,7 +18,7 @@ not ok 2 - never resolving promise # Subtest: fail not ok 3 - fail --- - duration_ms: ZERO + duration_ms: * location: '/test/fixtures/test-runner/output/unresolved_promise.js:(LINE):1' failureType: 'cancelledByParent' error: 'Promise resolution is still pending but the event loop has already resolved' diff --git a/test/parallel/test-runner-output.mjs b/test/parallel/test-runner-output.mjs index fff6fed92655e9..372ca8f3bae0ff 100644 --- a/test/parallel/test-runner-output.mjs +++ b/test/parallel/test-runner-output.mjs @@ -10,7 +10,6 @@ const skipForceColors = function replaceTestDuration(str) { return str - .replaceAll(/duration_ms: 0(\r?\n)/g, 'duration_ms: ZERO$1') .replaceAll(/duration_ms: [0-9.]+/g, 'duration_ms: *') .replaceAll(/duration_ms [0-9.]+/g, 'duration_ms *'); } @@ -20,7 +19,6 @@ const stackTraceBasePath = new RegExp(`${color}\\(${process.cwd()}/?${color}(.*) function replaceSpecDuration(str) { return str - .replaceAll(/\(0(\r?\n)ms\)/g, '(ZEROms)') .replaceAll(/[0-9.]+ms/g, '*ms') .replaceAll(/duration_ms [0-9.]+/g, 'duration_ms *') .replace(stackTraceBasePath, '$3'); @@ -28,7 +26,6 @@ function replaceSpecDuration(str) { function replaceJunitDuration(str) { return str - .replaceAll(/time="0"/g, 'time="ZERO"') .replaceAll(/time="[0-9.]+"/g, 'time="*"') .replaceAll(/duration_ms [0-9.]+/g, 'duration_ms *') .replaceAll(hostname(), 'HOSTNAME') From a6ad048b8996795a71142a07b771ba9bfe83fa55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vinicius=20Louren=C3=A7o?= <12551007+H4ad@users.noreply.github.com> Date: Tue, 26 Sep 2023 22:19:26 -0300 Subject: [PATCH 6/8] perf_hooks: reduce overhead of new performance_entries PR-URL: https://github.com/nodejs/node/pull/49803 Reviewed-By: Matteo Collina Reviewed-By: Benjamin Gruenbaum Reviewed-By: Yagiz Nizipli Reviewed-By: Stephen Belanger --- benchmark/perf_hooks/timerfied.js | 36 ++++++++++++++++++++++++++ lib/internal/perf/performance_entry.js | 26 ++++++++++++------- 2 files changed, 52 insertions(+), 10 deletions(-) create mode 100644 benchmark/perf_hooks/timerfied.js diff --git a/benchmark/perf_hooks/timerfied.js b/benchmark/perf_hooks/timerfied.js new file mode 100644 index 00000000000000..50be0a47fc1b5a --- /dev/null +++ b/benchmark/perf_hooks/timerfied.js @@ -0,0 +1,36 @@ +'use strict'; + +const assert = require('assert'); +const common = require('../common.js'); + +const { + PerformanceObserver, + performance, +} = require('perf_hooks'); + +function randomFn() { + return Math.random(); +} + +const bench = common.createBenchmark(main, { + n: [1e5], + observe: ['function'], +}); + +let _result; + +function main({ n, observe }) { + const obs = new PerformanceObserver(() => { + bench.end(n); + }); + obs.observe({ entryTypes: [observe], buffered: true }); + + const timerfied = performance.timerify(randomFn); + + bench.start(); + for (let i = 0; i < n; i++) + _result = timerfied(); + + // Avoid V8 deadcode (elimination) + assert.ok(_result); +} diff --git a/lib/internal/perf/performance_entry.js b/lib/internal/perf/performance_entry.js index 036bfc173bd024..aa97a652626606 100644 --- a/lib/internal/perf/performance_entry.js +++ b/lib/internal/perf/performance_entry.js @@ -2,7 +2,6 @@ const { ObjectDefineProperties, - ReflectConstruct, Symbol, } = primordials; @@ -25,14 +24,17 @@ const kEntryType = Symbol('PerformanceEntry.EntryType'); const kStartTime = Symbol('PerformanceEntry.StartTime'); const kDuration = Symbol('PerformanceEntry.Duration'); const kDetail = Symbol('NodePerformanceEntry.Detail'); +const kSkipThrow = Symbol('kSkipThrow'); function isPerformanceEntry(obj) { return obj?.[kName] !== undefined; } class PerformanceEntry { - constructor() { - throw new ERR_ILLEGAL_CONSTRUCTOR(); + constructor(skipThrowSymbol = undefined) { + if (skipThrowSymbol !== kSkipThrow) { + throw new ERR_ILLEGAL_CONSTRUCTOR(); + } } get name() { @@ -92,9 +94,11 @@ function initPerformanceEntry(entry, name, type, start, duration) { } function createPerformanceEntry(name, type, start, duration) { - return ReflectConstruct(function PerformanceEntry() { - initPerformanceEntry(this, name, type, start, duration); - }, [], PerformanceEntry); + const entry = new PerformanceEntry(kSkipThrow); + + initPerformanceEntry(entry, name, type, start, duration); + + return entry; } /** @@ -119,10 +123,12 @@ class PerformanceNodeEntry extends PerformanceEntry { } function createPerformanceNodeEntry(name, type, start, duration, detail) { - return ReflectConstruct(function PerformanceNodeEntry() { - initPerformanceEntry(this, name, type, start, duration); - this[kDetail] = detail; - }, [], PerformanceNodeEntry); + const entry = new PerformanceNodeEntry(kSkipThrow); + + initPerformanceEntry(entry, name, type, start, duration); + entry[kDetail] = detail; + + return entry; } module.exports = { From 7e0b6a59390cc60d99b718834bdb21e5733c0b6a Mon Sep 17 00:00:00 2001 From: CanadaHonk <19228318+CanadaHonk@users.noreply.github.com> Date: Wed, 27 Sep 2023 13:43:05 +0100 Subject: [PATCH 7/8] fs: improve error performance for `unlinkSync` PR-URL: https://github.com/nodejs/node/pull/49856 Refs: https://github.com/nodejs/performance/issues/106 Reviewed-By: Yagiz Nizipli Reviewed-By: Benjamin Gruenbaum --- benchmark/fs/bench-unlinkSync.js | 43 ++++++++++++++++++++++++++++++++ lib/fs.js | 5 +--- lib/internal/fs/sync.js | 6 +++++ src/node_file.cc | 23 +++++++++++++++++ 4 files changed, 73 insertions(+), 4 deletions(-) create mode 100644 benchmark/fs/bench-unlinkSync.js diff --git a/benchmark/fs/bench-unlinkSync.js b/benchmark/fs/bench-unlinkSync.js new file mode 100644 index 00000000000000..8b992198c8d368 --- /dev/null +++ b/benchmark/fs/bench-unlinkSync.js @@ -0,0 +1,43 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); +const tmpdir = require('../../test/common/tmpdir'); +tmpdir.refresh(); + +const bench = common.createBenchmark(main, { + type: ['existing', 'non-existing'], + n: [1e3], +}); + +function main({ n, type }) { + let files; + + switch (type) { + case 'existing': + files = []; + + // Populate tmpdir with mock files + for (let i = 0; i < n; i++) { + const path = tmpdir.resolve(`unlinksync-bench-file-${i}`); + fs.writeFileSync(path, 'bench'); + files.push(path); + } + break; + case 'non-existing': + files = new Array(n).fill(tmpdir.resolve(`.non-existing-file-${Date.now()}`)); + break; + default: + new Error('Invalid type'); + } + + bench.start(); + for (let i = 0; i < n; i++) { + try { + fs.unlinkSync(files[i]); + } catch { + // do nothing + } + } + bench.end(n); +} diff --git a/lib/fs.js b/lib/fs.js index 9b6b61dc8efd42..29f356a57cd22e 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1850,10 +1850,7 @@ function unlink(path, callback) { * @returns {void} */ function unlinkSync(path) { - path = getValidatedPath(path); - const ctx = { path }; - binding.unlink(pathModule.toNamespacedPath(path), undefined, ctx); - handleErrorFromBinding(ctx); + return syncFs.unlink(path); } /** diff --git a/lib/internal/fs/sync.js b/lib/internal/fs/sync.js index 0d4ba90150e186..fbcc2ad2e25b2a 100644 --- a/lib/internal/fs/sync.js +++ b/lib/internal/fs/sync.js @@ -88,6 +88,11 @@ function close(fd) { return binding.closeSync(fd); } +function unlink(path) { + path = pathModule.toNamespacedPath(getValidatedPath(path)); + return binding.unlinkSync(path); +} + module.exports = { readFileUtf8, exists, @@ -97,4 +102,5 @@ module.exports = { statfs, open, close, + unlink, }; diff --git a/src/node_file.cc b/src/node_file.cc index b76eb385295836..0e3134fb0fc687 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1667,6 +1667,27 @@ static void Unlink(const FunctionCallbackInfo& args) { } } +static void UnlinkSync(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + + const int argc = args.Length(); + CHECK_GE(argc, 1); + + BufferValue path(env->isolate(), args[0]); + CHECK_NOT_NULL(*path); + THROW_IF_INSUFFICIENT_PERMISSIONS( + env, permission::PermissionScope::kFileSystemWrite, path.ToStringView()); + + uv_fs_t req; + auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); }); + FS_SYNC_TRACE_BEGIN(unlink); + int err = uv_fs_unlink(nullptr, &req, *path, nullptr); + FS_SYNC_TRACE_END(unlink); + if (err < 0) { + return env->ThrowUVException(err, "unlink", nullptr, *path); + } +} + static void RMDir(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -3390,6 +3411,7 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, SetMethod(isolate, target, "symlink", Symlink); SetMethod(isolate, target, "readlink", ReadLink); SetMethod(isolate, target, "unlink", Unlink); + SetMethod(isolate, target, "unlinkSync", UnlinkSync); SetMethod(isolate, target, "writeBuffer", WriteBuffer); SetMethod(isolate, target, "writeBuffers", WriteBuffers); SetMethod(isolate, target, "writeString", WriteString); @@ -3515,6 +3537,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(Symlink); registry->Register(ReadLink); registry->Register(Unlink); + registry->Register(UnlinkSync); registry->Register(WriteBuffer); registry->Register(WriteBuffers); registry->Register(WriteString); From 783f64bd981a10dae7dc4ce297843ce4a0a26f5c Mon Sep 17 00:00:00 2001 From: CanadaHonk <19228318+CanadaHonk@users.noreply.github.com> Date: Wed, 27 Sep 2023 14:06:53 +0100 Subject: [PATCH 8/8] fs: replace `SetMethodNoSideEffect` in node_file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/49857 Reviewed-By: Yagiz Nizipli Reviewed-By: Michaël Zasso --- src/node_file.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index 0e3134fb0fc687..aca7ec82101a60 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3382,15 +3382,15 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, Isolate* isolate = isolate_data->isolate(); SetMethod(isolate, target, "access", Access); - SetMethodNoSideEffect(isolate, target, "accessSync", AccessSync); + SetMethod(isolate, target, "accessSync", AccessSync); SetMethod(isolate, target, "close", Close); SetMethod(isolate, target, "closeSync", CloseSync); - SetMethodNoSideEffect(isolate, target, "existsSync", ExistsSync); + SetMethod(isolate, target, "existsSync", ExistsSync); SetMethod(isolate, target, "open", Open); SetMethod(isolate, target, "openSync", OpenSync); SetMethod(isolate, target, "openFileHandle", OpenFileHandle); SetMethod(isolate, target, "read", Read); - SetMethodNoSideEffect(isolate, target, "readFileUtf8", ReadFileUtf8); + SetMethod(isolate, target, "readFileUtf8", ReadFileUtf8); SetMethod(isolate, target, "readBuffers", ReadBuffers); SetMethod(isolate, target, "fdatasync", Fdatasync); SetMethod(isolate, target, "fsync", Fsync); @@ -3417,7 +3417,7 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, SetMethod(isolate, target, "writeString", WriteString); SetMethod(isolate, target, "realpath", RealPath); SetMethod(isolate, target, "copyFile", CopyFile); - SetMethodNoSideEffect(isolate, target, "copyFileSync", CopyFileSync); + SetMethod(isolate, target, "copyFileSync", CopyFileSync); SetMethod(isolate, target, "chmod", Chmod); SetMethod(isolate, target, "fchmod", FChmod);