From aa3209b88023854c46f79ea59ae216f19b82889b Mon Sep 17 00:00:00 2001 From: CanadaHonk Date: Mon, 27 Nov 2023 00:01:39 +0000 Subject: [PATCH] fs: add c++ fast path for writeFileSync utf8 PR-URL: https://github.com/nodejs/node/pull/49884 Reviewed-By: Yagiz Nizipli Reviewed-By: Santiago Gimeno Reviewed-By: Stephen Belanger Reviewed-By: James M Snell --- benchmark/fs/bench-writeFileSync.js | 43 ++++++++++++++ lib/fs.js | 13 +++++ src/node_file.cc | 80 +++++++++++++++++++++++++++ test/parallel/test-fs-sync-fd-leak.js | 17 ++++++ typings/internalBinding/fs.d.ts | 3 + 5 files changed, 156 insertions(+) create mode 100644 benchmark/fs/bench-writeFileSync.js diff --git a/benchmark/fs/bench-writeFileSync.js b/benchmark/fs/bench-writeFileSync.js new file mode 100644 index 00000000000000..29c16f4aa2a12d --- /dev/null +++ b/benchmark/fs/bench-writeFileSync.js @@ -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); +} diff --git a/lib/fs.js b/lib/fs.js index ff32c5da123fea..a65bd0274e6f82 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -2341,6 +2341,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'); diff --git a/src/node_file.cc b/src/node_file.cc index 44972d9ec22fd1..f3d8c35d4963d8 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -2327,6 +2327,84 @@ static void WriteString(const FunctionCallbackInfo& args) { } } +static void WriteFileUtf8(const FunctionCallbackInfo& 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()->Value(); + + CHECK(args[3]->IsInt32()); + const int mode = args[3].As()->Value(); + + uv_file file; + + bool is_fd = args[0]->IsInt32(); + + // Check for file descriptor + if (is_fd) { + file = args[0].As()->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). @@ -3169,6 +3247,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); @@ -3289,6 +3368,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(WriteBuffer); registry->Register(WriteBuffers); registry->Register(WriteString); + registry->Register(WriteFileUtf8); registry->Register(RealPath); registry->Register(CopyFile); diff --git a/test/parallel/test-fs-sync-fd-leak.js b/test/parallel/test-fs-sync-fd-leak.js index 8a9eb39a67a906..1abb75964a46e2 100644 --- a/test/parallel/test-fs-sync-fd-leak.js +++ b/test/parallel/test-fs-sync-fd-leak.js @@ -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; diff --git a/typings/internalBinding/fs.d.ts b/typings/internalBinding/fs.d.ts index 2d45a2ff40a8f1..5e7b3482bf6260 100644 --- a/typings/internalBinding/fs.d.ts +++ b/typings/internalBinding/fs.d.ts @@ -231,6 +231,9 @@ declare namespace InternalFSBinding { function writeString(fd: number, value: string, pos: unknown, encoding: unknown, usePromises: typeof kUsePromises): Promise; 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 {