Skip to content

Commit

Permalink
fs: improve error performance of readlinkSync
Browse files Browse the repository at this point in the history
PR-URL: #49962
Refs: nodejs/performance#106
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
  • Loading branch information
anonrig authored and targos committed Oct 23, 2023
1 parent 2759878 commit bc6f279
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 19 deletions.
54 changes: 54 additions & 0 deletions benchmark/fs/bench-readlinkSync.js
Original file line number Diff line number Diff line change
@@ -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');
}
}
13 changes: 5 additions & 8 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
}

/**
Expand Down Expand Up @@ -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);

Expand Down
20 changes: 9 additions & 11 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1383,7 +1383,7 @@ static void ReadLink(const FunctionCallbackInfo<Value>& 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);
Expand All @@ -1392,21 +1392,20 @@ static void ReadLink(const FunctionCallbackInfo<Value>& 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<const char*>(req_wrap_sync.req.ptr);

Expand All @@ -1416,8 +1415,7 @@ static void ReadLink(const FunctionCallbackInfo<Value>& args) {
encoding,
&error);
if (rc.IsEmpty()) {
Local<Object> ctx = args[3].As<Object>();
ctx->Set(env->context(), env->error_string(), error).Check();
env->isolate()->ThrowException(error);
return;
}

Expand Down
1 change: 1 addition & 0 deletions typings/internalBinding/fs.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ declare namespace InternalFSBinding {
function readlink(path: StringOrBuffer, encoding: unknown, req: FSReqCallback<string | Buffer>): void;
function readlink(path: StringOrBuffer, encoding: unknown, req: undefined, ctx: FSSyncContext): string | Buffer;
function readlink(path: StringOrBuffer, encoding: unknown, usePromises: typeof kUsePromises): Promise<string | Buffer>;
function readlink(path: StringOrBuffer, encoding: unknown): StringOrBuffer;

function realpath(path: StringOrBuffer, encoding: unknown, req: FSReqCallback<string | Buffer>): void;
function realpath(path: StringOrBuffer, encoding: unknown, req: undefined, ctx: FSSyncContext): string | Buffer;
Expand Down

0 comments on commit bc6f279

Please sign in to comment.