From 986b46a567fa419d269cca4f658ac5cfb6ceb374 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Wed, 12 Jul 2023 14:37:42 -0400 Subject: [PATCH] fs: add a fast-path for readFileSync utf-8 PR-URL: https://github.com/nodejs/node/pull/48658 Reviewed-By: Matteo Collina Reviewed-By: Benjamin Gruenbaum --- benchmark/fs/readFileSync.js | 17 +++-- lib/fs.js | 11 +++- .../{read_file_context.js => read/context.js} | 0 lib/internal/fs/read/utf8.js | 24 +++++++ src/node_file.cc | 62 +++++++++++++++++++ test/parallel/test-bootstrap-modules.js | 1 + 6 files changed, 110 insertions(+), 5 deletions(-) rename lib/internal/fs/{read_file_context.js => read/context.js} (100%) create mode 100644 lib/internal/fs/read/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..3f4a6163ba3f65 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/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/context'); const context = new ReadFileContext(callback, options.encoding); context.isUserFd = isFd(path); // File descriptor ownership @@ -457,7 +458,15 @@ function tryReadSync(fd, isUserFd, buffer, pos, len) { */ function readFileSync(path, options) { options = getOptions(options, { flag: 'r' }); + const isUserFd = isFd(path); // File descriptor ownership + + // TODO(@anonrig): Do not handle file descriptor ownership for now. + if (!isUserFd && (options.encoding === 'utf8' || options.encoding === 'utf-8')) { + path = getValidatedPath(path); + return readFileSyncUtf8(pathModule.toNamespacedPath(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/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/utf8.js b/lib/internal/fs/read/utf8.js new file mode 100644 index 00000000000000..e916c918c11190 --- /dev/null +++ b/lib/internal/fs/read/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..619ba5841e29cd 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, -1, 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..9abe2dee22c1c7 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/utf8', 'NativeModule fs', 'Internal Binding options', 'NativeModule internal/options',