module: create relative resolve cache bucket lazily

Only create the per-directory relative resolve cache bucket once
there is a resolved filename to store in it, so builtin loads and
failed resolutions no longer leave empty buckets behind. Read
parent.path once per Module._load() call.

Signed-off-by: Sam Attard <sattard@anthropic.com>
Assisted-by: Claude

Author: MarshallOfSound

lib/internal/modules/cjs/loader.js

@@ -1273,20 +1273,16 @@ function loadBuiltinWithHooks(id, url, format) {
  * @returns {object}
  */
 Module._load = function(request, parent, isMain, internalOptions = kEmptyObject) {
+  let parentPath;
   let relResolveCacheByDir;
   if (parent) {
     debug('Module._load REQUEST %s parent: %s', request, parent.id);
     // Fast path for (lazy loaded) modules in the same directory. Keyed by
     // parent directory and then request, so no concatenated cache key
     // string is allocated per require() call.
-    relResolveCacheByDir = relativeResolveCache.get(parent.path);
-    if (relResolveCacheByDir === undefined) {
-      // A plain object handles dynamically built specifier strings
-      // better than a Map here.
-      relResolveCacheByDir = { __proto__: null };
-      relativeResolveCache.set(parent.path, relResolveCacheByDir);
-    }
-    const filename = relResolveCacheByDir[request];
+    parentPath = parent.path;
+    relResolveCacheByDir = relativeResolveCache.get(parentPath);
+    const filename = relResolveCacheByDir?.[request];
     if (filename !== undefined) {
       reportModuleToWatchMode(filename);
       reportModuleToWatchModeFromWorker(filename);
@@ -1401,7 +1397,15 @@ Module._load = function(request, parent, isMain, internalOptions = kEmptyObject)
     module[kFormat] ??= format;
   }
 
-  if (relResolveCacheByDir !== undefined) {
+  if (parent) {
+    // Only create the per-directory bucket once there is an entry to store,
+    // so builtins and failed resolutions don't leave empty buckets behind.
+    if (relResolveCacheByDir === undefined) {
+      // A plain object handles dynamically built specifier strings
+      // better than a Map here.
+      relResolveCacheByDir = { __proto__: null };
+      relativeResolveCache.set(parentPath, relResolveCacheByDir);
+    }
     relResolveCacheByDir[request] = filename;
   }
 

test/parallel/test-module-relative-resolve-cache.js

@@ -0,0 +1,58 @@
+'use strict';
+
+// Checks that Module._load() only creates a relative resolve cache entry for
+// the parent directory when there is a resolved filename to store in it.
+
+require('../common');
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const Module = require('module');
+const tmpdir = require('../common/tmpdir');
+
+tmpdir.refresh();
+
+let uniqueId = 0;
+
+function createParent() {
+  const dir = tmpdir.resolve(`dir-${uniqueId++}`);
+  fs.mkdirSync(dir);
+  const parent = new Module(path.join(dir, 'parent.js'));
+  parent.filename = parent.id;
+  parent.paths = Module._nodeModulePaths(dir);
+  let pathReads = 0;
+  Object.defineProperty(parent, 'path', {
+    __proto__: null,
+    get() {
+      pathReads++;
+      return dir;
+    },
+  });
+  return { dir, parent, getPathReads: () => pathReads };
+}
+
+// Builtins return before the relative resolve cache is populated, so the
+// parent directory should only be looked up, not also used to create a bucket.
+for (const request of ['node:path', 'path']) {
+  const { parent, getPathReads } = createParent();
+  assert.strictEqual(Module._load(request, parent, false), path);
+  assert.strictEqual(getPathReads(), 1);
+}
+
+// Same for resolutions that throw.
+{
+  const { parent, getPathReads } = createParent();
+  assert.throws(() => Module._load('./missing', parent, false),
+                { code: 'MODULE_NOT_FOUND' });
+  assert.strictEqual(getPathReads(), 1);
+}
+
+// A successful load populates the cache, and the second load hits it.
+{
+  const { dir, parent, getPathReads } = createParent();
+  fs.writeFileSync(path.join(dir, 'child.js'), 'module.exports = {};');
+  const first = Module._load('./child', parent, false);
+  assert.strictEqual(getPathReads(), 1);
+  assert.strictEqual(Module._load('./child', parent, false), first);
+  assert.strictEqual(getPathReads(), 2);
+}