From ea88a3e1f2ba38531806fbd6fb5c668fa1cf0104 Mon Sep 17 00:00:00 2001 From: CanadaHonk Date: Thu, 23 Nov 2023 00:25:15 +0000 Subject: [PATCH] fs: improve error perf of sync `lstat`+`fstat` PR-URL: https://github.com/nodejs/node/pull/49868 Refs: https://github.com/nodejs/performance/issues/106 Reviewed-By: Yagiz Nizipli Reviewed-By: Joyee Cheung Reviewed-By: James M Snell --- benchmark/fs/bench-statSync-failure.js | 22 ++++++--- benchmark/fs/bench-statSync.js | 2 +- lib/fs.js | 68 +++++++++++--------------- src/node_file.cc | 43 +++++++++------- test/parallel/test-fs-sync-fd-leak.js | 9 +--- typings/internalBinding/fs.d.ts | 12 ++--- 6 files changed, 79 insertions(+), 77 deletions(-) diff --git a/benchmark/fs/bench-statSync-failure.js b/benchmark/fs/bench-statSync-failure.js index de6128d8fc1845..b1005a78a7f6e0 100644 --- a/benchmark/fs/bench-statSync-failure.js +++ b/benchmark/fs/bench-statSync-failure.js @@ -5,21 +5,29 @@ const fs = require('fs'); const path = require('path'); const bench = common.createBenchmark(main, { - n: [1e6], - statSyncType: ['throw', 'noThrow'], + n: [1e4], + statSyncType: ['fstatSync', 'lstatSync', 'statSync'], + throwType: [ 'throw', 'noThrow' ], +}, { + // fstatSync does not support throwIfNoEntry + combinationFilter: ({ statSyncType, throwType }) => !(statSyncType === 'fstatSync' && throwType === 'noThrow'), }); -function main({ n, statSyncType }) { - const arg = path.join(__dirname, 'non.existent'); +function main({ n, statSyncType, throwType }) { + const arg = (statSyncType === 'fstatSync' ? + (1 << 30) : + path.join(__dirname, 'non.existent')); + + const fn = fs[statSyncType]; bench.start(); for (let i = 0; i < n; i++) { - if (statSyncType === 'noThrow') { - fs.statSync(arg, { throwIfNoEntry: false }); + if (throwType === 'noThrow') { + fn(arg, { throwIfNoEntry: false }); } else { try { - fs.statSync(arg); + fn(arg); } catch { // Continue regardless of error. } diff --git a/benchmark/fs/bench-statSync.js b/benchmark/fs/bench-statSync.js index 4ec2e860fb6ba7..e6680fe7cb85fe 100644 --- a/benchmark/fs/bench-statSync.js +++ b/benchmark/fs/bench-statSync.js @@ -4,7 +4,7 @@ const common = require('../common'); const fs = require('fs'); const bench = common.createBenchmark(main, { - n: [1e6], + n: [1e4], statSyncType: ['fstatSync', 'lstatSync', 'statSync'], }); diff --git a/lib/fs.js b/lib/fs.js index 80c6384c3e6c73..132a693d05aaa4 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -74,8 +74,6 @@ const { ERR_INVALID_ARG_VALUE, }, AbortError, - uvErrmapGet, - UVException, } = require('internal/errors'); const { @@ -402,11 +400,9 @@ function readFile(path, options, callback) { } function tryStatSync(fd, isUserFd) { - const ctx = {}; - const stats = binding.fstat(fd, false, undefined, ctx); - if (ctx.errno !== undefined && !isUserFd) { + const stats = binding.fstat(fd, false, undefined, true /* shouldNotThrow */); + if (stats === undefined && !isUserFd) { fs.closeSync(fd); - throw new UVException(ctx); } return stats; } @@ -1616,19 +1612,6 @@ function statfs(path, options = { bigint: false }, callback) { binding.statfs(pathModule.toNamespacedPath(path), options.bigint, req); } -function hasNoEntryError(ctx) { - if (ctx.errno) { - const uvErr = uvErrmapGet(ctx.errno); - return uvErr?.[0] === 'ENOENT'; - } - - if (ctx.error) { - return ctx.error.code === 'ENOENT'; - } - - return false; -} - /** * Synchronously retrieves the `fs.Stats` for * the file descriptor. @@ -1636,13 +1619,14 @@ function hasNoEntryError(ctx) { * @param {{ * bigint?: boolean; * }} [options] - * @returns {Stats} + * @returns {Stats | undefined} */ function fstatSync(fd, options = { bigint: false }) { fd = getValidatedFd(fd); - const ctx = { fd }; - const stats = binding.fstat(fd, options.bigint, undefined, ctx); - handleErrorFromBinding(ctx); + const stats = binding.fstat(fd, options.bigint, undefined, false); + if (stats === undefined) { + return; + } return getStatsFromBinding(stats); } @@ -1654,17 +1638,20 @@ function fstatSync(fd, options = { bigint: false }) { * bigint?: boolean; * throwIfNoEntry?: boolean; * }} [options] - * @returns {Stats} + * @returns {Stats | undefined} */ function lstatSync(path, options = { bigint: false, throwIfNoEntry: true }) { path = getValidatedPath(path); - const ctx = { path }; - const stats = binding.lstat(pathModule.toNamespacedPath(path), - options.bigint, undefined, ctx); - if (options.throwIfNoEntry === false && hasNoEntryError(ctx)) { - return undefined; + const stats = binding.lstat( + pathModule.toNamespacedPath(path), + options.bigint, + undefined, + options.throwIfNoEntry, + ); + + if (stats === undefined) { + return; } - handleErrorFromBinding(ctx); return getStatsFromBinding(stats); } @@ -2667,9 +2654,10 @@ function realpathSync(p, options) { // On windows, check that the root exists. On unix there is no need. if (isWindows) { - const ctx = { path: base }; - binding.lstat(pathModule.toNamespacedPath(base), false, undefined, ctx); - handleErrorFromBinding(ctx); + const out = binding.lstat(pathModule.toNamespacedPath(base), false, undefined, true /* throwIfNoEntry */); + if (out === undefined) { + return; + } knownHard.add(base); } @@ -2709,9 +2697,10 @@ function realpathSync(p, options) { // for our internal use. const baseLong = pathModule.toNamespacedPath(base); - const ctx = { path: base }; - const stats = binding.lstat(baseLong, true, undefined, ctx); - handleErrorFromBinding(ctx); + const stats = binding.lstat(baseLong, true, undefined, true /* throwIfNoEntry */); + if (stats === undefined) { + return; + } if (!isFileType(stats, S_IFLNK)) { knownHard.add(base); @@ -2750,9 +2739,10 @@ function realpathSync(p, options) { // On windows, check that the root exists. On unix there is no need. if (isWindows && !knownHard.has(base)) { - const ctx = { path: base }; - binding.lstat(pathModule.toNamespacedPath(base), false, undefined, ctx); - handleErrorFromBinding(ctx); + const out = binding.lstat(pathModule.toNamespacedPath(base), false, undefined, true /* throwIfNoEntry */); + if (out === undefined) { + return; + } knownHard.add(base); } } diff --git a/src/node_file.cc b/src/node_file.cc index 287036ab9d0b3c..502a9ab712072a 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1118,21 +1118,26 @@ static void LStat(const FunctionCallbackInfo& args) { CHECK_NOT_NULL(*path); bool use_bigint = args[1]->IsTrue(); - FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint); - if (req_wrap_async != nullptr) { // lstat(path, use_bigint, req) + if (!args[2]->IsUndefined()) { // lstat(path, use_bigint, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint); FS_ASYNC_TRACE_BEGIN1( UV_FS_LSTAT, req_wrap_async, "path", TRACE_STR_COPY(*path)) AsyncCall(env, req_wrap_async, args, "lstat", UTF8, AfterStat, uv_fs_lstat, *path); - } else { // lstat(path, use_bigint, undefined, ctx) - CHECK_EQ(argc, 4); - FSReqWrapSync req_wrap_sync; + } else { // lstat(path, use_bigint, undefined, throw_if_no_entry) + bool do_not_throw_if_no_entry = args[3]->IsFalse(); + FSReqWrapSync req_wrap_sync("lstat", *path); FS_SYNC_TRACE_BEGIN(lstat); - int err = SyncCall(env, args[3], &req_wrap_sync, "lstat", uv_fs_lstat, - *path); + int result; + if (do_not_throw_if_no_entry) { + result = SyncCallAndThrowIf( + is_uv_error_except_no_entry, env, &req_wrap_sync, uv_fs_lstat, *path); + } else { + result = SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_lstat, *path); + } FS_SYNC_TRACE_END(lstat); - if (err != 0) { - return; // error info is in ctx + if (is_uv_error(result)) { + return; } Local arr = FillGlobalStatsArray(binding_data, use_bigint, @@ -1153,19 +1158,23 @@ static void FStat(const FunctionCallbackInfo& args) { int fd = args[0].As()->Value(); bool use_bigint = args[1]->IsTrue(); - FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint); - if (req_wrap_async != nullptr) { // fstat(fd, use_bigint, req) + if (!args[2]->IsUndefined()) { // fstat(fd, use_bigint, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint); FS_ASYNC_TRACE_BEGIN0(UV_FS_FSTAT, req_wrap_async) AsyncCall(env, req_wrap_async, args, "fstat", UTF8, AfterStat, uv_fs_fstat, fd); - } else { // fstat(fd, use_bigint, undefined, ctx) - CHECK_EQ(argc, 4); - FSReqWrapSync req_wrap_sync; + } else { // fstat(fd, use_bigint, undefined, do_not_throw_error) + bool do_not_throw_error = args[2]->IsTrue(); + const auto should_throw = [do_not_throw_error](int result) { + return is_uv_error(result) && !do_not_throw_error; + }; + FSReqWrapSync req_wrap_sync("fstat"); FS_SYNC_TRACE_BEGIN(fstat); - int err = SyncCall(env, args[3], &req_wrap_sync, "fstat", uv_fs_fstat, fd); + int err = + SyncCallAndThrowIf(should_throw, env, &req_wrap_sync, uv_fs_fstat, fd); FS_SYNC_TRACE_END(fstat); - if (err != 0) { - return; // error info is in ctx + if (is_uv_error(err)) { + return; } Local arr = FillGlobalStatsArray(binding_data, use_bigint, diff --git a/test/parallel/test-fs-sync-fd-leak.js b/test/parallel/test-fs-sync-fd-leak.js index 52f6f38f9c0ea0..8a9eb39a67a906 100644 --- a/test/parallel/test-fs-sync-fd-leak.js +++ b/test/parallel/test-fs-sync-fd-leak.js @@ -25,7 +25,6 @@ require('../common'); const assert = require('assert'); const fs = require('fs'); const { internalBinding } = require('internal/test/binding'); -const { UV_EBADF } = internalBinding('uv'); // Ensure that (read|write|append)FileSync() closes the file descriptor fs.openSync = function() { @@ -42,15 +41,11 @@ fs.writeSync = function() { throw new Error('BAM'); }; -internalBinding('fs').fstat = function(fd, bigint, _, ctx) { - ctx.errno = UV_EBADF; - ctx.syscall = 'fstat'; +internalBinding('fs').fstat = function() { + throw new Error('EBADF: bad file descriptor, fstat'); }; let close_called = 0; -ensureThrows(function() { - fs.readFileSync('dummy'); -}, 'EBADF: bad file descriptor, fstat'); ensureThrows(function() { fs.writeFileSync('dummy', 'xxx'); }, 'BAM'); diff --git a/typings/internalBinding/fs.d.ts b/typings/internalBinding/fs.d.ts index 66cf7132e6826e..4c61110b2444a7 100644 --- a/typings/internalBinding/fs.d.ts +++ b/typings/internalBinding/fs.d.ts @@ -92,9 +92,9 @@ declare namespace InternalFSBinding { function fstat(fd: number, useBigint: boolean, req: FSReqCallback): void; function fstat(fd: number, useBigint: true, req: FSReqCallback): void; function fstat(fd: number, useBigint: false, req: FSReqCallback): void; - function fstat(fd: number, useBigint: boolean, req: undefined, ctx: FSSyncContext): Float64Array | BigUint64Array; - function fstat(fd: number, useBigint: true, req: undefined, ctx: FSSyncContext): BigUint64Array; - function fstat(fd: number, useBigint: false, req: undefined, ctx: FSSyncContext): Float64Array; + function fstat(fd: number, useBigint: boolean, req: undefined, shouldNotThrow: boolean): Float64Array | BigUint64Array; + function fstat(fd: number, useBigint: true, req: undefined, shouldNotThrow: boolean): BigUint64Array; + function fstat(fd: number, useBigint: false, req: undefined, shouldNotThrow: boolean): Float64Array; function fstat(fd: number, useBigint: boolean, usePromises: typeof kUsePromises): Promise; function fstat(fd: number, useBigint: true, usePromises: typeof kUsePromises): Promise; function fstat(fd: number, useBigint: false, usePromises: typeof kUsePromises): Promise; @@ -126,9 +126,9 @@ declare namespace InternalFSBinding { function lstat(path: StringOrBuffer, useBigint: boolean, req: FSReqCallback): void; function lstat(path: StringOrBuffer, useBigint: true, req: FSReqCallback): void; function lstat(path: StringOrBuffer, useBigint: false, req: FSReqCallback): void; - function lstat(path: StringOrBuffer, useBigint: boolean, req: undefined, ctx: FSSyncContext): Float64Array | BigUint64Array; - function lstat(path: StringOrBuffer, useBigint: true, req: undefined, ctx: FSSyncContext): BigUint64Array; - function lstat(path: StringOrBuffer, useBigint: false, req: undefined, ctx: FSSyncContext): Float64Array; + function lstat(path: StringOrBuffer, useBigint: boolean, req: undefined, throwIfNoEntry: boolean): Float64Array | BigUint64Array; + function lstat(path: StringOrBuffer, useBigint: true, req: undefined, throwIfNoEntry: boolean): BigUint64Array; + function lstat(path: StringOrBuffer, useBigint: false, req: undefined, throwIfNoEntry: boolean): Float64Array; function lstat(path: StringOrBuffer, useBigint: boolean, usePromises: typeof kUsePromises): Promise; function lstat(path: StringOrBuffer, useBigint: true, usePromises: typeof kUsePromises): Promise; function lstat(path: StringOrBuffer, useBigint: false, usePromises: typeof kUsePromises): Promise;