From 6bc7fa7906887799a05821773b52444328b8f068 Mon Sep 17 00:00:00 2001 From: CanadaHonk Date: Mon, 25 Sep 2023 16:30:42 +0100 Subject: [PATCH] fs: improve error perf of sync `chmod`+`fchmod` PR-URL: https://github.com/nodejs/node/pull/49859 Refs: https://github.com/nodejs/performance/issues/106 Reviewed-By: Yagiz Nizipli Reviewed-By: Darshan Sen Reviewed-By: Joyee Cheung --- benchmark/fs/bench-chmodSync.js | 41 ++++++++++++++++++++++ benchmark/fs/bench-fchmodSync.js | 60 ++++++++++++++++++++++++++++++++ lib/fs.js | 16 ++++----- src/node_file.cc | 24 ++++++------- typings/internalBinding/fs.d.ts | 4 +-- 5 files changed, 121 insertions(+), 24 deletions(-) create mode 100644 benchmark/fs/bench-chmodSync.js create mode 100644 benchmark/fs/bench-fchmodSync.js diff --git a/benchmark/fs/bench-chmodSync.js b/benchmark/fs/bench-chmodSync.js new file mode 100644 index 00000000000000..87b45c06b507de --- /dev/null +++ b/benchmark/fs/bench-chmodSync.js @@ -0,0 +1,41 @@ +'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 }) { + switch (type) { + case 'existing': { + for (let i = 0; i < n; i++) { + fs.writeFileSync(tmpdir.resolve(`chmodsync-bench-file-${i}`), 'bench'); + } + + bench.start(); + for (let i = 0; i < n; i++) { + fs.chmodSync(tmpdir.resolve(`chmodsync-bench-file-${i}`), 0o666); + } + bench.end(n); + break; + } + case 'non-existing': + bench.start(); + for (let i = 0; i < n; i++) { + try { + fs.chmodSync(tmpdir.resolve(`chmod-non-existing-file-${i}`), 0o666); + } catch { + // do nothing + } + } + bench.end(n); + break; + default: + new Error('Invalid type'); + } +} diff --git a/benchmark/fs/bench-fchmodSync.js b/benchmark/fs/bench-fchmodSync.js new file mode 100644 index 00000000000000..4ada8a6f5d8bf9 --- /dev/null +++ b/benchmark/fs/bench-fchmodSync.js @@ -0,0 +1,60 @@ +'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(`fchmodsync-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'); + } + + const fds = files.map((x) => { + // Try to open, if not return likely invalid fd (1 << 30) + try { + return fs.openSync(x, 'r'); + } catch { + return 1 << 30; + } + }); + + bench.start(); + for (let i = 0; i < n; i++) { + try { + fs.fchmodSync(fds[i], 0o666); + } catch { + // do nothing + } + } + bench.end(n); + + for (const x of fds) { + try { + fs.closeSync(x); + } catch { + // do nothing + } + } +} diff --git a/lib/fs.js b/lib/fs.js index 61187e8a66ff9a..037bbed9af66a1 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1903,11 +1903,10 @@ function fchmod(fd, mode, callback) { * @returns {void} */ function fchmodSync(fd, mode) { - fd = getValidatedFd(fd); - mode = parseFileMode(mode, 'mode'); - const ctx = {}; - binding.fchmod(fd, mode, undefined, ctx); - handleErrorFromBinding(ctx); + binding.fchmod( + getValidatedFd(fd), + parseFileMode(mode, 'mode'), + ); } /** @@ -1982,9 +1981,10 @@ function chmodSync(path, mode) { path = getValidatedPath(path); mode = parseFileMode(mode, 'mode'); - const ctx = { path }; - binding.chmod(pathModule.toNamespacedPath(path), mode, undefined, ctx); - handleErrorFromBinding(ctx); + binding.chmod( + pathModule.toNamespacedPath(path), + mode, + ); } /** diff --git a/src/node_file.cc b/src/node_file.cc index 9664d90b97cb50..20b91bb9b6e439 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -2517,18 +2517,16 @@ static void Chmod(const FunctionCallbackInfo& args) { CHECK(args[1]->IsInt32()); int mode = args[1].As()->Value(); - FSReqBase* req_wrap_async = GetReqWrap(args, 2); - if (req_wrap_async != nullptr) { // chmod(path, mode, req) + if (argc > 2) { // chmod(path, mode, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 2); FS_ASYNC_TRACE_BEGIN1( UV_FS_CHMOD, req_wrap_async, "path", TRACE_STR_COPY(*path)) AsyncCall(env, req_wrap_async, args, "chmod", UTF8, AfterNoArgs, uv_fs_chmod, *path, mode); - } else { // chmod(path, mode, undefined, ctx) - CHECK_EQ(argc, 4); - FSReqWrapSync req_wrap_sync; + } else { // chmod(path, mode) + FSReqWrapSync req_wrap_sync("chmod", *path); FS_SYNC_TRACE_BEGIN(chmod); - SyncCall(env, args[3], &req_wrap_sync, "chmod", - uv_fs_chmod, *path, mode); + SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_chmod, *path, mode); FS_SYNC_TRACE_END(chmod); } } @@ -2549,17 +2547,15 @@ static void FChmod(const FunctionCallbackInfo& args) { CHECK(args[1]->IsInt32()); const int mode = args[1].As()->Value(); - FSReqBase* req_wrap_async = GetReqWrap(args, 2); - if (req_wrap_async != nullptr) { // fchmod(fd, mode, req) + if (argc > 2) { // fchmod(fd, mode, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 2); FS_ASYNC_TRACE_BEGIN0(UV_FS_FCHMOD, req_wrap_async) AsyncCall(env, req_wrap_async, args, "fchmod", UTF8, AfterNoArgs, uv_fs_fchmod, fd, mode); - } else { // fchmod(fd, mode, undefined, ctx) - CHECK_EQ(argc, 4); - FSReqWrapSync req_wrap_sync; + } else { // fchmod(fd, mode) + FSReqWrapSync req_wrap_sync("fchmod"); FS_SYNC_TRACE_BEGIN(fchmod); - SyncCall(env, args[3], &req_wrap_sync, "fchmod", - uv_fs_fchmod, fd, mode); + SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_fchmod, fd, mode); FS_SYNC_TRACE_END(fchmod); } } diff --git a/typings/internalBinding/fs.d.ts b/typings/internalBinding/fs.d.ts index d129ce78e84115..67d80d1c9dff97 100644 --- a/typings/internalBinding/fs.d.ts +++ b/typings/internalBinding/fs.d.ts @@ -61,7 +61,7 @@ declare namespace InternalFSBinding { function access(path: StringOrBuffer, mode: number, usePromises: typeof kUsePromises): Promise; function chmod(path: string, mode: number, req: FSReqCallback): void; - function chmod(path: string, mode: number, req: undefined, ctx: FSSyncContext): void; + function chmod(path: string, mode: number): void; function chmod(path: string, mode: number, usePromises: typeof kUsePromises): Promise; function chown(path: string, uid: number, gid: number, req: FSReqCallback): void; @@ -76,7 +76,7 @@ declare namespace InternalFSBinding { function copyFile(src: StringOrBuffer, dest: StringOrBuffer, mode: number, usePromises: typeof kUsePromises): Promise; function fchmod(fd: number, mode: number, req: FSReqCallback): void; - function fchmod(fd: number, mode: number, req: undefined, ctx: FSSyncContext): void; + function fchmod(fd: number, mode: number): void; function fchmod(fd: number, mode: number, usePromises: typeof kUsePromises): Promise; function fchown(fd: number, uid: number, gid: number, req: FSReqCallback): void;