Skip to content

Commit ea88a3e

Browse files
CanadaHonkCanadaHonk
authored
fs: improve error perf of sync lstat+fstat
PR-URL: #49868 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent 5dd7ba6 commit ea88a3e

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 new UVException(ctx);
410406
}
411407
return stats;
412408
}
@@ -1616,33 +1612,21 @@ function statfs(path, options = { bigint: false }, callback) {
16161612
binding.statfs(pathModule.toNamespacedPath(path), options.bigint, req);
16171613
}
16181614

1619-
function hasNoEntryError(ctx) {
1620-
if (ctx.errno) {
1621-
const uvErr = uvErrmapGet(ctx.errno);
1622-
return uvErr?.[0] === 'ENOENT';
1623-
}
1624-
1625-
if (ctx.error) {
1626-
return ctx.error.code === 'ENOENT';
1627-
}
1628-
1629-
return false;
1630-
}
1631-
16321615
/**
16331616
* Synchronously retrieves the `fs.Stats` for
16341617
* the file descriptor.
16351618
* @param {number} fd
16361619
* @param {{
16371620
* bigint?: boolean;
16381621
* }} [options]
1639-
* @returns {Stats}
1622+
* @returns {Stats | undefined}
16401623
*/
16411624
function fstatSync(fd, options = { bigint: false }) {
16421625
fd = getValidatedFd(fd);
1643-
const ctx = { fd };
1644-
const stats = binding.fstat(fd, options.bigint, undefined, ctx);
1645-
handleErrorFromBinding(ctx);
1626+
const stats = binding.fstat(fd, options.bigint, undefined, false);
1627+
if (stats === undefined) {
1628+
return;
1629+
}
16461630
return getStatsFromBinding(stats);
16471631
}
16481632

@@ -1654,17 +1638,20 @@ function fstatSync(fd, options = { bigint: false }) {
16541638
* bigint?: boolean;
16551639
* throwIfNoEntry?: boolean;
16561640
* }} [options]
1657-
* @returns {Stats}
1641+
* @returns {Stats | undefined}
16581642
*/
16591643
function lstatSync(path, options = { bigint: false, throwIfNoEntry: true }) {
16601644
path = getValidatedPath(path);
1661-
const ctx = { path };
1662-
const stats = binding.lstat(pathModule.toNamespacedPath(path),
1663-
options.bigint, undefined, ctx);
1664-
if (options.throwIfNoEntry === false && hasNoEntryError(ctx)) {
1665-
return undefined;
1645+
const stats = binding.lstat(
1646+
pathModule.toNamespacedPath(path),
1647+
options.bigint,
1648+
undefined,
1649+
options.throwIfNoEntry,
1650+
);
1651+
1652+
if (stats === undefined) {
1653+
return;
16661654
}
1667-
handleErrorFromBinding(ctx);
16681655
return getStatsFromBinding(stats);
16691656
}
16701657

@@ -2667,9 +2654,10 @@ function realpathSync(p, options) {
26672654

26682655
// On windows, check that the root exists. On unix there is no need.
26692656
if (isWindows) {
2670-
const ctx = { path: base };
2671-
binding.lstat(pathModule.toNamespacedPath(base), false, undefined, ctx);
2672-
handleErrorFromBinding(ctx);
2657+
const out = binding.lstat(pathModule.toNamespacedPath(base), false, undefined, true /* throwIfNoEntry */);
2658+
if (out === undefined) {
2659+
return;
2660+
}
26732661
knownHard.add(base);
26742662
}
26752663

@@ -2709,9 +2697,10 @@ function realpathSync(p, options) {
27092697
// for our internal use.
27102698

27112699
const baseLong = pathModule.toNamespacedPath(base);
2712-
const ctx = { path: base };
2713-
const stats = binding.lstat(baseLong, true, undefined, ctx);
2714-
handleErrorFromBinding(ctx);
2700+
const stats = binding.lstat(baseLong, true, undefined, true /* throwIfNoEntry */);
2701+
if (stats === undefined) {
2702+
return;
2703+
}
27152704

27162705
if (!isFileType(stats, S_IFLNK)) {
27172706
knownHard.add(base);
@@ -2750,9 +2739,10 @@ function realpathSync(p, options) {
27502739

27512740
// On windows, check that the root exists. On unix there is no need.
27522741
if (isWindows && !knownHard.has(base)) {
2753-
const ctx = { path: base };
2754-
binding.lstat(pathModule.toNamespacedPath(base), false, undefined, ctx);
2755-
handleErrorFromBinding(ctx);
2742+
const out = binding.lstat(pathModule.toNamespacedPath(base), false, undefined, true /* throwIfNoEntry */);
2743+
if (out === undefined) {
2744+
return;
2745+
}
27562746
knownHard.add(base);
27572747
}
27582748
}

‎src/node_file.cc

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

11201120
bool use_bigint = args[1]->IsTrue();
1121-
FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint);
1122-
if (req_wrap_async != nullptr) { // lstat(path, use_bigint, req)
1121+
if (!args[2]->IsUndefined()) { // lstat(path, use_bigint, req)
1122+
FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint);
11231123
FS_ASYNC_TRACE_BEGIN1(
11241124
UV_FS_LSTAT, req_wrap_async, "path", TRACE_STR_COPY(*path))
11251125
AsyncCall(env, req_wrap_async, args, "lstat", UTF8, AfterStat,
11261126
uv_fs_lstat, *path);
1127-
} else { // lstat(path, use_bigint, undefined, ctx)
1128-
CHECK_EQ(argc, 4);
1129-
FSReqWrapSync req_wrap_sync;
1127+
} else { // lstat(path, use_bigint, undefined, throw_if_no_entry)
1128+
bool do_not_throw_if_no_entry = args[3]->IsFalse();
1129+
FSReqWrapSync req_wrap_sync("lstat", *path);
11301130
FS_SYNC_TRACE_BEGIN(lstat);
1131-
int err = SyncCall(env, args[3], &req_wrap_sync, "lstat", uv_fs_lstat,
1132-
*path);
1131+
int result;
1132+
if (do_not_throw_if_no_entry) {
1133+
result = SyncCallAndThrowIf(
1134+
is_uv_error_except_no_entry, env, &req_wrap_sync, uv_fs_lstat, *path);
1135+
} else {
1136+
result = SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_lstat, *path);
1137+
}
11331138
FS_SYNC_TRACE_END(lstat);
1134-
if (err != 0) {
1135-
return; // error info is in ctx
1139+
if (is_uv_error(result)) {
1140+
return;
11361141
}
11371142

11381143
Local<Value> arr = FillGlobalStatsArray(binding_data, use_bigint,
@@ -1153,19 +1158,23 @@ static void FStat(const FunctionCallbackInfo<Value>& args) {
11531158
int fd = args[0].As<Int32>()->Value();
11541159

11551160
bool use_bigint = args[1]->IsTrue();
1156-
FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint);
1157-
if (req_wrap_async != nullptr) { // fstat(fd, use_bigint, req)
1161+
if (!args[2]->IsUndefined()) { // fstat(fd, use_bigint, req)
1162+
FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint);
11581163
FS_ASYNC_TRACE_BEGIN0(UV_FS_FSTAT, req_wrap_async)
11591164
AsyncCall(env, req_wrap_async, args, "fstat", UTF8, AfterStat,
11601165
uv_fs_fstat, fd);
1161-
} else { // fstat(fd, use_bigint, undefined, ctx)
1162-
CHECK_EQ(argc, 4);
1163-
FSReqWrapSync req_wrap_sync;
1166+
} else { // fstat(fd, use_bigint, undefined, do_not_throw_error)
1167+
bool do_not_throw_error = args[2]->IsTrue();
1168+
const auto should_throw = [do_not_throw_error](int result) {
1169+
return is_uv_error(result) && !do_not_throw_error;
1170+
};
1171+
FSReqWrapSync req_wrap_sync("fstat");
11641172
FS_SYNC_TRACE_BEGIN(fstat);
1165-
int err = SyncCall(env, args[3], &req_wrap_sync, "fstat", uv_fs_fstat, fd);
1173+
int err =
1174+
SyncCallAndThrowIf(should_throw, env, &req_wrap_sync, uv_fs_fstat, fd);
11661175
FS_SYNC_TRACE_END(fstat);
1167-
if (err != 0) {
1168-
return; // error info is in ctx
1176+
if (is_uv_error(err)) {
1177+
return;
11691178
}
11701179

11711180
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>;
@@ -126,9 +126,9 @@ declare namespace InternalFSBinding {
126126
function lstat(path: StringOrBuffer, useBigint: boolean, req: FSReqCallback<Float64Array | BigUint64Array>): void;
127127
function lstat(path: StringOrBuffer, useBigint: true, req: FSReqCallback<BigUint64Array>): void;
128128
function lstat(path: StringOrBuffer, useBigint: false, req: FSReqCallback<Float64Array>): void;
129-
function lstat(path: StringOrBuffer, useBigint: boolean, req: undefined, ctx: FSSyncContext): Float64Array | BigUint64Array;
130-
function lstat(path: StringOrBuffer, useBigint: true, req: undefined, ctx: FSSyncContext): BigUint64Array;
131-
function lstat(path: StringOrBuffer, useBigint: false, req: undefined, ctx: FSSyncContext): Float64Array;
129+
function lstat(path: StringOrBuffer, useBigint: boolean, req: undefined, throwIfNoEntry: boolean): Float64Array | BigUint64Array;
130+
function lstat(path: StringOrBuffer, useBigint: true, req: undefined, throwIfNoEntry: boolean): BigUint64Array;
131+
function lstat(path: StringOrBuffer, useBigint: false, req: undefined, throwIfNoEntry: boolean): Float64Array;
132132
function lstat(path: StringOrBuffer, useBigint: boolean, usePromises: typeof kUsePromises): Promise<Float64Array | BigUint64Array>;
133133
function lstat(path: StringOrBuffer, useBigint: true, usePromises: typeof kUsePromises): Promise<BigUint64Array>;
134134
function lstat(path: StringOrBuffer, useBigint: false, usePromises: typeof kUsePromises): Promise<Float64Array>;

0 commit comments

Comments
 (0)