Skip to content

Commit 05e25e0

Browse files
CanadaHonkUlisesGascon
CanadaHonk
authored and
UlisesGascon
committed
fs: improve error perf of sync lstat+fstat
PR-URL: #49868 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 0be84e5 commit 05e25e0

File tree

6 files changed

+79
-77
lines changed

6 files changed

+79
-77
lines changed

‎benchmark/fs/bench-statSync-failure.js‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,21 +5,29 @@ const fs = require('fs');
55
const path = require('path');
66

77
const bench = common.createBenchmark(main, {
8-
n: [1e6],
9-
statSyncType: ['throw', 'noThrow'],
8+
n: [1e4],
9+
statSyncType: ['fstatSync', 'lstatSync', 'statSync'],
10+
throwType: [ 'throw', 'noThrow' ],
11+
}, {
12+
// fstatSync does not support throwIfNoEntry
13+
combinationFilter: ({ statSyncType, throwType }) => !(statSyncType === 'fstatSync' && throwType === 'noThrow'),
1014
});
1115

1216

13-
function main({ n, statSyncType }) {
14-
const arg = path.join(__dirname, 'non.existent');
17+
function main({ n, statSyncType, throwType }) {
18+
const arg = (statSyncType === 'fstatSync' ?
19+
(1 << 30) :
20+
path.join(__dirname, 'non.existent'));
21+
22+
const fn = fs[statSyncType];
1523

1624
bench.start();
1725
for (let i = 0; i < n; i++) {
18-
if (statSyncType === 'noThrow') {
19-
fs.statSync(arg, { throwIfNoEntry: false });
26+
if (throwType === 'noThrow') {
27+
fn(arg, { throwIfNoEntry: false });
2028
} else {
2129
try {
22-
fs.statSync(arg);
30+
fn(arg);
2331
} catch {
2432
// Continue regardless of error.
2533
}

‎benchmark/fs/bench-statSync.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ const common = require('../common');
44
const fs = require('fs');
55

66
const bench = common.createBenchmark(main, {
7-
n: [1e6],
7+
n: [1e4],
88
statSyncType: ['fstatSync', 'lstatSync', 'statSync'],
99
});
1010

‎lib/fs.js‎

Lines changed: 29 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,6 @@ const {
7474
ERR_INVALID_ARG_VALUE,
7575
},
7676
AbortError,
77-
uvErrmapGet,
78-
uvException,
7977
} = require('internal/errors');
8078

8179
const {
@@ -402,11 +400,9 @@ function readFile(path, options, callback) {
402400
}
403401

404402
function tryStatSync(fd, isUserFd) {
405-
const ctx = {};
406-
const stats = binding.fstat(fd, false, undefined, ctx);
407-
if (ctx.errno !== undefined && !isUserFd) {
403+
const stats = binding.fstat(fd, false, undefined, true /* shouldNotThrow */);
404+
if (stats === undefined && !isUserFd) {
408405
fs.closeSync(fd);
409-
throw uvException(ctx);
410406
}
411407
return stats;
412408
}
@@ -1614,33 +1610,21 @@ function statfs(path, options = { bigint: false }, callback) {
16141610
binding.statfs(pathModule.toNamespacedPath(path), options.bigint, req);
16151611
}
16161612

1617-
function hasNoEntryError(ctx) {
1618-
if (ctx.errno) {
1619-
const uvErr = uvErrmapGet(ctx.errno);
1620-
return uvErr?.[0] === 'ENOENT';
1621-
}
1622-
1623-
if (ctx.error) {
1624-
return ctx.error.code === 'ENOENT';
1625-
}
1626-
1627-
return false;
1628-
}
1629-
16301613
/**
16311614
* Synchronously retrieves the `fs.Stats` for
16321615
* the file descriptor.
16331616
* @param {number} fd
16341617
* @param {{
16351618
* bigint?: boolean;
16361619
* }} [options]
1637-
* @returns {Stats}
1620+
* @returns {Stats | undefined}
16381621
*/
16391622
function fstatSync(fd, options = { bigint: false }) {
16401623
fd = getValidatedFd(fd);
1641-
const ctx = { fd };
1642-
const stats = binding.fstat(fd, options.bigint, undefined, ctx);
1643-
handleErrorFromBinding(ctx);
1624+
const stats = binding.fstat(fd, options.bigint, undefined, false);
1625+
if (stats === undefined) {
1626+
return;
1627+
}
16441628
return getStatsFromBinding(stats);
16451629
}
16461630

@@ -1652,17 +1636,20 @@ function fstatSync(fd, options = { bigint: false }) {
16521636
* bigint?: boolean;
16531637
* throwIfNoEntry?: boolean;
16541638
* }} [options]
1655-
* @returns {Stats}
1639+
* @returns {Stats | undefined}
16561640
*/
16571641
function lstatSync(path, options = { bigint: false, throwIfNoEntry: true }) {
16581642
path = getValidatedPath(path);
1659-
const ctx = { path };
1660-
const stats = binding.lstat(pathModule.toNamespacedPath(path),
1661-
options.bigint, undefined, ctx);
1662-
if (options.throwIfNoEntry === false && hasNoEntryError(ctx)) {
1663-
return undefined;
1643+
const stats = binding.lstat(
1644+
pathModule.toNamespacedPath(path),
1645+
options.bigint,
1646+
undefined,
1647+
options.throwIfNoEntry,
1648+
);
1649+
1650+
if (stats === undefined) {
1651+
return;
16641652
}
1665-
handleErrorFromBinding(ctx);
16661653
return getStatsFromBinding(stats);
16671654
}
16681655

@@ -2665,9 +2652,10 @@ function realpathSync(p, options) {
26652652

26662653
// On windows, check that the root exists. On unix there is no need.
26672654
if (isWindows) {
2668-
const ctx = { path: base };
2669-
binding.lstat(pathModule.toNamespacedPath(base), false, undefined, ctx);
2670-
handleErrorFromBinding(ctx);
2655+
const out = binding.lstat(pathModule.toNamespacedPath(base), false, undefined, true /* throwIfNoEntry */);
2656+
if (out === undefined) {
2657+
return;
2658+
}
26712659
knownHard.add(base);
26722660
}
26732661

@@ -2707,9 +2695,10 @@ function realpathSync(p, options) {
27072695
// for our internal use.
27082696

27092697
const baseLong = pathModule.toNamespacedPath(base);
2710-
const ctx = { path: base };
2711-
const stats = binding.lstat(baseLong, true, undefined, ctx);
2712-
handleErrorFromBinding(ctx);
2698+
const stats = binding.lstat(baseLong, true, undefined, true /* throwIfNoEntry */);
2699+
if (stats === undefined) {
2700+
return;
2701+
}
27132702

27142703
if (!isFileType(stats, S_IFLNK)) {
27152704
knownHard.add(base);
@@ -2748,9 +2737,10 @@ function realpathSync(p, options) {
27482737

27492738
// On windows, check that the root exists. On unix there is no need.
27502739
if (isWindows && !knownHard.has(base)) {
2751-
const ctx = { path: base };
2752-
binding.lstat(pathModule.toNamespacedPath(base), false, undefined, ctx);
2753-
handleErrorFromBinding(ctx);
2740+
const out = binding.lstat(pathModule.toNamespacedPath(base), false, undefined, true /* throwIfNoEntry */);
2741+
if (out === undefined) {
2742+
return;
2743+
}
27542744
knownHard.add(base);
27552745
}
27562746
}

‎src/node_file.cc‎

Lines changed: 26 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1215,21 +1215,26 @@ static void LStat(const FunctionCallbackInfo<Value>& args) {
12151215
CHECK_NOT_NULL(*path);
12161216

12171217
bool use_bigint = args[1]->IsTrue();
1218-
FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint);
1219-
if (req_wrap_async != nullptr) { // lstat(path, use_bigint, req)
1218+
if (!args[2]->IsUndefined()) { // lstat(path, use_bigint, req)
1219+
FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint);
12201220
FS_ASYNC_TRACE_BEGIN1(
12211221
UV_FS_LSTAT, req_wrap_async, "path", TRACE_STR_COPY(*path))
12221222
AsyncCall(env, req_wrap_async, args, "lstat", UTF8, AfterStat,
12231223
uv_fs_lstat, *path);
1224-
} else { // lstat(path, use_bigint, undefined, ctx)
1225-
CHECK_EQ(argc, 4);
1226-
FSReqWrapSync req_wrap_sync;
1224+
} else { // lstat(path, use_bigint, undefined, throw_if_no_entry)
1225+
bool do_not_throw_if_no_entry = args[3]->IsFalse();
1226+
FSReqWrapSync req_wrap_sync("lstat", *path);
12271227
FS_SYNC_TRACE_BEGIN(lstat);
1228-
int err = SyncCall(env, args[3], &req_wrap_sync, "lstat", uv_fs_lstat,
1229-
*path);
1228+
int result;
1229+
if (do_not_throw_if_no_entry) {
1230+
result = SyncCallAndThrowIf(
1231+
is_uv_error_except_no_entry, env, &req_wrap_sync, uv_fs_lstat, *path);
1232+
} else {
1233+
result = SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_lstat, *path);
1234+
}
12301235
FS_SYNC_TRACE_END(lstat);
1231-
if (err != 0) {
1232-
return; // error info is in ctx
1236+
if (is_uv_error(result)) {
1237+
return;
12331238
}
12341239

12351240
Local<Value> arr = FillGlobalStatsArray(binding_data, use_bigint,
@@ -1250,19 +1255,23 @@ static void FStat(const FunctionCallbackInfo<Value>& args) {
12501255
int fd = args[0].As<Int32>()->Value();
12511256

12521257
bool use_bigint = args[1]->IsTrue();
1253-
FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint);
1254-
if (req_wrap_async != nullptr) { // fstat(fd, use_bigint, req)
1258+
if (!args[2]->IsUndefined()) { // fstat(fd, use_bigint, req)
1259+
FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint);
12551260
FS_ASYNC_TRACE_BEGIN0(UV_FS_FSTAT, req_wrap_async)
12561261
AsyncCall(env, req_wrap_async, args, "fstat", UTF8, AfterStat,
12571262
uv_fs_fstat, fd);
1258-
} else { // fstat(fd, use_bigint, undefined, ctx)
1259-
CHECK_EQ(argc, 4);
1260-
FSReqWrapSync req_wrap_sync;
1263+
} else { // fstat(fd, use_bigint, undefined, do_not_throw_error)
1264+
bool do_not_throw_error = args[2]->IsTrue();
1265+
const auto should_throw = [do_not_throw_error](int result) {
1266+
return is_uv_error(result) && !do_not_throw_error;
1267+
};
1268+
FSReqWrapSync req_wrap_sync("fstat");
12611269
FS_SYNC_TRACE_BEGIN(fstat);
1262-
int err = SyncCall(env, args[3], &req_wrap_sync, "fstat", uv_fs_fstat, fd);
1270+
int err =
1271+
SyncCallAndThrowIf(should_throw, env, &req_wrap_sync, uv_fs_fstat, fd);
12631272
FS_SYNC_TRACE_END(fstat);
1264-
if (err != 0) {
1265-
return; // error info is in ctx
1273+
if (is_uv_error(err)) {
1274+
return;
12661275
}
12671276

12681277
Local<Value> arr = FillGlobalStatsArray(binding_data, use_bigint,

‎test/parallel/test-fs-sync-fd-leak.js‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ require('../common');
2525
const assert = require('assert');
2626
const fs = require('fs');
2727
const { internalBinding } = require('internal/test/binding');
28-
const { UV_EBADF } = internalBinding('uv');
2928

3029
// Ensure that (read|write|append)FileSync() closes the file descriptor
3130
fs.openSync = function() {
@@ -42,15 +41,11 @@ fs.writeSync = function() {
4241
throw new Error('BAM');
4342
};
4443

45-
internalBinding('fs').fstat = function(fd, bigint, _, ctx) {
46-
ctx.errno = UV_EBADF;
47-
ctx.syscall = 'fstat';
44+
internalBinding('fs').fstat = function() {
45+
throw new Error('EBADF: bad file descriptor, fstat');
4846
};
4947

5048
let close_called = 0;
51-
ensureThrows(function() {
52-
fs.readFileSync('dummy');
53-
}, 'EBADF: bad file descriptor, fstat');
5449
ensureThrows(function() {
5550
fs.writeFileSync('dummy', 'xxx');
5651
}, 'BAM');

‎typings/internalBinding/fs.d.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -92,9 +92,9 @@ declare namespace InternalFSBinding {
9292
function fstat(fd: number, useBigint: boolean, req: FSReqCallback<Float64Array | BigUint64Array>): void;
9393
function fstat(fd: number, useBigint: true, req: FSReqCallback<BigUint64Array>): void;
9494
function fstat(fd: number, useBigint: false, req: FSReqCallback<Float64Array>): void;
95-
function fstat(fd: number, useBigint: boolean, req: undefined, ctx: FSSyncContext): Float64Array | BigUint64Array;
96-
function fstat(fd: number, useBigint: true, req: undefined, ctx: FSSyncContext): BigUint64Array;
97-
function fstat(fd: number, useBigint: false, req: undefined, ctx: FSSyncContext): Float64Array;
95+
function fstat(fd: number, useBigint: boolean, req: undefined, shouldNotThrow: boolean): Float64Array | BigUint64Array;
96+
function fstat(fd: number, useBigint: true, req: undefined, shouldNotThrow: boolean): BigUint64Array;
97+
function fstat(fd: number, useBigint: false, req: undefined, shouldNotThrow: boolean): Float64Array;
9898
function fstat(fd: number, useBigint: boolean, usePromises: typeof kUsePromises): Promise<Float64Array | BigUint64Array>;
9999
function fstat(fd: number, useBigint: true, usePromises: typeof kUsePromises): Promise<BigUint64Array>;
100100
function fstat(fd: number, useBigint: false, usePromises: typeof kUsePromises): Promise<Float64Array>;
@@ -127,9 +127,9 @@ declare namespace InternalFSBinding {
127127
function lstat(path: StringOrBuffer, useBigint: boolean, req: FSReqCallback<Float64Array | BigUint64Array>): void;
128128
function lstat(path: StringOrBuffer, useBigint: true, req: FSReqCallback<BigUint64Array>): void;
129129
function lstat(path: StringOrBuffer, useBigint: false, req: FSReqCallback<Float64Array>): void;
130-
function lstat(path: StringOrBuffer, useBigint: boolean, req: undefined, ctx: FSSyncContext): Float64Array | BigUint64Array;
131-
function lstat(path: StringOrBuffer, useBigint: true, req: undefined, ctx: FSSyncContext): BigUint64Array;
132-
function lstat(path: StringOrBuffer, useBigint: false, req: undefined, ctx: FSSyncContext): Float64Array;
130+
function lstat(path: StringOrBuffer, useBigint: boolean, req: undefined, throwIfNoEntry: boolean): Float64Array | BigUint64Array;
131+
function lstat(path: StringOrBuffer, useBigint: true, req: undefined, throwIfNoEntry: boolean): BigUint64Array;
132+
function lstat(path: StringOrBuffer, useBigint: false, req: undefined, throwIfNoEntry: boolean): Float64Array;
133133
function lstat(path: StringOrBuffer, useBigint: boolean, usePromises: typeof kUsePromises): Promise<Float64Array | BigUint64Array>;
134134
function lstat(path: StringOrBuffer, useBigint: true, usePromises: typeof kUsePromises): Promise<BigUint64Array>;
135135
function lstat(path: StringOrBuffer, useBigint: false, usePromises: typeof kUsePromises): Promise<Float64Array>;

0 commit comments

Comments
 (0)