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]>

Author: CanadaHonk

benchmark/fs/bench-statSync-failure.js

@@ -5,21 +5,29 @@ const fs = require('fs');
 constpath=require('path');
 
 constbench=common.createBenchmark(main,{
-n: [1e6],
-statSyncType: ['throw','noThrow'],
+n: [1e4],
+statSyncType: ['fstatSync','lstatSync','statSync'],
+throwType: ['throw','noThrow'],
+},{
+// fstatSync does not support throwIfNoEntry
+combinationFilter: ({ statSyncType, throwType })=>!(statSyncType==='fstatSync'&&throwType==='noThrow'),
 });
 
 
-functionmain({ n, statSyncType }){
-constarg=path.join(__dirname,'non.existent');
+functionmain({ n, statSyncType, throwType }){
+constarg=(statSyncType==='fstatSync' ?
+(1<<30) :
+path.join(__dirname,'non.existent'));
+
+constfn=fs[statSyncType];
 
 bench.start();
 for(leti=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.
 }

benchmark/fs/bench-statSync.js

@@ -4,7 +4,7 @@ const common = require('../common');
 constfs=require('fs');
 
 constbench=common.createBenchmark(main,{
-n: [1e6],
+n: [1e4],
 statSyncType: ['fstatSync','lstatSync','statSync'],
 });
 

lib/fs.js

@@ -74,8 +74,6 @@ const{
 ERR_INVALID_ARG_VALUE,
 },
  AbortError,
- uvErrmapGet,
- uvException,
 }=require('internal/errors');
 
 const{
@@ -402,11 +400,9 @@ function readFile(path, options, callback){
 }
 
 functiontryStatSync(fd,isUserFd){
-constctx={};
-conststats=binding.fstat(fd,false,undefined,ctx);
-if(ctx.errno!==undefined&&!isUserFd){
+conststats=binding.fstat(fd,false,undefined,true/* shouldNotThrow */);
+if(stats===undefined&&!isUserFd){
 fs.closeSync(fd);
-throwuvException(ctx);
 }
 returnstats;
 }
@@ -1614,33 +1610,21 @@ function statfs(path, options ={bigint: false }, callback){
 binding.statfs(pathModule.toNamespacedPath(path),options.bigint,req);
 }
 
-functionhasNoEntryError(ctx){
-if(ctx.errno){
-constuvErr=uvErrmapGet(ctx.errno);
-returnuvErr?.[0]==='ENOENT';
-}
-
-if(ctx.error){
-returnctx.error.code==='ENOENT';
-}
-
-returnfalse;
-}
-
 /**
  * Synchronously retrieves the `fs.Stats` for
  * the file descriptor.
  * @param{number} fd
  * @param{{
  * bigint?: boolean;
  * }} [options]
- * @returns{Stats}
+ * @returns{Stats | undefined}
  */
 functionfstatSync(fd,options={bigint: false}){
 fd=getValidatedFd(fd);
-constctx={ fd };
-conststats=binding.fstat(fd,options.bigint,undefined,ctx);
-handleErrorFromBinding(ctx);
+conststats=binding.fstat(fd,options.bigint,undefined,false);
+if(stats===undefined){
+return;
+}
 returngetStatsFromBinding(stats);
 }
 
@@ -1652,17 +1636,20 @@ function fstatSync(fd, options ={bigint: false }){
  * bigint?: boolean;
  * throwIfNoEntry?: boolean;
  * }} [options]
- * @returns{Stats}
+ * @returns{Stats | undefined}
  */
 functionlstatSync(path,options={bigint: false,throwIfNoEntry: true}){
 path=getValidatedPath(path);
-constctx={ path };
-conststats=binding.lstat(pathModule.toNamespacedPath(path),
-options.bigint,undefined,ctx);
-if(options.throwIfNoEntry===false&&hasNoEntryError(ctx)){
-returnundefined;
+conststats=binding.lstat(
+pathModule.toNamespacedPath(path),
+options.bigint,
+undefined,
+options.throwIfNoEntry,
+);
+
+if(stats===undefined){
+return;
 }
-handleErrorFromBinding(ctx);
 returngetStatsFromBinding(stats);
 }
 
@@ -2665,9 +2652,10 @@ function realpathSync(p, options){
 
 // On windows, check that the root exists. On unix there is no need.
 if(isWindows){
-constctx={path: base};
-binding.lstat(pathModule.toNamespacedPath(base),false,undefined,ctx);
-handleErrorFromBinding(ctx);
+constout=binding.lstat(pathModule.toNamespacedPath(base),false,undefined,true/* throwIfNoEntry */);
+if(out===undefined){
+return;
+}
 knownHard.add(base);
 }
 
@@ -2707,9 +2695,10 @@ function realpathSync(p, options){
 // for our internal use.
 
 constbaseLong=pathModule.toNamespacedPath(base);
-constctx={path: base};
-conststats=binding.lstat(baseLong,true,undefined,ctx);
-handleErrorFromBinding(ctx);
+conststats=binding.lstat(baseLong,true,undefined,true/* throwIfNoEntry */);
+if(stats===undefined){
+return;
+}
 
 if(!isFileType(stats,S_IFLNK)){
 knownHard.add(base);
@@ -2748,9 +2737,10 @@ function realpathSync(p, options){
 
 // On windows, check that the root exists. On unix there is no need.
 if(isWindows&&!knownHard.has(base)){
-constctx={path: base};
-binding.lstat(pathModule.toNamespacedPath(base),false,undefined,ctx);
-handleErrorFromBinding(ctx);
+constout=binding.lstat(pathModule.toNamespacedPath(base),false,undefined,true/* throwIfNoEntry */);
+if(out===undefined){
+return;
+}
 knownHard.add(base);
 }
 }

src/node_file.cc

@@ -1215,21 +1215,26 @@ static void LStat(const FunctionCallbackInfo<Value>& args){
 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 (!args[2]->IsUndefined()){// 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;
+ } else{// lstat(path, use_bigint, undefined, throw_if_no_entry)
+bool do_not_throw_if_no_entry = args[3]->IsFalse();
+ 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 result;
+if (do_not_throw_if_no_entry){
+ result = SyncCallAndThrowIf(
+ is_uv_error_except_no_entry, env, &req_wrap_sync, uv_fs_lstat, *path);
+ } else{
+ result = 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(result)){
+return;
  }
 
  Local<Value> arr = FillGlobalStatsArray(binding_data, use_bigint,
@@ -1250,19 +1255,23 @@ 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 (!args[2]->IsUndefined()){// 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, undefined, do_not_throw_error)
+bool do_not_throw_error = args[2]->IsTrue();
+constauto should_throw = [do_not_throw_error](int result){
+returnis_uv_error(result) && !do_not_throw_error;
+ };
+ 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 =
+SyncCallAndThrowIf(should_throw, 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,

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

@@ -25,7 +25,6 @@ require('../common');
 constassert=require('assert');
 constfs=require('fs');
 const{ internalBinding }=require('internal/test/binding');
-const{UV_EBADF}=internalBinding('uv');
 
 // Ensure that (read|write|append)FileSync() closes the file descriptor
 fs.openSync=function(){
@@ -42,15 +41,11 @@ fs.writeSync = function(){
 thrownewError('BAM');
 };
 
-internalBinding('fs').fstat=function(fd,bigint,_,ctx){
-ctx.errno=UV_EBADF;
-ctx.syscall='fstat';
+internalBinding('fs').fstat=function(){
+thrownewError('EBADF: bad file descriptor, fstat');
 };
 
 letclose_called=0;
-ensureThrows(function(){
-fs.readFileSync('dummy');
-},'EBADF: bad file descriptor, fstat');
 ensureThrows(function(){
 fs.writeFileSync('dummy','xxx');
 },'BAM');

typings/internalBinding/fs.d.ts

@@ -92,9 +92,9 @@ declare namespace InternalFSBinding{
 functionfstat(fd: number,useBigint: boolean,req: FSReqCallback<Float64Array|BigUint64Array>): void;
 functionfstat(fd: number,useBigint: true,req: FSReqCallback<BigUint64Array>): void;
 functionfstat(fd: number,useBigint: false,req: FSReqCallback<Float64Array>): void;
-functionfstat(fd: number,useBigint: boolean,req: undefined,ctx: FSSyncContext): Float64Array|BigUint64Array;
-functionfstat(fd: number,useBigint: true,req: undefined,ctx: FSSyncContext): BigUint64Array;
-functionfstat(fd: number,useBigint: false,req: undefined,ctx: FSSyncContext): Float64Array;
+functionfstat(fd: number,useBigint: boolean,req: undefined,shouldNotThrow: boolean): Float64Array|BigUint64Array;
+functionfstat(fd: number,useBigint: true,req: undefined,shouldNotThrow: boolean): BigUint64Array;
+functionfstat(fd: number,useBigint: false,req: undefined,shouldNotThrow: boolean): Float64Array;
 functionfstat(fd: number,useBigint: boolean,usePromises: typeofkUsePromises): Promise<Float64Array|BigUint64Array>;
 functionfstat(fd: number,useBigint: true,usePromises: typeofkUsePromises): Promise<BigUint64Array>;
 functionfstat(fd: number,useBigint: false,usePromises: typeofkUsePromises): Promise<Float64Array>;
@@ -127,9 +127,9 @@ declare namespace InternalFSBinding{
 functionlstat(path: StringOrBuffer,useBigint: boolean,req: FSReqCallback<Float64Array|BigUint64Array>): void;
 functionlstat(path: StringOrBuffer,useBigint: true,req: FSReqCallback<BigUint64Array>): void;
 functionlstat(path: StringOrBuffer,useBigint: false,req: FSReqCallback<Float64Array>): void;
-functionlstat(path: StringOrBuffer,useBigint: boolean,req: undefined,ctx: FSSyncContext): Float64Array|BigUint64Array;
-functionlstat(path: StringOrBuffer,useBigint: true,req: undefined,ctx: FSSyncContext): BigUint64Array;
-functionlstat(path: StringOrBuffer,useBigint: false,req: undefined,ctx: FSSyncContext): Float64Array;
+functionlstat(path: StringOrBuffer,useBigint: boolean,req: undefined,throwIfNoEntry: boolean): Float64Array|BigUint64Array;
+functionlstat(path: StringOrBuffer,useBigint: true,req: undefined,throwIfNoEntry: boolean): BigUint64Array;
+functionlstat(path: StringOrBuffer,useBigint: false,req: undefined,throwIfNoEntry: boolean): Float64Array;
 functionlstat(path: StringOrBuffer,useBigint: boolean,usePromises: typeofkUsePromises): Promise<Float64Array|BigUint64Array>;
 functionlstat(path: StringOrBuffer,useBigint: true,usePromises: typeofkUsePromises): Promise<BigUint64Array>;
 functionlstat(path: StringOrBuffer,useBigint: false,usePromises: typeofkUsePromises): Promise<Float64Array>;