From d1685b120bf7da5ba384806153f65d90ef156b77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Thu, 9 Mar 2023 20:22:27 -0400 Subject: [PATCH] refactor(core): remove RuntimeOptions::extensions_with_js (#18099) This commit removes "deno_core::RuntimeOptions::extensions_with_js". Now it's embedders' responsibility to properly register extensions that will not contains JavaScript sources when running from an existing snapshot. Prerequisite for https://github.com/denoland/deno/pull/18080 --- bench_util/js_runtime.rs | 2 +- cli/build.rs | 22 +++--- cli/standalone.rs | 1 - cli/worker.rs | 2 - core/extensions.rs | 16 ++--- core/ops_builtin.rs | 59 +++++++++------- core/runtime.rs | 107 +++++++++++------------------- core/snapshot_util.rs | 2 - ext/fetch/lib.rs | 12 +--- ext/ffi/lib.rs | 4 +- ext/fs/lib.rs | 4 +- runtime/build.rs | 7 +- runtime/examples/hello_runtime.rs | 1 - runtime/worker.rs | 20 ++---- 14 files changed, 100 insertions(+), 159 deletions(-) diff --git a/bench_util/js_runtime.rs b/bench_util/js_runtime.rs index edeb74fb44..e381ba16b6 100644 --- a/bench_util/js_runtime.rs +++ b/bench_util/js_runtime.rs @@ -9,7 +9,7 @@ use crate::profiling::is_profiling; pub fn create_js_runtime(setup: impl FnOnce() -> Vec) -> JsRuntime { JsRuntime::new(RuntimeOptions { - extensions_with_js: setup(), + extensions: setup(), module_loader: Some( std::rc::Rc::new(deno_core::ExtModuleLoader::default()), ), diff --git a/cli/build.rs b/cli/build.rs index 846224af22..804f1d8383 100644 --- a/cli/build.rs +++ b/cli/build.rs @@ -276,8 +276,7 @@ mod ts { cargo_manifest_dir: env!("CARGO_MANIFEST_DIR"), snapshot_path, startup_snapshot: None, - extensions: vec![], - extensions_with_js: vec![tsc_extension], + extensions: vec![tsc_extension], // NOTE(bartlomieju): Compressing the TSC snapshot in debug build took // ~45s on M1 MacBook Pro; without compression it took ~1s. @@ -321,7 +320,7 @@ mod ts { } fn create_cli_snapshot(snapshot_path: PathBuf) { - let extensions: Vec = vec![ + let mut extensions: Vec = vec![ deno_webidl::init(), deno_console::init(), deno_url::init_ops(), @@ -364,20 +363,21 @@ fn create_cli_snapshot(snapshot_path: PathBuf) { std::path::PathBuf::from(deno_runtime::js::PATH_FOR_99_MAIN_JS), ), }); - let extensions_with_js = vec![Extension::builder("cli") - // FIXME(bartlomieju): information about which extensions were - // already snapshotted is not preserved in the snapshot. This should be - // fixed, so we can reliably depend on that information. - // .dependencies(vec!["runtime"]) - .esm(esm_files) - .build()]; + extensions.push( + Extension::builder("cli") + // FIXME(bartlomieju): information about which extensions were + // already snapshotted is not preserved in the snapshot. This should be + // fixed, so we can reliably depend on that information. + // .dependencies(vec!["runtime"]) + .esm(esm_files) + .build(), + ); create_snapshot(CreateSnapshotOptions { cargo_manifest_dir: env!("CARGO_MANIFEST_DIR"), snapshot_path, startup_snapshot: Some(deno_runtime::js::deno_isolate_init()), extensions, - extensions_with_js, compression_cb: Some(Box::new(|vec, snapshot_slice| { lzzzz::lz4_hc::compress_to_vec( snapshot_slice, diff --git a/cli/standalone.rs b/cli/standalone.rs index 7d13f91a84..87e0be23aa 100644 --- a/cli/standalone.rs +++ b/cli/standalone.rs @@ -278,7 +278,6 @@ pub async fn run( inspect: ps.options.is_inspecting(), }, extensions: ops::cli_exts(ps), - extensions_with_js: vec![], startup_snapshot: Some(crate::js::deno_isolate_init()), unsafely_ignore_certificate_errors: metadata .unsafely_ignore_certificate_errors, diff --git a/cli/worker.rs b/cli/worker.rs index 17d1d3c432..050891256e 100644 --- a/cli/worker.rs +++ b/cli/worker.rs @@ -524,7 +524,6 @@ async fn create_main_worker_internal( inspect: ps.options.is_inspecting(), }, extensions, - extensions_with_js: vec![], startup_snapshot: Some(crate::js::deno_isolate_init()), unsafely_ignore_certificate_errors: ps .options @@ -758,7 +757,6 @@ mod tests { inspect: false, }, extensions: vec![], - extensions_with_js: vec![], startup_snapshot: Some(crate::js::deno_isolate_init()), unsafely_ignore_certificate_errors: None, root_cert_store: None, diff --git a/core/extensions.rs b/core/extensions.rs index 3d17db5922..2c6e1669c6 100644 --- a/core/extensions.rs +++ b/core/extensions.rs @@ -104,7 +104,7 @@ impl Extension { /// Check if dependencies have been loaded, and errors if either: /// - The extension is depending on itself or an extension with the same name. /// - A dependency hasn't been loaded yet. - pub fn check_dependencies(&self, previous_exts: &[&mut Extension]) { + pub fn check_dependencies(&self, previous_exts: &[Extension]) { if let Some(deps) = self.deps { 'dep_loop: for dep in deps { if dep == &self.name { @@ -124,18 +124,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) -> &[ExtensionFileSource] { - match &self.js_files { - Some(files) => files, - None => &[], - } + pub fn get_js_sources(&self) -> Option<&Vec> { + self.js_files.as_ref() } - pub fn get_esm_sources(&self) -> &[ExtensionFileSource] { - match &self.esm_files { - Some(files) => files, - None => &[], - } + pub fn get_esm_sources(&self) -> Option<&Vec> { + self.esm_files.as_ref() } pub fn get_esm_entry_point(&self) -> Option<&'static str> { diff --git a/core/ops_builtin.rs b/core/ops_builtin.rs index d225b86249..7b66ffaa48 100644 --- a/core/ops_builtin.rs +++ b/core/ops_builtin.rs @@ -1,3 +1,4 @@ +use crate::ExtensionBuilder; // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. use crate::error::format_file_name; use crate::error::type_error; @@ -18,38 +19,50 @@ use std::io::stdout; use std::io::Write; use std::rc::Rc; -pub(crate) fn init_builtins() -> Extension { +fn ext() -> ExtensionBuilder { Extension::builder("core") +} + +fn ops(ext: &mut ExtensionBuilder) -> &mut ExtensionBuilder { + let mut ops = vec![ + op_close::decl(), + op_try_close::decl(), + op_print::decl(), + op_resources::decl(), + op_wasm_streaming_feed::decl(), + op_wasm_streaming_set_url::decl(), + op_void_sync::decl(), + op_void_async::decl(), + op_add::decl(), + // // TODO(@AaronO): track IO metrics for builtin streams + op_read::decl(), + op_read_all::decl(), + op_write::decl(), + op_write_all::decl(), + op_shutdown::decl(), + op_metrics::decl(), + op_format_file_name::decl(), + op_is_proxy::decl(), + op_str_byte_length::decl(), + ]; + ops.extend(crate::ops_builtin_v8::init_builtins_v8()); + ext.ops(ops) +} + +pub(crate) fn init_builtin_ops_and_esm() -> Extension { + ops(&mut ext()) .js(include_js_files!( "00_primordials.js", "01_core.js", "02_error.js", )) - .ops(vec![ - op_close::decl(), - op_try_close::decl(), - op_print::decl(), - op_resources::decl(), - op_wasm_streaming_feed::decl(), - op_wasm_streaming_set_url::decl(), - op_void_sync::decl(), - op_void_async::decl(), - op_add::decl(), - // // TODO(@AaronO): track IO metrics for builtin streams - op_read::decl(), - op_read_all::decl(), - op_write::decl(), - op_write_all::decl(), - op_shutdown::decl(), - op_metrics::decl(), - op_format_file_name::decl(), - op_is_proxy::decl(), - op_str_byte_length::decl(), - ]) - .ops(crate::ops_builtin_v8::init_builtins_v8()) .build() } +pub(crate) fn init_builtin_ops() -> Extension { + ops(&mut ext()).build() +} + /// Return map of resources with id as key /// and string representation as value. #[op] diff --git a/core/runtime.rs b/core/runtime.rs index b9da063467..75bdb45196 100644 --- a/core/runtime.rs +++ b/core/runtime.rs @@ -89,7 +89,6 @@ pub struct JsRuntime { snapshot_options: SnapshotOptions, allocations: IsolateAllocations, extensions: Vec, - extensions_with_js: Vec, event_loop_middlewares: Vec>, // Marks if this is considered the top-level runtime. Used only be inspector. is_main: bool, @@ -253,20 +252,12 @@ pub struct RuntimeOptions { /// JsRuntime extensions, not to be confused with ES modules. /// Only ops registered by extensions will be initialized. If you need - /// to execute JS code from extensions, use `extensions_with_js` options - /// instead. - pub extensions: Vec, - - /// JsRuntime extensions, not to be confused with ES modules. - /// Ops registered by extensions will be initialized and JS code will be - /// executed. If you don't need to execute JS code from extensions, use - /// `extensions` option instead. + /// to execute JS code from extensions, pass source files in `js` or `esm` + /// option on `ExtensionBuilder`. /// - /// This is useful when creating snapshots, in such case you would pass - /// extensions using `extensions_with_js`, later when creating a runtime - /// from the snapshot, you would pass these extensions using `extensions` - /// option. - pub extensions_with_js: Vec, + /// If you are creating a runtime from a snapshot take care not to include + /// JavaScript sources in the extensions. + pub extensions: Vec, /// V8 snapshot that should be loaded on startup. pub startup_snapshot: Option, @@ -373,18 +364,15 @@ impl JsRuntime { let has_startup_snapshot = options.startup_snapshot.is_some(); if !has_startup_snapshot { options - .extensions_with_js - .insert(0, crate::ops_builtin::init_builtins()); + .extensions + .insert(0, crate::ops_builtin::init_builtin_ops_and_esm()); } else { options .extensions - .insert(0, crate::ops_builtin::init_builtins()); + .insert(0, crate::ops_builtin::init_builtin_ops()); } - let ops = Self::collect_ops( - &mut options.extensions, - &mut options.extensions_with_js, - ); + let ops = Self::collect_ops(&mut options.extensions); let mut op_state = OpState::new(ops.len()); if let Some(get_error_class_fn) = options.get_error_class_fn { @@ -624,9 +612,12 @@ impl JsRuntime { let loader = if snapshot_options != SnapshotOptions::Load { let esm_sources = options - .extensions_with_js + .extensions .iter() - .flat_map(|ext| ext.get_esm_sources().to_owned()) + .flat_map(|ext| match ext.get_esm_sources() { + Some(s) => s.to_owned(), + None => vec![], + }) .collect::>(); #[cfg(feature = "include_js_files_for_snapshotting")] @@ -689,7 +680,6 @@ impl JsRuntime { allocations: IsolateAllocations::default(), event_loop_middlewares: Vec::with_capacity(options.extensions.len()), extensions: options.extensions, - extensions_with_js: options.extensions_with_js, state: state_rc, module_map: Some(module_map_rc), is_main: options.is_main, @@ -752,7 +742,7 @@ impl JsRuntime { /// Creates a new realm (V8 context) in this JS execution context, /// pre-initialized with all of the extensions that were passed in - /// [`RuntimeOptions::extensions_with_js`] when the [`JsRuntime`] was + /// [`RuntimeOptions::extensions`] when the [`JsRuntime`] was /// constructed. pub fn create_realm(&mut self) -> Result { let realm = { @@ -868,50 +858,45 @@ impl JsRuntime { } // Take extensions to avoid double-borrow - let extensions = std::mem::take(&mut self.extensions_with_js); + let extensions = std::mem::take(&mut self.extensions); for ext in &extensions { { - let esm_files = ext.get_esm_sources(); - if let Some(entry_point) = ext.get_esm_entry_point() { - let file_source = esm_files - .iter() - .find(|file| file.specifier == entry_point) - .unwrap(); - load_and_evaluate_module(self, file_source)?; - } else { - for file_source in esm_files { + if let Some(esm_files) = ext.get_esm_sources() { + if let Some(entry_point) = ext.get_esm_entry_point() { + let file_source = esm_files + .iter() + .find(|file| file.specifier == entry_point) + .unwrap(); load_and_evaluate_module(self, file_source)?; + } else { + for file_source in esm_files { + load_and_evaluate_module(self, file_source)?; + } } } } { - let js_files = ext.get_js_sources(); - for file_source in js_files { - // TODO(@AaronO): use JsRuntime::execute_static() here to move src off heap - realm.execute_script( - self.v8_isolate(), - &file_source.specifier, - &file_source.code.load()?, - )?; + if let Some(js_files) = ext.get_js_sources() { + for file_source in js_files { + // TODO(@AaronO): use JsRuntime::execute_static() here to move src off heap + realm.execute_script( + self.v8_isolate(), + &file_source.specifier, + &file_source.code.load()?, + )?; + } } } } // Restore extensions - self.extensions_with_js = extensions; + self.extensions = extensions; Ok(()) } /// Collects ops from extensions & applies middleware - fn collect_ops( - extensions: &mut [Extension], - extensions_with_js: &mut [Extension], - ) -> Vec { - let mut exts = vec![]; - exts.extend(extensions); - exts.extend(extensions_with_js); - + fn collect_ops(exts: &mut [Extension]) -> Vec { for (ext, previous_exts) in exts.iter().enumerate().map(|(i, ext)| (ext, &exts[..i])) { @@ -970,24 +955,6 @@ impl JsRuntime { // Restore extensions self.extensions = extensions; } - { - let mut extensions: Vec = - std::mem::take(&mut self.extensions_with_js); - - // Setup state - for e in extensions.iter_mut() { - // ops are already registered during in bindings::initialize_context(); - e.init_state(&mut op_state.borrow_mut()); - - // Setup event-loop middleware - if let Some(middleware) = e.init_event_loop_middleware() { - self.event_loop_middlewares.push(middleware); - } - } - - // Restore extensions - self.extensions_with_js = extensions; - } Ok(()) } diff --git a/core/snapshot_util.rs b/core/snapshot_util.rs index 8b4fa3fa40..38ab18b4f6 100644 --- a/core/snapshot_util.rs +++ b/core/snapshot_util.rs @@ -17,7 +17,6 @@ pub struct CreateSnapshotOptions { pub snapshot_path: PathBuf, pub startup_snapshot: Option, pub extensions: Vec, - pub extensions_with_js: Vec, pub compression_cb: Option>, pub snapshot_module_load_cb: Option, } @@ -29,7 +28,6 @@ pub fn create_snapshot(create_snapshot_options: CreateSnapshotOptions) { will_snapshot: true, startup_snapshot: create_snapshot_options.startup_snapshot, extensions: create_snapshot_options.extensions, - extensions_with_js: create_snapshot_options.extensions_with_js, snapshot_module_load_cb: create_snapshot_options.snapshot_module_load_cb, ..Default::default() }); diff --git a/ext/fetch/lib.rs b/ext/fetch/lib.rs index 647e0ec7fe..75f72233ea 100644 --- a/ext/fetch/lib.rs +++ b/ext/fetch/lib.rs @@ -149,17 +149,7 @@ pub fn init_ops(options: Options) -> Extension where FP: FetchPermissions + 'static, { - ops::(&mut ext(), options) - .esm(include_js_files!( - "20_headers.js", - "21_formdata.js", - "22_body.js", - "22_http_client.js", - "23_request.js", - "23_response.js", - "26_fetch.js", - )) - .build() + ops::(&mut ext(), options).build() } pub type CancelableResponseFuture = diff --git a/ext/ffi/lib.rs b/ext/ffi/lib.rs index f93e2e1213..2b90cd1c43 100644 --- a/ext/ffi/lib.rs +++ b/ext/ffi/lib.rs @@ -169,7 +169,5 @@ pub fn init_ops_and_esm( } pub fn init_ops(unstable: bool) -> Extension { - ops::

(&mut ext(), unstable) - .esm(include_js_files!("00_ffi.js",)) - .build() + ops::

(&mut ext(), unstable).build() } diff --git a/ext/fs/lib.rs b/ext/fs/lib.rs index 31782d38d2..26904e7fea 100644 --- a/ext/fs/lib.rs +++ b/ext/fs/lib.rs @@ -202,9 +202,7 @@ pub fn init_ops_and_esm( } pub fn init_ops(unstable: bool) -> Extension { - ops::

(&mut ext(), unstable) - .esm(include_js_files!("30_fs.js",)) - .build() + ops::

(&mut ext(), unstable).build() } fn default_err_mapper(err: Error, desc: String) -> Error { diff --git a/runtime/build.rs b/runtime/build.rs index 784e46d4fc..2bb2b4cf10 100644 --- a/runtime/build.rs +++ b/runtime/build.rs @@ -250,7 +250,7 @@ mod startup_snapshot { )) .build(); - let mut extensions_with_js: Vec = vec![ + let mut extensions: Vec = vec![ deno_webidl::init_esm(), deno_console::init_esm(), deno_url::init_ops_and_esm(), @@ -291,15 +291,14 @@ mod startup_snapshot { ]; if let Some(additional_extension) = maybe_additional_extension { - extensions_with_js.push(additional_extension); + extensions.push(additional_extension); } create_snapshot(CreateSnapshotOptions { cargo_manifest_dir: env!("CARGO_MANIFEST_DIR"), snapshot_path, startup_snapshot: None, - extensions: vec![], - extensions_with_js, + extensions, compression_cb: Some(Box::new(|vec, snapshot_slice| { lzzzz::lz4_hc::compress_to_vec( snapshot_slice, diff --git a/runtime/examples/hello_runtime.rs b/runtime/examples/hello_runtime.rs index a47e9c908e..1f09551a6b 100644 --- a/runtime/examples/hello_runtime.rs +++ b/runtime/examples/hello_runtime.rs @@ -43,7 +43,6 @@ async fn main() -> Result<(), AnyError> { inspect: false, }, extensions: vec![], - extensions_with_js: vec![], startup_snapshot: None, unsafely_ignore_certificate_errors: None, root_cert_store: None, diff --git a/runtime/worker.rs b/runtime/worker.rs index ff55e27cc8..7ce6eb58dc 100644 --- a/runtime/worker.rs +++ b/runtime/worker.rs @@ -72,21 +72,11 @@ pub struct WorkerOptions { pub bootstrap: BootstrapOptions, /// JsRuntime extensions, not to be confused with ES modules. - /// Only ops registered by extensions will be initialized. If you need - /// to execute JS code from extensions, use `extensions_with_js` options - /// instead. - pub extensions: Vec, - - /// JsRuntime extensions, not to be confused with ES modules. - /// Ops registered by extensions will be initialized and JS code will be - /// executed. If you don't need to execute JS code from extensions, use - /// `extensions` option instead. /// - /// This is useful when creating snapshots, in such case you would pass - /// extensions using `extensions_with_js`, later when creating a runtime - /// from the snapshot, you would pass these extensions using `extensions` - /// option. - pub extensions_with_js: Vec, + /// Extensions register "ops" and JavaScript sources provided in `js` or `esm` + /// configuration. If you are using a snapshot, then extensions shouldn't + /// provide JavaScript sources that were already snapshotted. + pub extensions: Vec, /// V8 snapshot that should be loaded on startup. pub startup_snapshot: Option, @@ -174,7 +164,6 @@ impl Default for WorkerOptions { npm_resolver: Default::default(), blob_store: Default::default(), extensions: Default::default(), - extensions_with_js: Default::default(), startup_snapshot: Default::default(), bootstrap: Default::default(), stdio: Default::default(), @@ -299,7 +288,6 @@ impl MainWorker { shared_array_buffer_store: options.shared_array_buffer_store.clone(), compiled_wasm_module_store: options.compiled_wasm_module_store.clone(), extensions, - extensions_with_js: options.extensions_with_js, inspector: options.maybe_inspector_server.is_some(), is_main: true, leak_isolate: options.leak_isolate,