From 95b10d6039de93825906e4c003b7b699c3fcb41c Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Wed, 5 Jul 2023 10:44:39 -0400 Subject: [PATCH 1/5] fs: add a fast-path for readFileSync utf-8 --- benchmark/fs/readFileSync.js | 17 +++-- lib/fs.js | 10 ++- .../context.js} | 0 lib/internal/fs/read-file/utf8.js | 24 +++++++ src/node_file.cc | 62 +++++++++++++++++++ test/parallel/test-bootstrap-modules.js | 1 + 6 files changed, 109 insertions(+), 5 deletions(-) rename lib/internal/fs/{read_file_context.js => read-file/context.js} (100%) create mode 100644 lib/internal/fs/read-file/utf8.js diff --git a/benchmark/fs/readFileSync.js b/benchmark/fs/readFileSync.js index 8cf3b4c28fb25c..b81bdce8f27f69 100644 --- a/benchmark/fs/readFileSync.js +++ b/benchmark/fs/readFileSync.js @@ -4,12 +4,21 @@ const common = require('../common.js'); const fs = require('fs'); const bench = common.createBenchmark(main, { - n: [60e4], + encoding: ['undefined', 'utf8'], + path: ['existing', 'non-existing'], + n: [60e1], }); -function main({ n }) { +function main({ n, encoding, path }) { + const enc = encoding === 'undefined' ? undefined : encoding; + const file = path === 'existing' ? __filename : '/tmp/not-found'; bench.start(); - for (let i = 0; i < n; ++i) - fs.readFileSync(__filename); + for (let i = 0; i < n; ++i) { + try { + fs.readFileSync(file, enc); + } catch { + // do nothing + } + } bench.end(n); } diff --git a/lib/fs.js b/lib/fs.js index c9896e45fcfe64..16157baaedc064 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -142,6 +142,7 @@ const { validateObject, validateString, } = require('internal/validators'); +const { readFileSyncUtf8 } = require('internal/fs/read-file/utf8'); let truncateWarn = true; let fs; @@ -380,7 +381,7 @@ function checkAborted(signal, callback) { function readFile(path, options, callback) { callback = maybeCallback(callback || options); options = getOptions(options, { flag: 'r' }); - const ReadFileContext = require('internal/fs/read_file_context'); + const ReadFileContext = require('internal/fs/read-file/context'); const context = new ReadFileContext(callback, options.encoding); context.isUserFd = isFd(path); // File descriptor ownership @@ -457,7 +458,14 @@ function tryReadSync(fd, isUserFd, buffer, pos, len) { */ function readFileSync(path, options) { options = getOptions(options, { flag: 'r' }); + const isUserFd = isFd(path); // File descriptor ownership + + // TODO: Do not handle file descriptor ownership for now. + if (!isUserFd && options.encoding === 'utf8') { + return readFileSyncUtf8(getValidatedPath(path), stringToFlags(options.flag)); + } + const fd = isUserFd ? path : fs.openSync(path, options.flag, 0o666); const stats = tryStatSync(fd, isUserFd); diff --git a/lib/internal/fs/read_file_context.js b/lib/internal/fs/read-file/context.js similarity index 100% rename from lib/internal/fs/read_file_context.js rename to lib/internal/fs/read-file/context.js diff --git a/lib/internal/fs/read-file/utf8.js b/lib/internal/fs/read-file/utf8.js new file mode 100644 index 00000000000000..e916c918c11190 --- /dev/null +++ b/lib/internal/fs/read-file/utf8.js @@ -0,0 +1,24 @@ +'use strict'; + +const { handleErrorFromBinding } = require('internal/fs/utils'); + +const binding = internalBinding('fs'); + +/** + * @param {string} path + * @param {number} flag + * @return {string} + */ +function readFileSyncUtf8(path, flag) { + const response = binding.readFileSync(path, flag); + + if (typeof response === 'string') { + return response; + } + + handleErrorFromBinding({ errno: response, path }); +} + +module.exports = { + readFileSyncUtf8, +}; diff --git a/src/node_file.cc b/src/node_file.cc index 49674385297c7a..945cf4beeda475 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1994,6 +1994,66 @@ static inline Maybe CheckOpenPermissions(Environment* env, return JustVoid(); } +static void ReadFileSync(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + + CHECK_GE(args.Length(), 2); + + BufferValue path(env->isolate(), args[0]); + CHECK_NOT_NULL(*path); + + CHECK(args[1]->IsInt32()); + const int flags = args[1].As()->Value(); + + if (CheckOpenPermissions(env, path, flags).IsNothing()) return; + + uv_fs_t req; + auto defer_req_cleanup = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); }); + + FS_SYNC_TRACE_BEGIN(open); + uv_file file = uv_fs_open(nullptr, &req, *path, flags, 438, nullptr); + FS_SYNC_TRACE_END(open); + if (req.result < 0) { + // req will be cleaned up by scope leave. + return args.GetReturnValue().Set( + v8::Integer::New(env->isolate(), req.result)); + } + uv_fs_req_cleanup(&req); + + auto defer_close = OnScopeLeave([file]() { + uv_fs_t close_req; + CHECK_EQ(0, uv_fs_close(nullptr, &close_req, file, nullptr)); + uv_fs_req_cleanup(&close_req); + }); + + std::string result{}; + char buffer[8192]; + uv_buf_t buf = uv_buf_init(buffer, sizeof(buffer)); + + FS_SYNC_TRACE_BEGIN(read); + while (true) { + auto r = uv_fs_read(nullptr, &req, file, &buf, 1, result.length(), nullptr); + if (req.result < 0) { + FS_SYNC_TRACE_END(read); + // req will be cleaned up by scope leave. + return args.GetReturnValue().Set( + v8::Integer::New(env->isolate(), req.result)); + } + uv_fs_req_cleanup(&req); + if (r <= 0) { + break; + } + result.append(buf.base, r); + } + FS_SYNC_TRACE_END(read); + + args.GetReturnValue().Set(String::NewFromUtf8(env->isolate(), + result.data(), + v8::NewStringType::kNormal, + result.size()) + .ToLocalChecked()); +} + static void Open(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -3149,6 +3209,7 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data, SetMethod(isolate, target, "stat", Stat); SetMethod(isolate, target, "lstat", LStat); SetMethod(isolate, target, "fstat", FStat); + SetMethodNoSideEffect(isolate, target, "readFileSync", ReadFileSync); SetMethod(isolate, target, "statfs", StatFs); SetMethod(isolate, target, "link", Link); SetMethod(isolate, target, "symlink", Symlink); @@ -3266,6 +3327,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(Stat); registry->Register(LStat); registry->Register(FStat); + registry->Register(ReadFileSync); registry->Register(StatFs); registry->Register(Link); registry->Register(Symlink); diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index 78db466a95b38e..60e99a08930f92 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -74,6 +74,7 @@ const expectedModules = new Set([ 'NativeModule internal/webstreams/queuingstrategies', 'NativeModule internal/blob', 'NativeModule internal/fs/utils', + 'NativeModule internal/fs/read-file/utf8', 'NativeModule fs', 'Internal Binding options', 'NativeModule internal/options', From 3237d9de2d63aecd337a31a009dbf634c12cecec Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Wed, 5 Jul 2023 17:17:40 -0400 Subject: [PATCH 2/5] fixup! fs: add a fast-path for readFileSync utf-8 --- src/node_file.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_file.cc b/src/node_file.cc index 945cf4beeda475..619ba5841e29cd 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -2032,7 +2032,7 @@ static void ReadFileSync(const FunctionCallbackInfo& args) { FS_SYNC_TRACE_BEGIN(read); while (true) { - auto r = uv_fs_read(nullptr, &req, file, &buf, 1, result.length(), nullptr); + auto r = uv_fs_read(nullptr, &req, file, &buf, 1, -1, nullptr); if (req.result < 0) { FS_SYNC_TRACE_END(read); // req will be cleaned up by scope leave. From e9652527f83ffbe149f39664b419456b43c161de Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Thu, 6 Jul 2023 10:08:13 -0400 Subject: [PATCH 3/5] fixup! fs: add a fast-path for readFileSync utf-8 --- lib/fs.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 16157baaedc064..e57c61ce812af3 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -461,8 +461,8 @@ function readFileSync(path, options) { const isUserFd = isFd(path); // File descriptor ownership - // TODO: Do not handle file descriptor ownership for now. - if (!isUserFd && options.encoding === 'utf8') { + // TODO(@anonrig): Do not handle file descriptor ownership for now. + if (!isUserFd && (options.encoding === 'utf8' || options.encoding === 'utf-8')) { return readFileSyncUtf8(getValidatedPath(path), stringToFlags(options.flag)); } From 731dafc20ae0b882d4d01fb6251eebce29328d04 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Thu, 6 Jul 2023 12:49:19 -0400 Subject: [PATCH 4/5] fixup! fs: add a fast-path for readFileSync utf-8 --- lib/fs.js | 4 ++-- lib/internal/fs/{read-file => read}/context.js | 0 lib/internal/fs/{read-file => read}/utf8.js | 0 test/parallel/test-bootstrap-modules.js | 2 +- 4 files changed, 3 insertions(+), 3 deletions(-) rename lib/internal/fs/{read-file => read}/context.js (100%) rename lib/internal/fs/{read-file => read}/utf8.js (100%) diff --git a/lib/fs.js b/lib/fs.js index e57c61ce812af3..5bb8f502cc8ef5 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -142,7 +142,7 @@ const { validateObject, validateString, } = require('internal/validators'); -const { readFileSyncUtf8 } = require('internal/fs/read-file/utf8'); +const { readFileSyncUtf8 } = require('internal/fs/read/utf8'); let truncateWarn = true; let fs; @@ -381,7 +381,7 @@ function checkAborted(signal, callback) { function readFile(path, options, callback) { callback = maybeCallback(callback || options); options = getOptions(options, { flag: 'r' }); - const ReadFileContext = require('internal/fs/read-file/context'); + const ReadFileContext = require('internal/fs/read/context'); const context = new ReadFileContext(callback, options.encoding); context.isUserFd = isFd(path); // File descriptor ownership diff --git a/lib/internal/fs/read-file/context.js b/lib/internal/fs/read/context.js similarity index 100% rename from lib/internal/fs/read-file/context.js rename to lib/internal/fs/read/context.js diff --git a/lib/internal/fs/read-file/utf8.js b/lib/internal/fs/read/utf8.js similarity index 100% rename from lib/internal/fs/read-file/utf8.js rename to lib/internal/fs/read/utf8.js diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index 60e99a08930f92..9abe2dee22c1c7 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -74,7 +74,7 @@ const expectedModules = new Set([ 'NativeModule internal/webstreams/queuingstrategies', 'NativeModule internal/blob', 'NativeModule internal/fs/utils', - 'NativeModule internal/fs/read-file/utf8', + 'NativeModule internal/fs/read/utf8', 'NativeModule fs', 'Internal Binding options', 'NativeModule internal/options', From 6e8623b06a26181464882ce8614dfce0329a10fb Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Sat, 8 Jul 2023 22:38:55 -0400 Subject: [PATCH 5/5] fixup! fs: add a fast-path for readFileSync utf-8 --- lib/fs.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/fs.js b/lib/fs.js index 5bb8f502cc8ef5..3f4a6163ba3f65 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -463,7 +463,8 @@ function readFileSync(path, options) { // TODO(@anonrig): Do not handle file descriptor ownership for now. if (!isUserFd && (options.encoding === 'utf8' || options.encoding === 'utf-8')) { - return readFileSyncUtf8(getValidatedPath(path), stringToFlags(options.flag)); + path = getValidatedPath(path); + return readFileSyncUtf8(pathModule.toNamespacedPath(path), stringToFlags(options.flag)); } const fd = isUserFd ? path : fs.openSync(path, options.flag, 0o666);