From f413971a2bb2c529d155e1e41399d916efa1558c Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Wed, 27 Sep 2023 17:46:32 +0530 Subject: [PATCH 1/7] fs: improve performance of `existsSync` --- src/node_external_reference.h | 5 ++++ src/node_file.cc | 50 ++++++++++++++++++++++++++++++++++- 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/src/node_external_reference.h b/src/node_external_reference.h index ae37094c8e117e..b0dc5fd64385a3 100644 --- a/src/node_external_reference.h +++ b/src/node_external_reference.h @@ -27,6 +27,10 @@ using CFunctionCallbackWithStrings = const v8::FastOneByteString& base); using CFunctionWithUint32 = uint32_t (*)(v8::Local, const uint32_t input); +using CFunctionCallbackWithStringOptions = + bool (*)(v8::Local, const v8::FastOneByteString& input, + // NOLINTNEXTLINE(runtime/references) This is V8 api. + v8::FastApiCallbackOptions& options); // This class manages the external references from the V8 heap // to the C++ addresses in Node.js. @@ -41,6 +45,7 @@ class ExternalReferenceRegistry { V(CFunctionCallbackWithInt64) \ V(CFunctionCallbackWithBool) \ V(CFunctionCallbackWithString) \ + V(CFunctionCallbackWithStringOptions) \ V(CFunctionCallbackWithStrings) \ V(CFunctionWithUint32) \ V(const v8::CFunctionInfo*) \ diff --git a/src/node_file.cc b/src/node_file.cc index b76eb385295836..6dd8f17fc900d6 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -31,6 +31,7 @@ #include "node_stat_watcher.h" #include "permission/permission.h" #include "util-inl.h" +#include "v8-fast-api-calls.h" #include "tracing/trace_event.h" @@ -57,8 +58,11 @@ namespace fs { using v8::Array; using v8::BigInt; +using v8::CFunction; using v8::Context; using v8::EscapableHandleScope; +using v8::FastApiCallbackOptions; +using v8::FastOneByteString; using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; @@ -1095,6 +1099,47 @@ static void ExistsSync(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(err == 0); } +bool FastExistsSync(v8::Local recv, + const FastOneByteString& string, + // NOLINTNEXTLINE(runtime/references) This is V8 api. + FastApiCallbackOptions& options) { + Environment* env = Environment::GetCurrent(recv->GetCreationContextChecked()); + + MaybeStackBuffer path; + + path.AllocateSufficientStorage(string.length + 1); + memcpy(path.out(), string.data, string.length); + path.SetLengthAndZeroTerminate(string.length); + + if (UNLIKELY(!env->permission() + ->is_granted(permission::PermissionScope::kFileSystemRead, + path.ToStringView()))) { + options.fallback = true; + return false; + } + + uv_fs_t req; + auto make = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); }); + FS_SYNC_TRACE_BEGIN(access); + int err = uv_fs_access(nullptr, &req, path.out(), 0, nullptr); + FS_SYNC_TRACE_END(access); + +#ifdef _WIN32 + // In case of an invalid symlink, `uv_fs_access` on win32 + // will **not** return an error and is therefore not enough. + // Double check with `uv_fs_stat()`. + if (err == 0) { + FS_SYNC_TRACE_BEGIN(stat); + err = uv_fs_stat(nullptr, &req, path.out(), nullptr); + FS_SYNC_TRACE_END(stat); + } +#endif // _WIN32 + + return err == 0; +} + +CFunction fast_exists_sync_(CFunction::Make(FastExistsSync)); + // Used to speed up module loading. Returns an array [string, boolean] static void InternalModuleReadJSON(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -3364,7 +3409,8 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, SetMethodNoSideEffect(isolate, target, "accessSync", AccessSync); SetMethod(isolate, target, "close", Close); SetMethod(isolate, target, "closeSync", CloseSync); - SetMethodNoSideEffect(isolate, target, "existsSync", ExistsSync); + SetFastMethodNoSideEffect(isolate, target, "existsSync", + ExistsSync, &fast_exists_sync_); SetMethod(isolate, target, "open", Open); SetMethod(isolate, target, "openSync", OpenSync); SetMethod(isolate, target, "openFileHandle", OpenFileHandle); @@ -3490,6 +3536,8 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(Close); registry->Register(CloseSync); registry->Register(ExistsSync); + registry->Register(FastExistsSync); + registry->Register(fast_exists_sync_.GetTypeInfo()); registry->Register(Open); registry->Register(OpenSync); registry->Register(OpenFileHandle); From dd08af96f25a78cdbd1bae8f231ae85b1ffdc2be Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Wed, 27 Sep 2023 17:47:11 +0530 Subject: [PATCH 2/7] Format --- src/node_external_reference.h | 3 ++- src/node_file.cc | 9 ++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/node_external_reference.h b/src/node_external_reference.h index b0dc5fd64385a3..ae2422f919ad99 100644 --- a/src/node_external_reference.h +++ b/src/node_external_reference.h @@ -28,7 +28,8 @@ using CFunctionCallbackWithStrings = using CFunctionWithUint32 = uint32_t (*)(v8::Local, const uint32_t input); using CFunctionCallbackWithStringOptions = - bool (*)(v8::Local, const v8::FastOneByteString& input, + bool (*)(v8::Local, + const v8::FastOneByteString& input, // NOLINTNEXTLINE(runtime/references) This is V8 api. v8::FastApiCallbackOptions& options); diff --git a/src/node_file.cc b/src/node_file.cc index 6dd8f17fc900d6..e7774f96e3183c 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1111,9 +1111,8 @@ bool FastExistsSync(v8::Local recv, memcpy(path.out(), string.data, string.length); path.SetLengthAndZeroTerminate(string.length); - if (UNLIKELY(!env->permission() - ->is_granted(permission::PermissionScope::kFileSystemRead, - path.ToStringView()))) { + if (UNLIKELY(!env->permission()->is_granted( + permission::PermissionScope::kFileSystemRead, path.ToStringView()))) { options.fallback = true; return false; } @@ -3409,8 +3408,8 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, SetMethodNoSideEffect(isolate, target, "accessSync", AccessSync); SetMethod(isolate, target, "close", Close); SetMethod(isolate, target, "closeSync", CloseSync); - SetFastMethodNoSideEffect(isolate, target, "existsSync", - ExistsSync, &fast_exists_sync_); + SetFastMethodNoSideEffect( + isolate, target, "existsSync", ExistsSync, &fast_exists_sync_); SetMethod(isolate, target, "open", Open); SetMethod(isolate, target, "openSync", OpenSync); SetMethod(isolate, target, "openFileHandle", OpenFileHandle); From 69736c27795ba73243b1a1ed89cf1f8f5416b5c5 Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Wed, 27 Sep 2023 19:33:40 +0530 Subject: [PATCH 3/7] Add test for fast call --- test/parallel/test-fs-exists.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/parallel/test-fs-exists.js b/test/parallel/test-fs-exists.js index 857f3f26174549..2d1d45c1ccf4f4 100644 --- a/test/parallel/test-fs-exists.js +++ b/test/parallel/test-fs-exists.js @@ -54,3 +54,11 @@ assert(!fs.existsSync(`${f}-NO`)); assert(!fs.existsSync()); assert(!fs.existsSync({})); assert(!fs.existsSync(new URL('https://foo'))); + +{ + // This test is to ensure that the v8 fast api works. + const oneBytePath = 'hello.txt' + for (let i = 0; i < 1e5; i++) { + assert(!fs.existsSync(oneBytePath)); + } +} From 176cbdc6abc2b3c8b655359f656ec0e36cd32d03 Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Wed, 27 Sep 2023 19:38:58 +0530 Subject: [PATCH 4/7] Fix lint js --- test/parallel/test-fs-exists.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-fs-exists.js b/test/parallel/test-fs-exists.js index 2d1d45c1ccf4f4..c0eb8a0e21f48e 100644 --- a/test/parallel/test-fs-exists.js +++ b/test/parallel/test-fs-exists.js @@ -57,7 +57,7 @@ assert(!fs.existsSync(new URL('https://foo'))); { // This test is to ensure that the v8 fast api works. - const oneBytePath = 'hello.txt' + const oneBytePath = 'hello.txt'; for (let i = 0; i < 1e5; i++) { assert(!fs.existsSync(oneBytePath)); } From 8d331edfd506e44c64c1dbbbee4c9507a740fb94 Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Wed, 27 Sep 2023 19:42:29 +0530 Subject: [PATCH 5/7] Add test for true existsSync --- test/parallel/test-fs-exists.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/parallel/test-fs-exists.js b/test/parallel/test-fs-exists.js index c0eb8a0e21f48e..ad939d6fb5899a 100644 --- a/test/parallel/test-fs-exists.js +++ b/test/parallel/test-fs-exists.js @@ -60,5 +60,6 @@ assert(!fs.existsSync(new URL('https://foo'))); const oneBytePath = 'hello.txt'; for (let i = 0; i < 1e5; i++) { assert(!fs.existsSync(oneBytePath)); + assert(fs.existsSync(__filename)); } } From 7a37e48b220ba85bcc1cc09a1ca6184e684f4ae7 Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Wed, 27 Sep 2023 21:14:32 +0530 Subject: [PATCH 6/7] Use SetFastMethod --- src/node_file.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index 5942eca6174425..771032985fc887 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3429,8 +3429,8 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, SetMethod(isolate, target, "accessSync", AccessSync); SetMethod(isolate, target, "close", Close); SetMethod(isolate, target, "closeSync", CloseSync); - SetFastMethodNoSideEffect( - isolate, target, "existsSync", ExistsSync, &fast_exists_sync_); + SetFastMethod(isolate, target, "existsSync", + ExistsSync, &fast_exists_sync_); SetMethod(isolate, target, "open", Open); SetMethod(isolate, target, "openSync", OpenSync); SetMethod(isolate, target, "openFileHandle", OpenFileHandle); From e2e734196b4a46f3ef132ec0242afb1c5830bac9 Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Wed, 27 Sep 2023 21:19:40 +0530 Subject: [PATCH 7/7] Format --- src/node_file.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index 771032985fc887..43e4145e6193ab 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -3429,8 +3429,7 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, SetMethod(isolate, target, "accessSync", AccessSync); SetMethod(isolate, target, "close", Close); SetMethod(isolate, target, "closeSync", CloseSync); - SetFastMethod(isolate, target, "existsSync", - ExistsSync, &fast_exists_sync_); + SetFastMethod(isolate, target, "existsSync", ExistsSync, &fast_exists_sync_); SetMethod(isolate, target, "open", Open); SetMethod(isolate, target, "openSync", OpenSync); SetMethod(isolate, target, "openFileHandle", OpenFileHandle);