From 156d2485987f014a684e082090132619320c251f Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 3 Jun 2020 01:18:58 +0000 Subject: [PATCH 1/4] lib: add throws option to fs.f/l/statSync For consumers that aren't interested in *why* a `statSync` call failed, allocating and throwing an exception is an unnecessary expense. This PR adds an option that will cause it to return `undefined` in such cases instead. As a motivating example, the JavaScript & TypeScript language service shared between Visual Studio and Visual Studio Code is stuck with synchronous file IO for architectural and backward-compatibility reasons. It frequently needs to speculatively check for the existence of files and directories that may not exist (and cares about file vs directory, so `existsSync` is insufficient), but ignores file system entries it can't access, regardless of the reason. Benchmarking the language service is difficult because it's so hard to get good coverage of both code bases and user behaviors, but, as a representative metric, we measured batch compilation of a few hundred popular projects (by star count) from GitHub and found that, on average, we saved about 1-2% of total compilation time. We speculate that the savings could be even more significant in interactive (language service or watch mode) scenarios, where the same (non-existent) files need to be polled over and over again. It's not a huge improvement, but it's a very small change and it will affect a lot of users (and CI runs). For reference, our measurements were against `v12.x` (3637a061a at the time) on an Ubuntu Server desktop with an SSD. --- benchmark/fs/bench-statSync-failure.js | 28 ++++++++++++++++++++++++ doc/api/fs.md | 9 ++++++++ lib/fs.js | 30 ++++++++++++++++++++------ test/parallel/test-fs-stat-bigint.js | 17 +++++++++++++++ 4 files changed, 78 insertions(+), 6 deletions(-) create mode 100644 benchmark/fs/bench-statSync-failure.js diff --git a/benchmark/fs/bench-statSync-failure.js b/benchmark/fs/bench-statSync-failure.js new file mode 100644 index 00000000000000..8b2a52ae36a162 --- /dev/null +++ b/benchmark/fs/bench-statSync-failure.js @@ -0,0 +1,28 @@ +'use strict'; + +const common = require('../common'); +const fs = require('fs'); +const path = require('path'); + +const bench = common.createBenchmark(main, { + n: [1e6], + statSyncType: ['throw', 'noThrow'] +}); + + +function main({ n, statSyncType }) { + const arg = path.join(__dirname, 'non.existent'); + + bench.start(); + for (let i = 0; i < n; i++) { + if (statSyncType === 'noThrow') { + fs.statSync(arg, { throws: false }); + } else { + try { + fs.statSync(arg); + } catch { + } + } + } + bench.end(n); +} diff --git a/doc/api/fs.md b/doc/api/fs.md index 713ba1bf26bbee..e00ec461cc8d10 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -2202,6 +2202,9 @@ changes: * `options` {Object} * `bigint` {boolean} Whether the numeric values in the returned [`fs.Stats`][] object should be `bigint`. **Default:** `false`. + * `throws` {boolean} Whether an exception will be thrown + if stats are not available, rather than returning `undefined`. + **Default:** `true`. * Returns: {fs.Stats} Synchronous fstat(2). @@ -2568,6 +2571,9 @@ changes: * `options` {Object} * `bigint` {boolean} Whether the numeric values in the returned [`fs.Stats`][] object should be `bigint`. **Default:** `false`. + * `throws` {boolean} Whether an exception will be thrown + if stats are not available, rather than returning `undefined`. + **Default:** `true`. * Returns: {fs.Stats} Synchronous lstat(2). @@ -3810,6 +3816,9 @@ changes: * `options` {Object} * `bigint` {boolean} Whether the numeric values in the returned [`fs.Stats`][] object should be `bigint`. **Default:** `false`. + * `throws` {boolean} Whether an exception will be thrown + if stats are not available, rather than returning `undefined`. + **Default:** `true`. * Returns: {fs.Stats} Synchronous stat(2). diff --git a/lib/fs.js b/lib/fs.js index 474b8c5d6c1d3e..db65f28818ca03 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1076,29 +1076,47 @@ function stat(path, options = { bigint: false }, callback) { binding.stat(pathModule.toNamespacedPath(path), options.bigint, req); } -function fstatSync(fd, options = { bigint: false }) { +function fstatSync(fd, options = { bigint: false, throws: true }) { validateInt32(fd, 'fd', 0); const ctx = { fd }; const stats = binding.fstat(fd, options.bigint, undefined, ctx); - handleErrorFromBinding(ctx); + if (options.throws === false) { + if (ctx.errno || ctx.error) { + return undefined; + } + } else { + handleErrorFromBinding(ctx); + } return getStatsFromBinding(stats); } -function lstatSync(path, options = { bigint: false }) { +function lstatSync(path, options = { bigint: false, throws: true }) { path = getValidatedPath(path); const ctx = { path }; const stats = binding.lstat(pathModule.toNamespacedPath(path), options.bigint, undefined, ctx); - handleErrorFromBinding(ctx); + if (options.throws === false) { + if (ctx.errno || ctx.error) { + return undefined; + } + } else { + handleErrorFromBinding(ctx); + } return getStatsFromBinding(stats); } -function statSync(path, options = { bigint: false }) { +function statSync(path, options = { bigint: false, throws: true }) { path = getValidatedPath(path); const ctx = { path }; const stats = binding.stat(pathModule.toNamespacedPath(path), options.bigint, undefined, ctx); - handleErrorFromBinding(ctx); + if (options.throws === false) { + if (ctx.errno || ctx.error) { + return undefined; + } + } else { + handleErrorFromBinding(ctx); + } return getStatsFromBinding(stats); } diff --git a/test/parallel/test-fs-stat-bigint.js b/test/parallel/test-fs-stat-bigint.js index 6f1db6078eac6f..62c3b8538a926a 100644 --- a/test/parallel/test-fs-stat-bigint.js +++ b/test/parallel/test-fs-stat-bigint.js @@ -122,6 +122,23 @@ if (!common.isWindows) { fs.closeSync(fd); } +const runSyncFailureTest = (func, arg, error) => { + assert.throws(() => func(arg), error); + assert.strictEqual(func(arg, { throws: false }), undefined); +}; + +{ + runSyncFailureTest(fs.statSync, 'does_not_exist', { code: 'ENOENT' }); +} + +{ + runSyncFailureTest(fs.lstatSync, 'does_not_exist', { code: 'ENOENT' }); +} + +{ + runSyncFailureTest(fs.fstatSync, 9999, { code: 'EBADF' }); +} + const runCallbackTest = (func, arg, done) => { const startTime = process.hrtime.bigint(); func(arg, { bigint: true }, common.mustCall((err, bigintStats) => { From b3ea0b8f5b1e5d3bd04cc693a2d9db8f0d134b9c Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 30 Oct 2020 21:40:21 +0000 Subject: [PATCH 2/4] Restrict exception suppression to ENOENT --- benchmark/fs/bench-statSync-failure.js | 2 +- doc/api/fs.md | 12 +++---- lib/fs.js | 47 ++++++++++++++------------ test/parallel/test-fs-stat-bigint.js | 26 +++++++++----- 4 files changed, 51 insertions(+), 36 deletions(-) diff --git a/benchmark/fs/bench-statSync-failure.js b/benchmark/fs/bench-statSync-failure.js index 8b2a52ae36a162..82cb24c09f4af2 100644 --- a/benchmark/fs/bench-statSync-failure.js +++ b/benchmark/fs/bench-statSync-failure.js @@ -16,7 +16,7 @@ function main({ n, statSyncType }) { bench.start(); for (let i = 0; i < n; i++) { if (statSyncType === 'noThrow') { - fs.statSync(arg, { throws: false }); + fs.statSync(arg, { throwIfNoEntry: false }); } else { try { fs.statSync(arg); diff --git a/doc/api/fs.md b/doc/api/fs.md index e00ec461cc8d10..da4b556e897b61 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -2202,8 +2202,8 @@ changes: * `options` {Object} * `bigint` {boolean} Whether the numeric values in the returned [`fs.Stats`][] object should be `bigint`. **Default:** `false`. - * `throws` {boolean} Whether an exception will be thrown - if stats are not available, rather than returning `undefined`. + * `throwIfNoEntry` {boolean} Whether an exception will be thrown + if no file system entry exists, rather than returning `undefined`. **Default:** `true`. * Returns: {fs.Stats} @@ -2571,8 +2571,8 @@ changes: * `options` {Object} * `bigint` {boolean} Whether the numeric values in the returned [`fs.Stats`][] object should be `bigint`. **Default:** `false`. - * `throws` {boolean} Whether an exception will be thrown - if stats are not available, rather than returning `undefined`. + * `throwIfNoEntry` {boolean} Whether an exception will be thrown + if no file system entry exists, rather than returning `undefined`. **Default:** `true`. * Returns: {fs.Stats} @@ -3816,8 +3816,8 @@ changes: * `options` {Object} * `bigint` {boolean} Whether the numeric values in the returned [`fs.Stats`][] object should be `bigint`. **Default:** `false`. - * `throws` {boolean} Whether an exception will be thrown - if stats are not available, rather than returning `undefined`. + * `throwIfNoEntry` {boolean} Whether an exception will be thrown + if no file system entry exists, rather than returning `undefined`. **Default:** `true`. * Returns: {fs.Stats} diff --git a/lib/fs.js b/lib/fs.js index db65f28818ca03..add92f41bfc0f5 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -72,6 +72,7 @@ const { ERR_FEATURE_UNAVAILABLE_ON_PLATFORM }, hideStackFrames, + uvErrmapGet, uvException } = require('internal/errors'); @@ -1076,47 +1077,51 @@ function stat(path, options = { bigint: false }, callback) { binding.stat(pathModule.toNamespacedPath(path), options.bigint, req); } -function fstatSync(fd, options = { bigint: false, throws: true }) { +function hasNoEntryError(ctx) { + if (ctx.errno) { + const uvErr = uvErrmapGet(ctx.errno); + return uvErr && uvErr[0] === 'ENOENT'; + } + + if (ctx.error) { + return ctx.error.code === 'ENOENT'; + } + + return false; +} + +function fstatSync(fd, options = { bigint: false, throwIfNoEntry: true }) { validateInt32(fd, 'fd', 0); const ctx = { fd }; const stats = binding.fstat(fd, options.bigint, undefined, ctx); - if (options.throws === false) { - if (ctx.errno || ctx.error) { - return undefined; - } - } else { - handleErrorFromBinding(ctx); + if (options.throwIfNoEntry === false && hasNoEntryError(ctx)) { + return undefined; } + handleErrorFromBinding(ctx); return getStatsFromBinding(stats); } -function lstatSync(path, options = { bigint: false, throws: true }) { +function lstatSync(path, options = { bigint: false, throwIfNoEntry: true }) { path = getValidatedPath(path); const ctx = { path }; const stats = binding.lstat(pathModule.toNamespacedPath(path), options.bigint, undefined, ctx); - if (options.throws === false) { - if (ctx.errno || ctx.error) { - return undefined; - } - } else { - handleErrorFromBinding(ctx); + if (options.throwIfNoEntry === false && hasNoEntryError(ctx)) { + return undefined; } + handleErrorFromBinding(ctx); return getStatsFromBinding(stats); } -function statSync(path, options = { bigint: false, throws: true }) { +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.throws === false) { - if (ctx.errno || ctx.error) { - return undefined; - } - } else { - handleErrorFromBinding(ctx); + if (options.throwIfNoEntry === false && hasNoEntryError(ctx)) { + return undefined; } + handleErrorFromBinding(ctx); return getStatsFromBinding(stats); } diff --git a/test/parallel/test-fs-stat-bigint.js b/test/parallel/test-fs-stat-bigint.js index 62c3b8538a926a..cffec39288de4a 100644 --- a/test/parallel/test-fs-stat-bigint.js +++ b/test/parallel/test-fs-stat-bigint.js @@ -122,21 +122,31 @@ if (!common.isWindows) { fs.closeSync(fd); } -const runSyncFailureTest = (func, arg, error) => { - assert.throws(() => func(arg), error); - assert.strictEqual(func(arg, { throws: false }), undefined); -}; - { - runSyncFailureTest(fs.statSync, 'does_not_exist', { code: 'ENOENT' }); + assert.throws( + () => fs.statSync('does_not_exist'), + { code: 'ENOENT' }); + assert.strictEqual( + fs.statSync('does_not_exist', { throwIfNoEntry: false }), + undefined); } { - runSyncFailureTest(fs.lstatSync, 'does_not_exist', { code: 'ENOENT' }); + assert.throws( + () => fs.lstatSync('does_not_exist'), + { code: 'ENOENT' }); + assert.strictEqual( + fs.lstatSync('does_not_exist', { throwIfNoEntry: false }), + undefined); } { - runSyncFailureTest(fs.fstatSync, 9999, { code: 'EBADF' }); + assert.throws( + () => fs.fstatSync(9999), + { code: 'EBADF' }); + assert.throws( + () => fs.fstatSync(9999, { throwIfNoEntry: false }), + { code: 'EBADF' }); } const runCallbackTest = (func, arg, done) => { From f5836e84286c82e8bb981936d3fe4fe1430fb967 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 30 Oct 2020 21:52:04 +0000 Subject: [PATCH 3/4] Drop option from fstatSync --- doc/api/fs.md | 3 --- lib/fs.js | 3 --- test/parallel/test-fs-stat-bigint.js | 7 +------ 3 files changed, 1 insertion(+), 12 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index da4b556e897b61..c83625e8516448 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -2202,9 +2202,6 @@ changes: * `options` {Object} * `bigint` {boolean} Whether the numeric values in the returned [`fs.Stats`][] object should be `bigint`. **Default:** `false`. - * `throwIfNoEntry` {boolean} Whether an exception will be thrown - if no file system entry exists, rather than returning `undefined`. - **Default:** `true`. * Returns: {fs.Stats} Synchronous fstat(2). diff --git a/lib/fs.js b/lib/fs.js index add92f41bfc0f5..cb9312c9d13fbe 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1094,9 +1094,6 @@ function fstatSync(fd, options = { bigint: false, throwIfNoEntry: true }) { validateInt32(fd, 'fd', 0); const ctx = { fd }; const stats = binding.fstat(fd, options.bigint, undefined, ctx); - if (options.throwIfNoEntry === false && hasNoEntryError(ctx)) { - return undefined; - } handleErrorFromBinding(ctx); return getStatsFromBinding(stats); } diff --git a/test/parallel/test-fs-stat-bigint.js b/test/parallel/test-fs-stat-bigint.js index cffec39288de4a..0b122c9f184a4c 100644 --- a/test/parallel/test-fs-stat-bigint.js +++ b/test/parallel/test-fs-stat-bigint.js @@ -141,12 +141,7 @@ if (!common.isWindows) { } { - assert.throws( - () => fs.fstatSync(9999), - { code: 'EBADF' }); - assert.throws( - () => fs.fstatSync(9999, { throwIfNoEntry: false }), - { code: 'EBADF' }); + assert.throws(() => fs.fstatSync(9999), { code: 'EBADF' }); } const runCallbackTest = (func, arg, done) => { From a1815fb2f2187f1e9bd8604be125788a017970af Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 5 Nov 2020 09:49:53 -0700 Subject: [PATCH 4/4] Update test/parallel/test-fs-stat-bigint.js MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Michaël Zasso --- test/parallel/test-fs-stat-bigint.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-fs-stat-bigint.js b/test/parallel/test-fs-stat-bigint.js index 0b122c9f184a4c..cffec39288de4a 100644 --- a/test/parallel/test-fs-stat-bigint.js +++ b/test/parallel/test-fs-stat-bigint.js @@ -141,7 +141,12 @@ if (!common.isWindows) { } { - assert.throws(() => fs.fstatSync(9999), { code: 'EBADF' }); + assert.throws( + () => fs.fstatSync(9999), + { code: 'EBADF' }); + assert.throws( + () => fs.fstatSync(9999, { throwIfNoEntry: false }), + { code: 'EBADF' }); } const runCallbackTest = (func, arg, done) => {