From 8f8dd826f5412229a4119f4f72b575439b95c69c Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Sat, 1 Jul 2023 23:14:41 +0100 Subject: [PATCH] refactor(core): don't use extension macro for core js (#19616) Seems like too much of a special case because `init_cbs()` needs to be called right after them. Removes the `Extension::is_core` stuff. --- core/extensions.rs | 42 +++++-------------- core/modules/loaders.rs | 1 - core/ops_builtin.rs | 4 -- core/runtime/jsruntime.rs | 77 +++++++++++++++++++---------------- core/runtime/snapshot_util.rs | 11 ++++- 5 files changed, 61 insertions(+), 74 deletions(-) diff --git a/core/extensions.rs b/core/extensions.rs index e4de3d217..dac19ec50 100644 --- a/core/extensions.rs +++ b/core/extensions.rs @@ -174,7 +174,6 @@ macro_rules! ops { /// * bounds: a comma-separated list of additional type bounds, eg: `bounds = [ P::MyAssociatedType: MyTrait ]` /// * ops: a comma-separated list of [`OpDecl`]s to provide, eg: `ops = [ op_foo, op_bar ]` /// * esm: a comma-separated list of ESM module filenames (see [`include_js_files`]), eg: `esm = [ dir "dir", "my_file.js" ]` -/// * esm_setup_script: see [`ExtensionBuilder::esm_setup_script`] /// * js: a comma-separated list of JS filenames (see [`include_js_files`]), eg: `js = [ dir "dir", "my_file.js" ]` /// * config: a structure-like definition for configuration parameters which will be required when initializing this extension, eg: `config = { my_param: Option }` /// * middleware: an [`OpDecl`] middleware function with the signature `fn (OpDecl) -> OpDecl` @@ -191,7 +190,6 @@ macro_rules! extension { $(, ops = [ $( $(#[$m:meta])* $( $op:ident )::+ $( < $( $op_param:ident ),* > )? ),+ $(,)? ] )? $(, esm_entry_point = $esm_entry_point:literal )? $(, esm = [ $( dir $dir_esm:literal , )? $( $esm:literal ),* $(,)? ] )? - $(, esm_setup_script = $esm_setup_script:expr )? $(, js = [ $( dir $dir_js:literal , )? $( $js:literal ),* $(,)? ] )? $(, options = { $( $options_id:ident : $options_type:ty ),* $(,)? } )? $(, middleware = $middleware_fn:expr )? @@ -220,12 +218,6 @@ macro_rules! extension { $( ext.esm( $crate::include_js_files!( $name $( dir $dir_esm , )? $( $esm , )* ) ); )? - $( - ext.esm(vec![ExtensionFileSource { - specifier: "ext:setup", - code: ExtensionFileSourceCode::IncludedInBinary($esm_setup_script), - }]); - )? $( ext.esm_entry_point($esm_entry_point); )? @@ -355,8 +347,8 @@ macro_rules! extension { #[derive(Default)] pub struct Extension { pub(crate) name: &'static str, - js_files: Option>, - esm_files: Option>, + js_files: Vec, + esm_files: Vec, esm_entry_point: Option<&'static str>, ops: Option>, opstate_fn: Option>, @@ -365,7 +357,6 @@ pub struct Extension { initialized: bool, enabled: bool, deps: Option<&'static [&'static str]>, - pub(crate) is_core: bool, } // Note: this used to be a trait, but we "downgraded" it to a single concrete type @@ -412,12 +403,12 @@ impl Extension { /// returns JS source code to be loaded into the isolate (either at snapshotting, /// or at startup). as a vector of a tuple of the file name, and the source code. - pub fn get_js_sources(&self) -> Option<&Vec> { - self.js_files.as_ref() + pub fn get_js_sources(&self) -> &Vec { + &self.js_files } - pub fn get_esm_sources(&self) -> Option<&Vec> { - self.esm_files.as_ref() + pub fn get_esm_sources(&self) -> &Vec { + &self.esm_files } pub fn get_esm_entry_point(&self) -> Option<&'static str> { @@ -488,7 +479,6 @@ pub struct ExtensionBuilder { event_loop_middleware: Option>, name: &'static str, deps: &'static [&'static str], - is_core: bool, } impl ExtensionBuilder { @@ -538,13 +528,11 @@ impl ExtensionBuilder { /// Consume the [`ExtensionBuilder`] and return an [`Extension`]. pub fn take(self) -> Extension { - let js_files = Some(self.js); - let esm_files = Some(self.esm); let ops = Some(self.ops); let deps = Some(self.deps); Extension { - js_files, - esm_files, + js_files: self.js, + esm_files: self.esm, esm_entry_point: self.esm_entry_point, ops, opstate_fn: self.state, @@ -554,18 +542,15 @@ impl ExtensionBuilder { enabled: true, name: self.name, deps, - is_core: self.is_core, } } pub fn build(&mut self) -> Extension { - let js_files = Some(std::mem::take(&mut self.js)); - let esm_files = Some(std::mem::take(&mut self.esm)); let ops = Some(std::mem::take(&mut self.ops)); let deps = Some(std::mem::take(&mut self.deps)); Extension { - js_files, - esm_files, + js_files: std::mem::take(&mut self.js), + esm_files: std::mem::take(&mut self.esm), esm_entry_point: self.esm_entry_point.take(), ops, opstate_fn: self.state.take(), @@ -575,15 +560,8 @@ impl ExtensionBuilder { enabled: true, name: self.name, deps, - is_core: self.is_core, } } - - #[doc(hidden)] - pub(crate) fn deno_core(&mut self) -> &mut Self { - self.is_core = true; - self - } } /// Helps embed JS files in an extension. Returns a vector of diff --git a/core/modules/loaders.rs b/core/modules/loaders.rs index d4dbf1ec2..bc645567e 100644 --- a/core/modules/loaders.rs +++ b/core/modules/loaders.rs @@ -119,7 +119,6 @@ impl ExtModuleLoader { extensions .iter() .flat_map(|e| e.get_esm_sources()) - .flatten() .map(|s| (s.specifier.to_string(), s.clone())), ); ExtModuleLoader { diff --git a/core/ops_builtin.rs b/core/ops_builtin.rs index 3728d132e..eeb753d5a 100644 --- a/core/ops_builtin.rs +++ b/core/ops_builtin.rs @@ -79,10 +79,6 @@ crate::extension!( ops_builtin_v8::op_has_pending_promise_rejection, ops_builtin_v8::op_arraybuffer_was_detached, ], - js = ["00_primordials.js", "01_core.js", "02_error.js"], - customizer = |ext: &mut crate::ExtensionBuilder| { - ext.deno_core(); - } ); /// Return map of resources with id as key diff --git a/core/runtime/jsruntime.rs b/core/runtime/jsruntime.rs index 3b2ac49aa..cea8fcb1f 100644 --- a/core/runtime/jsruntime.rs +++ b/core/runtime/jsruntime.rs @@ -10,6 +10,7 @@ use crate::error::GetErrorClassFn; use crate::error::JsError; use crate::extensions::OpDecl; use crate::extensions::OpEventLoopFn; +use crate::include_js_files; use crate::inspector::JsRuntimeInspector; use crate::module_specifier::ModuleSpecifier; use crate::modules::AssertedModuleType; @@ -27,6 +28,7 @@ use crate::runtime::JsRealm; use crate::source_map::SourceMapCache; use crate::source_map::SourceMapGetter; use crate::Extension; +use crate::ExtensionFileSource; use crate::NoopModuleLoader; use crate::OpMiddlewareFn; use crate::OpResult; @@ -38,6 +40,7 @@ use anyhow::Error; use futures::channel::oneshot; use futures::future::poll_fn; use futures::stream::StreamExt; +use once_cell::sync::Lazy; use smallvec::SmallVec; use std::any::Any; use std::cell::RefCell; @@ -196,6 +199,16 @@ impl InitMode { } } +pub(crate) static BUILTIN_SOURCES: Lazy> = + Lazy::new(|| { + include_js_files!( + core + "00_primordials.js", + "01_core.js", + "02_error.js", + ) + }); + /// A single execution context of JavaScript. Corresponds roughly to the "Web /// Worker" concept in the DOM. //// @@ -504,7 +517,7 @@ impl JsRuntime { maybe_load_callback: Option, ) -> JsRuntime { let init_mode = InitMode::from_options(&options); - let (op_state, ops) = Self::create_opstate(&mut options, init_mode); + let (op_state, ops) = Self::create_opstate(&mut options); let op_state = Rc::new(RefCell::new(op_state)); // Collect event-loop middleware @@ -840,36 +853,39 @@ impl JsRuntime { let mut esm_entrypoints = vec![]; futures::executor::block_on(async { + if self.init_mode == InitMode::New { + for file_source in &*BUILTIN_SOURCES { + realm.execute_script( + self.v8_isolate(), + file_source.specifier, + file_source.load()?, + )?; + } + } + self.init_cbs(realm); + for extension in &extensions { let maybe_esm_entry_point = extension.get_esm_entry_point(); - if let Some(esm_files) = extension.get_esm_sources() { - for file_source in esm_files { - self - .load_side_module( - &ModuleSpecifier::parse(file_source.specifier)?, - None, - ) - .await?; - } + for file_source in extension.get_esm_sources() { + self + .load_side_module( + &ModuleSpecifier::parse(file_source.specifier)?, + None, + ) + .await?; } if let Some(entry_point) = maybe_esm_entry_point { esm_entrypoints.push(entry_point); } - if let Some(js_files) = extension.get_js_sources() { - for file_source in js_files { - realm.execute_script( - self.v8_isolate(), - file_source.specifier, - file_source.load()?, - )?; - } - } - - if extension.is_core { - self.init_cbs(realm); + for file_source in extension.get_js_sources() { + realm.execute_script( + self.v8_isolate(), + file_source.specifier, + file_source.load()?, + )?; } } @@ -966,20 +982,11 @@ impl JsRuntime { } /// Initializes ops of provided Extensions - fn create_opstate( - options: &mut RuntimeOptions, - init_mode: InitMode, - ) -> (OpState, Vec) { + fn create_opstate(options: &mut RuntimeOptions) -> (OpState, Vec) { // Add built-in extension - if init_mode == InitMode::FromSnapshot { - options - .extensions - .insert(0, crate::ops_builtin::core::init_ops()); - } else { - options - .extensions - .insert(0, crate::ops_builtin::core::init_ops_and_esm()); - } + options + .extensions + .insert(0, crate::ops_builtin::core::init_ops()); let ops = Self::collect_ops(&mut options.extensions); diff --git a/core/runtime/snapshot_util.rs b/core/runtime/snapshot_util.rs index 88c273147..e664856c6 100644 --- a/core/runtime/snapshot_util.rs +++ b/core/runtime/snapshot_util.rs @@ -4,9 +4,11 @@ use std::path::Path; use std::path::PathBuf; use std::time::Instant; +use crate::runtime::jsruntime::BUILTIN_SOURCES; use crate::runtime::RuntimeSnapshotOptions; use crate::ExtModuleLoaderCb; use crate::Extension; +use crate::ExtensionFileSourceCode; use crate::JsRuntimeForSnapshot; use crate::RuntimeOptions; use crate::Snapshot; @@ -52,14 +54,19 @@ pub fn create_snapshot( mark = Instant::now(); let mut files_loaded_during_snapshot = vec![]; + for source in &*BUILTIN_SOURCES { + if let ExtensionFileSourceCode::LoadedFromFsDuringSnapshot(path) = + &source.code + { + files_loaded_during_snapshot.push(path.clone()); + } + } for source in js_runtime .extensions() .iter() .flat_map(|e| vec![e.get_esm_sources(), e.get_js_sources()]) .flatten() - .flatten() { - use crate::ExtensionFileSourceCode; if let ExtensionFileSourceCode::LoadedFromFsDuringSnapshot(path) = &source.code {