From 569c96b33023f0290397d1874c87f46c15b297a0 Mon Sep 17 00:00:00 2001 From: CanadaHonk Date: Mon, 25 Sep 2023 06:19:04 +0100 Subject: [PATCH] fs: improve error performance for `mkdirSync` --- benchmark/fs/bench-mkdirSync.js | 44 ++++++++++++++++++++++++++++++ lib/fs.js | 11 ++++---- src/node_file.cc | 47 +++++++++++---------------------- typings/internalBinding/fs.d.ts | 6 ++--- 4 files changed, 68 insertions(+), 40 deletions(-) create mode 100644 benchmark/fs/bench-mkdirSync.js diff --git a/benchmark/fs/bench-mkdirSync.js b/benchmark/fs/bench-mkdirSync.js new file mode 100644 index 00000000000000..1db1c587877671 --- /dev/null +++ b/benchmark/fs/bench-mkdirSync.js @@ -0,0 +1,44 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); +const tmpdir = require('../../test/common/tmpdir'); +tmpdir.refresh(); + +const bench = common.createBenchmark(main, { + type: ['existing', 'non-existing'], + recursive: ['true', 'false'], + n: [1e3], +}); + +function main({ n, type, recursive }) { + recursive = recursive === 'true'; + let files; + + switch (type) { + case 'non-existing': + files = []; + + // Populate tmpdir with target dirs + for (let i = 0; i < n; i++) { + const path = tmpdir.resolve(recursive ? `rmdirsync-bench-dir-${process.pid}-${i}/a/b/c` : `rmdirsync-bench-dir-${process.pid}-${i}`); + files.push(path); + } + break; + case 'existing': + files = new Array(n).fill(__dirname); + break; + default: + new Error('Invalid type'); + } + + bench.start(); + for (let i = 0; i < n; i++) { + try { + fs.mkdirSync(files[i], { recursive }); + } catch { + // do nothing + } + } + bench.end(n); +} diff --git a/lib/fs.js b/lib/fs.js index 2d5170cd2f44b0..75a6d159f65f5d 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1385,11 +1385,12 @@ function mkdirSync(path, options) { path = getValidatedPath(path); validateBoolean(recursive, 'options.recursive'); - const ctx = { path }; - const result = binding.mkdir(pathModule.toNamespacedPath(path), - parseFileMode(mode, 'mode'), recursive, - undefined, ctx); - handleErrorFromBinding(ctx); + const result = binding.mkdir( + pathModule.toNamespacedPath(path), + parseFileMode(mode, 'mode'), + recursive, + ); + if (recursive) { return result; } diff --git a/src/node_file.cc b/src/node_file.cc index afaa028784e710..77139c9d9fa020 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1757,30 +1757,11 @@ int MKDirpAsync(uv_loop_t* loop, return err; } -int CallMKDirpSync(Environment* env, const FunctionCallbackInfo& args, - FSReqWrapSync* req_wrap, const char* path, int mode) { - env->PrintSyncTrace(); - int err = MKDirpSync(env->event_loop(), &req_wrap->req, path, mode, - nullptr); - if (err < 0) { - v8::Local context = env->context(); - v8::Local ctx_obj = args[4].As(); - v8::Isolate* isolate = env->isolate(); - ctx_obj->Set(context, - env->errno_string(), - v8::Integer::New(isolate, err)).Check(); - ctx_obj->Set(context, - env->syscall_string(), - OneByteString(isolate, "mkdir")).Check(); - } - return err; -} - static void MKDir(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); const int argc = args.Length(); - CHECK_GE(argc, 4); + CHECK_GE(argc, 3); BufferValue path(env->isolate(), args[0]); CHECK_NOT_NULL(*path); @@ -1793,21 +1774,25 @@ static void MKDir(const FunctionCallbackInfo& args) { CHECK(args[2]->IsBoolean()); bool mkdirp = args[2]->IsTrue(); - FSReqBase* req_wrap_async = GetReqWrap(args, 3); - if (req_wrap_async != nullptr) { // mkdir(path, mode, req) + if (argc > 3) { // mkdir(path, mode, recursive, req) + FSReqBase* req_wrap_async = GetReqWrap(args, 3); FS_ASYNC_TRACE_BEGIN1( UV_FS_UNLINK, req_wrap_async, "path", TRACE_STR_COPY(*path)) AsyncCall(env, req_wrap_async, args, "mkdir", UTF8, mkdirp ? AfterMkdirp : AfterNoArgs, mkdirp ? MKDirpAsync : uv_fs_mkdir, *path, mode); - } else { // mkdir(path, mode, undefined, ctx) - CHECK_EQ(argc, 5); - FSReqWrapSync req_wrap_sync; + } else { // mkdir(path, mode, recursive) + FSReqWrapSync req_wrap_sync("mkdir", *path); FS_SYNC_TRACE_BEGIN(mkdir); if (mkdirp) { - int err = CallMKDirpSync(env, args, &req_wrap_sync, *path, mode); - if (err == 0 && - !req_wrap_sync.continuation_data()->first_path().empty()) { + env->PrintSyncTrace(); + int err = MKDirpSync( + env->event_loop(), &req_wrap_sync.req, *path, mode, nullptr); + if (is_uv_error(err)) { + env->ThrowUVException(err, "mkdir", nullptr, *path); + return; + } + if (!req_wrap_sync.continuation_data()->first_path().empty()) { Local error; std::string first_path(req_wrap_sync.continuation_data()->first_path()); FromNamespacedPath(&first_path); @@ -1815,15 +1800,13 @@ static void MKDir(const FunctionCallbackInfo& args) { first_path.c_str(), UTF8, &error); if (path.IsEmpty()) { - Local ctx = args[4].As(); - ctx->Set(env->context(), env->error_string(), error).Check(); + env->isolate()->ThrowException(error); return; } args.GetReturnValue().Set(path.ToLocalChecked()); } } else { - SyncCall(env, args[4], &req_wrap_sync, "mkdir", - uv_fs_mkdir, *path, mode); + SyncCallAndThrowOnError(env, &req_wrap_sync, uv_fs_mkdir, *path, mode); } FS_SYNC_TRACE_END(mkdir); } diff --git a/typings/internalBinding/fs.d.ts b/typings/internalBinding/fs.d.ts index c4dd973293ab22..6608a5763a7107 100644 --- a/typings/internalBinding/fs.d.ts +++ b/typings/internalBinding/fs.d.ts @@ -142,9 +142,9 @@ declare namespace InternalFSBinding { function mkdir(path: string, mode: number, recursive: boolean, req: FSReqCallback): void; function mkdir(path: string, mode: number, recursive: true, req: FSReqCallback): void; function mkdir(path: string, mode: number, recursive: false, req: FSReqCallback): void; - function mkdir(path: string, mode: number, recursive: boolean, req: undefined, ctx: FSSyncContext): void | string; - function mkdir(path: string, mode: number, recursive: true, req: undefined, ctx: FSSyncContext): string; - function mkdir(path: string, mode: number, recursive: false, req: undefined, ctx: FSSyncContext): void; + function mkdir(path: string, mode: number, recursive: boolean): void | string; + function mkdir(path: string, mode: number, recursive: true): string; + function mkdir(path: string, mode: number, recursive: false): void; function mkdir(path: string, mode: number, recursive: boolean, usePromises: typeof kUsePromises): Promise; function mkdir(path: string, mode: number, recursive: true, usePromises: typeof kUsePromises): Promise; function mkdir(path: string, mode: number, recursive: false, usePromises: typeof kUsePromises): Promise;