fs: improve error performance for mkdirSync

PR-URL: #49847 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
nodejs · Oct 23, 2023 · 6ec5aba · 6ec5aba
1 parent 465ad2a
commit 6ec5aba
Show file tree

Hide file tree

Showing 4 changed files with 68 additions and 40 deletions.
diff --git a/benchmark/fs/bench-mkdirSync.js 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
@@ -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
@@ -1751,30 +1751,11 @@ int MKDirpAsync(uv_loop_t* loop,
   return err;
 }
 
-int CallMKDirpSync(Environment* env, const FunctionCallbackInfo<Value>& 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<v8::Context> context = env->context();
-    v8::Local<v8::Object> ctx_obj = args[4].As<v8::Object>();
-    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<Value>& 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);
@@ -1787,37 +1768,39 @@ static void MKDir(const FunctionCallbackInfo<Value>& 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<Value> error;
         std::string first_path(req_wrap_sync.continuation_data()->first_path());
         FromNamespacedPath(&first_path);
         MaybeLocal<Value> path = StringBytes::Encode(env->isolate(),
                                                      first_path.c_str(),
                                                      UTF8, &error);
         if (path.IsEmpty()) {
-          Local<Object> ctx = args[4].As<Object>();
-          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
@@ -146,9 +146,9 @@ declare namespace InternalFSBinding {
   function mkdir(path: string, mode: number, recursive: boolean, req: FSReqCallback<void | string>): void;
   function mkdir(path: string, mode: number, recursive: true, req: FSReqCallback<string>): void;
   function mkdir(path: string, mode: number, recursive: false, req: FSReqCallback<void>): 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<void | string>;
   function mkdir(path: string, mode: number, recursive: true, usePromises: typeof kUsePromises): Promise<string>;
   function mkdir(path: string, mode: number, recursive: false, usePromises: typeof kUsePromises): Promise<void>;