From 6836e5a6eb9b108d4f5f7fa952f53ee966899eee Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Thu, 1 Jun 2023 06:35:14 +0100 Subject: [PATCH] refactor(core): don't pass op state to prepare_module_load() (#19332) It's not used anymore. Subsequently allows removing `ModuleMap::op_state`, allowing `ModuleMap` to have a sane default so `JsRuntime::module_map` no longer needs to be optional. --- cli/module_loader.rs | 3 -- core/modules.rs | 33 ++++++------------ core/runtime.rs | 82 ++++++++++++++++++++------------------------ 3 files changed, 48 insertions(+), 70 deletions(-) diff --git a/cli/module_loader.rs b/cli/module_loader.rs index cc416970fc..3352cb951f 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -37,7 +37,6 @@ use deno_core::ModuleLoader; use deno_core::ModuleSource; use deno_core::ModuleSpecifier; use deno_core::ModuleType; -use deno_core::OpState; use deno_core::ResolutionKind; use deno_core::SourceMapGetter; use deno_graph::source::Resolver; @@ -54,7 +53,6 @@ use deno_runtime::permissions::PermissionsContainer; use deno_semver::npm::NpmPackageNvReference; use deno_semver::npm::NpmPackageReqReference; use std::borrow::Cow; -use std::cell::RefCell; use std::collections::HashSet; use std::pin::Pin; use std::rc::Rc; @@ -566,7 +564,6 @@ impl ModuleLoader for CliModuleLoader { fn prepare_load( &self, - _op_state: Rc>, specifier: &ModuleSpecifier, _maybe_referrer: Option, is_dynamic: bool, diff --git a/core/modules.rs b/core/modules.rs index 353119660c..4f1875ae59 100644 --- a/core/modules.rs +++ b/core/modules.rs @@ -11,7 +11,6 @@ use crate::resolve_url; use crate::snapshot_util::SnapshottedData; use crate::Extension; use crate::JsRuntime; -use crate::OpState; use anyhow::anyhow; use anyhow::Error; use futures::future::FutureExt; @@ -340,7 +339,6 @@ pub trait ModuleLoader { /// It's not required to implement this method. fn prepare_load( &self, - _op_state: Rc>, _module_specifier: &ModuleSpecifier, _maybe_referrer: Option, _is_dyn_import: bool, @@ -453,7 +451,6 @@ impl ModuleLoader for ExtModuleLoader { fn prepare_load( &self, - _op_state: Rc>, _specifier: &ModuleSpecifier, _maybe_referrer: Option, _is_dyn_import: bool, @@ -567,9 +564,8 @@ pub(crate) struct RecursiveModuleLoad { module_map_rc: Rc>, pending: FuturesUnordered>>, visited: HashSet, - // These three fields are copied from `module_map_rc`, but they are cloned + // The loader is copied from `module_map_rc`, but its reference is cloned // ahead of time to avoid already-borrowed errors. - op_state: Rc>, loader: Rc, } @@ -612,7 +608,6 @@ impl RecursiveModuleLoad { module_map.next_load_id += 1; id }; - let op_state = module_map_rc.borrow().op_state.clone(); let loader = module_map_rc.borrow().loader.clone(); let asserted_module_type = match init { LoadInit::DynamicImport(_, _, module_type) => module_type, @@ -626,7 +621,6 @@ impl RecursiveModuleLoad { init, state: LoadState::Init, module_map_rc: module_map_rc.clone(), - op_state, loader, pending: FuturesUnordered::new(), visited: HashSet::new(), @@ -669,8 +663,6 @@ impl RecursiveModuleLoad { } async fn prepare(&self) -> Result<(), Error> { - let op_state = self.op_state.clone(); - let (module_specifier, maybe_referrer) = match self.init { LoadInit::Main(ref specifier) => { let spec = @@ -698,12 +690,7 @@ impl RecursiveModuleLoad { self .loader - .prepare_load( - op_state, - &module_specifier, - maybe_referrer, - self.is_dynamic_import(), - ) + .prepare_load(&module_specifier, maybe_referrer, self.is_dynamic_import()) .await } @@ -997,7 +984,6 @@ pub(crate) struct ModuleMap { // Handling of futures for loading module sources pub loader: Rc, - op_state: Rc>, pub(crate) dynamic_import_map: HashMap>, pub(crate) preparing_dynamic_imports: @@ -1304,10 +1290,7 @@ impl ModuleMap { self.handles = snapshotted_data.module_handles; } - pub(crate) fn new( - loader: Rc, - op_state: Rc>, - ) -> ModuleMap { + pub(crate) fn new(loader: Rc) -> ModuleMap { Self { handles: vec![], info: vec![], @@ -1315,7 +1298,6 @@ impl ModuleMap { by_name_json: HashMap::new(), next_load_id: 1, loader, - op_state, dynamic_import_map: HashMap::new(), preparing_dynamic_imports: FuturesUnordered::new(), pending_dynamic_imports: FuturesUnordered::new(), @@ -1493,7 +1475,7 @@ impl ModuleMap { } pub(crate) fn clear(&mut self) { - *self = Self::new(self.loader.clone(), self.op_state.clone()) + *self = Self::new(self.loader.clone()) } pub(crate) fn get_handle_by_name( @@ -1714,6 +1696,12 @@ impl ModuleMap { } } +impl Default for ModuleMap { + fn default() -> Self { + Self::new(Rc::new(NoopModuleLoader)) + } +} + #[cfg(test)] mod tests { use super::*; @@ -2339,7 +2327,6 @@ import "/a.js"; fn prepare_load( &self, - _op_state: Rc>, _module_specifier: &ModuleSpecifier, _maybe_referrer: Option, _is_dyn_import: bool, diff --git a/core/runtime.rs b/core/runtime.rs index b4876f0afb..f95c4a8ef5 100644 --- a/core/runtime.rs +++ b/core/runtime.rs @@ -202,7 +202,7 @@ impl Drop for InnerIsolateState { /// type aliases. pub struct JsRuntimeImpl { inner: InnerIsolateState, - module_map: Option>>, + module_map: Rc>, allocations: IsolateAllocations, extensions: Vec, event_loop_middlewares: Vec>, @@ -607,7 +607,7 @@ impl JsRuntimeImpl { STATE_DATA_OFFSET, Rc::into_raw(state_rc.clone()) as *mut c_void, ); - let module_map_rc = Rc::new(RefCell::new(ModuleMap::new(loader, op_state))); + let module_map_rc = Rc::new(RefCell::new(ModuleMap::new(loader))); if let Some(snapshotted_data) = snapshotted_data { let mut module_map = module_map_rc.borrow_mut(); module_map.update_with_snapshotted_data(scope, snapshotted_data); @@ -629,7 +629,7 @@ impl JsRuntimeImpl { allocations: IsolateAllocations::default(), event_loop_middlewares, extensions: options.extensions, - module_map: Some(module_map_rc), + module_map: module_map_rc, is_main: options.is_main, }; @@ -686,9 +686,10 @@ impl JsRuntimeImpl { (isolate, context, snapshotted_data) } + #[cfg(test)] #[inline] pub(crate) fn module_map(&self) -> &Rc> { - self.module_map.as_ref().unwrap() + &self.module_map } #[inline] @@ -807,13 +808,12 @@ impl JsRuntimeImpl { let extensions = std::mem::take(&mut self.extensions); // TODO(nayeemrmn): Module maps should be per-realm. - let module_map = self.module_map.as_ref().unwrap(); - let loader = module_map.borrow().loader.clone(); + let loader = self.module_map.borrow().loader.clone(); let ext_loader = Rc::new(ExtModuleLoader::new( &extensions, maybe_load_callback.map(Rc::new), )); - module_map.borrow_mut().loader = ext_loader; + self.module_map.borrow_mut().loader = ext_loader; let mut esm_entrypoints = vec![]; @@ -853,9 +853,8 @@ impl JsRuntimeImpl { for specifier in esm_entrypoints { let mod_id = { - let module_map = self.module_map.as_ref().unwrap(); - - module_map + self + .module_map .borrow() .get_id(specifier, AssertedModuleType::JavaScriptOrWasm) .unwrap_or_else(|| { @@ -871,7 +870,7 @@ impl JsRuntimeImpl { #[cfg(debug_assertions)] { - let module_map_rc = self.module_map.clone().unwrap(); + let module_map_rc = self.module_map.clone(); let mut scope = realm.handle_scope(self.v8_isolate()); let module_map = module_map_rc.borrow(); module_map.assert_all_modules_evaluated(&mut scope); @@ -881,7 +880,7 @@ impl JsRuntimeImpl { })?; self.extensions = extensions; - self.module_map.as_ref().unwrap().borrow_mut().loader = loader; + self.module_map.borrow_mut().loader = loader; Ok(()) } @@ -1133,9 +1132,8 @@ impl JsRuntimeImpl { &mut self, module_id: ModuleId, ) -> Result, Error> { - let module_map_rc = self.module_map(); - - let module_handle = module_map_rc + let module_handle = self + .module_map .borrow() .get_handle(module_id) .expect("ModuleInfo not found"); @@ -1467,7 +1465,7 @@ impl JsRuntimeImpl { EventLoopPendingState::new( &mut scope, &mut self.inner.state.borrow_mut(), - &self.module_map.as_ref().unwrap().borrow(), + &self.module_map.borrow(), ) } } @@ -1566,7 +1564,10 @@ impl JsRuntimeForSnapshot { // Serialize the module map and store its data in the snapshot. { let snapshotted_data = { - let module_map_rc = self.module_map.take().unwrap(); + // `self.module_map` points directly to the v8 isolate data slot, which + // we must explicitly drop before destroying the isolate. We have to + // take and drop this `Rc` before that. + let module_map_rc = std::mem::take(&mut self.module_map); let module_map = module_map_rc.borrow(); module_map.serialize_for_snapshotting(&mut self.handle_scope()) }; @@ -1804,7 +1805,7 @@ impl JsRuntimeImpl { &mut self, id: ModuleId, ) -> Result<(), v8::Global> { - let module_map_rc = self.module_map().clone(); + let module_map_rc = self.module_map.clone(); let scope = &mut self.handle_scope(); let tc_scope = &mut v8::TryCatch::new(scope); @@ -1837,9 +1838,8 @@ impl JsRuntimeImpl { load_id: ModuleLoadId, id: ModuleId, ) -> Result<(), Error> { - let module_map_rc = self.module_map(); - - let module_handle = module_map_rc + let module_handle = self + .module_map .borrow() .get_handle(id) .expect("ModuleInfo not found"); @@ -1929,7 +1929,7 @@ impl JsRuntimeImpl { ) -> oneshot::Receiver> { let global_realm = self.global_realm(); let state_rc = self.inner.state.clone(); - let module_map_rc = self.module_map().clone(); + let module_map_rc = self.module_map.clone(); let scope = &mut self.handle_scope(); let tc_scope = &mut v8::TryCatch::new(scope); @@ -2049,7 +2049,7 @@ impl JsRuntimeImpl { &self, exceptions: impl Iterator, ) { - let mut module_map = self.module_map.as_ref().unwrap().borrow_mut(); + let mut module_map = self.module_map.borrow_mut(); let handles = exceptions .map(|(old_name, new_name)| { (module_map.get_handle_by_name(old_name).unwrap(), new_name) @@ -2070,7 +2070,7 @@ impl JsRuntimeImpl { id: ModuleLoadId, exception: v8::Global, ) { - let module_map_rc = self.module_map().clone(); + let module_map_rc = self.module_map.clone(); let scope = &mut self.handle_scope(); let resolver_handle = module_map_rc @@ -2091,7 +2091,7 @@ impl JsRuntimeImpl { fn dynamic_import_resolve(&mut self, id: ModuleLoadId, mod_id: ModuleId) { let state_rc = self.inner.state.clone(); - let module_map_rc = self.module_map().clone(); + let module_map_rc = self.module_map.clone(); let scope = &mut self.handle_scope(); let resolver_handle = module_map_rc @@ -2126,7 +2126,7 @@ impl JsRuntimeImpl { cx: &mut Context, ) -> Poll> { if self - .module_map() + .module_map .borrow() .preparing_dynamic_imports .is_empty() @@ -2134,10 +2134,9 @@ impl JsRuntimeImpl { return Poll::Ready(Ok(())); } - let module_map_rc = self.module_map().clone(); - loop { - let poll_result = module_map_rc + let poll_result = self + .module_map .borrow_mut() .preparing_dynamic_imports .poll_next_unpin(cx); @@ -2148,7 +2147,8 @@ impl JsRuntimeImpl { match prepare_result { Ok(load) => { - module_map_rc + self + .module_map .borrow_mut() .pending_dynamic_imports .push(load.into_future()); @@ -2168,19 +2168,13 @@ impl JsRuntimeImpl { } fn poll_dyn_imports(&mut self, cx: &mut Context) -> Poll> { - if self - .module_map() - .borrow() - .pending_dynamic_imports - .is_empty() - { + if self.module_map.borrow().pending_dynamic_imports.is_empty() { return Poll::Ready(Ok(())); } - let module_map_rc = self.module_map().clone(); - loop { - let poll_result = module_map_rc + let poll_result = self + .module_map .borrow_mut() .pending_dynamic_imports .poll_next_unpin(cx); @@ -2205,7 +2199,8 @@ impl JsRuntimeImpl { match register_result { Ok(()) => { // Keep importing until it's fully drained - module_map_rc + self + .module_map .borrow_mut() .pending_dynamic_imports .push(load.into_future()); @@ -2374,7 +2369,7 @@ impl JsRuntimeImpl { specifier: &ModuleSpecifier, code: Option, ) -> Result { - let module_map_rc = self.module_map().clone(); + let module_map_rc = self.module_map.clone(); if let Some(code) = code { let specifier = specifier.as_str().to_owned().into(); let scope = &mut self.handle_scope(); @@ -2429,7 +2424,7 @@ impl JsRuntimeImpl { specifier: &ModuleSpecifier, code: Option, ) -> Result { - let module_map_rc = self.module_map().clone(); + let module_map_rc = self.module_map.clone(); if let Some(code) = code { let specifier = specifier.as_str().to_owned().into(); let scope = &mut self.handle_scope(); @@ -3612,8 +3607,7 @@ pub mod tests { runtime: &mut JsRuntimeImpl, modules: &Vec, ) { - let module_map_rc = runtime.module_map(); - let module_map = module_map_rc.borrow(); + let module_map = runtime.module_map.borrow(); assert_eq!(module_map.handles.len(), modules.len()); assert_eq!(module_map.info.len(), modules.len()); assert_eq!(