Skip to content

Commit

Permalink
fs: add c++ fast path for writeFileSync utf8
Browse files Browse the repository at this point in the history
PR-URL: #49884
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
CanadaHonk authored and RafaelGSS committed Nov 30, 2023
1 parent d44dd9b commit 45a8263
Show file tree
Hide file tree
Showing 5 changed files with 156 additions and 0 deletions.
43 changes: 43 additions & 0 deletions benchmark/fs/bench-writeFileSync.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
'use strict';

const common = require('../common.js');
const fs = require('fs');
const tmpdir = require('../../test/common/tmpdir');
tmpdir.refresh();

// Some variants are commented out as they do not show a change and just slow
const bench = common.createBenchmark(main, {
encoding: ['utf8'],
useFd: ['true', 'false'],
length: [1024, 102400, 1024 * 1024],

// useBuffer: ['true', 'false'],
useBuffer: ['false'],

// func: ['appendFile', 'writeFile'],
func: ['writeFile'],

n: [1e3],
});

function main({ n, func, encoding, length, useFd, useBuffer }) {
tmpdir.refresh();
const enc = encoding === 'undefined' ? undefined : encoding;
const path = tmpdir.resolve(`.writefilesync-file-${Date.now()}`);

useFd = useFd === 'true';
const file = useFd ? fs.openSync(path, 'w') : path;

let data = 'a'.repeat(length);
if (useBuffer === 'true') data = Buffer.from(data, encoding);

const fn = fs[func + 'Sync'];

bench.start();
for (let i = 0; i < n; ++i) {
fn(file, data, enc);
}
bench.end(n);

if (useFd) fs.closeSync(file);
}
13 changes: 13 additions & 0 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -2343,6 +2343,19 @@ function writeFileSync(path, data, options) {

validateBoolean(flush, 'options.flush');

// C++ fast path for string data and UTF8 encoding
if (typeof data === 'string' && (options.encoding === 'utf8' || options.encoding === 'utf-8')) {
if (!isInt32(path)) {
path = pathModule.toNamespacedPath(getValidatedPath(path));
}

return binding.writeFileUtf8(
path, data,
stringToFlags(options.flag),
parseFileMode(options.mode, 'mode', 0o666),
);
}

if (!isArrayBufferView(data)) {
validateStringAfterArrayBufferView(data, 'data');
data = Buffer.from(data, options.encoding || 'utf8');
Expand Down
80 changes: 80 additions & 0 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2292,6 +2292,84 @@ static void WriteString(const FunctionCallbackInfo<Value>& args) {
}
}

static void WriteFileUtf8(const FunctionCallbackInfo<Value>& args) {
// Fast C++ path for fs.writeFileSync(path, data) with utf8 encoding
// (file, data, options.flag, options.mode)

Environment* env = Environment::GetCurrent(args);
auto isolate = env->isolate();

CHECK_EQ(args.Length(), 4);

BufferValue value(isolate, args[1]);
CHECK_NOT_NULL(*value);

CHECK(args[2]->IsInt32());
const int flags = args[2].As<Int32>()->Value();

CHECK(args[3]->IsInt32());
const int mode = args[3].As<Int32>()->Value();

uv_file file;

bool is_fd = args[0]->IsInt32();

// Check for file descriptor
if (is_fd) {
file = args[0].As<Int32>()->Value();
} else {
BufferValue path(isolate, args[0]);
CHECK_NOT_NULL(*path);
if (CheckOpenPermissions(env, path, flags).IsNothing()) return;

FSReqWrapSync req_open("open", *path);

FS_SYNC_TRACE_BEGIN(open);
file =
SyncCallAndThrowOnError(env, &req_open, uv_fs_open, *path, flags, mode);
FS_SYNC_TRACE_END(open);

if (is_uv_error(file)) {
return;
}
}

int bytesWritten = 0;
uint32_t offset = 0;

const size_t length = value.length();
uv_buf_t uvbuf = uv_buf_init(value.out(), length);

FS_SYNC_TRACE_BEGIN(write);
while (offset < length) {
FSReqWrapSync req_write("write");
bytesWritten = SyncCallAndThrowOnError(
env, &req_write, uv_fs_write, file, &uvbuf, 1, -1);

// Write errored out
if (bytesWritten < 0) {
break;
}

offset += bytesWritten;
DCHECK_LE(offset, length);
uvbuf.base += bytesWritten;
uvbuf.len -= bytesWritten;
}
FS_SYNC_TRACE_END(write);

if (!is_fd) {
FSReqWrapSync req_close("close");

FS_SYNC_TRACE_BEGIN(close);
int result = SyncCallAndThrowOnError(env, &req_close, uv_fs_close, file);
FS_SYNC_TRACE_END(close);

if (is_uv_error(result)) {
return;
}
}
}

/*
* Wrapper for read(2).
Expand Down Expand Up @@ -3134,6 +3212,7 @@ static void CreatePerIsolateProperties(IsolateData* isolate_data,
SetMethod(isolate, target, "writeBuffer", WriteBuffer);
SetMethod(isolate, target, "writeBuffers", WriteBuffers);
SetMethod(isolate, target, "writeString", WriteString);
SetMethod(isolate, target, "writeFileUtf8", WriteFileUtf8);
SetMethod(isolate, target, "realpath", RealPath);
SetMethod(isolate, target, "copyFile", CopyFile);

Expand Down Expand Up @@ -3254,6 +3333,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
registry->Register(WriteBuffer);
registry->Register(WriteBuffers);
registry->Register(WriteString);
registry->Register(WriteFileUtf8);
registry->Register(RealPath);
registry->Register(CopyFile);

Expand Down
17 changes: 17 additions & 0 deletions test/parallel/test-fs-sync-fd-leak.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,17 +41,34 @@ fs.writeSync = function() {
throw new Error('BAM');
};

// Internal fast paths are pure C++, can't error inside write
internalBinding('fs').writeFileUtf8 = function() {
// Fake close
close_called++;
throw new Error('BAM');
};

internalBinding('fs').fstat = function() {
throw new Error('EBADF: bad file descriptor, fstat');
};

let close_called = 0;
ensureThrows(function() {
// Fast path: writeFileSync utf8
fs.writeFileSync('dummy', 'xxx');
}, 'BAM');
ensureThrows(function() {
// Non-fast path
fs.writeFileSync('dummy', 'xxx', { encoding: 'base64' });
}, 'BAM');
ensureThrows(function() {
// Fast path: writeFileSync utf8
fs.appendFileSync('dummy', 'xxx');
}, 'BAM');
ensureThrows(function() {
// Non-fast path
fs.appendFileSync('dummy', 'xxx', { encoding: 'base64' });
}, 'BAM');

function ensureThrows(cb, message) {
let got_exception = false;
Expand Down
3 changes: 3 additions & 0 deletions typings/internalBinding/fs.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,9 @@ declare namespace InternalFSBinding {
function writeString(fd: number, value: string, pos: unknown, encoding: unknown, usePromises: typeof kUsePromises): Promise<number>;

function getFormatOfExtensionlessFile(url: string): ConstantsBinding['fs'];

function writeFileUtf8(path: string, data: string, flag: number, mode: number): void;
function writeFileUtf8(fd: number, data: string, flag: number, mode: number): void;
}

export interface FsBinding {
Expand Down

0 comments on commit 45a8263

Please sign in to comment.