Skip to content

Commit

Permalink
fs: add a fast-path for readFileSync utf-8
Browse files Browse the repository at this point in the history
Backport-PR-URL: nodejs#48658
  • Loading branch information
H4ad committed Dec 2, 2023
1 parent 7fa1551 commit 9cbe99b
Show file tree
Hide file tree
Showing 6 changed files with 108 additions and 6 deletions.
17 changes: 13 additions & 4 deletions benchmark/fs/readFileSync.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
10 changes: 9 additions & 1 deletion lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ const {
validateInteger,
validateObject,
} = require('internal/validators');
const { readFileSyncUtf8 } = require('internal/fs/read-file/utf8');

let truncateWarn = true;
let fs;
Expand Down Expand Up @@ -383,7 +384,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

Expand Down Expand Up @@ -460,7 +461,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);
Expand Down
File renamed without changes.
24 changes: 24 additions & 0 deletions lib/internal/fs/read-file/utf8.js
Original file line number Diff line number Diff line change
@@ -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,
};
62 changes: 61 additions & 1 deletion src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1892,6 +1892,64 @@ static void ReadDir(const FunctionCallbackInfo<Value>& args) {
}
}

static void ReadFileSync(const FunctionCallbackInfo<Value>& 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<Int32>()->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<Value>& args) {
Environment* env = Environment::GetCurrent(args);

Expand Down Expand Up @@ -2718,6 +2776,8 @@ void Initialize(Local<Object> 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);

Expand All @@ -2728,7 +2788,6 @@ void Initialize(Local<Object> target,
SetMethod(context, target, "utimes", UTimes);
SetMethod(context, target, "futimes", FUTimes);
SetMethod(context, target, "lutimes", LUTimes);

SetMethod(context, target, "mkdtemp", Mkdtemp);

target
Expand Down Expand Up @@ -2820,6 +2879,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);
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-bootstrap-modules.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ const expectedModules = new Set([
'NativeModule internal/event_target',
'NativeModule internal/fixed_queue',
'NativeModule internal/fs/utils',
'NativeModule internal/fs/read-file/utf8',
'NativeModule internal/linkedlist',
'NativeModule internal/modules/cjs/loader',
'NativeModule internal/modules/esm/utils',
Expand Down

0 comments on commit 9cbe99b

Please sign in to comment.