fs: improve error performance of sync methods

PR-URL: #49593 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Robert Nagy <[email protected]>
nodejs · Sep 17, 2023 · 7e12d0e · 7e12d0e
1 parent c177ac0
commit 7e12d0e
Show file tree

Hide file tree

Showing 10 changed files with 529 additions and 198 deletions.
diff --git a/benchmark/fs/bench-accessSync.js b/benchmark/fs/bench-accessSync.js
@@ -0,0 +1,42 @@
+'use strict';
+
+const common = require('../common');
+const fs = require('fs');
+const tmpdir = require('../../test/common/tmpdir');
+tmpdir.refresh();
+
+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'],
+  n: [1e5],
+});
+
+function main({ n, type }) {
+  let path;
+
+  switch (type) {
+    case 'existing':
+      path = __filename;
+      break;
+    case 'non-flat-existing':
+      path = tmpfile;
+      break;
+    case 'non-existing':
+      path = tmpdir.resolve(`.non-existing-file-${process.pid}`);
+      break;
+    default:
+      new Error('Invalid type');
+  }
+
+  bench.start();
+  for (let i = 0; i < n; i++) {
+    try {
+      fs.accessSync(path);
+    } catch {
+      // do nothing
+    }
+  }
+  bench.end(n);
+}
diff --git a/benchmark/fs/bench-copyFileSync.js b/benchmark/fs/bench-copyFileSync.js
@@ -0,0 +1,37 @@
+'use strict';
+
+const common = require('../common');
+const fs = require('fs');
+const tmpdir = require('../../test/common/tmpdir');
+tmpdir.refresh();
+
+const bench = common.createBenchmark(main, {
+  type: ['invalid', 'valid'],
+  n: [1e4],
+});
+
+function main({ n, type }) {
+  tmpdir.refresh();
+  const dest = tmpdir.resolve(`copy-file-bench-${process.pid}`);
+  let path;
+
+  switch (type) {
+    case 'invalid':
+      path = tmpdir.resolve(`.existing-file-${process.pid}`);
+      break;
+    case 'valid':
+      path = __filename;
+      break;
+    default:
+      throw new Error('Invalid type');
+  }
+  bench.start();
+  for (let i = 0; i < n; i++) {
+    try {
+      fs.copyFileSync(path, dest);
+    } catch {
+      // do nothing
+    }
+  }
+  bench.end(n);
+}
diff --git a/benchmark/fs/bench-existsSync.js b/benchmark/fs/bench-existsSync.js
@@ -0,0 +1,38 @@
+'use strict';
+
+const common = require('../common');
+const fs = require('fs');
+const tmpdir = require('../../test/common/tmpdir');
+tmpdir.refresh();
+
+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'],
+  n: [1e6],
+});
+
+function main({ n, type }) {
+  let path;
+
+  switch (type) {
+    case 'existing':
+      path = __filename;
+      break;
+    case 'non-flat-existing':
+      path = tmpfile;
+      break;
+    case 'non-existing':
+      path = tmpdir.resolve(`.non-existing-file-${process.pid}`);
+      break;
+    default:
+      new Error('Invalid type');
+  }
+
+  bench.start();
+  for (let i = 0; i < n; i++) {
+    fs.existsSync(path);
+  }
+  bench.end(n);
+}
diff --git a/benchmark/fs/bench-openSync.js b/benchmark/fs/bench-openSync.js
@@ -0,0 +1,37 @@
+'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'],
+  n: [1e5],
+});
+
+function main({ n, type }) {
+  let path;
+
+  switch (type) {
+    case 'existing':
+      path = __filename;
+      break;
+    case 'non-existing':
+      path = tmpdir.resolve(`.non-existing-file-${process.pid}`);
+      break;
+    default:
+      new Error('Invalid type');
+  }
+
+  bench.start();
+  for (let i = 0; i < n; i++) {
+    try {
+      const fd = fs.openSync(path, 'r', 0o666);
+      fs.closeSync(fd);
+    } catch {
+      // do nothing
+    }
+  }
+  bench.end(n);
+}
diff --git a/lib/fs.js b/lib/fs.js
@@ -141,7 +141,7 @@ const {
   validateObject,
   validateString,
 } = require('internal/validators');
-const { readFileSyncUtf8 } = require('internal/fs/read/utf8');
+const syncFs = require('internal/fs/sync');
 
 let truncateWarn = true;
 let fs;
@@ -243,12 +243,7 @@ function access(path, mode, callback) {
  * @returns {void}
  */
 function accessSync(path, mode) {
-  path = getValidatedPath(path);
-  mode = getValidMode(mode, 'access');
-
-  const ctx = { path };
-  binding.access(pathModule.toNamespacedPath(path), mode, undefined, ctx);
-  handleErrorFromBinding(ctx);
+  syncFs.access(path, mode);
 }
 
 /**
@@ -290,23 +285,7 @@ ObjectDefineProperty(exists, kCustomPromisifiedSymbol, {
  * @returns {boolean}
  */
 function existsSync(path) {
-  try {
-    path = getValidatedPath(path);
-  } catch {
-    return false;
-  }
-  const ctx = { path };
-  const nPath = pathModule.toNamespacedPath(path);
-  binding.access(nPath, F_OK, undefined, ctx);
-
-  // In case of an invalid symlink, `binding.access()` on win32
-  // will **not** return an error and is therefore not enough.
-  // Double check with `binding.stat()`.
-  if (isWindows && ctx.errno === undefined) {
-    binding.stat(nPath, false, undefined, ctx);
-  }
-
-  return ctx.errno === undefined;
+  return syncFs.exists(path);
 }
 
 function readFileAfterOpen(err, fd) {
@@ -462,8 +441,7 @@ function readFileSync(path, options) {
 
   // TODO(@anonrig): Do not handle file descriptor ownership for now.
   if (!isUserFd && (options.encoding === 'utf8' || options.encoding === 'utf-8')) {
-    path = getValidatedPath(path);
-    return readFileSyncUtf8(pathModule.toNamespacedPath(path), stringToFlags(options.flag));
+    return syncFs.readFileUtf8(path, options.flag);
   }
 
   const fd = isUserFd ? path : fs.openSync(path, options.flag, 0o666);
@@ -540,11 +518,7 @@ function close(fd, callback = defaultCloseCallback) {
  * @returns {void}
  */
 function closeSync(fd) {
-  fd = getValidatedFd(fd);
-
-  const ctx = {};
-  binding.close(fd, undefined, ctx);
-  handleErrorFromBinding(ctx);
+  return syncFs.close(fd);
 }
 
 /**
@@ -590,16 +564,7 @@ function open(path, flags, mode, callback) {
  * @returns {number}
  */
 function openSync(path, flags, mode) {
-  path = getValidatedPath(path);
-  const flagsNumber = stringToFlags(flags);
-  mode = parseFileMode(mode, 'mode', 0o666);
-
-  const ctx = { path };
-  const result = binding.open(pathModule.toNamespacedPath(path),
-                              flagsNumber, mode,
-                              undefined, ctx);
-  handleErrorFromBinding(ctx);
-  return result;
+  return syncFs.open(path, flags, mode);
 }
 
 /**
@@ -1702,25 +1667,12 @@ function lstatSync(path, options = { bigint: false, throwIfNoEntry: true }) {
  *   }} [options]
  * @returns {Stats}
  */
-function statSync(path, options = { bigint: false, throwIfNoEntry: true }) {
-  path = getValidatedPath(path);
-  const ctx = { path };
-  const stats = binding.stat(pathModule.toNamespacedPath(path),
-                             options.bigint, undefined, ctx);
-  if (options.throwIfNoEntry === false && hasNoEntryError(ctx)) {
-    return undefined;
-  }
-  handleErrorFromBinding(ctx);
-  return getStatsFromBinding(stats);
+function statSync(path, options) {
+  return syncFs.stat(path, options);
 }
 
-function statfsSync(path, options = { bigint: false }) {
-  path = getValidatedPath(path);
-  const ctx = { path };
-  const stats = binding.statfs(pathModule.toNamespacedPath(path),
-                               options.bigint, undefined, ctx);
-  handleErrorFromBinding(ctx);
-  return getStatFsFromBinding(stats);
+function statfsSync(path, options) {
+  return syncFs.statfs(path, options);
 }
 
 /**
@@ -2999,16 +2951,7 @@ function copyFile(src, dest, mode, callback) {
  * @returns {void}
  */
 function copyFileSync(src, dest, mode) {
-  src = getValidatedPath(src, 'src');
-  dest = getValidatedPath(dest, 'dest');
-
-  const ctx = { path: src, dest };  // non-prefixed
-
-  src = pathModule._makeLong(src);
-  dest = pathModule._makeLong(dest);
-  mode = getValidMode(mode, 'copyFile');
-  binding.copyFile(src, dest, mode, undefined, ctx);
-  handleErrorFromBinding(ctx);
+  syncFs.copyFile(src, dest, mode);
 }
 
 /**

diff --git a/lib/internal/fs/read/utf8.js b/lib/internal/fs/read/utf8.js