lib: convert WeakMaps in cjs loader with private symbol properties

legendecas · aduh95 · commit f6145aa2ca3f · 2024-04-29T22:46:21.000+02:00
Symbol properties are typically more GC-efficient than using WeakMaps, since WeakMap requires ephemeron GC. `module[kModuleExportNames]` would be easier to read than `importedCJSCache.get(module).exportNames` as well. PR-URL: #52095 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -50,7 +50,6 @@ const {
   ReflectSet,
   RegExpPrototypeExec,
   SafeMap,
-  SafeWeakMap,
   String,
   StringPrototypeCharAt,
   StringPrototypeCharCodeAt,
@@ -62,25 +61,50 @@ const {
   StringPrototypeStartsWith,
   Symbol,
 } = primordials;
+const {
+  privateSymbols: {
+    module_source_private_symbol,
+    module_export_names_private_symbol,
+    module_circular_visited_private_symbol,
+    module_export_private_symbol,
+    module_parent_private_symbol,
+  },
+} = internalBinding('util');
 
 const { kEvaluated } = internalBinding('module_wrap');
 
-// Map used to store CJS parsing data or for ESM loading.
-const importedCJSCache = new SafeWeakMap();
+// Internal properties for Module instances.
+/**
+ * Cached {@link Module} source string.
+ */
+const kModuleSource = module_source_private_symbol;
+/**
+ * Cached {@link Module} export names for ESM loader.
+ */
+const kModuleExportNames = module_export_names_private_symbol;
+/**
+ * {@link Module} circular dependency visited flag.
+ */
+const kModuleCircularVisited = module_circular_visited_private_symbol;
 /**
- * Map of already-loaded CJS modules to use.
+ * {@link Module} export object snapshot for ESM loader.
  */
-const cjsExportsCache = new SafeWeakMap();
-const requiredESMSourceCache = new SafeWeakMap();
+const kModuleExport = module_export_private_symbol;
+/**
+ * {@link Module} parent module.
+ */
+const kModuleParent = module_parent_private_symbol;
 
 const kIsMainSymbol = Symbol('kIsMainSymbol');
 const kIsCachedByESMLoader = Symbol('kIsCachedByESMLoader');
 const kRequiredModuleSymbol = Symbol('kRequiredModuleSymbol');
 const kIsExecuting = Symbol('kIsExecuting');
 // Set first due to cycle with ESM loader functions.
 module.exports = {
-  cjsExportsCache,
-  importedCJSCache,
+  kModuleSource,
+  kModuleExport,
+  kModuleExportNames,
+  kModuleCircularVisited,
   initializeCJS,
   entryPointSource: undefined, // Set below.
   Module,
@@ -257,8 +281,6 @@ function reportModuleNotFoundToWatchMode(basePath, extensions) {
   }
 }
 
-/** @type {Map<Module, Module>} */
-const moduleParentCache = new SafeWeakMap();
 /**
  * Create a new module instance.
  * @param {string} id
@@ -268,7 +290,7 @@ function Module(id = '', parent) {
   this.id = id;
   this.path = path.dirname(id);
   setOwnProperty(this, 'exports', {});
-  moduleParentCache.set(this, parent);
+  this[kModuleParent] = parent;
   updateChildren(parent, this, false);
   this.filename = null;
   this.loaded = false;
@@ -356,17 +378,19 @@ ObjectDefineProperty(BuiltinModule.prototype, 'isPreloading', isPreloadingDesc);
 
 /**
  * Get the parent of the current module from our cache.
+ * @this {Module}
  */
 function getModuleParent() {
-  return moduleParentCache.get(this);
+  return this[kModuleParent];
 }
 
 /**
  * Set the parent of the current module in our cache.
+ * @this {Module}
  * @param {Module} value
  */
 function setModuleParent(value) {
-  moduleParentCache.set(this, value);
+  this[kModuleParent] = value;
 }
 
 let debug = require('internal/util/debuglog').debuglog('module', (fn) => {
@@ -955,7 +979,7 @@ function getExportsForCircularRequire(module) {
   const requiredESM = module[kRequiredModuleSymbol];
   if (requiredESM && requiredESM.getStatus() !== kEvaluated) {
     let message = `Cannot require() ES Module ${module.id} in a cycle.`;
-    const parent = moduleParentCache.get(module);
+    const parent = module[kModuleParent];
     if (parent) {
       message += ` (from ${parent.filename})`;
     }
@@ -1028,25 +1052,24 @@ Module._load = function(request, parent, isMain) {
   const cachedModule = Module._cache[filename];
   if (cachedModule !== undefined) {
     updateChildren(parent, cachedModule, true);
-    if (!cachedModule.loaded) {
-      // If it's not cached by the ESM loader, the loading request
-      // comes from required CJS, and we can consider it a circular
-      // dependency when it's cached.
-      if (!cachedModule[kIsCachedByESMLoader]) {
-        return getExportsForCircularRequire(cachedModule);
-      }
-      // If it's cached by the ESM loader as a way to indirectly pass
-      // the module in to avoid creating it twice, the loading request
-      // come from imported CJS. In that case use the importedCJSCache
-      // to determine if it's loading or not.
-      const importedCJSMetadata = importedCJSCache.get(cachedModule);
-      if (importedCJSMetadata.loading) {
-        return getExportsForCircularRequire(cachedModule);
-      }
-      importedCJSMetadata.loading = true;
-    } else {
+    if (cachedModule.loaded) {
       return cachedModule.exports;
     }
+    // If it's not cached by the ESM loader, the loading request
+    // comes from required CJS, and we can consider it a circular
+    // dependency when it's cached.
+    if (!cachedModule[kIsCachedByESMLoader]) {
+      return getExportsForCircularRequire(cachedModule);
+    }
+    // If it's cached by the ESM loader as a way to indirectly pass
+    // the module in to avoid creating it twice, the loading request
+    // come from imported CJS. In that case use the kModuleCircularVisited
+    // to determine if it's loading or not.
+    if (cachedModule[kModuleCircularVisited]) {
+      return getExportsForCircularRequire(cachedModule);
+    }
+    // This is an ESM loader created cache entry, mark it as visited and fallthrough to loading the module.
+    cachedModule[kModuleCircularVisited] = true;
   }
 
   if (BuiltinModule.canBeRequiredWithoutScheme(filename)) {
@@ -1190,7 +1213,7 @@ Module._resolveFilename = function(request, parent, isMain, options) {
   const requireStack = [];
   for (let cursor = parent;
     cursor;
-    cursor = moduleParentCache.get(cursor)) {
+    cursor = cursor[kModuleParent]) {
     ArrayPrototypePush(requireStack, cursor.filename || cursor.id);
   }
   let message = `Cannot find module '${request}'`;
@@ -1268,9 +1291,7 @@ Module.prototype.load = function(filename) {
   // Create module entry at load time to snapshot exports correctly
   const exports = this.exports;
   // Preemptively cache for ESM loader.
-  if (!cjsExportsCache.has(this)) {
-    cjsExportsCache.set(this, exports);
-  }
+  this[kModuleExport] = exports;
 };
 
 /**
@@ -1313,7 +1334,7 @@ function loadESMFromCJS(mod, filename) {
   const isMain = mod[kIsMainSymbol];
   // TODO(joyeecheung): we may want to invent optional special handling for default exports here.
   // For now, it's good enough to be identical to what `import()` returns.
-  mod.exports = cascadedLoader.importSyncForRequire(mod, filename, source, isMain, moduleParentCache.get(mod));
+  mod.exports = cascadedLoader.importSyncForRequire(mod, filename, source, isMain, mod[kModuleParent]);
 }
 
 /**
@@ -1406,7 +1427,7 @@ Module.prototype._compile = function(content, filename, loadAsESM = false) {
   // Only modules being require()'d really need to avoid TLA.
   if (loadAsESM) {
     // Pass the source into the .mjs extension handler indirectly through the cache.
-    requiredESMSourceCache.set(this, content);
+    this[kModuleSource] = content;
     loadESMFromCJS(this, filename);
     return;
   }
@@ -1467,15 +1488,15 @@ Module.prototype._compile = function(content, filename, loadAsESM = false) {
  * @returns {string}
  */
 function getMaybeCachedSource(mod, filename) {
-  const cached = importedCJSCache.get(mod);
+  // If already analyzed the source, then it will be cached.
   let content;
-  if (cached?.source) {
-    content = cached.source;
-    cached.source = undefined;
+  if (mod[kModuleSource] !== undefined) {
+    content = mod[kModuleSource];
+    mod[kModuleSource] = undefined;
   } else {
     // TODO(joyeecheung): we can read a buffer instead to speed up
     // compilation.
-    content = requiredESMSourceCache.get(mod) ?? fs.readFileSync(filename, 'utf8');
+    content = fs.readFileSync(filename, 'utf8');
   }
   return content;
 }
@@ -1499,7 +1520,7 @@ Module._extensions['.js'] = function(module, filename) {
       }
 
       // This is an error path because `require` of a `.js` file in a `"type": "module"` scope is not allowed.
-      const parent = moduleParentCache.get(module);
+      const parent = module[kModuleParent];
       const parentPath = parent?.filename;
       const packageJsonPath = path.resolve(pkg.path, 'package.json');
       const usesEsm = containsModuleSyntax(content, filename);
diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js
@@ -43,10 +43,11 @@ const {
   stripBOM,
 } = require('internal/modules/helpers');
 const {
-  cjsExportsCache,
-  importedCJSCache,
   kIsCachedByESMLoader,
   Module: CJSModule,
+  kModuleSource,
+  kModuleExport,
+  kModuleExportNames,
 } = require('internal/modules/cjs/loader');
 const { fileURLToPath, pathToFileURL, URL } = require('internal/url');
 let debug = require('internal/util/debuglog').debuglog('esm', (fn) => {
@@ -285,9 +286,9 @@ function createCJSModuleWrap(url, source, isMain, loadCJS = loadCJSModule) {
     }
 
     let exports;
-    if (cjsExportsCache.has(module)) {
-      exports = cjsExportsCache.get(module);
-      cjsExportsCache.delete(module);
+    if (module[kModuleExport] !== undefined) {
+      exports = module[kModuleExport];
+      module[kModuleExport] = undefined;
     } else {
       ({ exports } = module);
     }
@@ -366,18 +367,16 @@ translators.set('commonjs', async function commonjsStrategy(url, source,
 function cjsPreparseModuleExports(filename, source) {
   // TODO: Do we want to keep hitting the user mutable CJS loader here?
   let module = CJSModule._cache[filename];
-  if (module) {
-    const cached = importedCJSCache.get(module);
-    if (cached) {
-      return { module, exportNames: cached.exportNames };
-    }
+  if (module && module[kModuleExportNames] !== undefined) {
+    return { module, exportNames: module[kModuleExportNames] };
   }
   const loaded = Boolean(module);
   if (!loaded) {
     module = new CJSModule(filename);
     module.filename = filename;
     module.paths = CJSModule._nodeModulePaths(module.path);
     module[kIsCachedByESMLoader] = true;
+    module[kModuleSource] = source;
     CJSModule._cache[filename] = module;
   }
 
@@ -392,7 +391,7 @@ function cjsPreparseModuleExports(filename, source) {
   const exportNames = new SafeSet(new SafeArrayIterator(exports));
 
   // Set first for cycles.
-  importedCJSCache.set(module, { source, exportNames });
+  module[kModuleExportNames] = exportNames;
 
   if (reexports.length) {
     module.filename = filename;
diff --git a/src/env_properties.h b/src/env_properties.h
@@ -26,6 +26,11 @@
   V(js_transferable_wrapper_private_symbol, "node:js_transferable_wrapper")    \
   V(entry_point_module_private_symbol, "node:entry_point_module")              \
   V(entry_point_promise_private_symbol, "node:entry_point_promise")            \
+  V(module_source_private_symbol, "node:module_source")                        \
+  V(module_export_names_private_symbol, "node:module_export_names")            \
+  V(module_circular_visited_private_symbol, "node:module_circular_visited")    \
+  V(module_export_private_symbol, "node:module_export")                        \
+  V(module_parent_private_symbol, "node:module_parent")                        \
   V(napi_type_tag, "node:napi:type_tag")                                       \
   V(napi_wrapper, "node:napi:wrapper")                                         \
   V(untransferable_object_private_symbol, "node:untransferableObject")         \
