Make the resolve cache a resolve cache

nodejs · nodejs-github-bot · Jul 26, 2023 · Apr 14, 2023 · Jun 10, 2023 · Jul 2, 2023
commit aabe393c137ef4b7b645039e7dd93327553146a6
diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js
@@ -6,7 +6,6 @@ require('internal/modules/cjs/loader');
 const {
   FunctionPrototypeCall,
   ObjectSetPrototypeOf,
-  PromisePrototypeThen,
   SafeWeakMap,
 } = primordials;
 
@@ -16,22 +15,21 @@ const {
 const { getOptionValue } = require('internal/options');
 const { pathToFileURL } = require('internal/url');
 const { emitExperimentalWarning } = require('internal/util');
-const { isPromise } = require('internal/util/types');
 const {
   getDefaultConditions,
 } = require('internal/modules/esm/utils');
 let defaultResolve, defaultLoad, importMetaInitializer;
 
-function newResolveCache() {
-  const { ResolveCache } = require('internal/modules/esm/module_map');
-  return new ResolveCache();
-}
-
 function newLoadCache() {
   const { LoadCache } = require('internal/modules/esm/module_map');
   return new LoadCache();
 }
 
+function newResolveCache() {
+  const { ResolveCache } = require('internal/modules/esm/module_map');
+  return new ResolveCache();
+}
+
 function getTranslators() {
   const { translators } = require('internal/modules/esm/translators');
   return translators;
@@ -68,11 +66,6 @@ class ModuleLoader {
    */
   #defaultConditions = getDefaultConditions();
 
-  /**
-   * Import cache
-   */
-  #resolveCache = newResolveCache();
-
   /**
    * Map of already-loaded CJS modules to use
    */
@@ -86,7 +79,12 @@ class ModuleLoader {
   /**
    * Registry of loaded modules, akin to `require.cache`
    */
-  moduleMap = newLoadCache();
+  loadCache = newLoadCache();
+
+  /**
+   * Registry of resolved URLs
+   */
+  #resolveCache = newResolveCache();
 
   /**
    * Methods which translate input code or other information into ES modules
@@ -195,7 +193,7 @@ class ModuleLoader {
     const ModuleJob = require('internal/modules/esm/module_job');
     const job = new ModuleJob(
       this, url, undefined, evalInstance, false, false);
-    this.moduleMap.set(url, undefined, job);
+    this.loadCache.set(url, undefined, job);
     const { module } = await job.run();
 
     return {
@@ -225,11 +223,11 @@ class ModuleLoader {
   getJobFromResolveResult(resolveResult, parentURL, importAssertions) {
     const { url, format } = resolveResult;
     const resolvedImportAssertions = resolveResult.importAssertions ?? importAssertions;
-    let job = this.moduleMap.get(url, resolvedImportAssertions.type);
+    let job = this.loadCache.get(url, resolvedImportAssertions.type);
 
     // CommonJS will set functions for lazy job evaluation.
     if (typeof job === 'function') {
-      this.moduleMap.set(url, undefined, job = job());
+      this.loadCache.set(url, undefined, job = job());
     }
 
     if (job === undefined) {
@@ -289,15 +287,11 @@ class ModuleLoader {
       inspectBrk,
     );
 
-    this.moduleMap.set(url, importAssertions.type, job);
+    this.loadCache.set(url, importAssertions.type, job);
 
     return job;
   }
 
-  cacheStaticImportResult(specifier, parentURL, importAttributes, job) {
-    this.#resolveCache.setLazy(specifier, parentURL, importAttributes, () => job.module.getNamespace());
-  }
-
   /**
    * This method is usually called indirectly as part of the loading processes.
    * Use directly with caution.
@@ -308,50 +302,8 @@ class ModuleLoader {
    * @returns {Promise<ModuleExports>}
    */
   async import(specifier, parentURL, importAssertions) {
-    const { internalCache, serializedModuleRequest } =
-      this.#resolveCache.getSerialized(specifier, parentURL, importAssertions);
-    const removeCache = () => {
-      // Remove the cache entry if the import fails.
-      delete internalCache[serializedModuleRequest];
-    };
-
-    // If there is no entry in the cache for this import, create one:
-    if (internalCache[serializedModuleRequest] == null) {
-      const result = this.#import(specifier, parentURL, importAssertions);
-      // Cache the Promise for now:
-      internalCache[serializedModuleRequest] = result;
-      PromisePrototypeThen(result, (result) => {
-        // Once the promise has resolved, we can cache the ModuleJob itself.
-        internalCache[serializedModuleRequest] = result;
-      }, removeCache);
-      return result;
-    }
-
-    // If the cached value is not a promise, it's already been successfully loaded:
-    if (!isPromise(internalCache[serializedModuleRequest])) {
-      return internalCache[serializedModuleRequest];
-    }
-
-    // If it's still a promise, we must have a fallback in case it fails:
-    const fallback = () => {
-      // If another fallback has already cached a promise, use this one:
-      if (internalCache[serializedModuleRequest] != null) {
-        return PromisePrototypeThen(internalCache[serializedModuleRequest], undefined, fallback);
-      }
-      // Otherwise create a new cache entry:
-      const result = this.#import(specifier, parentURL, importAssertions);
-      internalCache[serializedModuleRequest] = result;
-      PromisePrototypeThen(result, undefined, removeCache);
-      return result;
-    };
-    return PromisePrototypeThen(internalCache[serializedModuleRequest], undefined, fallback);
-  }
-
-  async #import(specifier, parentURL, importAssertions) {
-    const moduleJob = this.getModuleJob(specifier, parentURL, importAssertions);
+    const moduleJob = await this.getModuleJob(specifier, parentURL, importAssertions);
     const { module } = await moduleJob.run();
-
-    this.#resolveCache.set(specifier, parentURL, importAssertions, moduleJob);
     return module.getNamespace();
   }
 
@@ -373,13 +325,20 @@ class ModuleLoader {
    * @param {string} [parentURL] The URL path of the module's parent.
    * @param {ImportAssertions} importAssertions Assertions from the import
    *                                            statement or expression.
-   * @returns {Promise<{ format: string, url: URL['href'] }>}
+   * @returns {{ format: string, url: URL['href'] }}
    */
   resolve(originalSpecifier, parentURL, importAssertions) {
     if (this.#customizations) {
       return this.#customizations.resolve(originalSpecifier, parentURL, importAssertions);
     }
-    return this.defaultResolve(originalSpecifier, parentURL, importAssertions);
+    const requestKey = this.#resolveCache.serializeKey(originalSpecifier, importAssertions);
+    const cachedResult = this.#resolveCache.get(requestKey, parentURL);
+    if (cachedResult != null) {
+      return cachedResult;
+    }
+    const result = this.defaultResolve(originalSpecifier, parentURL, importAssertions);
+    this.#resolveCache.set(requestKey, parentURL, result);
+    return result;
   }
 
   /**

diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js
@@ -75,9 +75,7 @@ class ModuleJob {
       const promises = this.module.link(async (specifier, assertions) => {
         const job = await this.loader.getModuleJob(specifier, url, assertions);
         ArrayPrototypePush(dependencyJobs, job);
-        const result = await job.modulePromise;
-        this.loader.cacheStaticImportResult(specifier, url, attributes, job);
-        return result;
+        return job.modulePromise;
       });
 
       if (promises !== undefined)

diff --git a/lib/internal/modules/esm/module_map.js b/lib/internal/modules/esm/module_map.js
@@ -5,12 +5,10 @@ const {
   ArrayPrototypeMap,
   ArrayPrototypeSort,
   JSONStringify,
-  ObjectDefineProperty,
   ObjectKeys,
   SafeMap,
 } = primordials;
 const { kImplicitAssertType } = require('internal/modules/esm/assert');
-const { setOwnProperty } = require('internal/util');
 let debug = require('internal/util/debuglog').debuglog('esm', (fn) => {
   debug = fn;
 });
@@ -31,82 +29,54 @@ class ResolveCache extends SafeMap {
    *
    * It is exposed to allow more efficient read and overwrite a cache entry.
    * @param {string} specifier
-   * @param {string} parentURL
    * @param {Record<string,string>} importAssertions
-   * @returns {{
-   *   serializedModuleRequest: string,
-   *   internalCache: Record<string, Promise<import('./loader').ModuleExports> | import('./loader').ModuleExports>,
-   * }}
+   * @returns {string}
    */
-  getSerialized(specifier, parentURL, importAssertions) {
-    let internalCache = super.get(parentURL);
-    if (internalCache == null) {
-      super.set(parentURL, internalCache = { __proto__: null });
-    }
-
+  serializeKey(specifier, importAssertions) {
     // To serialize the ModuleRequest (specifier + list of import attributes),
     // we need to sort the attributes by key, then stringifying,
     // so that different import statements with the same attributes are always treated
     // as identical.
-    const serializedModuleRequest = specifier + ArrayPrototypeJoin(
+    return specifier + '::' + ArrayPrototypeJoin(
       ArrayPrototypeMap(
         ArrayPrototypeSort(ObjectKeys(importAssertions)),
         (key) => JSONStringify(key) + JSONStringify(importAssertions[key])),
       ',');
-
-    return { internalCache, serializedModuleRequest };
   }
 
   /**
-   * @param {string} specifier
+   * @param {string} serializedKey
    * @param {string} parentURL
-   * @param {Record<string, string>} importAttributes
    * @returns {import('./loader').ModuleExports | Promise<import('./loader').ModuleExports>}
    */
-  get(specifier, parentURL, importAttributes) {
-    const { internalCache, serializedModuleRequest } = this.getSerialized(specifier, parentURL, importAttributes);
-    return internalCache[serializedModuleRequest];
-  }
-
-  /**
-   * @param {string} specifier
-   * @param {string} parentURL
-   * @param {Record<string, string>} importAttributes
-   * @param {import('./loader').ModuleExports | Promise<import('./loader').ModuleExports>} job
-   */
-  set(specifier, parentURL, importAttributes, job) {
-    const { internalCache, serializedModuleRequest } = this.getSerialized(specifier, parentURL, importAttributes);
-    internalCache[serializedModuleRequest] = job;
-    return this;
+  get(serializedKey, parentURL) {
+    let internalCache = super.get(parentURL);
+    if (internalCache == null) {
+      super.set(parentURL, internalCache = { __proto__: null });
+    }
+    return internalCache[serializedKey];
   }
 
   /**
-   * Adds a cache entry that won't be evaluated until the first time someone tries to read it.
-   *
-   * Static imports need to be lazily cached as the link step is done before the
-   * module exports are actually available.
-   * @param {string} specifier
+   * @param {string} serializedKey
    * @param {string} parentURL
-   * @param {Record<string, string>} importAttributes
-   * @param {() => import('./loader').ModuleExports} getJob
+   * @param {{ format: string, url: URL['href'] }} result
    */
-  setLazy(specifier, parentURL, importAttributes, getJob) {
-    const { internalCache, serializedModuleRequest } = this.getSerialized(specifier, parentURL, importAttributes);
-    ObjectDefineProperty(internalCache, serializedModuleRequest, {
-      __proto__: null,
-      configurable: true,
-      get() {
-        const val = getJob();
-        setOwnProperty(internalCache, serializedModuleRequest, val);
-        return val;
-      },
-    });
+  set(serializedKey, parentURL, result) {
+    let internalCache = super.get(parentURL);
+    if (internalCache == null) {
+      super.set(parentURL, internalCache = { __proto__: null });
+    }
+    internalCache[serializedKey] = result;
     return this;
   }
 
-  has(specifier, parentURL, importAttributes) {
-    const { internalCache, serializedModuleRequest } = this.getSerialized(specifier, parentURL, importAttributes);
-    return serializedModuleRequest in internalCache;
+  has(serializedKey, parentURL) {
+    let internalCache = super.get(parentURL);
+    if (internalCache == null) {
+      super.set(parentURL, internalCache = { __proto__: null });
+    }
+    return serializedKey in internalCache;
   }
 }
 

diff --git a/test/es-module/test-esm-loader-modulemap.js b/test/es-module/test-esm-loader-modulemap.js
@@ -5,7 +5,7 @@ require('../common');
 
 const { strictEqual, throws } = require('assert');
 const { createModuleLoader } = require('internal/modules/esm/loader');
-const { LoadCache } = require('internal/modules/esm/module_map');
+const { LoadCache, ResolveCache } = require('internal/modules/esm/module_map');
 const ModuleJob = require('internal/modules/esm/module_job');
 const createDynamicModule = require(
   'internal/modules/esm/create_dynamic_module');
@@ -98,3 +98,21 @@ const jsonModuleJob = new ModuleJob(loader, stubJsonModule.module,
     });
   });
 }
+
+{
+  const resolveMap = new ResolveCache();
+
+  strictEqual(resolveMap.serializeKey('./file', { __proto__: null }), './file::');
+  strictEqual(resolveMap.serializeKey('./file', { __proto__: null, type: 'json' }), './file::"type""json"');
+  strictEqual(resolveMap.serializeKey('./file::"type""json"', { __proto__: null }), './file::"type""json"::');
+  strictEqual(resolveMap.serializeKey('./file', { __proto__: null, c: 'd', a: 'b' }), './file::"a""b","c""d"');
+  strictEqual(resolveMap.serializeKey('./s', { __proto__: null, c: 'd', a: 'b', b: 'c' }), './s::"a""b","b""c","c""d"');
+
+  resolveMap.set('key1', 'parent1', 1);
+  resolveMap.set('key2', 'parent1', 2);
+  resolveMap.set('key2', 'parent2', 3);
+
+  strictEqual(resolveMap.get('key1', 'parent1'), 1);
+  strictEqual(resolveMap.get('key2', 'parent1'), 2);
+  strictEqual(resolveMap.get('key2', 'parent2'), 3);
+}