lib,src: iterate module requests of a module wrap in JS

Avoid repetitively calling into JS callback from C++ in `ModuleWrap::Link`. This removes the convoluted callback style of the internal `ModuleWrap` link step. PR-URL: nodejs#52058 Reviewed-By: Joyee Cheung <[email protected]>
joyeecheung · Jun 18, 2024 · 6cc0e5f · 6cc0e5f
1 parent fc4db31
commit 6cc0e5f
Show file tree

Hide file tree

Showing 7 changed files with 196 additions and 229 deletions.
diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js
@@ -318,7 +318,7 @@ class ModuleLoader {
    * @param {object} importAttributes import attributes from the import statement.
    * @returns {ModuleJobBase}
    */
-  getModuleWrapForRequire(specifier, parentURL, importAttributes) {
+  getModuleJobForRequire(specifier, parentURL, importAttributes) {
     assert(getOptionValue('--experimental-require-module'));
 
     if (canParse(specifier)) {

diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js
@@ -1,8 +1,8 @@
 'use strict';
 
 const {
+  Array,
   ArrayPrototypeJoin,
-  ArrayPrototypePush,
   ArrayPrototypeSome,
   FunctionPrototype,
   ObjectSetPrototypeOf,
@@ -82,31 +82,8 @@ class ModuleJob extends ModuleJobBase {
       this.modulePromise = PromiseResolve(this.modulePromise);
     }
 
-    // Wait for the ModuleWrap instance being linked with all dependencies.
-    const link = async () => {
-      this.module = await this.modulePromise;
-      assert(this.module instanceof ModuleWrap);
-
-      // Explicitly keeping track of dependency jobs is needed in order
-      // to flatten out the dependency graph below in `_instantiate()`,
-      // so that circular dependencies can't cause a deadlock by two of
-      // these `link` callbacks depending on each other.
-      const dependencyJobs = [];
-      const promises = this.module.link(async (specifier, attributes) => {
-        const job = await this.#loader.getModuleJob(specifier, url, attributes);
-        debug(`async link() ${this.url} -> ${specifier}`, job);
-        ArrayPrototypePush(dependencyJobs, job);
-        return job.modulePromise;
-      });
-
-      if (promises !== undefined) {
-        await SafePromiseAllReturnVoid(promises);
-      }
-
-      return SafePromiseAllReturnArrayLike(dependencyJobs);
-    };
     // Promise for the list of all dependencyJobs.
-    this.linked = link();
+    this.linked = this._link();
     // This promise is awaited later anyway, so silence
     // 'unhandled rejection' warnings.
     PromisePrototypeThen(this.linked, undefined, noop);
@@ -116,6 +93,49 @@ class ModuleJob extends ModuleJobBase {
     this.instantiated = undefined;
   }
 
+  /**
+   * Iterates the module requests and links with the loader.
+   * @returns {Promise<ModuleJob[]>} Dependency module jobs.
+   */
+  async _link() {
+    this.module = await this.modulePromise;
+    assert(this.module instanceof ModuleWrap);
+
+    const moduleRequests = this.module.getModuleRequests();
+    // Explicitly keeping track of dependency jobs is needed in order
+    // to flatten out the dependency graph below in `_instantiate()`,
+    // so that circular dependencies can't cause a deadlock by two of
+    // these `link` callbacks depending on each other.
+    // Create an ArrayLike to avoid calling into userspace with `.then`
+    // when returned from the async function.
+    const dependencyJobs = Array(moduleRequests.length);
+    ObjectSetPrototypeOf(dependencyJobs, null);
+
+    // Specifiers should be aligned with the moduleRequests array in order.
+    const specifiers = Array(moduleRequests.length);
+    const modulePromises = Array(moduleRequests.length);
+    // Iterate with index to avoid calling into userspace with `Symbol.iterator`.
+    for (let idx = 0; idx < moduleRequests.length; idx++) {
+      const { specifier, attributes } = moduleRequests[idx];
+
+      const dependencyJobPromise = this.#loader.getModuleJob(
+        specifier, this.url, attributes,
+      );
+      const modulePromise = PromisePrototypeThen(dependencyJobPromise, (job) => {
+        debug(`async link() ${this.url} -> ${specifier}`, job);
+        dependencyJobs[idx] = job;
+        return job.modulePromise;
+      });
+      modulePromises[idx] = modulePromise;
+      specifiers[idx] = specifier;
+    }
+
+    const modules = await SafePromiseAllReturnArrayLike(modulePromises);
+    this.module.link(specifiers, modules);
+
+    return dependencyJobs;
+  }
+
   instantiate() {
     if (this.instantiated === undefined) {
       this.instantiated = this._instantiate();
@@ -269,18 +289,20 @@ class ModuleJobSync extends ModuleJobBase {
     super(url, importAttributes, moduleWrap, isMain, inspectBrk, true);
     assert(this.module instanceof ModuleWrap);
     this.#loader = loader;
-    const moduleRequests = this.module.getModuleRequestsSync();
-    const linked = [];
+    const moduleRequests = this.module.getModuleRequests();
+    // Specifiers should be aligned with the moduleRequests array in order.
+    const specifiers = Array(moduleRequests.length);
+    const modules = Array(moduleRequests.length);
+    const jobs = Array(moduleRequests.length);
     for (let i = 0; i < moduleRequests.length; ++i) {
-      const { 0: specifier, 1: attributes } = moduleRequests[i];
-      const job = this.#loader.getModuleWrapForRequire(specifier, url, attributes);
-      const isLast = (i === moduleRequests.length - 1);
-      // TODO(joyeecheung): make the resolution callback deal with both promisified
-      // an raw module wraps, then we don't need to wrap it with a promise here.
-      this.module.cacheResolvedWrapsSync(specifier, PromiseResolve(job.module), isLast);
-      ArrayPrototypePush(linked, job);
+      const { specifier, attributes } = moduleRequests[i];
+      const job = this.#loader.getModuleJobForRequire(specifier, url, attributes);
+      specifiers[i] = specifier;
+      modules[i] = job.module;
+      jobs[i] = job;
     }
-    this.linked = linked;
+    this.module.link(specifiers, modules);
+    this.linked = jobs;
   }
 
   get modulePromise() {

diff --git a/lib/internal/vm/module.js b/lib/internal/vm/module.js
@@ -2,16 +2,20 @@
 
 const assert = require('internal/assert');
 const {
+  Array,
   ArrayIsArray,
   ArrayPrototypeForEach,
   ArrayPrototypeIndexOf,
+  ArrayPrototypeMap,
   ArrayPrototypeSome,
   ObjectDefineProperty,
   ObjectGetPrototypeOf,
   ObjectPrototypeHasOwnProperty,
   ObjectSetPrototypeOf,
+  PromiseResolve,
+  PromisePrototypeThen,
   ReflectApply,
-  SafePromiseAllReturnVoid,
+  SafePromiseAllReturnArrayLike,
   Symbol,
   SymbolToStringTag,
   TypeError,
@@ -293,45 +297,60 @@ class SourceTextModule extends Module {
       importModuleDynamically,
     });
 
-    this[kLink] = async (linker) => {
-      this.#statusOverride = 'linking';
+    this[kDependencySpecifiers] = undefined;
+  }
 
-      const promises = this[kWrap].link(async (identifier, attributes) => {
-        const module = await linker(identifier, this, { attributes, assert: attributes });
-        if (!isModule(module)) {
-          throw new ERR_VM_MODULE_NOT_MODULE();
-        }
-        if (module.context !== this.context) {
-          throw new ERR_VM_MODULE_DIFFERENT_CONTEXT();
-        }
-        if (module.status === 'errored') {
-          throw new ERR_VM_MODULE_LINK_FAILURE(`request for '${identifier}' resolved to an errored module`, module.error);
-        }
-        if (module.status === 'unlinked') {
-          await module[kLink](linker);
-        }
-        return module[kWrap];
+  async [kLink](linker) {
+    this.#statusOverride = 'linking';
+
+    const moduleRequests = this[kWrap].getModuleRequests();
+    // Iterates the module requests and links with the linker.
+    // Specifiers should be aligned with the moduleRequests array in order.
+    const specifiers = Array(moduleRequests.length);
+    const modulePromises = Array(moduleRequests.length);
+    // Iterates with index to avoid calling into userspace with `Symbol.iterator`.
+    for (let idx = 0; idx < moduleRequests.length; idx++) {
+      const { specifier, attributes } = moduleRequests[idx];
+
+      const linkerResult = linker(specifier, this, {
+        attributes,
+        assert: attributes,
       });
+      const modulePromise = PromisePrototypeThen(
+        PromiseResolve(linkerResult), async (module) => {
+          if (!isModule(module)) {
+            throw new ERR_VM_MODULE_NOT_MODULE();
+          }
+          if (module.context !== this.context) {
+            throw new ERR_VM_MODULE_DIFFERENT_CONTEXT();
+          }
+          if (module.status === 'errored') {
+            throw new ERR_VM_MODULE_LINK_FAILURE(`request for '${specifier}' resolved to an errored module`, module.error);
+          }
+          if (module.status === 'unlinked') {
+            await module[kLink](linker);
+          }
+          return module[kWrap];
+        });
+      modulePromises[idx] = modulePromise;
+      specifiers[idx] = specifier;
+    }
 
-      try {
-        if (promises !== undefined) {
-          await SafePromiseAllReturnVoid(promises);
-        }
-      } catch (e) {
-        this.#error = e;
-        throw e;
-      } finally {
-        this.#statusOverride = undefined;
-      }
-    };
-
-    this[kDependencySpecifiers] = undefined;
+    try {
+      const modules = await SafePromiseAllReturnArrayLike(modulePromises);
+      this[kWrap].link(specifiers, modules);
+    } catch (e) {
+      this.#error = e;
+      throw e;
+    } finally {
+      this.#statusOverride = undefined;
+    }
   }
 
   get dependencySpecifiers() {
     validateInternalField(this, kDependencySpecifiers, 'SourceTextModule');
     if (this[kDependencySpecifiers] === undefined) {
-      this[kDependencySpecifiers] = this[kWrap].getStaticDependencySpecifiers();
+      this[kDependencySpecifiers] = ArrayPrototypeMap(this[kWrap].getModuleRequests(), (request) => request.specifier);
     }
     return this[kDependencySpecifiers];
   }
@@ -394,10 +413,10 @@ class SyntheticModule extends Module {
       context,
       identifier,
     });
+  }
 
-    this[kLink] = () => this[kWrap].link(() => {
-      assert.fail('link callback should not be called');
-    });
+  [kLink]() {
+    /** nothing to do for synthetic modules */
   }
 
   setExport(name, value) {

diff --git a/src/env_properties.h b/src/env_properties.h
@@ -70,6 +70,7 @@
   V(args_string, "args")                                                       \
   V(asn1curve_string, "asn1Curve")                                             \
   V(async_ids_stack_string, "async_ids_stack")                                 \
+  V(attributes_string, "attributes")                                           \
   V(base_string, "base")                                                       \
   V(bits_string, "bits")                                                       \
   V(block_list_string, "blockList")                                            \
@@ -299,6 +300,7 @@
   V(sni_context_string, "sni_context")                                         \
   V(source_string, "source")                                                   \
   V(source_map_url_string, "sourceMapURL")                                     \
+  V(specifier_string, "specifier")                                             \
   V(stack_string, "stack")                                                     \
   V(standard_name_string, "standardName")                                      \
   V(start_time_string, "startTime")                                            \
@@ -373,6 +375,7 @@
   V(intervalhistogram_constructor_template, v8::FunctionTemplate)              \
   V(libuv_stream_wrap_ctor_template, v8::FunctionTemplate)                     \
   V(message_port_constructor_template, v8::FunctionTemplate)                   \
+  V(module_wrap_constructor_template, v8::FunctionTemplate)                    \
   V(microtask_queue_ctor_template, v8::FunctionTemplate)                       \
   V(pipe_constructor_template, v8::FunctionTemplate)                           \
   V(promise_wrap_template, v8::ObjectTemplate)                                 \