From 841367078ef2b0aa9192338561237f9439bb7502 Mon Sep 17 00:00:00 2001 From: CanadaHonk Date: Mon, 25 Sep 2023 21:24:21 +0100 Subject: [PATCH] fs: improve error perf of sync `*times` PR-URL: https://github.com/nodejs/node/pull/49864 Refs: https://github.com/nodejs/performance/issues/106 Reviewed-By: Yagiz Nizipli Reviewed-By: Stephen Belanger Reviewed-By: Joyee Cheung --- benchmark/fs/bench-timesSync.js | 61 +++++++++++++++++++++++++++++++++ lib/fs.js | 32 ++++++++--------- src/node_file.cc | 39 ++++++++++----------- typings/internalBinding/fs.d.ts | 6 ++-- 4 files changed, 97 insertions(+), 41 deletions(-) create mode 100644 benchmark/fs/bench-timesSync.js diff --git a/benchmark/fs/bench-timesSync.js b/benchmark/fs/bench-timesSync.js new file mode 100644 index 00000000000000..8ea879a63ef9b7 --- /dev/null +++ b/benchmark/fs/bench-timesSync.js @@ -0,0 +1,61 @@ +'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'], + func: [ 'utimes', 'futimes', 'lutimes' ], + n: [1e3], +}); + +function main({ n, type, func }) { + const useFds = func === 'futimes'; + const fsFunc = fs[func + 'Sync']; + + switch (type) { + case 'existing': { + const files = []; + + // Populate tmpdir with mock files + for (let i = 0; i < n; i++) { + const path = tmpdir.resolve(`timessync-bench-file-${i}`); + fs.writeFileSync(path, 'bench'); + files.push(useFds ? fs.openSync(path, 'r+') : path); + } + + bench.start(); + for (let i = 0; i < n; i++) { + fsFunc(files[i], i, i); + } + bench.end(n); + + if (useFds) files.forEach((x) => { + try { + fs.closeSync(x); + } catch { + // do nothing + } + }); + + break; + } + case 'non-existing': { + bench.start(); + for (let i = 0; i < n; i++) { + try { + fsFunc(useFds ? (1 << 30) : tmpdir.resolve(`.non-existing-file-${Date.now()}`), i, i); + } catch { + // do nothing + } + } + bench.end(n); + + break; + } + default: + new Error('Invalid type'); + } +} diff --git a/lib/fs.js b/lib/fs.js index de1fd1f50929f5..a37bfe62de00a8 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -2123,11 +2123,11 @@ function utimes(path, atime, mtime, callback) { */ function utimesSync(path, atime, mtime) { path = getValidatedPath(path); - const ctx = { path }; - binding.utimes(pathModule.toNamespacedPath(path), - toUnixTimestamp(atime), toUnixTimestamp(mtime), - undefined, ctx); - handleErrorFromBinding(ctx); + binding.utimes( + pathModule.toNamespacedPath(path), + toUnixTimestamp(atime), + toUnixTimestamp(mtime), + ); } /** @@ -2160,12 +2160,11 @@ function futimes(fd, atime, mtime, callback) { * @returns {void} */ function futimesSync(fd, atime, mtime) { - fd = getValidatedFd(fd); - atime = toUnixTimestamp(atime, 'atime'); - mtime = toUnixTimestamp(mtime, 'mtime'); - const ctx = {}; - binding.futimes(fd, atime, mtime, undefined, ctx); - handleErrorFromBinding(ctx); + binding.futimes( + getValidatedFd(fd), + toUnixTimestamp(atime, 'atime'), + toUnixTimestamp(mtime, 'mtime'), + ); } /** @@ -2199,12 +2198,11 @@ function lutimes(path, atime, mtime, callback) { */ function lutimesSync(path, atime, mtime) { path = getValidatedPath(path); - const ctx = { path }; - binding.lutimes(pathModule.toNamespacedPath(path), - toUnixTimestamp(atime), - toUnixTimestamp(mtime), - undefined, ctx); - handleErrorFromBinding(ctx); + binding.lutimes( + pathModule.toNamespacedPath(path), + toUnixTimestamp(atime), + toUnixTimestamp(mtime), + ); } function writeAll(fd, isUserFd, buffer, offset, length, signal, flush, callback) { diff --git a/src/node_file.cc b/src/node_file.cc index c79d3cc9da1b22..ce89c2b4dba0c7 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -2721,18 +2721,17 @@ static void UTimes(const FunctionCallbackInfo& args) { CHECK(args[2]->IsNumber()); const double mtime = args[2].As()->Value(); - FSReqBase* req_wrap_async = GetReqWrap(args, 3); - if (req_wrap_async != nullptr) { // utimes(path, atime, mtime, req) + if (argc > 3) { // utimes(path, atime, mtime, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 3); FS_ASYNC_TRACE_BEGIN1( UV_FS_UTIME, req_wrap_async, "path", TRACE_STR_COPY(*path)) AsyncCall(env, req_wrap_async, args, "utime", UTF8, AfterNoArgs, uv_fs_utime, *path, atime, mtime); - } else { // utimes(path, atime, mtime, undefined, ctx) - CHECK_EQ(argc, 5); - FSReqWrapSync req_wrap_sync; + } else { // utimes(path, atime, mtime) + FSReqWrapSync req_wrap_sync("utime", *path); FS_SYNC_TRACE_BEGIN(utimes); - SyncCall(env, args[4], &req_wrap_sync, "utime", - uv_fs_utime, *path, atime, mtime); + SyncCallAndThrowOnError( + env, &req_wrap_sync, uv_fs_utime, *path, atime, mtime); FS_SYNC_TRACE_END(utimes); } } @@ -2752,17 +2751,16 @@ static void FUTimes(const FunctionCallbackInfo& args) { CHECK(args[2]->IsNumber()); const double mtime = args[2].As()->Value(); - FSReqBase* req_wrap_async = GetReqWrap(args, 3); - if (req_wrap_async != nullptr) { // futimes(fd, atime, mtime, req) + if (argc > 3) { // futimes(fd, atime, mtime, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 3); FS_ASYNC_TRACE_BEGIN0(UV_FS_FUTIME, req_wrap_async) AsyncCall(env, req_wrap_async, args, "futime", UTF8, AfterNoArgs, uv_fs_futime, fd, atime, mtime); - } else { // futimes(fd, atime, mtime, undefined, ctx) - CHECK_EQ(argc, 5); - FSReqWrapSync req_wrap_sync; + } else { // futimes(fd, atime, mtime) + FSReqWrapSync req_wrap_sync("futime"); FS_SYNC_TRACE_BEGIN(futimes); - SyncCall(env, args[4], &req_wrap_sync, "futime", - uv_fs_futime, fd, atime, mtime); + SyncCallAndThrowOnError( + env, &req_wrap_sync, uv_fs_futime, fd, atime, mtime); FS_SYNC_TRACE_END(futimes); } } @@ -2784,18 +2782,17 @@ static void LUTimes(const FunctionCallbackInfo& args) { CHECK(args[2]->IsNumber()); const double mtime = args[2].As()->Value(); - FSReqBase* req_wrap_async = GetReqWrap(args, 3); - if (req_wrap_async != nullptr) { // lutimes(path, atime, mtime, req) + if (argc > 3) { // lutimes(path, atime, mtime, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 3); FS_ASYNC_TRACE_BEGIN1( UV_FS_LUTIME, req_wrap_async, "path", TRACE_STR_COPY(*path)) AsyncCall(env, req_wrap_async, args, "lutime", UTF8, AfterNoArgs, uv_fs_lutime, *path, atime, mtime); - } else { // lutimes(path, atime, mtime, undefined, ctx) - CHECK_EQ(argc, 5); - FSReqWrapSync req_wrap_sync; + } else { // lutimes(path, atime, mtime) + FSReqWrapSync req_wrap_sync("lutime", *path); FS_SYNC_TRACE_BEGIN(lutimes); - SyncCall(env, args[4], &req_wrap_sync, "lutime", - uv_fs_lutime, *path, atime, mtime); + SyncCallAndThrowOnError( + env, &req_wrap_sync, uv_fs_lutime, *path, atime, mtime); FS_SYNC_TRACE_END(lutimes); } } diff --git a/typings/internalBinding/fs.d.ts b/typings/internalBinding/fs.d.ts index b4c4cd6e79a91c..d129ce78e84115 100644 --- a/typings/internalBinding/fs.d.ts +++ b/typings/internalBinding/fs.d.ts @@ -107,7 +107,7 @@ declare namespace InternalFSBinding { function ftruncate(fd: number, len: number, usePromises: typeof kUsePromises): Promise; function futimes(fd: number, atime: number, mtime: number, req: FSReqCallback): void; - function futimes(fd: number, atime: number, mtime: number, req: undefined, ctx: FSSyncContext): void; + function futimes(fd: number, atime: number, mtime: number): void; function futimes(fd: number, atime: number, mtime: number, usePromises: typeof kUsePromises): Promise; function internalModuleReadJSON(path: string): [] | [string, boolean]; @@ -132,7 +132,7 @@ declare namespace InternalFSBinding { function lstat(path: StringOrBuffer, useBigint: false, usePromises: typeof kUsePromises): Promise; function lutimes(path: string, atime: number, mtime: number, req: FSReqCallback): void; - function lutimes(path: string, atime: number, mtime: number, req: undefined, ctx: FSSyncContext): void; + function lutimes(path: string, atime: number, mtime: number): void; function lutimes(path: string, atime: number, mtime: number, usePromises: typeof kUsePromises): Promise; function mkdtemp(prefix: string, encoding: unknown, req: FSReqCallback): void; @@ -207,7 +207,7 @@ declare namespace InternalFSBinding { function unlink(path: string, usePromises: typeof kUsePromises): Promise; function utimes(path: string, atime: number, mtime: number, req: FSReqCallback): void; - function utimes(path: string, atime: number, mtime: number, req: undefined, ctx: FSSyncContext): void; + function utimes(path: string, atime: number, mtime: number): void; function utimes(path: string, atime: number, mtime: number, usePromises: typeof kUsePromises): Promise; function writeBuffer(fd: number, buffer: ArrayBufferView, offset: number, length: number, position: number | null, req: FSReqCallback): void;