Skip to content

Commit

Permalink
lib,src: iterate module requests of a module wrap in JS
Browse files Browse the repository at this point in the history 
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: #52058
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
  • Loading branch information
@legendecas
legendecas authored Apr 25, 2024
1 parent 708bffa commit d5c7ffd
Show file tree
Hide file tree
Showing 7 changed files with 199 additions and 229 deletions.
2 changes: 1 addition & 1 deletion lib/internal/modules/esm/loader.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,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)) {
Expand Down
92 changes: 57 additions & 35 deletions lib/internal/modules/esm/module_job.js
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
'use strict';

const {
Array,
ArrayPrototypeJoin,
ArrayPrototypePush,
ArrayPrototypeSome,
FunctionPrototype,
ObjectSetPrototypeOf,
Expand Down Expand Up @@ -87,31 +87,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);
Expand All @@ -121,6 +98,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();
Expand Down Expand Up @@ -277,18 +297,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() {
Expand Down
92 changes: 57 additions & 35 deletions lib/internal/vm/module.js
View file
Original file line number Diff line number Diff line change
@@ -1,17 +1,21 @@
'use strict';

const {
Array,
ArrayIsArray,
ArrayPrototypeForEach,
ArrayPrototypeIndexOf,
ArrayPrototypeMap,
ArrayPrototypeSome,
ObjectDefineProperty,
ObjectFreeze,
ObjectGetPrototypeOf,
ObjectPrototypeHasOwnProperty,
ObjectSetPrototypeOf,
PromiseResolve,
PromisePrototypeThen,

Check failure on line 16 in lib/internal/vm/module.js

View workflow job for this annotation

GitHub Actions / lint-js-and-md 

Out of ASCIIbetical order - PromiseResolve >= PromisePrototypeThen
ReflectApply,
SafePromiseAllReturnVoid,
SafePromiseAllReturnArrayLike,
Symbol,
SymbolToStringTag,
TypeError,
Expand Down Expand Up @@ -294,44 +298,62 @@ 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');
this[kDependencySpecifiers] ??= ObjectFreeze(this[kWrap].getStaticDependencySpecifiers());
// TODO(legendecas): add a new getter to expose the import attributes as the value type
// of [[RequestedModules]] is changed in https://tc39.es/proposal-import-attributes/#table-cyclic-module-fields.
this[kDependencySpecifiers] ??= ObjectFreeze(
ArrayPrototypeMap(this[kWrap].getModuleRequests(), (request) => request.specifier));
return this[kDependencySpecifiers];
}

Expand Down Expand Up @@ -393,10 +415,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) {
Expand Down
3 changes: 3 additions & 0 deletions src/env_properties.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,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") \
Expand Down Expand Up @@ -307,6 +308,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") \
Expand Down Expand Up @@ -384,6 +386,7 @@
V(js_transferable_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) \
Expand Down
Loading

0 comments on commit d5c7ffd

Please sign in to comment.