Skip to content

[v22.x backport] src: backport fast API changes #59065

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aduh95 merged 3 commits into from
Jul 28, 2025
Merged

[v22.x backport] src: backport fast API changes #59065

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
390654e
src: fix internalModuleStat v8 fast path
anonrig Apr 29, 2025
a381b4d
src: remove fast API for InternalModuleStat
joyeecheung Jun 3, 2025
6905258
src: simplify adding fast APIs to ExternalReferenceRegistry
Renegade334 Jul 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
src: remove fast API for InternalModuleStat 
There are several motivation for removing this:

1. The implementation does not align with InternalModuleStat,
   most noticably it does not namespace the path or convert
   it to UTF-16 before using it with std::filesystem::path
   on Windows which could crash on non-English locale.
2. It needs the Environment - if not for decoding the string,
   at least for env->exec_path() to resolve the path for
   namespacing - and therefore needs a handle to the Context
   which requires a handle scope which actually makes the
   fast API version slower than the normal binding.

For simplicity this just removes the fast API to fix the bug and
improve the performance.

PR-URL: #58489
Backport-PR-URL: #59065
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
  • Loading branch information
@joyeecheung @aduh95
joyeecheung authored and aduh95 committed Jul 28, 2025
commit a381b4d990995f0c0d609e2b284899202ee70cf7
42 changes: 1 addition & 41 deletions src/node_file.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ using v8::Array;
using v8::BigInt;
using v8::Context;
using v8::EscapableHandleScope;
using v8::FastApiCallbackOptions;
using v8::FunctionCallbackInfo;
using v8::FunctionTemplate;
using v8::HandleScope;
Expand Down Expand Up @@ -1067,39 +1066,6 @@ static void InternalModuleStat(const FunctionCallbackInfo<Value>& args) {
args.GetReturnValue().Set(rc);
}

static int32_t FastInternalModuleStat(
Local<Value> recv,
Local<Value> input_,
// NOLINTNEXTLINE(runtime/references) This is V8 api.
FastApiCallbackOptions& options) {
// This needs a HandleScope which needs an isolate.
Isolate* isolate = Isolate::TryGetCurrent();
if (!isolate) {
options.fallback = true;
return -1;
}

TRACK_V8_FAST_API_CALL("fs.internalModuleStat");
HandleScope scope(isolate);

CHECK(input_->IsString());
Utf8Value input(isolate, input_.As<String>());

auto path = std::filesystem::path(input.ToStringView());

switch (std::filesystem::status(path).type()) {
case std::filesystem::file_type::directory:
return 1;
case std::filesystem::file_type::regular:
return 0;
default:
return -1;
}
}

v8::CFunction fast_internal_module_stat_(
v8::CFunction::Make(FastInternalModuleStat));

constexpr bool is_uv_error_except_no_entry(int result) {
return result < 0 && result != UV_ENOENT;
}
Expand Down Expand Up @@ -3819,11 +3785,7 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data,
SetMethod(isolate, target, "rmdir", RMDir);
SetMethod(isolate, target, "mkdir", MKDir);
SetMethod(isolate, target, "readdir", ReadDir);
SetFastMethod(isolate,
target,
"internalModuleStat",
InternalModuleStat,
&fast_internal_module_stat_);
SetMethod(isolate, target, "internalModuleStat", InternalModuleStat);
SetMethod(isolate, target, "stat", Stat);
SetMethod(isolate, target, "lstat", LStat);
SetMethod(isolate, target, "fstat", FStat);
Expand Down Expand Up @@ -3948,8 +3910,6 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
registry->Register(MKDir);
registry->Register(ReadDir);
registry->Register(InternalModuleStat);
registry->Register(FastInternalModuleStat);
registry->Register(fast_internal_module_stat_.GetTypeInfo());
registry->Register(Stat);
registry->Register(LStat);
registry->Register(FStat);
Expand Down
16 changes: 0 additions & 16 deletions test/parallel/test-permission-fs-internal-module-stat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,3 @@ const blockedFile = fixtures.path('permission', 'deny', 'protected-file.md');
const internalFsBinding = internalBinding('fs');

strictEqual(internalFsBinding.internalModuleStat(blockedFile), 0);

// Only javascript methods can be optimized through %OptimizeFunctionOnNextCall
// This is why we surround the C++ method we want to optimize with a JS function.
function testFastPaths(file) {
return internalFsBinding.internalModuleStat(file);
}

eval('%PrepareFunctionForOptimization(testFastPaths)');
testFastPaths(blockedFile);
eval('%OptimizeFunctionOnNextCall(testFastPaths)');
strictEqual(testFastPaths(blockedFile), 0);

if (common.isDebug) {
const { getV8FastApiCallCount } = internalBinding('debug');
strictEqual(getV8FastApiCallCount('fs.internalModuleStat'), 1);
}