From 16fd0b6273b7a3b78ee44d8b5377d4b8061f7184 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Fri, 31 May 2024 18:50:02 -0700 Subject: [PATCH 1/5] It works --- core/modules/loaders.rs | 10 ++-- core/modules/map.rs | 96 ++++++++++++++++++++++++---------- core/modules/recursive_load.rs | 6 ++- 3 files changed, 79 insertions(+), 33 deletions(-) diff --git a/core/modules/loaders.rs b/core/modules/loaders.rs index edab4a809..df8809a0d 100644 --- a/core/modules/loaders.rs +++ b/core/modules/loaders.rs @@ -76,7 +76,7 @@ pub trait ModuleLoader { /// It's not required to implement this method. fn prepare_load( &self, - _module_specifier: &ModuleSpecifier, + _module_specifiers: &[ModuleSpecifier], _maybe_referrer: Option, _is_dyn_import: bool, ) -> Pin>>> { @@ -220,7 +220,7 @@ impl ModuleLoader for ExtModuleLoader { fn prepare_load( &self, - _specifier: &ModuleSpecifier, + _specifiers: &[ModuleSpecifier], _maybe_referrer: Option, _is_dyn_import: bool, ) -> Pin>>> { @@ -275,7 +275,7 @@ impl ModuleLoader for LazyEsmModuleLoader { fn prepare_load( &self, - _specifier: &ModuleSpecifier, + _specifiers: &[ModuleSpecifier], _maybe_referrer: Option, _is_dyn_import: bool, ) -> Pin>>> { @@ -464,14 +464,14 @@ impl ModuleLoader for TestingModuleLoader { fn prepare_load( &self, - module_specifier: &ModuleSpecifier, + module_specifiers: &[ModuleSpecifier], maybe_referrer: Option, is_dyn_import: bool, ) -> Pin>>> { self.prepare_count.set(self.prepare_count.get() + 1); self .loader - .prepare_load(module_specifier, maybe_referrer, is_dyn_import) + .prepare_load(module_specifiers, maybe_referrer, is_dyn_import) } fn load( diff --git a/core/modules/map.rs b/core/modules/map.rs index 64f069c91..937d290d1 100644 --- a/core/modules/map.rs +++ b/core/modules/map.rs @@ -27,6 +27,7 @@ use crate::runtime::JsRealm; use crate::runtime::SnapshotLoadDataStore; use crate::runtime::SnapshotStoreDataStore; use crate::FastStaticString; +use crate::FastString; use crate::JsRuntime; use crate::ModuleLoadResponse; use crate::ModuleSource; @@ -41,6 +42,8 @@ use futures::task::noop_waker_ref; use futures::task::AtomicWaker; use futures::Future; use futures::StreamExt; +use smallvec::smallvec; +use smallvec::SmallVec; use v8::Function; use v8::PromiseState; @@ -61,7 +64,7 @@ use super::LazyEsmModuleLoader; use super::RequestedModuleType; type PrepareLoadFuture = - dyn Future)>; + dyn Future, Result<(), Error>)>; type CodeCacheReadyFuture = dyn Future; @@ -140,6 +143,13 @@ pub(crate) struct ModuleMap { module_waker: AtomicWaker, data: RefCell, + buffered_pending_dynamic_imports: RefCell< + HashMap< + FastString, + Vec<(FastString, RequestedModuleType, RecursiveModuleLoad)>, + >, + >, + /// A counter used to delay our dynamic import deadlock detection by one spin /// of the event loop. pub(crate) dyn_module_evaluate_idle_counter: Cell, @@ -210,6 +220,7 @@ impl ModuleMap { pending_code_cache_ready: Default::default(), module_waker: Default::default(), data: Default::default(), + buffered_pending_dynamic_imports: Default::default(), } } @@ -930,24 +941,17 @@ impl ModuleMap { .borrow_mut() .insert(load.id, resolver_handle); - let resolve_result = - self.resolve(specifier, referrer, ResolutionKind::DynamicImport); - let fut = match resolve_result { - Ok(module_specifier) => { - if self - .data - .borrow() - .is_registered(module_specifier.as_str(), requested_module_type) - { - async move { (load.id, Ok(load)) }.boxed_local() - } else { - async move { (load.id, load.prepare().await.map(|()| load)) } - .boxed_local() - } - } - Err(error) => async move { (load.id, Err(error)) }.boxed_local(), - }; - self.preparing_dynamic_imports.borrow_mut().push(fut); + self + .buffered_pending_dynamic_imports + .borrow_mut() + .entry(FastString::from(referrer.to_string())) + .or_default() + .push(( + FastString::from(specifier.to_string()), + requested_module_type, + load, + )); + self.preparing_dynamic_imports_pending.set(true); } @@ -1346,7 +1350,7 @@ impl ModuleMap { /// Poll for progress in the module loading logic. Note that this takes a waker but /// doesn't act like a normal polling method. pub(crate) fn poll_progress( - &self, + self: &Rc, cx: &mut Context, scope: &mut v8::HandleScope, ) -> Result<(), Error> { @@ -1405,6 +1409,42 @@ impl ModuleMap { return Poll::Ready(Ok(())); } + let buffered = self.buffered_pending_dynamic_imports.take(); + + if !buffered.is_empty() { + self.preparing_dynamic_imports_pending.set(true); + } + for (referrer, files) in buffered { + let mut loads = SmallVec::with_capacity(files.len()); + let mut module_specifiers = Vec::with_capacity(files.len()); + for (specifier, ty, load) in files { + let specifier = + self.resolve(&specifier, &referrer, ResolutionKind::DynamicImport)?; + if self.data.borrow().is_registered(specifier.as_str(), ty) { + self + .preparing_dynamic_imports + .borrow_mut() + .push(async move { (smallvec![load], Ok(())) }.boxed_local()); + continue; + } + module_specifiers.push(specifier); + loads.push(load); + } + + if loads.is_empty() { + continue; + } + let prepare_fut = self.loader.borrow().prepare_load( + &module_specifiers, + Some(referrer.to_string()), + true, + ); + self + .preparing_dynamic_imports + .borrow_mut() + .push(async move { (loads, prepare_fut.await) }.boxed_local()); + } + loop { let poll_result = self .preparing_dynamic_imports @@ -1412,20 +1452,22 @@ impl ModuleMap { .poll_next_unpin(cx); if let Poll::Ready(Some(prepare_poll)) = poll_result { - let dyn_import_id = prepare_poll.0; + let dyn_imports = prepare_poll.0; let prepare_result = prepare_poll.1; match prepare_result { - Ok(load) => { - self - .pending_dynamic_imports - .borrow_mut() - .push(load.into_future()); + Ok(()) => { + for load in dyn_imports { + self + .pending_dynamic_imports + .borrow_mut() + .push(load.into_future()); + } self.pending_dynamic_imports_pending.set(true); } Err(err) => { let exception = to_v8_type_error(scope, err); - self.dynamic_import_reject(scope, dyn_import_id, exception); + self.dynamic_import_reject(scope, dyn_imports[0].id, exception); } } // Continue polling for more prepared dynamic imports. diff --git a/core/modules/recursive_load.rs b/core/modules/recursive_load.rs index 39778b5c1..5c6e65651 100644 --- a/core/modules/recursive_load.rs +++ b/core/modules/recursive_load.rs @@ -172,7 +172,11 @@ impl RecursiveModuleLoad { self .loader - .prepare_load(&module_specifier, maybe_referrer, self.is_dynamic_import()) + .prepare_load( + &[module_specifier], + maybe_referrer, + self.is_dynamic_import(), + ) .await } From c124ae17ab8c932559bccd85856dbdfb67b2d19a Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Mon, 3 Jun 2024 13:13:56 -0700 Subject: [PATCH 2/5] Use smallvec instead of hashmap --- core/modules/map.rs | 122 +++++++++++++++++++++++++++++--------------- 1 file changed, 80 insertions(+), 42 deletions(-) diff --git a/core/modules/map.rs b/core/modules/map.rs index 937d290d1..c08d4d2b4 100644 --- a/core/modules/map.rs +++ b/core/modules/map.rs @@ -27,7 +27,6 @@ use crate::runtime::JsRealm; use crate::runtime::SnapshotLoadDataStore; use crate::runtime::SnapshotStoreDataStore; use crate::FastStaticString; -use crate::FastString; use crate::JsRuntime; use crate::ModuleLoadResponse; use crate::ModuleSource; @@ -118,6 +117,13 @@ struct DynImportModEvaluate { module: v8::Global, } +struct BufferedPendingDynImport { + referrer: String, + specifier: String, + module_type: RequestedModuleType, + load: RecursiveModuleLoad, +} + /// A collection of JS modules. pub(crate) struct ModuleMap { // Handling of futures for loading module sources @@ -143,12 +149,8 @@ pub(crate) struct ModuleMap { module_waker: AtomicWaker, data: RefCell, - buffered_pending_dynamic_imports: RefCell< - HashMap< - FastString, - Vec<(FastString, RequestedModuleType, RecursiveModuleLoad)>, - >, - >, + buffered_pending_dynamic_imports: + RefCell>, /// A counter used to delay our dynamic import deadlock detection by one spin /// of the event loop. @@ -941,17 +943,14 @@ impl ModuleMap { .borrow_mut() .insert(load.id, resolver_handle); - self - .buffered_pending_dynamic_imports - .borrow_mut() - .entry(FastString::from(referrer.to_string())) - .or_default() - .push(( - FastString::from(specifier.to_string()), - requested_module_type, + self.buffered_pending_dynamic_imports.borrow_mut().push( + BufferedPendingDynImport { + referrer: referrer.to_string(), + specifier: specifier.to_string(), + module_type: requested_module_type, load, - )); - + }, + ); self.preparing_dynamic_imports_pending.set(true); } @@ -1400,43 +1399,68 @@ impl ModuleMap { Ok(()) } - fn poll_prepare_dyn_imports( - &self, - cx: &mut Context, - scope: &mut v8::HandleScope, - ) -> Poll> { - if !self.preparing_dynamic_imports_pending.get() { - return Poll::Ready(Ok(())); + fn prepare_buffered_dyn_imports(&self) -> Result<(), Error> { + let mut buffered = self.buffered_pending_dynamic_imports.take(); + + if buffered.is_empty() { + return Ok(()); } - let buffered = self.buffered_pending_dynamic_imports.take(); + self.preparing_dynamic_imports_pending.set(true); - if !buffered.is_empty() { - self.preparing_dynamic_imports_pending.set(true); - } - for (referrer, files) in buffered { - let mut loads = SmallVec::with_capacity(files.len()); - let mut module_specifiers = Vec::with_capacity(files.len()); - for (specifier, ty, load) in files { - let specifier = - self.resolve(&specifier, &referrer, ResolutionKind::DynamicImport)?; - if self.data.borrow().is_registered(specifier.as_str(), ty) { + let mut loads = SmallVec::new(); + let mut module_specifiers = SmallVec::<[ModuleSpecifier; 1]>::new(); + let mut current_referrer = None; + + // Go through all the imports and prepare them, batching by referrer + buffered.sort_by(|a, b| a.referrer.cmp(&b.referrer)); + for import in buffered { + if current_referrer.as_ref() != Some(&import.referrer) { + // Prepare the previous batch of imports + let prev_referrer = current_referrer.take(); + if let Some(prev_referrer) = prev_referrer { + let module_specifiers = std::mem::take(&mut module_specifiers); + let loads = std::mem::take(&mut loads); + let prepare_fut = self.loader.borrow().prepare_load( + &module_specifiers, + Some(prev_referrer), + true, + ); self .preparing_dynamic_imports .borrow_mut() - .push(async move { (smallvec![load], Ok(())) }.boxed_local()); - continue; + .push(async move { (loads, prepare_fut.await) }.boxed_local()); } + } + let specifier = self.resolve( + &import.specifier, + &import.referrer, + ResolutionKind::DynamicImport, + )?; + if self + .data + .borrow() + .is_registered(specifier.as_str(), &import.module_type) + { + // Already registered, skip preparing it. + self + .preparing_dynamic_imports + .borrow_mut() + .push(async move { (smallvec![import.load], Ok(())) }.boxed_local()); + } else { module_specifiers.push(specifier); - loads.push(load); + loads.push(import.load); } - - if loads.is_empty() { - continue; + if current_referrer.is_none() { + current_referrer = Some(import.referrer); } + } + + // Prepare the last batch of imports + if !loads.is_empty() { let prepare_fut = self.loader.borrow().prepare_load( &module_specifiers, - Some(referrer.to_string()), + Some(current_referrer.unwrap()), true, ); self @@ -1445,6 +1469,20 @@ impl ModuleMap { .push(async move { (loads, prepare_fut.await) }.boxed_local()); } + Ok(()) + } + + fn poll_prepare_dyn_imports( + &self, + cx: &mut Context, + scope: &mut v8::HandleScope, + ) -> Poll> { + if !self.preparing_dynamic_imports_pending.get() { + return Poll::Ready(Ok(())); + } + + self.prepare_buffered_dyn_imports()?; + loop { let poll_result = self .preparing_dynamic_imports From 28301ab40a792c3a20450d1203e2a2968ba9392e Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Mon, 3 Jun 2024 13:44:43 -0700 Subject: [PATCH 3/5] Handle resolve error more correctly --- core/modules/map.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/core/modules/map.rs b/core/modules/map.rs index c08d4d2b4..ca82b09b9 100644 --- a/core/modules/map.rs +++ b/core/modules/map.rs @@ -1432,11 +1432,19 @@ impl ModuleMap { .push(async move { (loads, prepare_fut.await) }.boxed_local()); } } - let specifier = self.resolve( + let specifier = match self.resolve( &import.specifier, &import.referrer, ResolutionKind::DynamicImport, - )?; + ) { + Ok(s) => s, + Err(err) => { + self.preparing_dynamic_imports.borrow_mut().push( + async move { (smallvec![import.load], Err(err)) }.boxed_local(), + ); + continue; + } + }; if self .data .borrow() @@ -1495,6 +1503,7 @@ impl ModuleMap { match prepare_result { Ok(()) => { + debug_assert!(!dyn_imports.is_empty()); for load in dyn_imports { self .pending_dynamic_imports From bbb57684d62a3804c3f4ea78739bfb48fcb13f8c Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Mon, 3 Jun 2024 14:21:02 -0700 Subject: [PATCH 4/5] Cleanup / reject all --- core/modules/map.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/core/modules/map.rs b/core/modules/map.rs index ca82b09b9..dab0248bf 100644 --- a/core/modules/map.rs +++ b/core/modules/map.rs @@ -1399,11 +1399,11 @@ impl ModuleMap { Ok(()) } - fn prepare_buffered_dyn_imports(&self) -> Result<(), Error> { + fn prepare_buffered_dyn_imports(&self) { let mut buffered = self.buffered_pending_dynamic_imports.take(); if buffered.is_empty() { - return Ok(()); + return; } self.preparing_dynamic_imports_pending.set(true); @@ -1468,7 +1468,7 @@ impl ModuleMap { if !loads.is_empty() { let prepare_fut = self.loader.borrow().prepare_load( &module_specifiers, - Some(current_referrer.unwrap()), + current_referrer, true, ); self @@ -1476,8 +1476,6 @@ impl ModuleMap { .borrow_mut() .push(async move { (loads, prepare_fut.await) }.boxed_local()); } - - Ok(()) } fn poll_prepare_dyn_imports( @@ -1489,7 +1487,7 @@ impl ModuleMap { return Poll::Ready(Ok(())); } - self.prepare_buffered_dyn_imports()?; + self.prepare_buffered_dyn_imports(); loop { let poll_result = self @@ -1514,7 +1512,9 @@ impl ModuleMap { } Err(err) => { let exception = to_v8_type_error(scope, err); - self.dynamic_import_reject(scope, dyn_imports[0].id, exception); + for load in dyn_imports { + self.dynamic_import_reject(scope, load.id, exception.clone()); + } } } // Continue polling for more prepared dynamic imports. From 9c3bd511f3da444391039a0a42f6682c00804a8b Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Mon, 3 Jun 2024 14:21:49 -0700 Subject: [PATCH 5/5] Update tests --- core/modules/tests.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/core/modules/tests.rs b/core/modules/tests.rs index 5054cec71..cb1998c94 100644 --- a/core/modules/tests.rs +++ b/core/modules/tests.rs @@ -988,7 +988,7 @@ async fn dyn_import_err() { // We should get an error here. let result = runtime.poll_event_loop(cx, Default::default()); assert!(matches!(result, Poll::Ready(Err(_)))); - assert_eq!(loader.counts(), ModuleLoadEventCounts::new(4, 1, 1)); + assert_eq!(loader.counts(), ModuleLoadEventCounts::new(3, 1, 1)); Poll::Ready(()) }) .await; @@ -1029,12 +1029,12 @@ async fn dyn_import_ok() { runtime.poll_event_loop(cx, Default::default()), Poll::Ready(Ok(_)) )); - assert_eq!(loader.counts(), ModuleLoadEventCounts::new(7, 1, 1)); + assert_eq!(loader.counts(), ModuleLoadEventCounts::new(6, 1, 1)); assert!(matches!( runtime.poll_event_loop(cx, Default::default()), Poll::Ready(Ok(_)) )); - assert_eq!(loader.counts(), ModuleLoadEventCounts::new(7, 1, 1)); + assert_eq!(loader.counts(), ModuleLoadEventCounts::new(6, 1, 1)); Poll::Ready(()) }) .await; @@ -1072,10 +1072,10 @@ async fn dyn_import_borrow_mut_error() { // Old comments that are likely wrong: // First poll runs `prepare_load` hook. let _ = runtime.poll_event_loop(cx, Default::default()); - assert_eq!(loader.counts(), ModuleLoadEventCounts::new(4, 1, 1)); + assert_eq!(loader.counts(), ModuleLoadEventCounts::new(3, 1, 1)); // Second poll triggers error let _ = runtime.poll_event_loop(cx, Default::default()); - assert_eq!(loader.counts(), ModuleLoadEventCounts::new(4, 1, 1)); + assert_eq!(loader.counts(), ModuleLoadEventCounts::new(3, 1, 1)); Poll::Ready(()) }) .await;