Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: Batch calls to ModuleLoader::prepare_load for dynamic imports #761

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions core/modules/loaders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<String>,
_is_dyn_import: bool,
) -> Pin<Box<dyn Future<Output = Result<(), Error>>>> {
Expand Down Expand Up @@ -220,7 +220,7 @@ impl ModuleLoader for ExtModuleLoader {

fn prepare_load(
&self,
_specifier: &ModuleSpecifier,
_specifiers: &[ModuleSpecifier],
_maybe_referrer: Option<String>,
_is_dyn_import: bool,
) -> Pin<Box<dyn Future<Output = Result<(), Error>>>> {
Expand Down Expand Up @@ -275,7 +275,7 @@ impl ModuleLoader for LazyEsmModuleLoader {

fn prepare_load(
&self,
_specifier: &ModuleSpecifier,
_specifiers: &[ModuleSpecifier],
_maybe_referrer: Option<String>,
_is_dyn_import: bool,
) -> Pin<Box<dyn Future<Output = Result<(), Error>>>> {
Expand Down Expand Up @@ -464,14 +464,14 @@ impl<L: ModuleLoader> ModuleLoader for TestingModuleLoader<L> {

fn prepare_load(
&self,
module_specifier: &ModuleSpecifier,
module_specifiers: &[ModuleSpecifier],
maybe_referrer: Option<String>,
is_dyn_import: bool,
) -> Pin<Box<dyn Future<Output = Result<(), Error>>>> {
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(
Expand Down
143 changes: 116 additions & 27 deletions core/modules/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,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;

Expand All @@ -61,7 +63,7 @@ use super::LazyEsmModuleLoader;
use super::RequestedModuleType;

type PrepareLoadFuture =
dyn Future<Output = (ModuleLoadId, Result<RecursiveModuleLoad, Error>)>;
dyn Future<Output = (SmallVec<[RecursiveModuleLoad; 1]>, Result<(), Error>)>;

type CodeCacheReadyFuture = dyn Future<Output = ()>;

Expand Down Expand Up @@ -115,6 +117,13 @@ struct DynImportModEvaluate {
module: v8::Global<v8::Module>,
}

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
Expand All @@ -140,6 +149,9 @@ pub(crate) struct ModuleMap {
module_waker: AtomicWaker,
data: RefCell<ModuleMapData>,

buffered_pending_dynamic_imports:
RefCell<SmallVec<[BufferedPendingDynImport; 1]>>,

/// 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<u32>,
Expand Down Expand Up @@ -210,6 +222,7 @@ impl ModuleMap {
pending_code_cache_ready: Default::default(),
module_waker: Default::default(),
data: Default::default(),
buffered_pending_dynamic_imports: Default::default(),
}
}

Expand Down Expand Up @@ -930,24 +943,14 @@ 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().push(
BufferedPendingDynImport {
referrer: referrer.to_string(),
specifier: specifier.to_string(),
module_type: requested_module_type,
load,
},
);
self.preparing_dynamic_imports_pending.set(true);
}

Expand Down Expand Up @@ -1346,7 +1349,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<Self>,
cx: &mut Context,
scope: &mut v8::HandleScope,
) -> Result<(), Error> {
Expand Down Expand Up @@ -1396,6 +1399,85 @@ impl ModuleMap {
Ok(())
}

fn prepare_buffered_dyn_imports(&self) {
let mut buffered = self.buffered_pending_dynamic_imports.take();

if buffered.is_empty() {
return;
}

self.preparing_dynamic_imports_pending.set(true);

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 { (loads, prepare_fut.await) }.boxed_local());
}
}
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()
.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(import.load);
}
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,
current_referrer,
true,
);
self
.preparing_dynamic_imports
.borrow_mut()
.push(async move { (loads, prepare_fut.await) }.boxed_local());
}
}

fn poll_prepare_dyn_imports(
&self,
cx: &mut Context,
Expand All @@ -1405,27 +1487,34 @@ impl ModuleMap {
return Poll::Ready(Ok(()));
}

self.prepare_buffered_dyn_imports();

loop {
let poll_result = self
.preparing_dynamic_imports
.borrow_mut()
.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(()) => {
debug_assert!(!dyn_imports.is_empty());
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);
for load in dyn_imports {
self.dynamic_import_reject(scope, load.id, exception.clone());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the part I'm most concerned about, since we just reject all of the import promises in this batch even though the error might only be caused by a subset.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's change the return type of this hook to Vec<Result<(), AnyError>> 👍

}
}
}
// Continue polling for more prepared dynamic imports.
Expand Down
6 changes: 5 additions & 1 deletion core/modules/recursive_load.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
10 changes: 5 additions & 5 deletions core/modules/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
Loading