fs: move getValidMode to c++ for better performance

Author: andremralves

benchmark/fs/bench-accessSync.js

@@ -9,12 +9,12 @@ const tmpfile = tmpdir.resolve(`.existing-file-${process.pid}`);
 fs.writeFileSync(tmpfile, 'this-is-for-a-benchmark', 'utf8');
 
 const bench = common.createBenchmark(main, {
-  type: ['existing', 'non-existing', 'non-flat-existing'],
+  type: ['existing', 'non-existing', 'non-flat-existing', 'invalid-mode'],
   n: [1e5],
 });
 
 function main({ n, type }) {
-  let path;
+  let path, mode;
 
   switch (type) {
     case 'existing':
@@ -26,14 +26,18 @@ function main({ n, type }) {
     case 'non-existing':
       path = tmpdir.resolve(`.non-existing-file-${process.pid}`);
       break;
+    case 'invalid-mode':
+      path = __filename;
+      mode = -1;
+      break;
     default:
       new Error('Invalid type');
   }
 
   bench.start();
   for (let i = 0; i < n; i++) {
     try {
-      fs.accessSync(path);
+      fs.accessSync(path, mode);
     } catch {
       // do nothing
     }

benchmark/fs/bench-copyFileSync.js

@@ -6,19 +6,23 @@ const tmpdir = require('../../test/common/tmpdir');
 tmpdir.refresh();
 
 const bench = common.createBenchmark(main, {
-  type: ['invalid', 'valid'],
+  type: ['invalid-mode', 'invalid-path', 'valid'],
   n: [1e4],
 });
 
 function main({ n, type }) {
   tmpdir.refresh();
   const dest = tmpdir.resolve(`copy-file-bench-${process.pid}`);
-  let path;
+  let path, mode;
 
   switch (type) {
-    case 'invalid':
+    case 'invalid-path':
       path = tmpdir.resolve(`.existing-file-${process.pid}`);
       break;
+    case 'invalid-mode':
+      path = __filename;
+      mode = -1;
+      break;
     case 'valid':
       path = __filename;
       break;
@@ -28,7 +32,7 @@ function main({ n, type }) {
   bench.start();
   for (let i = 0; i < n; i++) {
     try {
-      fs.copyFileSync(path, dest);
+      fs.copyFileSync(path, dest, mode);
     } catch {
       // do nothing
     }

lib/fs.js

@@ -103,7 +103,6 @@ const {
   getOptions,
   getValidatedFd,
   getValidatedPath,
-  getValidMode,
   handleErrorFromBinding,
   possiblyTransformPath,
   preprocessSymlinkDestination,
@@ -226,7 +225,6 @@ function access(path, mode, callback) {
   }
 
   path = getValidatedPath(path);
-  mode = getValidMode(mode, 'access');
   callback = makeCallback(callback);
 
   const req = new FSReqCallback();
@@ -243,7 +241,6 @@ function access(path, mode, callback) {
  */
 function accessSync(path, mode) {
   path = getValidatedPath(path);
-  mode = getValidMode(mode, 'access');
 
   binding.access(pathModule.toNamespacedPath(path), mode);
 }
@@ -2962,7 +2959,6 @@ function copyFile(src, dest, mode, callback) {
 
   src = pathModule._makeLong(src);
   dest = pathModule._makeLong(dest);
-  mode = getValidMode(mode, 'copyFile');
   callback = makeCallback(callback);
 
   const req = new FSReqCallback();
@@ -2985,7 +2981,7 @@ function copyFileSync(src, dest, mode) {
   binding.copyFile(
     pathModule.toNamespacedPath(src),
     pathModule.toNamespacedPath(dest),
-    getValidMode(mode, 'copyFile'),
+    mode,
   );
 }
 

lib/internal/fs/promises.js

@@ -57,7 +57,6 @@ const {
   getStatFsFromBinding,
   getStatsFromBinding,
   getValidatedPath,
-  getValidMode,
   preprocessSymlinkDestination,
   stringToFlags,
   stringToSymlinkType,
@@ -559,7 +558,6 @@ async function readFileHandle(filehandle, options) {
 async function access(path, mode = F_OK) {
   path = getValidatedPath(path);
 
-  mode = getValidMode(mode, 'access');
   return binding.access(pathModule.toNamespacedPath(path), mode,
                         kUsePromises);
 }
@@ -574,7 +572,6 @@ async function cp(src, dest, options) {
 async function copyFile(src, dest, mode) {
   src = getValidatedPath(src, 'src');
   dest = getValidatedPath(dest, 'dest');
-  mode = getValidMode(mode, 'copyFile');
   return binding.copyFile(pathModule.toNamespacedPath(src),
                           pathModule.toNamespacedPath(dest),
                           mode,

lib/internal/fs/utils.js

@@ -107,26 +107,6 @@ const {
   },
 } = internalBinding('constants');
 
-// The access modes can be any of F_OK, R_OK, W_OK or X_OK. Some might not be
-// available on specific systems. They can be used in combination as well
-// (F_OK | R_OK | W_OK | X_OK).
-const kMinimumAccessMode = MathMin(F_OK, W_OK, R_OK, X_OK);
-const kMaximumAccessMode = F_OK | W_OK | R_OK | X_OK;
-
-const kDefaultCopyMode = 0;
-// The copy modes can be any of COPYFILE_EXCL, COPYFILE_FICLONE or
-// COPYFILE_FICLONE_FORCE. They can be used in combination as well
-// (COPYFILE_EXCL | COPYFILE_FICLONE | COPYFILE_FICLONE_FORCE).
-const kMinimumCopyMode = MathMin(
-  kDefaultCopyMode,
-  COPYFILE_EXCL,
-  COPYFILE_FICLONE,
-  COPYFILE_FICLONE_FORCE,
-);
-const kMaximumCopyMode = COPYFILE_EXCL |
-                         COPYFILE_FICLONE |
-                         COPYFILE_FICLONE_FORCE;
-
 // Most platforms don't allow reads or writes >= 2 GiB.
 // See https://github.com/libuv/libuv/pull/1501.
 const kIoMaxLength = 2 ** 31 - 1;
@@ -795,7 +775,6 @@ const validateCpOptions = hideStackFrames((options) => {
   validateBoolean(options.preserveTimestamps, 'options.preserveTimestamps');
   validateBoolean(options.recursive, 'options.recursive');
   validateBoolean(options.verbatimSymlinks, 'options.verbatimSymlinks');
-  options.mode = getValidMode(options.mode, 'copyFile');
   if (options.dereference === true && options.verbatimSymlinks === true) {
     throw new ERR_INCOMPATIBLE_OPTION_PAIR('dereference', 'verbatimSymlinks');
   }
@@ -888,24 +867,6 @@ const validateRmdirOptions = hideStackFrames(
     return options;
   });
 
-const getValidMode = hideStackFrames((mode, type) => {
-  let min = kMinimumAccessMode;
-  let max = kMaximumAccessMode;
-  let def = F_OK;
-  if (type === 'copyFile') {
-    min = kMinimumCopyMode;
-    max = kMaximumCopyMode;
-    def = mode || kDefaultCopyMode;
-  } else {
-    assert(type === 'access');
-  }
-  if (mode == null) {
-    return def;
-  }
-  validateInteger(mode, 'mode', min, max);
-  return mode;
-});
-
 const validateStringAfterArrayBufferView = hideStackFrames((buffer, name) => {
   if (typeof buffer !== 'string') {
     throw new ERR_INVALID_ARG_TYPE(
@@ -949,7 +910,6 @@ module.exports = {
   getOptions,
   getValidatedFd,
   getValidatedPath,
-  getValidMode,
   handleErrorFromBinding,
   nullCheck,
   possiblyTransformPath,

src/node_file.cc

@@ -89,6 +89,25 @@ constexpr char kPathSeparator = '/';
 const char* const kPathSeparator = "\\/";
 #endif
 
+// The access modes can be any of F_OK, R_OK, W_OK or X_OK. Some might not be
+// available on specific systems. They can be used in combination as well
+// (F_OK | R_OK | W_OK | X_OK).
+constexpr int kMaximumAccessMode = F_OK | W_OK | R_OK | X_OK;
+constexpr int kMinimumAccessMode = std::min({F_OK, W_OK, R_OK, X_OK});
+
+constexpr int kDefaultCopyMode = 0;
+// The copy modes can be any of UV_FS_COPYFILE_EXCL, UV_FS_COPYFILE_FICLONE or
+// UV_FS_COPYFILE_FICLONE_FORCE. They can be used in combination as well
+// (US_FS_COPYFILE_EXCL | US_FS_COPYFILE_FICLONE |
+// US_FS_COPYFILE_FICLONE_FORCE).
+constexpr int kMinimumCopyMode = std::min({
+    kDefaultCopyMode,
+    UV_FS_COPYFILE_EXCL,
+    UV_FS_COPYFILE_FICLONE,
+    UV_FS_COPYFILE_FICLONE_FORCE});
+constexpr int kMaximumCopyMode = UV_FS_COPYFILE_EXCL |
+    UV_FS_COPYFILE_FICLONE | UV_FS_COPYFILE_FICLONE_FORCE;
+
 std::string Basename(const std::string& str, const std::string& extension) {
   // Remove everything leading up to and including the final path separator.
   std::string::size_type pos = str.find_last_of(kPathSeparator);
@@ -854,6 +873,44 @@ void FromNamespacedPath(std::string* path) {
 #endif
 }
 
+
+static inline int GetValidMode(Environment* env,
+    Local<Value> mode_v, std::string_view type) {
+  if (!mode_v->IsInt32() && !mode_v->IsNullOrUndefined()) {
+    THROW_ERR_INVALID_ARG_TYPE(env, "mode must be int32 or null/undefined");
+    return -1;
+  }
+
+  int min = kMinimumAccessMode;
+  int max = kMaximumAccessMode;
+  int def = F_OK;
+
+  if (type == "copyFile") {
+    min = kMinimumCopyMode;
+    max = kMaximumCopyMode;
+    def = mode_v->IsNullOrUndefined() ?
+          kDefaultCopyMode :
+          mode_v.As<Int32>()->Value();
+  } else if (type != "access") {
+    THROW_ERR_INVALID_ARG_TYPE(env,
+        "type must be equal to \"copyFile\" or \"access\"");
+    return -1;
+  }
+
+  if (mode_v->IsNullOrUndefined()) {
+    return def;
+  }
+
+  const int mode = mode_v.As<Int32>()->Value();
+  if (mode < min || mode > max) {
+    THROW_ERR_OUT_OF_RANGE(env,
+    "mode is out of range: >= %d && <= %d", min, max);
+    return -1;
+  }
+
+  return mode;
+}
+
 void AfterMkdirp(uv_fs_t* req) {
   FSReqBase* req_wrap = FSReqBase::from_req(req);
   FSReqAfterScope after(req_wrap, req);
@@ -973,8 +1030,8 @@ void Access(const FunctionCallbackInfo<Value>& args) {
   const int argc = args.Length();
   CHECK_GE(argc, 2);
 
-  CHECK(args[1]->IsInt32());
-  int mode = args[1].As<Int32>()->Value();
+  int mode = GetValidMode(env, args[1], "access");
+  if (mode == -1) return;
 
   BufferValue path(isolate, args[0]);
   CHECK_NOT_NULL(*path);
@@ -2086,6 +2143,9 @@ static void CopyFile(const FunctionCallbackInfo<Value>& args) {
   const int argc = args.Length();
   CHECK_GE(argc, 3);
 
+  const int flags = GetValidMode(env, args[2], "copyFile");
+  if (flags == -1) return;
+
   BufferValue src(isolate, args[0]);
   CHECK_NOT_NULL(*src);
   THROW_IF_INSUFFICIENT_PERMISSIONS(
@@ -2096,9 +2156,6 @@ static void CopyFile(const FunctionCallbackInfo<Value>& args) {
   THROW_IF_INSUFFICIENT_PERMISSIONS(
       env, permission::PermissionScope::kFileSystemWrite, dest.ToStringView());
 
-  CHECK(args[2]->IsInt32());
-  const int flags = args[2].As<Int32>()->Value();
-
   if (argc > 3) {  // copyFile(src, dest, flags, req)
     FSReqBase* req_wrap_async = GetReqWrap(args, 3);
     FS_ASYNC_TRACE_BEGIN2(UV_FS_COPYFILE,

test/parallel/test-fs-access.js

@@ -163,6 +163,8 @@ fs.accessSync(readWriteFile, mode);
   { [Symbol.toPrimitive]() { return fs.constants.R_OK; } },
   [1],
   'r',
+  NaN,
+  Infinity,
 ].forEach((mode, i) => {
   console.log(mode, i);
   assert.throws(
@@ -183,8 +185,6 @@ fs.accessSync(readWriteFile, mode);
 [
   -1,
   8,
-  Infinity,
-  NaN,
 ].forEach((mode, i) => {
   console.log(mode, i);
   assert.throws(

test/parallel/test-fs-cp.mjs

@@ -133,8 +133,10 @@ function nextdir() {
 
 // It rejects if options.mode is invalid.
 {
+  const src = './test/fixtures/copy/kitchen-sink';
+  const dest = nextdir();
   assert.throws(
-    () => cpSync('a', 'b', { mode: -1 }),
+    () => cpSync(src, dest, mustNotMutateObjectDeep({ recursive: true, mode: -1 })),
     { code: 'ERR_OUT_OF_RANGE' }
   );
 }
@@ -858,10 +860,11 @@ if (!isWindows) {
 
 // It throws if options is not object.
 {
-  assert.throws(
-    () => cp('a', 'b', { mode: -1 }, () => {}),
-    { code: 'ERR_OUT_OF_RANGE' }
-  );
+  const src = './test/fixtures/copy/kitchen-sink';
+  const dest = nextdir();
+  cp(src, dest, mustNotMutateObjectDeep({ recursive: true, mode: -1 }), (err) => {
+    assert.strictEqual(err.code, 'ERR_OUT_OF_RANGE');
+  });
 }
 
 // Promises implementation of copy.
@@ -943,10 +946,13 @@ if (!isWindows) {
 
 // It rejects if options.mode is invalid.
 {
+  const src = './test/fixtures/copy/kitchen-sink';
+  const dest = nextdir();
   await assert.rejects(
-    fs.promises.cp('a', 'b', {
+    fs.promises.cp(src, dest, mustNotMutateObjectDeep({
+      recursive: true,
       mode: -1,
-    }),
+    })),
     { code: 'ERR_OUT_OF_RANGE' }
   );
 }