Skip to content

Commit f13589f

Browse files
legendecasmarco-ippolito
authored andcommitted
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: #52058 Backport-PR-URL: #56927 Reviewed-By: Joyee Cheung <[email protected]> Refs: #52697
1 parent b07ad39 commit f13589f

File tree

9 files changed

+219
-229
lines changed

9 files changed

+219
-229
lines changed

‎lib/internal/modules/esm/loader.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -318,7 +318,7 @@ class ModuleLoader{
318318
* @param{object} importAttributes import attributes from the import statement.
319319
* @returns{ModuleJobBase}
320320
*/
321-
getModuleWrapForRequire(specifier,parentURL,importAttributes){
321+
getModuleJobForRequire(specifier,parentURL,importAttributes){
322322
assert(getOptionValue('--experimental-require-module'));
323323

324324
if(canParse(specifier)){

‎lib/internal/modules/esm/module_job.js‎

Lines changed: 57 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
'use strict';
22

33
const{
4+
Array,
45
ArrayPrototypeJoin,
5-
ArrayPrototypePush,
66
ArrayPrototypeSome,
77
FunctionPrototype,
88
ObjectSetPrototypeOf,
@@ -82,31 +82,8 @@ class ModuleJob extends ModuleJobBase{
8282
this.modulePromise=PromiseResolve(this.modulePromise);
8383
}
8484

85-
// Wait for the ModuleWrap instance being linked with all dependencies.
86-
constlink=async()=>{
87-
this.module=awaitthis.modulePromise;
88-
assert(this.moduleinstanceofModuleWrap);
89-
90-
// Explicitly keeping track of dependency jobs is needed in order
91-
// to flatten out the dependency graph below in `_instantiate()`,
92-
// so that circular dependencies can't cause a deadlock by two of
93-
// these `link` callbacks depending on each other.
94-
constdependencyJobs=[];
95-
constpromises=this.module.link(async(specifier,attributes)=>{
96-
constjob=awaitthis.#loader.getModuleJob(specifier,url,attributes);
97-
debug(`async link() ${this.url} -> ${specifier}`,job);
98-
ArrayPrototypePush(dependencyJobs,job);
99-
returnjob.modulePromise;
100-
});
101-
102-
if(promises!==undefined){
103-
awaitSafePromiseAllReturnVoid(promises);
104-
}
105-
106-
returnSafePromiseAllReturnArrayLike(dependencyJobs);
107-
};
10885
// Promise for the list of all dependencyJobs.
109-
this.linked=link();
86+
this.linked=this._link();
11087
// This promise is awaited later anyway, so silence
11188
// 'unhandled rejection' warnings.
11289
PromisePrototypeThen(this.linked,undefined,noop);
@@ -116,6 +93,49 @@ class ModuleJob extends ModuleJobBase{
11693
this.instantiated=undefined;
11794
}
11895

96+
/**
97+
* Iterates the module requests and links with the loader.
98+
* @returns{Promise<ModuleJob[]>} Dependency module jobs.
99+
*/
100+
async_link(){
101+
this.module=awaitthis.modulePromise;
102+
assert(this.moduleinstanceofModuleWrap);
103+
104+
constmoduleRequests=this.module.getModuleRequests();
105+
// Explicitly keeping track of dependency jobs is needed in order
106+
// to flatten out the dependency graph below in `_instantiate()`,
107+
// so that circular dependencies can't cause a deadlock by two of
108+
// these `link` callbacks depending on each other.
109+
// Create an ArrayLike to avoid calling into userspace with `.then`
110+
// when returned from the async function.
111+
constdependencyJobs=Array(moduleRequests.length);
112+
ObjectSetPrototypeOf(dependencyJobs,null);
113+
114+
// Specifiers should be aligned with the moduleRequests array in order.
115+
constspecifiers=Array(moduleRequests.length);
116+
constmodulePromises=Array(moduleRequests.length);
117+
// Iterate with index to avoid calling into userspace with `Symbol.iterator`.
118+
for(letidx=0;idx<moduleRequests.length;idx++){
119+
const{ specifier, attributes }=moduleRequests[idx];
120+
121+
constdependencyJobPromise=this.#loader.getModuleJob(
122+
specifier,this.url,attributes,
123+
);
124+
constmodulePromise=PromisePrototypeThen(dependencyJobPromise,(job)=>{
125+
debug(`async link() ${this.url} -> ${specifier}`,job);
126+
dependencyJobs[idx]=job;
127+
returnjob.modulePromise;
128+
});
129+
modulePromises[idx]=modulePromise;
130+
specifiers[idx]=specifier;
131+
}
132+
133+
constmodules=awaitSafePromiseAllReturnArrayLike(modulePromises);
134+
this.module.link(specifiers,modules);
135+
136+
returndependencyJobs;
137+
}
138+
119139
instantiate(){
120140
if(this.instantiated===undefined){
121141
this.instantiated=this._instantiate();
@@ -269,18 +289,20 @@ class ModuleJobSync extends ModuleJobBase{
269289
super(url,importAttributes,moduleWrap,isMain,inspectBrk,true);
270290
assert(this.moduleinstanceofModuleWrap);
271291
this.#loader =loader;
272-
constmoduleRequests=this.module.getModuleRequestsSync();
273-
constlinked=[];
292+
constmoduleRequests=this.module.getModuleRequests();
293+
// Specifiers should be aligned with the moduleRequests array in order.
294+
constspecifiers=Array(moduleRequests.length);
295+
constmodules=Array(moduleRequests.length);
296+
constjobs=Array(moduleRequests.length);
274297
for(leti=0;i<moduleRequests.length;++i){
275-
const{0: specifier,1: attributes}=moduleRequests[i];
276-
constjob=this.#loader.getModuleWrapForRequire(specifier,url,attributes);
277-
constisLast=(i===moduleRequests.length-1);
278-
// TODO(joyeecheung): make the resolution callback deal with both promisified
279-
// an raw module wraps, then we don't need to wrap it with a promise here.
280-
this.module.cacheResolvedWrapsSync(specifier,PromiseResolve(job.module),isLast);
281-
ArrayPrototypePush(linked,job);
298+
const{ specifier, attributes }=moduleRequests[i];
299+
constjob=this.#loader.getModuleJobForRequire(specifier,url,attributes);
300+
specifiers[i]=specifier;
301+
modules[i]=job.module;
302+
jobs[i]=job;
282303
}
283-
this.linked=linked;
304+
this.module.link(specifiers,modules);
305+
this.linked=jobs;
284306
}
285307

286308
getmodulePromise(){

‎lib/internal/vm/module.js‎

Lines changed: 56 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -2,16 +2,20 @@
22

33
constassert=require('internal/assert');
44
const{
5+
Array,
56
ArrayIsArray,
67
ArrayPrototypeForEach,
78
ArrayPrototypeIndexOf,
9+
ArrayPrototypeMap,
810
ArrayPrototypeSome,
911
ObjectDefineProperty,
1012
ObjectGetPrototypeOf,
1113
ObjectPrototypeHasOwnProperty,
1214
ObjectSetPrototypeOf,
15+
PromiseResolve,
16+
PromisePrototypeThen,
1317
ReflectApply,
14-
SafePromiseAllReturnVoid,
18+
SafePromiseAllReturnArrayLike,
1519
Symbol,
1620
SymbolToStringTag,
1721
TypeError,
@@ -293,44 +297,61 @@ class SourceTextModule extends Module{
293297
importModuleDynamically,
294298
});
295299

296-
this[kLink]=async(linker)=>{
297-
this.#statusOverride ='linking';
300+
this[kDependencySpecifiers]=undefined;
301+
}
298302

299-
constpromises=this[kWrap].link(async(identifier,attributes)=>{
300-
constmodule=awaitlinker(identifier,this,{ attributes,assert: attributes});
301-
if(!isModule(module)){
302-
thrownewERR_VM_MODULE_NOT_MODULE();
303-
}
304-
if(module.context!==this.context){
305-
thrownewERR_VM_MODULE_DIFFERENT_CONTEXT();
306-
}
307-
if(module.status==='errored'){
308-
thrownewERR_VM_MODULE_LINK_FAILURE(`request for '${identifier}' resolved to an errored module`,module.error);
309-
}
310-
if(module.status==='unlinked'){
311-
awaitmodule[kLink](linker);
312-
}
313-
returnmodule[kWrap];
303+
async[kLink](linker){
304+
this.#statusOverride ='linking';
305+
306+
constmoduleRequests=this[kWrap].getModuleRequests();
307+
// Iterates the module requests and links with the linker.
308+
// Specifiers should be aligned with the moduleRequests array in order.
309+
constspecifiers=Array(moduleRequests.length);
310+
constmodulePromises=Array(moduleRequests.length);
311+
// Iterates with index to avoid calling into userspace with `Symbol.iterator`.
312+
for(letidx=0;idx<moduleRequests.length;idx++){
313+
const{ specifier, attributes }=moduleRequests[idx];
314+
315+
constlinkerResult=linker(specifier,this,{
316+
attributes,
317+
assert: attributes,
314318
});
319+
constmodulePromise=PromisePrototypeThen(
320+
PromiseResolve(linkerResult),async(module)=>{
321+
if(!isModule(module)){
322+
thrownewERR_VM_MODULE_NOT_MODULE();
323+
}
324+
if(module.context!==this.context){
325+
thrownewERR_VM_MODULE_DIFFERENT_CONTEXT();
326+
}
327+
if(module.status==='errored'){
328+
thrownewERR_VM_MODULE_LINK_FAILURE(`request for '${specifier}' resolved to an errored module`,module.error);
329+
}
330+
if(module.status==='unlinked'){
331+
awaitmodule[kLink](linker);
332+
}
333+
returnmodule[kWrap];
334+
});
335+
modulePromises[idx]=modulePromise;
336+
specifiers[idx]=specifier;
337+
}
315338

316-
try{
317-
if(promises!==undefined){
318-
awaitSafePromiseAllReturnVoid(promises);
319-
}
320-
}catch(e){
321-
this.#error =e;
322-
throwe;
323-
}finally{
324-
this.#statusOverride =undefined;
325-
}
326-
};
327-
328-
this[kDependencySpecifiers]=undefined;
339+
try{
340+
constmodules=awaitSafePromiseAllReturnArrayLike(modulePromises);
341+
this[kWrap].link(specifiers,modules);
342+
}catch(e){
343+
this.#error =e;
344+
throwe;
345+
}finally{
346+
this.#statusOverride =undefined;
347+
}
329348
}
330349

331350
getdependencySpecifiers(){
332351
validateInternalField(this,kDependencySpecifiers,'SourceTextModule');
333-
this[kDependencySpecifiers]??=this[kWrap].getStaticDependencySpecifiers();
352+
// TODO(legendecas): add a new getter to expose the import attributes as the value type
353+
// of [[RequestedModules]] is changed in https://tc39.es/proposal-import-attributes/#table-cyclic-module-fields.
354+
this[kDependencySpecifiers]??=ArrayPrototypeMap(this[kWrap].getModuleRequests(),(request)=>request.specifier);
334355
returnthis[kDependencySpecifiers];
335356
}
336357

@@ -392,10 +413,10 @@ class SyntheticModule extends Module{
392413
context,
393414
identifier,
394415
});
416+
}
395417

396-
this[kLink]=()=>this[kWrap].link(()=>{
397-
assert.fail('link callback should not be called');
398-
});
418+
[kLink](){
419+
/** nothing to do for synthetic modules */
399420
}
400421

401422
setExport(name,value){

‎src/env_properties.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@
7171
V(args_string, "args") \
7272
V(asn1curve_string, "asn1Curve") \
7373
V(async_ids_stack_string, "async_ids_stack") \
74+
V(attributes_string, "attributes") \
7475
V(base_string, "base") \
7576
V(bits_string, "bits") \
7677
V(block_list_string, "blockList") \
@@ -303,6 +304,7 @@
303304
V(sni_context_string, "sni_context") \
304305
V(source_string, "source") \
305306
V(source_map_url_string, "sourceMapURL") \
307+
V(specifier_string, "specifier") \
306308
V(stack_string, "stack") \
307309
V(standard_name_string, "standardName") \
308310
V(start_time_string, "startTime") \
@@ -377,6 +379,7 @@
377379
V(intervalhistogram_constructor_template, v8::FunctionTemplate) \
378380
V(libuv_stream_wrap_ctor_template, v8::FunctionTemplate) \
379381
V(message_port_constructor_template, v8::FunctionTemplate) \
382+
V(module_wrap_constructor_template, v8::FunctionTemplate) \
380383
V(microtask_queue_ctor_template, v8::FunctionTemplate) \
381384
V(pipe_constructor_template, v8::FunctionTemplate) \
382385
V(promise_wrap_template, v8::ObjectTemplate) \

0 commit comments

Comments
(0)