From 70a89315917167f8c36f38984c55071932d67424 Mon Sep 17 00:00:00 2001 From: pluris Date: Thu, 28 Sep 2023 00:01:01 +0900 Subject: [PATCH 1/4] fs: improve error performance for `fdatasyncSync` --- lib/fs.js | 5 +---- src/node_file.cc | 38 +++++++++++++++++++++++++++++++++ typings/internalBinding/fs.d.ts | 1 + 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index d682759d8b0a14..90a18facc72f8b 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1303,10 +1303,7 @@ function fdatasync(fd, callback) { * @returns {void} */ function fdatasyncSync(fd) { - fd = getValidatedFd(fd); - const ctx = {}; - binding.fdatasync(fd, undefined, ctx); - handleErrorFromBinding(ctx); + syncFs.fdatasync(fd); } /** diff --git a/src/node_file.cc b/src/node_file.cc index 9430aff5c29a37..03ce77d14135e3 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -116,6 +116,24 @@ inline int64_t GetOffset(Local value) { return IsSafeJsInt(value) ? value.As()->Value() : -1; } +inline int GetValidatedFd(Environment* env, Local value) { + if (!value->IsInt32()) { + env->isolate()->ThrowException(ERR_INVALID_ARG_TYPE( + env->isolate(), "Invalid argument. The fd must be int32.")); + return 1 << 30; + } + + const int fd = value.As()->Value(); + + if (fd < 0 || fd > INT32_MAX) { + env->isolate()->ThrowException(ERR_OUT_OF_RANGE( + env->isolate(), "It must be >= 0 && <= INT32_MAX. Received %d", fd)); + return 1 << 30; + } + + return fd; +} + static const char* get_fs_func_name_by_type(uv_fs_type req_type) { switch (req_type) { #define FS_TYPE_TO_NAME(type, name) \ @@ -1520,6 +1538,24 @@ static void Fdatasync(const FunctionCallbackInfo& args) { } } +static void FdatasyncSync(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + + CHECK_EQ(args.Length(), 1); + + const int fd = GetValidatedFd(env, args[0]); + if (fd == (1 << 30)) return; + + uv_fs_t req; + auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); }); + FS_SYNC_TRACE_BEGIN(fdatasync); + int err = uv_fs_fdatasync(nullptr, &req, fd, nullptr); + FS_SYNC_TRACE_END(fdatasync); + if (err < 0) { + return env->ThrowUVException(err, "fdatasync"); + } +} + static void Fsync(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -3218,6 +3254,7 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, SetMethod(isolate, target, "readFileUtf8", ReadFileUtf8); SetMethod(isolate, target, "readBuffers", ReadBuffers); SetMethod(isolate, target, "fdatasync", Fdatasync); + SetMethod(isolate, target, "fdatasyncSync", FdatasyncSync); SetMethod(isolate, target, "fsync", Fsync); SetMethod(isolate, target, "rename", Rename); SetMethod(isolate, target, "ftruncate", FTruncate); @@ -3337,6 +3374,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(ReadFileUtf8); registry->Register(ReadBuffers); registry->Register(Fdatasync); + registry->Register(FdatasyncSync); registry->Register(Fsync); registry->Register(Rename); registry->Register(FTruncate); diff --git a/typings/internalBinding/fs.d.ts b/typings/internalBinding/fs.d.ts index d87b326e42efc8..d0f641aaecb714 100644 --- a/typings/internalBinding/fs.d.ts +++ b/typings/internalBinding/fs.d.ts @@ -84,6 +84,7 @@ declare namespace InternalFSBinding { function fdatasync(fd: number, req: FSReqCallback): void; function fdatasync(fd: number, req: undefined, ctx: FSSyncContext): void; function fdatasync(fd: number, usePromises: typeof kUsePromises): Promise; + function fdatasyncSync(fd: number): void; function fstat(fd: number, useBigint: boolean, req: FSReqCallback): void; function fstat(fd: number, useBigint: true, req: FSReqCallback): void; From 78b27c081e579b6eb0f1c35582b708a94aae1da5 Mon Sep 17 00:00:00 2001 From: pluris Date: Mon, 2 Oct 2023 00:53:43 +0900 Subject: [PATCH 2/4] apply #49913 --- lib/fs.js | 2 +- src/node_file.cc | 37 +++++++-------------------------- typings/internalBinding/fs.d.ts | 2 +- 3 files changed, 10 insertions(+), 31 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 90a18facc72f8b..ed26eb74c52ca8 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1303,7 +1303,7 @@ function fdatasync(fd, callback) { * @returns {void} */ function fdatasyncSync(fd) { - syncFs.fdatasync(fd); + return binding.fdatasync(fd); } /** diff --git a/src/node_file.cc b/src/node_file.cc index 03ce77d14135e3..8e4a42677c823e 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1519,43 +1519,24 @@ static void Fdatasync(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); const int argc = args.Length(); - CHECK_GE(argc, 2); + CHECK_GE(argc, 1); - CHECK(args[0]->IsInt32()); - const int fd = args[0].As()->Value(); + const int fd = GetValidatedFd(env, args[0]); - FSReqBase* req_wrap_async = GetReqWrap(args, 1); - if (req_wrap_async != nullptr) { + if (argc > 1) { // fdatasync(fd, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 1); + CHECK_NOT_NULL(req_wrap_async); FS_ASYNC_TRACE_BEGIN0(UV_FS_FDATASYNC, req_wrap_async) AsyncCall(env, req_wrap_async, args, "fdatasync", UTF8, AfterNoArgs, uv_fs_fdatasync, fd); - } else { - CHECK_EQ(argc, 3); - FSReqWrapSync req_wrap_sync; + } else { // fdatasync(fd) + FSReqWrapSync req_wrap_sync("fdatasync"); FS_SYNC_TRACE_BEGIN(fdatasync); - SyncCall(env, args[2], &req_wrap_sync, "fdatasync", uv_fs_fdatasync, fd); + SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_fdatasync, fd); FS_SYNC_TRACE_END(fdatasync); } } -static void FdatasyncSync(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - - CHECK_EQ(args.Length(), 1); - - const int fd = GetValidatedFd(env, args[0]); - if (fd == (1 << 30)) return; - - uv_fs_t req; - auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); }); - FS_SYNC_TRACE_BEGIN(fdatasync); - int err = uv_fs_fdatasync(nullptr, &req, fd, nullptr); - FS_SYNC_TRACE_END(fdatasync); - if (err < 0) { - return env->ThrowUVException(err, "fdatasync"); - } -} - static void Fsync(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -3254,7 +3235,6 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, SetMethod(isolate, target, "readFileUtf8", ReadFileUtf8); SetMethod(isolate, target, "readBuffers", ReadBuffers); SetMethod(isolate, target, "fdatasync", Fdatasync); - SetMethod(isolate, target, "fdatasyncSync", FdatasyncSync); SetMethod(isolate, target, "fsync", Fsync); SetMethod(isolate, target, "rename", Rename); SetMethod(isolate, target, "ftruncate", FTruncate); @@ -3374,7 +3354,6 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(ReadFileUtf8); registry->Register(ReadBuffers); registry->Register(Fdatasync); - registry->Register(FdatasyncSync); registry->Register(Fsync); registry->Register(Rename); registry->Register(FTruncate); diff --git a/typings/internalBinding/fs.d.ts b/typings/internalBinding/fs.d.ts index d0f641aaecb714..5977ad5d17206d 100644 --- a/typings/internalBinding/fs.d.ts +++ b/typings/internalBinding/fs.d.ts @@ -84,7 +84,7 @@ declare namespace InternalFSBinding { function fdatasync(fd: number, req: FSReqCallback): void; function fdatasync(fd: number, req: undefined, ctx: FSSyncContext): void; function fdatasync(fd: number, usePromises: typeof kUsePromises): Promise; - function fdatasyncSync(fd: number): void; + function fdatasync(fd: number): void; function fstat(fd: number, useBigint: boolean, req: FSReqCallback): void; function fstat(fd: number, useBigint: true, req: FSReqCallback): void; From 4c0958cc0bf1a4f09631526b14b1b74619f1668b Mon Sep 17 00:00:00 2001 From: pluris Date: Mon, 2 Oct 2023 01:14:03 +0900 Subject: [PATCH 3/4] add benchmark for fdatasyncSync --- benchmark/fs/bench_fdatasyncSync.js | 42 +++++++++++++++++++++++++++++ src/node_file.cc | 1 + 2 files changed, 43 insertions(+) create mode 100644 benchmark/fs/bench_fdatasyncSync.js diff --git a/benchmark/fs/bench_fdatasyncSync.js b/benchmark/fs/bench_fdatasyncSync.js new file mode 100644 index 00000000000000..9aedb0a314fb24 --- /dev/null +++ b/benchmark/fs/bench_fdatasyncSync.js @@ -0,0 +1,42 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); +const tmpdir = require('../../test/common/tmpdir'); +tmpdir.refresh(); + +const tmpfile = tmpdir.resolve(`.existing-file-${process.pid}`); +fs.writeFileSync(tmpfile, 'this-is-for-a-benchmark', 'utf8'); + +const bench = common.createBenchmark(main, { + type: ['existing', 'non-existing'], + n: [1e4], +}); + +function main({ n, type }) { + let fd; + + switch (type) { + case 'existing': + fd = fs.openSync(tmpfile, 'r', 0o666); + break; + case 'non-existing': + fd = 1 << 30; + break; + default: + new Error('Invalid type'); + } + + bench.start(); + for (let i = 0; i < n; i++) { + try { + fs.fdatasyncSync(fd); + } catch { + // do nothing + } + } + + bench.end(n); + + if (type === 'existing') fs.closeSync(fd); +} diff --git a/src/node_file.cc b/src/node_file.cc index 8e4a42677c823e..1a7a56549223b9 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1522,6 +1522,7 @@ static void Fdatasync(const FunctionCallbackInfo& args) { CHECK_GE(argc, 1); const int fd = GetValidatedFd(env, args[0]); + if (fd == (1 << 30)) return; if (argc > 1) { // fdatasync(fd, req) FSReqBase* req_wrap_async = GetReqWrap(args, 1); From bba51b3995fa40c6d9f57d8c611a47b352427f5c Mon Sep 17 00:00:00 2001 From: pluris Date: Mon, 2 Oct 2023 23:04:03 +0900 Subject: [PATCH 4/4] src: remove GetValidateFd() --- lib/fs.js | 1 + src/node_file.cc | 22 ++-------------------- 2 files changed, 3 insertions(+), 20 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index ed26eb74c52ca8..9a60da22801a27 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1303,6 +1303,7 @@ function fdatasync(fd, callback) { * @returns {void} */ function fdatasyncSync(fd) { + fd = getValidatedFd(fd); return binding.fdatasync(fd); } diff --git a/src/node_file.cc b/src/node_file.cc index 1a7a56549223b9..7c2507ce30593b 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -116,24 +116,6 @@ inline int64_t GetOffset(Local value) { return IsSafeJsInt(value) ? value.As()->Value() : -1; } -inline int GetValidatedFd(Environment* env, Local value) { - if (!value->IsInt32()) { - env->isolate()->ThrowException(ERR_INVALID_ARG_TYPE( - env->isolate(), "Invalid argument. The fd must be int32.")); - return 1 << 30; - } - - const int fd = value.As()->Value(); - - if (fd < 0 || fd > INT32_MAX) { - env->isolate()->ThrowException(ERR_OUT_OF_RANGE( - env->isolate(), "It must be >= 0 && <= INT32_MAX. Received %d", fd)); - return 1 << 30; - } - - return fd; -} - static const char* get_fs_func_name_by_type(uv_fs_type req_type) { switch (req_type) { #define FS_TYPE_TO_NAME(type, name) \ @@ -1521,8 +1503,8 @@ static void Fdatasync(const FunctionCallbackInfo& args) { const int argc = args.Length(); CHECK_GE(argc, 1); - const int fd = GetValidatedFd(env, args[0]); - if (fd == (1 << 30)) return; + CHECK(args[0]->IsInt32()); + const int fd = args[0].As()->Value(); if (argc > 1) { // fdatasync(fd, req) FSReqBase* req_wrap_async = GetReqWrap(args, 1);