Skip to content

Commit

Permalink
fs: improve error perf of sync lstat+fstat
Browse files Browse the repository at this point in the history
  • Loading branch information
@CanadaHonk @anonrig
CanadaHonk authored and anonrig committed Oct 11, 2023
1 parent dc1c50b commit 749a2ab
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 70 deletions.
22 changes: 15 additions & 7 deletions benchmark/fs/bench-statSync-failure.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,29 @@ const fs = require('fs');
const path = require('path');

const bench = common.createBenchmark(main, {
n: [1e6],
statSyncType: ['throw', 'noThrow'],
n: [1e4],
statSyncType: ['fstatSync', 'lstatSync', 'statSync'],
throwType: [ 'throw', 'noThrow' ],
}, {
// Do not allow noThrow fstatSync
combinationFilter: ({ statSyncType, throwType }) => !(statSyncType === 'fstatSync' && throwType === 'noThrow'),
});


function main({ n, statSyncType }) {
const arg = path.join(__dirname, 'non.existent');
function main({ n, statSyncType, throwType }) {
const arg = (statSyncType === 'fstatSync' ?
(1 << 30) :
path.join(__dirname, 'non.existent'));

const fn = fs[statSyncType];

bench.start();
for (let i = 0; i < n; i++) {
if (statSyncType === 'noThrow') {
fs.statSync(arg, { throwIfNoEntry: false });
if (throwType === 'noThrow') {
fn(arg, { throwIfNoEntry: false });
} else {
try {
fs.statSync(arg);
fn(arg);
} catch {
// Continue regardless of error.
}
Expand Down
2 changes: 1 addition & 1 deletion benchmark/fs/bench-statSync.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const common = require('../common');
const fs = require('fs');

const bench = common.createBenchmark(main, {
n: [1e6],
n: [1e4],
statSyncType: ['fstatSync', 'lstatSync', 'statSync'],
});

Expand Down
66 changes: 27 additions & 39 deletions lib/fs.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,6 @@ const {
ERR_INVALID_ARG_VALUE,
},
AbortError,
uvErrmapGet,
uvException,
} = require('internal/errors');

const {
Expand Down Expand Up @@ -398,11 +396,9 @@ function readFile(path, options, callback) {
}

function tryStatSync(fd, isUserFd) {
const ctx = {};
const stats = binding.fstat(fd, false, undefined, ctx);
if (ctx.errno !== undefined && !isUserFd) {
const stats = binding.fstat(fd, false);
if (stats === undefined && !isUserFd) {
fs.closeSync(fd);
throw uvException(ctx);
}
return stats;
}
Expand Down Expand Up @@ -1621,33 +1617,21 @@ function statfs(path, options = { bigint: false }, callback) {
binding.statfs(pathModule.toNamespacedPath(path), options.bigint, req);
}

function hasNoEntryError(ctx) {
if (ctx.errno) {
const uvErr = uvErrmapGet(ctx.errno);
return uvErr?.[0] === 'ENOENT';
}

if (ctx.error) {
return ctx.error.code === 'ENOENT';
}

return false;
}

/**
* Synchronously retrieves the `fs.Stats` for
* the file descriptor.
* @param {number} fd
* @param {{
* bigint?: boolean;
* }} [options]
* @returns {Stats}
* @returns {Stats | undefined}
*/
function fstatSync(fd, options = { bigint: false }) {
fd = getValidatedFd(fd);
const ctx = { fd };
const stats = binding.fstat(fd, options.bigint, undefined, ctx);
handleErrorFromBinding(ctx);
const stats = binding.fstat(fd, options.bigint);
if (stats === undefined) {
return;
}
return getStatsFromBinding(stats);
}

Expand All @@ -1659,17 +1643,18 @@ function fstatSync(fd, options = { bigint: false }) {
* bigint?: boolean;
* throwIfNoEntry?: boolean;
* }} [options]
* @returns {Stats}
* @returns {Stats | undefined}
*/
function lstatSync(path, options = { bigint: false, throwIfNoEntry: true }) {
path = getValidatedPath(path);
const ctx = { path };
const stats = binding.lstat(pathModule.toNamespacedPath(path),
options.bigint, undefined, ctx);
if (options.throwIfNoEntry === false && hasNoEntryError(ctx)) {
return undefined;
const stats = binding.lstat(
pathModule.toNamespacedPath(path),
options.bigint,
);

if (stats === undefined) {
return;
}
handleErrorFromBinding(ctx);
return getStatsFromBinding(stats);
}

Expand Down Expand Up @@ -2656,9 +2641,10 @@ function realpathSync(p, options) {

// On windows, check that the root exists. On unix there is no need.
if (isWindows) {
const ctx = { path: base };
binding.lstat(pathModule.toNamespacedPath(base), false, undefined, ctx);
handleErrorFromBinding(ctx);
const out = binding.lstat(pathModule.toNamespacedPath(base), false);
if (out === undefined) {
return;
}
knownHard.add(base);
}

Expand Down Expand Up @@ -2698,9 +2684,10 @@ function realpathSync(p, options) {
// for our internal use.

const baseLong = pathModule.toNamespacedPath(base);
const ctx = { path: base };
const stats = binding.lstat(baseLong, true, undefined, ctx);
handleErrorFromBinding(ctx);
const stats = binding.lstat(baseLong, true);
if (stats === undefined) {
return;
}

if (!isFileType(stats, S_IFLNK)) {
knownHard.add(base);
Expand Down Expand Up @@ -2741,9 +2728,10 @@ function realpathSync(p, options) {

// On windows, check that the root exists. On unix there is no need.
if (isWindows && !knownHard.has(base)) {
const ctx = { path: base };
binding.lstat(pathModule.toNamespacedPath(base), false, undefined, ctx);
handleErrorFromBinding(ctx);
const out = binding.lstat(pathModule.toNamespacedPath(base), false);
if (out === undefined) {
return;
}
knownHard.add(base);
}
}
Expand Down
31 changes: 14 additions & 17 deletions src/node_file.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1186,27 +1186,25 @@ static void LStat(const FunctionCallbackInfo<Value>& args) {
Environment* env = realm->env();

const int argc = args.Length();
CHECK_GE(argc, 3);
CHECK_GE(argc, 2);

BufferValue path(realm->isolate(), args[0]);
CHECK_NOT_NULL(*path);

bool use_bigint = args[1]->IsTrue();
FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint);
if (req_wrap_async != nullptr) { // lstat(path, use_bigint, req)
if (argc > 2) { // lstat(path, use_bigint, req)
FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint);
FS_ASYNC_TRACE_BEGIN1(
UV_FS_LSTAT, req_wrap_async, "path", TRACE_STR_COPY(*path))
AsyncCall(env, req_wrap_async, args, "lstat", UTF8, AfterStat,
uv_fs_lstat, *path);
} else { // lstat(path, use_bigint, undefined, ctx)
CHECK_EQ(argc, 4);
FSReqWrapSync req_wrap_sync;
FSReqWrapSync req_wrap_sync("lstat", *path);
FS_SYNC_TRACE_BEGIN(lstat);
int err = SyncCall(env, args[3], &req_wrap_sync, "lstat", uv_fs_lstat,
*path);
int err = SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_lstat, *path);
FS_SYNC_TRACE_END(lstat);
if (err != 0) {
return; // error info is in ctx
if (is_uv_error(err)) {
return;
}

Local<Value> arr = FillGlobalStatsArray(binding_data, use_bigint,
Expand All @@ -1227,19 +1225,18 @@ static void FStat(const FunctionCallbackInfo<Value>& args) {
int fd = args[0].As<Int32>()->Value();

bool use_bigint = args[1]->IsTrue();
FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint);
if (req_wrap_async != nullptr) { // fstat(fd, use_bigint, req)
if (argc > 2) { // fstat(fd, use_bigint, req)
FSReqBase* req_wrap_async = GetReqWrap(args, 2, use_bigint);
FS_ASYNC_TRACE_BEGIN0(UV_FS_FSTAT, req_wrap_async)
AsyncCall(env, req_wrap_async, args, "fstat", UTF8, AfterStat,
uv_fs_fstat, fd);
} else { // fstat(fd, use_bigint, undefined, ctx)
CHECK_EQ(argc, 4);
FSReqWrapSync req_wrap_sync;
} else { // fstat(fd, use_bigint)
FSReqWrapSync req_wrap_sync("fstat");
FS_SYNC_TRACE_BEGIN(fstat);
int err = SyncCall(env, args[3], &req_wrap_sync, "fstat", uv_fs_fstat, fd);
int err = SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_fstat, fd);
FS_SYNC_TRACE_END(fstat);
if (err != 0) {
return; // error info is in ctx
if (is_uv_error(err)) {
return;
}

Local<Value> arr = FillGlobalStatsArray(binding_data, use_bigint,
Expand Down
12 changes: 6 additions & 6 deletions typings/internalBinding/fs.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,9 @@ declare namespace InternalFSBinding {
function fstat(fd: number, useBigint: boolean, req: FSReqCallback<Float64Array | BigUint64Array>): void;
function fstat(fd: number, useBigint: true, req: FSReqCallback<BigUint64Array>): void;
function fstat(fd: number, useBigint: false, req: FSReqCallback<Float64Array>): void;
function fstat(fd: number, useBigint: boolean, req: undefined, ctx: FSSyncContext): Float64Array | BigUint64Array;
function fstat(fd: number, useBigint: true, req: undefined, ctx: FSSyncContext): BigUint64Array;
function fstat(fd: number, useBigint: false, req: undefined, ctx: FSSyncContext): Float64Array;
function fstat(fd: number, useBigint: boolean): Float64Array | BigUint64Array;
function fstat(fd: number, useBigint: true): BigUint64Array;
function fstat(fd: number, useBigint: false): Float64Array;
function fstat(fd: number, useBigint: boolean, usePromises: typeof kUsePromises): Promise<Float64Array | BigUint64Array>;
function fstat(fd: number, useBigint: true, usePromises: typeof kUsePromises): Promise<BigUint64Array>;
function fstat(fd: number, useBigint: false, usePromises: typeof kUsePromises): Promise<Float64Array>;
Expand Down Expand Up @@ -124,9 +124,9 @@ declare namespace InternalFSBinding {
function lstat(path: StringOrBuffer, useBigint: boolean, req: FSReqCallback<Float64Array | BigUint64Array>): void;
function lstat(path: StringOrBuffer, useBigint: true, req: FSReqCallback<BigUint64Array>): void;
function lstat(path: StringOrBuffer, useBigint: false, req: FSReqCallback<Float64Array>): void;
function lstat(path: StringOrBuffer, useBigint: boolean, req: undefined, ctx: FSSyncContext): Float64Array | BigUint64Array;
function lstat(path: StringOrBuffer, useBigint: true, req: undefined, ctx: FSSyncContext): BigUint64Array;
function lstat(path: StringOrBuffer, useBigint: false, req: undefined, ctx: FSSyncContext): Float64Array;
function lstat(path: StringOrBuffer, useBigint: boolean): Float64Array | BigUint64Array;
function lstat(path: StringOrBuffer, useBigint: true): BigUint64Array;
function lstat(path: StringOrBuffer, useBigint: false): Float64Array;
function lstat(path: StringOrBuffer, useBigint: boolean, usePromises: typeof kUsePromises): Promise<Float64Array | BigUint64Array>;
function lstat(path: StringOrBuffer, useBigint: true, usePromises: typeof kUsePromises): Promise<BigUint64Array>;
function lstat(path: StringOrBuffer, useBigint: false, usePromises: typeof kUsePromises): Promise<Float64Array>;
Expand Down

0 comments on commit 749a2ab

Please sign in to comment.