module: limit negative stat caching to resolution probes

Caching every negative stat reverted the behaviour of
#36642 and broke
parallel/test-module-cache: a module missing on a failed require() and
then created was no longer picked up by a later require() in the same
tree.

Narrow the negative caching to speculative probes -- paths resolution
guesses at rather than paths the user named: the extension candidates
tried by `tryExtensions` and the node_modules ancestors walked for bare
specifiers. A cached negative is likewise only read back by a
speculative probe, so a stat of a user-named path always re-stats. That
restores the #36642 behaviour while keeping the repeated misses, which
are where the win comes from, out of the filesystem.

Rewrite test-module-negative-stat-cache to assert both halves of the
scoped behaviour, and add a benchmark. The benchmark spawns its workload
as a child process's main module because `benchmark/common.js` invokes
main() from a process.nextTick callback, by which point statCache is
already null. At deps=200 depth=12 it shows ~8% improvement.

Signed-off-by: Maxime David <maxday@amazon.com>

Author: maxday

benchmark/module/module-resolve-misses.js

@@ -0,0 +1,58 @@
+'use strict';
+
+const fs = require('fs');
+const path = require('path');
+const { spawnSync } = require('child_process');
+const common = require('../common.js');
+
+const tmpdir = require('../../test/common/tmpdir');
+const benchmarkDirectory = tmpdir.resolve('nodejs-benchmark-module');
+
+const bench = common.createBenchmark(main, {
+  depth: [4, 12],
+  deps: [200],
+  n: [30],
+});
+
+function main({ depth, deps, n }) {
+  tmpdir.refresh();
+
+  // The only `node_modules` that exists: at the root, above the requirer.
+  const nodeModules = path.join(benchmarkDirectory, 'node_modules');
+  for (let i = 0; i < deps; i++) {
+    const dep = path.join(nodeModules, `dep${i}`);
+    fs.mkdirSync(dep, { recursive: true });
+    fs.writeFileSync(
+      path.join(dep, 'package.json'),
+      `{"name":"dep${i}","main":"index.js"}`,
+    );
+    fs.writeFileSync(path.join(dep, 'index.js'), 'module.exports = {};');
+  }
+
+  // The requirer, nested `depth` levels down. Resolution walks up from here and
+  // finds no `node_modules` until the root.
+  const nested = path.join(benchmarkDirectory, ...'x'.repeat(depth).split(''));
+  fs.mkdirSync(nested, { recursive: true });
+
+  let entrySource = '';
+  for (let i = 0; i < deps; i++) {
+    entrySource += `require('dep${i}');\n`;
+  }
+  const entry = path.join(nested, 'entry.js');
+  fs.writeFileSync(entry, entrySource);
+
+  const cmd = process.execPath || process.argv[0];
+  const warmup = 3;
+  for (let i = -warmup; i < n; i++) {
+    if (i === 0) {
+      bench.start();
+    }
+    const child = spawnSync(cmd, [entry]);
+    if (child.status !== 0) {
+      throw new Error(`Child process stopped with exit code ${child.status}`);
+    }
+  }
+  bench.end(n);
+
+  tmpdir.refresh();
+}

lib/internal/modules/cjs/loader.js

@@ -266,29 +266,37 @@ function wrapModuleLoad(request, parent, isMain, options) {
 
 /**
  * Get a path's properties, using an in-memory cache to minimize lookups.
+ *
+ * Successful results (0 = file, 1 = directory) are always cached. Negative
+ * results (libuv error codes, e.g. -ENOENT) are only cached for speculative
+ * probes, i.e. paths that resolution guesses at rather than paths the user
+ * actually asked for: the extension candidates tried by `tryExtensions` and
+ * the `node_modules` ancestor directories walked for bare specifiers. Those
+ * misses are numerous and repeat across sibling and descendant modules, so
+ * caching them is where the win is.
+ *
+ * A cached negative is also only *read back* by a speculative probe. A stat of
+ * a path the user named directly ignores it and re-stats. That keeps the
+ * behaviour of https://github.com/nodejs/node/pull/36642 intact: a module that
+ * is missing on a failed `require()` and then created can still be picked up by
+ * a later `require()` in the same tree.
  * @param {string} filename Absolute path to the file
+ * @param {boolean} [isSpeculativeProbe] Whether this is a resolution guess
+ *   rather than a path the user named, which makes the negative result
+ *   cacheable and lets a previously cached negative be reused.
  * @returns {number}
  */
-function stat(filename) {
+function stat(filename, isSpeculativeProbe = false) {
   // Guard against internal bugs where a non-string filename is passed in by mistake.
   assert(typeof filename === 'string');
 
   filename = path.toNamespacedPath(filename);
   if (statCache !== null) {
     const result = statCache.get(filename);
-    if (result !== undefined) { return result; }
+    if (result !== undefined && (result >= 0 || isSpeculativeProbe)) { return result; }
   }
   const result = internalFsBinding.internalModuleStat(filename);
-  if (statCache !== null) {
-    // Cache both successful results (0 = file, 1 = directory) and negative
-    // results (libuv error codes, e.g. -ENOENT). Negative results are common
-    // and repeated during resolution: `tryExtensions` probes several
-    // non-existent extensions, and bare specifiers walk the `node_modules`
-    // chain upward stat-ing many parent directories that do not exist. The
-    // cache is scoped to a single top-level `require` tree (created and torn
-    // down around `requireDepth === 0`), so caching a negative result carries
-    // the same bounded staleness window that caching a positive one already
-    // does.
+  if (statCache !== null && (result >= 0 || isSpeculativeProbe)) {
     statCache.set(filename, result);
   }
   return result;
@@ -598,10 +606,12 @@ function tryPackage(requestPath, exts, isMain, originalPath) {
  * `--preserve-symlinks-main` and `isMain` is true , keep symlinks intact, otherwise resolve to the absolute realpath.
  * @param {string} requestPath The path to the file to load.
  * @param {boolean} isMain Whether the file is the main module.
+ * @param {boolean} [isSpeculativeProbe] Whether `requestPath` is a resolution
+ *   guess rather than a path the user named. See {@link stat}.
  * @returns {string|undefined}
  */
-function tryFile(requestPath, isMain) {
-  const rc = _stat(requestPath);
+function tryFile(requestPath, isMain, isSpeculativeProbe = false) {
+  const rc = _stat(requestPath, isSpeculativeProbe);
   if (rc !== 0) { return; }
   if (getOptionValue(isMain ? '--preserve-symlinks-main' : '--preserve-symlinks')) {
     return path.resolve(requestPath);
@@ -611,14 +621,16 @@ function tryFile(requestPath, isMain) {
 
 /**
  * Given a path, check if the file exists with any of the set extensions.
+ * Each candidate is a speculative probe: the extension is appended by
+ * resolution, not named by the user, so its misses are cacheable.
  * @param {string} basePath The path and filename without extension
  * @param {string[]} exts The extensions to try
  * @param {boolean} isMain Whether the module is the main module
  * @returns {string|false}
  */
 function tryExtensions(basePath, exts, isMain) {
   for (let i = 0; i < exts.length; i++) {
-    const filename = tryFile(basePath + exts[i], isMain);
+    const filename = tryFile(basePath + exts[i], isMain, true);
 
     if (filename) {
       return filename;
@@ -829,7 +841,10 @@ Module._findPath = function(request, paths, isMain, conditions = getCjsCondition
     if (typeof curPath !== 'string') {
       throw new ERR_INVALID_ARG_TYPE('paths', 'array of strings', paths);
     }
-    if (insidePath && curPath && _stat(curPath) < 1) {
+    // A candidate lookup directory (typically a `node_modules` ancestor) is a
+    // speculative probe: most of the chain does not exist, and every bare
+    // specifier in the tree re-walks the same missing ancestors.
+    if (insidePath && curPath && _stat(curPath, true) < 1) {
       continue;
     }
 

test/parallel/test-module-negative-stat-cache.js

@@ -1,34 +1,70 @@
 'use strict';
 require('../common');
 
-// This tests that the CommonJS loader's per-require-tree stat cache also caches
-// negative (not-found) results, so a path that is missing when first probed is
-// not re-stat-ed for the rest of the require tree.
+// This tests that the CommonJS loader's per-require-tree stat cache caches
+// negative (not-found) results for *speculative* probes: the extension
+// candidates tried by `tryExtensions` and the `node_modules` ancestor
+// directories walked for bare specifiers.
+//
+// A path the user named directly is not negatively cached, so the behaviour of
+// https://github.com/nodejs/node/pull/36642 is preserved: a module that is
+// missing on a failed `require()` and then created is still picked up by a
+// later `require()` in the same tree. That case is covered by
+// test-module-cache.js and asserted again at the end of this file.
+//
+// The stat cache is populated and read internally by the loader, so it is not
+// directly observable from user code. These tests make it observable by
+// mutating the filesystem between two probes of the same path within one
+// require tree.
 
 const assert = require('assert');
 const fs = require('fs');
+const path = require('path');
 const tmpdir = require('../common/tmpdir');
 
 tmpdir.refresh();
 
-// A module path that does not exist yet.
-const generated = tmpdir.resolve('generated.js');
-
-// First probe: the file does not exist -> negative stat, cached.
-assert.throws(
-  () => require(generated),
-  { code: 'MODULE_NOT_FOUND' },
-  'expected the module to be missing before it is created',
-);
-
-// Create the file mid-traversal, in the same require tree.
-fs.writeFileSync(generated, 'module.exports = 1;');
-
-// Second probe, still in the same tree: the negative result is cached, so the
-// loader must serve the cached miss instead of re-stat-ing and observing the
-// freshly-created file.
-assert.throws(
-  () => require(generated),
-  { code: 'MODULE_NOT_FOUND' },
-  'a negative stat result must be cached for the rest of the require tree',
-);
+// An extensionless specifier is resolved by probing `<name>.js`, `<name>.json`,
+// `<name>.node`, ... in order. Those candidate paths are appended by resolution
+// rather than named by the user, so their misses are cached for the rest of the
+// tree.
+{
+  const dir = tmpdir.resolve('speculative');
+  fs.mkdirSync(dir);
+
+  const specifier = path.join(dir, 'mod');
+
+  // Nothing exists yet: every extension candidate misses and is cached.
+  assert.throws(
+    () => require(specifier),
+    { code: 'MODULE_NOT_FOUND' },
+    'expected the module to be missing before it is created',
+  );
+
+  // Create one of the candidates that was just probed and missed.
+  fs.writeFileSync(`${specifier}.js`, 'module.exports = "late";');
+
+  // Resolving the same extensionless specifier probes `mod.js` again, but that
+  // negative result is cached, so the freshly-created file is not observed.
+  assert.throws(
+    () => require(specifier),
+    { code: 'MODULE_NOT_FOUND' },
+    'a negative result for a speculative extension probe must be cached',
+  );
+}
+
+// A path the user named directly is not negatively cached, so creating the file
+// mid-tree makes a later require in the same tree resolve it.
+{
+  const explicit = tmpdir.resolve('explicit.js');
+
+  assert.throws(
+    () => require(explicit),
+    { code: 'MODULE_NOT_FOUND' },
+  );
+
+  fs.writeFileSync(explicit, 'module.exports = "created";');
+
+  // A negative result for a user-named path must not be cached.
+  assert.strictEqual(require(explicit), 'created');
+}