From bc6f279261d6b07cd01bdeacd1d095566e3a895f Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Fri, 29 Sep 2023 13:12:17 -0400 Subject: [PATCH] fs: improve error performance of `readlinkSync` 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-readlinkSync.js | 54 ++++++++++++++++++++++++++++++ lib/fs.js | 13 +++---- src/node_file.cc | 20 +++++------ typings/internalBinding/fs.d.ts | 1 + 4 files changed, 69 insertions(+), 19 deletions(-) create mode 100644 benchmark/fs/bench-readlinkSync.js diff --git a/benchmark/fs/bench-readlinkSync.js b/benchmark/fs/bench-readlinkSync.js new file mode 100644 index 00000000000000..15c22273e557b6 --- /dev/null +++ b/benchmark/fs/bench-readlinkSync.js @@ -0,0 +1,54 @@ +'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 `symlinkSync`'); + process.exit(0); +} + +const bench = common.createBenchmark(main, { + type: ['valid', 'invalid'], + n: [1e3], +}); + +function main({ n, type }) { + switch (type) { + case 'valid': { + tmpdir.refresh(); + const tmpfile = tmpdir.resolve(`.readlink-file-${process.pid}`); + fs.writeFileSync(tmpfile, 'data', 'utf8'); + let returnValue; + for (let i = 0; i < n; i++) { + fs.symlinkSync(tmpfile, tmpdir.resolve(`.readlink-sync-${i}`), 'file'); + } + bench.start(); + for (let i = 0; i < n; i++) { + returnValue = fs.readlinkSync(tmpdir.resolve(`.readlink-sync-${i}`), { encoding: 'utf8' }); + } + bench.end(n); + assert(returnValue); + break; + } + + case 'invalid': { + let hasError = false; + bench.start(); + for (let i = 0; i < n; i++) { + try { + fs.readlinkSync(tmpdir.resolve('.non-existing-file-for-readlinkSync')); + } 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 36d55e6e4989ca..15c9623f2cb840 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1728,11 +1728,10 @@ function readlink(path, options, callback) { function readlinkSync(path, options) { options = getOptions(options); path = getValidatedPath(path, 'oldPath'); - const ctx = { path }; - const result = binding.readlink(pathModule.toNamespacedPath(path), - options.encoding, undefined, ctx); - handleErrorFromBinding(ctx); - return result; + return binding.readlink( + pathModule.toNamespacedPath(path), + options.encoding, + ); } /** @@ -2714,10 +2713,8 @@ function realpathSync(p, options) { } } if (linkTarget === null) { - const ctx = { path: base }; binding.stat(baseLong, false, undefined, true); - linkTarget = binding.readlink(baseLong, undefined, undefined, ctx); - handleErrorFromBinding(ctx); + linkTarget = binding.readlink(baseLong, undefined); } resolvedLink = pathModule.resolve(previous, linkTarget); diff --git a/src/node_file.cc b/src/node_file.cc index 93af6f46e0ba79..8b0f46c00c8685 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1383,7 +1383,7 @@ static void ReadLink(const FunctionCallbackInfo& args) { Isolate* isolate = env->isolate(); const int argc = args.Length(); - CHECK_GE(argc, 3); + CHECK_GE(argc, 2); BufferValue path(isolate, args[0]); CHECK_NOT_NULL(*path); @@ -1392,21 +1392,20 @@ static void ReadLink(const FunctionCallbackInfo& args) { const enum encoding encoding = ParseEncoding(isolate, args[1], UTF8); - FSReqBase* req_wrap_async = GetReqWrap(args, 2); - if (req_wrap_async != nullptr) { // readlink(path, encoding, req) + if (argc > 2) { // readlink(path, encoding, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 2); FS_ASYNC_TRACE_BEGIN1( UV_FS_READLINK, req_wrap_async, "path", TRACE_STR_COPY(*path)) AsyncCall(env, req_wrap_async, args, "readlink", encoding, AfterStringPtr, uv_fs_readlink, *path); - } else { - CHECK_EQ(argc, 4); - FSReqWrapSync req_wrap_sync; + } else { // readlink(path, encoding) + FSReqWrapSync req_wrap_sync("readlink", *path); FS_SYNC_TRACE_BEGIN(readlink); - int err = SyncCall(env, args[3], &req_wrap_sync, "readlink", - uv_fs_readlink, *path); + int err = + SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_readlink, *path); FS_SYNC_TRACE_END(readlink); if (err < 0) { - return; // syscall failed, no need to continue, error info is in ctx + return; } const char* link_path = static_cast(req_wrap_sync.req.ptr); @@ -1416,8 +1415,7 @@ static void ReadLink(const FunctionCallbackInfo& args) { encoding, &error); if (rc.IsEmpty()) { - Local ctx = args[3].As(); - ctx->Set(env->context(), env->error_string(), error).Check(); + env->isolate()->ThrowException(error); return; } diff --git a/typings/internalBinding/fs.d.ts b/typings/internalBinding/fs.d.ts index 2c03c9934d2775..619cd255f10aee 100644 --- a/typings/internalBinding/fs.d.ts +++ b/typings/internalBinding/fs.d.ts @@ -178,6 +178,7 @@ declare namespace InternalFSBinding { function readlink(path: StringOrBuffer, encoding: unknown, req: FSReqCallback): void; function readlink(path: StringOrBuffer, encoding: unknown, req: undefined, ctx: FSSyncContext): string | Buffer; function readlink(path: StringOrBuffer, encoding: unknown, usePromises: typeof kUsePromises): Promise; + function readlink(path: StringOrBuffer, encoding: unknown): StringOrBuffer; function realpath(path: StringOrBuffer, encoding: unknown, req: FSReqCallback): void; function realpath(path: StringOrBuffer, encoding: unknown, req: undefined, ctx: FSSyncContext): string | Buffer;