From 1cc56d45ba9aa92a609e302902e7843428f90186 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Wed, 5 Jul 2023 10:44:39 -0400 Subject: [PATCH] fs: add a fast-path for readFileSync utf-8 Backport-PR-URL: https://github.com/nodejs/node/pull/48658 --- 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 | 2 +- 6 files changed, 108 insertions(+), 7 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 1f695f6b08540a..d1d67185c0370d 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -145,9 +145,9 @@ const { validateObject, validateString, } = require('internal/validators'); +const { readFileSyncUtf8 } = require('internal/fs/read-file/utf8'); const watchers = require('internal/fs/watchers'); -const ReadFileContext = require('internal/fs/read_file_context'); let truncateWarn = true; let fs; @@ -391,6 +391,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 context = new ReadFileContext(callback, options.encoding); context.isUserFd = isFd(path); // File descriptor ownership @@ -467,7 +468,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 4993da585322db..bbef6f8e898c78 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1894,6 +1894,64 @@ static void ReadDir(const FunctionCallbackInfo& args) { } } +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(); + + 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); @@ -2720,6 +2778,8 @@ void Initialize(Local target, SetMethod(context, target, "realpath", RealPath); SetMethod(context, target, "copyFile", CopyFile); + SetMethodNoSideEffect(context, target, "readFileSync", ReadFileSync); + SetMethod(context, target, "chmod", Chmod); SetMethod(context, target, "fchmod", FChmod); @@ -2730,7 +2790,6 @@ void Initialize(Local target, SetMethod(context, target, "utimes", UTimes); SetMethod(context, target, "futimes", FUTimes); SetMethod(context, target, "lutimes", LUTimes); - SetMethod(context, target, "mkdtemp", Mkdtemp); target @@ -2826,6 +2885,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 b68a3d3da256a8..557f19deeacead 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -64,9 +64,9 @@ const expectedModules = new Set([ 'NativeModule internal/fixed_queue', 'NativeModule internal/fs/dir', 'NativeModule internal/fs/promises', - 'NativeModule internal/fs/read_file_context', 'NativeModule internal/fs/rimraf', 'NativeModule internal/fs/utils', + 'NativeModule internal/fs/read/utf8', 'NativeModule internal/fs/watchers', 'NativeModule internal/heap_utils', 'NativeModule internal/histogram',