From 89e7878e44af56e9bf73094f79c396e6e2ccd115 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Fri, 29 Sep 2023 13:12:42 -0400 Subject: [PATCH] fs: improve error performance of `symlinkSync` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/49962 Refs: https://github.com/nodejs/performance/issues/106 Reviewed-By: Matteo Collina Reviewed-By: Stephen Belanger Reviewed-By: Vinícius Lourenço Claro Cardoso --- benchmark/fs/bench-symlinkSync.js | 51 +++++++++++++++++++++++++++++++ lib/fs.js | 11 +++---- src/node_file.cc | 13 ++++---- typings/internalBinding/fs.d.ts | 1 + 4 files changed, 63 insertions(+), 13 deletions(-) create mode 100644 benchmark/fs/bench-symlinkSync.js diff --git a/benchmark/fs/bench-symlinkSync.js b/benchmark/fs/bench-symlinkSync.js new file mode 100644 index 00000000000000..5bf4e0e50779d9 --- /dev/null +++ b/benchmark/fs/bench-symlinkSync.js @@ -0,0 +1,51 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); +const assert = require('assert'); +const tmpdir = require('../../test/common/tmpdir'); + +if (process.platform === 'win32') { + console.log('Skipping: Windows does not play well with `symlink`'); + process.exit(0); +} + +const bench = common.createBenchmark(main, { + type: ['valid', 'invalid'], + n: [1e3], +}); + +function main({ n, type }) { + switch (type) { + case 'valid': { + tmpdir.refresh(); + bench.start(); + for (let i = 0; i < n; i++) { + fs.symlinkSync(tmpdir.resolve('.non-existent-symlink-file'), tmpdir.resolve(`.valid-${i}`), 'file'); + } + bench.end(n); + break; + } + + case 'invalid': { + let hasError = false; + bench.start(); + for (let i = 0; i < n; i++) { + try { + fs.symlinkSync( + tmpdir.resolve('.non-existent-symlink-file'), + __filename, + 'file', + ); + } catch { + hasError = true; + } + } + bench.end(n); + assert(hasError); + break; + } + default: + new Error('Invalid type'); + } +} diff --git a/lib/fs.js b/lib/fs.js index c4a4335e555b01..602384ad07bcae 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1802,13 +1802,12 @@ function symlinkSync(target, path, type) { } target = getValidatedPath(target, 'target'); path = getValidatedPath(path); - const flags = stringToSymlinkType(type); - - const ctx = { path: target, dest: path }; - binding.symlink(preprocessSymlinkDestination(target, type, path), - pathModule.toNamespacedPath(path), flags, undefined, ctx); - handleErrorFromBinding(ctx); + binding.symlink( + preprocessSymlinkDestination(target, type, path), + pathModule.toNamespacedPath(path), + stringToSymlinkType(type), + ); } /** diff --git a/src/node_file.cc b/src/node_file.cc index cdbb51977ef6de..0f9f617f9ee3ef 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1333,7 +1333,7 @@ static void Symlink(const FunctionCallbackInfo& args) { Isolate* isolate = env->isolate(); const int argc = args.Length(); - CHECK_GE(argc, 4); + CHECK_GE(argc, 3); BufferValue target(isolate, args[0]); CHECK_NOT_NULL(*target); @@ -1352,8 +1352,8 @@ static void Symlink(const FunctionCallbackInfo& args) { CHECK(args[2]->IsInt32()); int flags = args[2].As()->Value(); - FSReqBase* req_wrap_async = GetReqWrap(args, 3); - if (req_wrap_async != nullptr) { // symlink(target, path, flags, req) + if (argc > 3) { // symlink(target, path, flags, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 3); FS_ASYNC_TRACE_BEGIN2(UV_FS_SYMLINK, req_wrap_async, "target", @@ -1363,11 +1363,10 @@ static void Symlink(const FunctionCallbackInfo& args) { AsyncDestCall(env, req_wrap_async, args, "symlink", *path, path.length(), UTF8, AfterNoArgs, uv_fs_symlink, *target, *path, flags); } else { // symlink(target, path, flags, undefined, ctx) - CHECK_EQ(argc, 5); - FSReqWrapSync req_wrap_sync; + FSReqWrapSync req_wrap_sync("symlink", *target, *path); FS_SYNC_TRACE_BEGIN(symlink); - SyncCall(env, args[4], &req_wrap_sync, "symlink", - uv_fs_symlink, *target, *path, flags); + SyncCallAndThrowOnError( + env, &req_wrap_sync, uv_fs_symlink, *target, *path, flags); FS_SYNC_TRACE_END(symlink); } } diff --git a/typings/internalBinding/fs.d.ts b/typings/internalBinding/fs.d.ts index 619cd255f10aee..8aab4a3ae8d242 100644 --- a/typings/internalBinding/fs.d.ts +++ b/typings/internalBinding/fs.d.ts @@ -206,6 +206,7 @@ declare namespace InternalFSBinding { function symlink(target: StringOrBuffer, path: StringOrBuffer, type: number, req: FSReqCallback): void; function symlink(target: StringOrBuffer, path: StringOrBuffer, type: number, req: undefined, ctx: FSSyncContext): void; function symlink(target: StringOrBuffer, path: StringOrBuffer, type: number, usePromises: typeof kUsePromises): Promise; + function symlink(target: StringOrBuffer, path: StringOrBuffer, type: number): void; function unlink(path: string, req: FSReqCallback): void; function unlink(path: string): void;