Repository: denoland/deno_core (rust)

Issue:
`ExtensionFileSourceCode` variant for computed strings
As part of the approach I'm favouring for https://github.com/denoland/deno/issues/19399, we need another variant for computed strings. Since it would need to be cloned on `.load()` it should be stored as `Arc<str>`.
```rust
pub enum ExtensionFileSourceCode {
  IncludedInBinary(&'static str),
  LoadedFromFsDuringSnapshot(&'static str), // <- Path
  Computed(Arc<str>),
}
```
And then I think we can remove `snapshot_module_load_cb` in favour of just computing the TS transpilations, which we would still only do during snapshot creation (or a no-snapshot mode for dev in the future).

---

Using a `module_load_cb` (expanding that feature to work outside of snapshot creation) has the advantage of holding one less copy of modules throughout the process runtime including the one allocated by V8. But it also means it needs to be recomputed for other realms, so that allocation is meaningful. For CLI we don't care very much since again it's only done on snapshot creation.

Another solution is `ExtensionFileSourceCode::IncludedInBinary(computed_string.leak())` which will be stable later this month. That can take advantage of static ascii opts. But leaking may not always be an option for embedders, there's no harm in having this option.

Pull request:
feat(extensions): computed extension code
Closes #116.

Patch:
diff --git a/core/examples/http_bench_json_ops/main.rs b/core/examples/http_bench_json_ops/main.rs
index e1ca293..ca86fd4 100644
--- a/core/examples/http_bench_json_ops/main.rs
+++ b/core/examples/http_bench_json_ops/main.rs
@@ -106,13 +106,10 @@ fn create_js_runtime() -> JsRuntimeForSnapshot {
     ..Default::default()
   };
 
-  JsRuntimeForSnapshot::new(
-    deno_core::RuntimeOptions {
-      extensions: vec![ext],
-      ..Default::default()
-    },
-    Default::default(),
-  )
+  JsRuntimeForSnapshot::new(deno_core::RuntimeOptions {
+    extensions: vec![ext],
+    ..Default::default()
+  })
 }
 
 #[op]
diff --git a/core/extensions.rs b/core/extensions.rs
index 91a1939..64f89d5 100644
--- a/core/extensions.rs
+++ b/core/extensions.rs
@@ -6,11 +6,12 @@ use anyhow::Error;
 use std::borrow::Cow;
 use std::cell::RefCell;
 use std::rc::Rc;
+use std::sync::Arc;
 use std::task::Context;
 use v8::fast_api::FastFunction;
 use v8::ExternalReference;
 
-#[derive(Copy, Clone, Debug)]
+#[derive(Clone, Debug)]
 pub enum ExtensionFileSourceCode {
   /// Source code is included in the binary produced. Either by being defined
   /// inline, or included using `include_str!()`. If you are snapshotting, this
@@ -22,9 +23,12 @@ pub enum ExtensionFileSourceCode {
   // embedder is creating snapshots. Files will be loaded from the filesystem
   // during the build time and they will only be present in the V8 snapshot.
   LoadedFromFsDuringSnapshot(&'static str), // <- Path
+
+  /// Source code may be computed at runtime.
+  Computed(Arc<str>),
 }
 
-#[derive(Copy, Clone, Debug)]
+#[derive(Clone, Debug)]
 pub struct ExtensionFileSource {
   pub specifier: &'static str,
   pub code: ExtensionFileSourceCode,
@@ -57,6 +61,15 @@ impl ExtensionFileSource {
         );
         Ok(s.into())
       }
+      ExtensionFileSourceCode::Computed(code) => {
+        debug_assert!(
+          code.is_ascii(),
+          "Extension code must be 7-bit ASCII: {} (found {})",
+          self.specifier,
+          Self::find_non_ascii(code)
+        );
+        Ok(ModuleCode::Arc(code.clone()))
+      }
     }
   }
 }
diff --git a/core/lib.rs b/core/lib.rs
index 1c9aa18..7b752cb 100644
--- a/core/lib.rs
+++ b/core/lib.rs
@@ -121,7 +121,6 @@ pub use crate::runtime::JsRealm;
 pub use crate::runtime::JsRuntime;
 pub use crate::runtime::JsRuntimeForSnapshot;
 pub use crate::runtime::RuntimeOptions;
-pub use crate::runtime::RuntimeSnapshotOptions;
 pub use crate::runtime::SharedArrayBufferStore;
 pub use crate::runtime::Snapshot;
 pub use crate::runtime::V8_WRAPPER_OBJECT_INDEX;
diff --git a/core/modules/loaders.rs b/core/modules/loaders.rs
index bd4d035..72ddc84 100644
--- a/core/modules/loaders.rs
+++ b/core/modules/loaders.rs
@@ -20,7 +20,6 @@ use std::collections::HashMap;
 use std::collections::HashSet;
 use std::future::Future;
 use std::pin::Pin;
-use std::rc::Rc;
 
 pub trait ModuleLoader {
   /// Returns an absolute URL.
@@ -107,25 +106,20 @@ pub type ExtModuleLoaderCb =
   Box<dyn Fn(&ExtensionFileSource) -> Result<ModuleCode, Error>>;
 
 pub(crate) struct ExtModuleLoader {
-  maybe_load_callback: Option<Rc<ExtModuleLoaderCb>>,
   sources: RefCell<HashMap<String, ExtensionFileSource>>,
   used_specifiers: RefCell<HashSet<String>>,
 }
 
 impl ExtModuleLoader {
-  pub fn new(
-    extensions: &[Extension],
-    maybe_load_callback: Option<Rc<ExtModuleLoaderCb>>,
-  ) -> Self {
+  pub fn new(extensions: &[Extension]) -> Self {
     let mut sources = HashMap::new();
     sources.extend(
       extensions
         .iter()
         .flat_map(|e| e.get_esm_sources())
-        .map(|s| (s.specifier.to_string(), *s)),
+        .map(|s| (s.specifier.to_string(), s.clone())),
     );
     ExtModuleLoader {
-      maybe_load_callback,
       sources: RefCell::new(sources),
       used_specifiers: Default::default(),
     }
@@ -157,11 +151,7 @@ impl ModuleLoader for ExtModuleLoader {
       .used_specifiers
       .borrow_mut()
       .insert(specifier.to_string());
-    let result = if let Some(load_callback) = &self.maybe_load_callback {
-      load_callback(source)
-    } else {
-      source.load()
-    };
+    let result = source.load();
     match result {
       Ok(code) => {
         let res = ModuleSource::new(ModuleType::JavaScript, code, specifier);
diff --git a/core/runtime/jsruntime.rs b/core/runtime/jsruntime.rs
index c150458..86c015c 100644
--- a/core/runtime/jsruntime.rs
+++ b/core/runtime/jsruntime.rs
@@ -16,7 +16,6 @@ use crate::inspector::JsRuntimeInspector;
 use crate::module_specifier::ModuleSpecifier;
 use crate::modules::AssertedModuleType;
 use crate::modules::ExtModuleLoader;
-use crate::modules::ExtModuleLoaderCb;
 use crate::modules::ModuleCode;
 use crate::modules::ModuleId;
 use crate::modules::ModuleLoader;
@@ -436,14 +435,6 @@ impl RuntimeOptions {
   }
 }
 
-#[derive(Default)]
-pub struct RuntimeSnapshotOptions {
-  /// An optional callback that will be called for each module that is loaded
-  /// during snapshotting. This callback can be used to transpile source on the
-  /// fly, during snapshotting, eg. to transpile TypeScript to JavaScript.
-  pub snapshot_module_load_cb: Option<ExtModuleLoaderCb>,
-}
-
 #[derive(Default)]
 pub struct CreateRealmOptions {
   /// Implementation of `ModuleLoader` which will be
@@ -462,7 +453,7 @@ impl JsRuntime {
       cfg!(test),
       options.unsafe_expose_natives_and_gc(),
     );
-    JsRuntime::new_inner(options, false, None)
+    JsRuntime::new_inner(options, false)
   }
 
   pub(crate) fn state_from(
@@ -513,11 +504,7 @@ impl JsRuntime {
       .call_once(move || v8_init(v8_platform, predictable, expose_natives));
   }
 
-  fn new_inner(
-    mut options: RuntimeOptions,
-    will_snapshot: bool,
-    maybe_load_callback: Option<ExtModuleLoaderCb>,
-  ) -> JsRuntime {
+  fn new_inner(mut options: RuntimeOptions, will_snapshot: bool) -> JsRuntime {
     let init_mode = InitMode::from_options(&options);
     let (op_state, ops) = Self::create_opstate(&mut options);
     let op_state = Rc::new(RefCell::new(op_state));
@@ -734,9 +721,7 @@ impl JsRuntime {
 
     let realm = js_runtime.main_realm();
     // TODO(mmastrac): We should thread errors back out of the runtime
-    js_runtime
-      .init_extension_js(&realm, maybe_load_callback)
-      .unwrap();
+    js_runtime.init_extension_js(&realm).unwrap();
 
     // If the embedder has requested to clear the module map resulting from
     // extensions, possibly with exceptions.
@@ -871,7 +856,7 @@ impl JsRuntime {
       JsRealm::new(realm)
     };
 
-    self.init_extension_js(&realm, None)?;
+    self.init_extension_js(&realm)?;
 
     // If the embedder has requested to clear the module map resulting from
     // extensions, possibly with exceptions.
@@ -894,11 +879,7 @@ impl JsRuntime {
   }
 
   /// Initializes JS of provided Extensions in the given realm.
-  fn init_extension_js(
-    &mut self,
-    realm: &JsRealm,
-    maybe_load_callback: Option<ExtModuleLoaderCb>,
-  ) -> Result<(), Error> {
+  fn init_extension_js(&mut self, realm: &JsRealm) -> Result<(), Error> {
     // Initialization of JS happens in phases:
     // 1. Iterate through all extensions:
     //  a. Execute all extension "script" JS files
@@ -912,10 +893,7 @@ impl JsRuntime {
     let extensions = std::mem::take(&mut self.extensions);
 
     let loader = module_map_rc.borrow().loader.clone();
-    let ext_loader = Rc::new(ExtModuleLoader::new(
-      &extensions,
-      maybe_load_callback.map(Rc::new),
-    ));
+    let ext_loader = Rc::new(ExtModuleLoader::new(&extensions));
     module_map_rc.borrow_mut().loader = ext_loader;
 
     let mut esm_entrypoints = vec![];
@@ -1683,20 +1661,13 @@ fn create_context<'a>(
 }
 
 impl JsRuntimeForSnapshot {
-  pub fn new(
-    mut options: RuntimeOptions,
-    runtime_snapshot_options: RuntimeSnapshotOptions,
-  ) -> JsRuntimeForSnapshot {
+  pub fn new(mut options: RuntimeOptions) -> JsRuntimeForSnapshot {
     JsRuntime::init_v8(
       options.v8_platform.take(),
       true,
       options.unsafe_expose_natives_and_gc(),
     );
-    JsRuntimeForSnapshot(JsRuntime::new_inner(
-      options,
-      true,
-      runtime_snapshot_options.snapshot_module_load_cb,
-    ))
+    JsRuntimeForSnapshot(JsRuntime::new_inner(options, true))
   }
 
   /// Takes a snapshot and consumes the runtime.
diff --git a/core/runtime/mod.rs b/core/runtime/mod.rs
index d30ee67..8cdd189 100644
--- a/core/runtime/mod.rs
+++ b/core/runtime/mod.rs
@@ -22,7 +22,6 @@ pub use jsruntime::JsRuntime;
 pub use jsruntime::JsRuntimeForSnapshot;
 pub use jsruntime::JsRuntimeState;
 pub use jsruntime::RuntimeOptions;
-pub use jsruntime::RuntimeSnapshotOptions;
 pub use jsruntime::SharedArrayBufferStore;
 pub use jsruntime::Snapshot;
 pub use snapshot_util::create_snapshot;
diff --git a/core/runtime/snapshot_util.rs b/core/runtime/snapshot_util.rs
index 8559e95..8febb13 100644
--- a/core/runtime/snapshot_util.rs
+++ b/core/runtime/snapshot_util.rs
@@ -5,8 +5,6 @@ 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;
@@ -22,7 +20,6 @@ pub struct CreateSnapshotOptions {
   pub startup_snapshot: Option<Snapshot>,
   pub extensions: Vec<Extension>,
   pub compression_cb: Option<Box<CompressionCb>>,
-  pub snapshot_module_load_cb: Option<ExtModuleLoaderCb>,
   pub with_runtime_cb: Option<Box<WithRuntimeCb>>,
 }
 
@@ -38,16 +35,11 @@ pub fn create_snapshot(
 ) -> CreateSnapshotOutput {
   let mut mark = Instant::now();
 
-  let mut js_runtime = JsRuntimeForSnapshot::new(
-    RuntimeOptions {
-      startup_snapshot: create_snapshot_options.startup_snapshot,
-      extensions: create_snapshot_options.extensions,
-      ..Default::default()
-    },
-    RuntimeSnapshotOptions {
-      snapshot_module_load_cb: create_snapshot_options.snapshot_module_load_cb,
-    },
-  );
+  let mut js_runtime = JsRuntimeForSnapshot::new(RuntimeOptions {
+    startup_snapshot: create_snapshot_options.startup_snapshot,
+    extensions: create_snapshot_options.extensions,
+    ..Default::default()
+  });
   println!(
     "JsRuntime for snapshot prepared, took {:#?} ({})",
     Instant::now().saturating_duration_since(mark),


Test patch:
diff --git a/core/modules/tests.rs b/core/modules/tests.rs
index b65e54e..6d78e24 100644
--- a/core/modules/tests.rs
+++ b/core/modules/tests.rs
@@ -985,13 +985,10 @@ fn dynamic_imports_snapshot() {
     );
 
     let loader = MockLoader::new();
-    let mut runtime = JsRuntimeForSnapshot::new(
-      RuntimeOptions {
-        module_loader: Some(loader),
-        ..Default::default()
-      },
-      Default::default(),
-    );
+    let mut runtime = JsRuntimeForSnapshot::new(RuntimeOptions {
+      module_loader: Some(loader),
+      ..Default::default()
+    });
     // In default resolution code should be empty.
     // Instead we explicitly pass in our own code.
     // The behavior should be very similar to /a.js.
@@ -1029,13 +1026,10 @@ fn import_meta_snapshot() {
     );
 
     let loader = MockLoader::new();
-    let mut runtime = JsRuntimeForSnapshot::new(
-      RuntimeOptions {
-        module_loader: Some(loader),
-        ..Default::default()
-      },
-      Default::default(),
-    );
+    let mut runtime = JsRuntimeForSnapshot::new(RuntimeOptions {
+      module_loader: Some(loader),
+      ..Default::default()
+    });
     // In default resolution code should be empty.
     // Instead we explicitly pass in our own code.
     // The behavior should be very similar to /a.js.
diff --git a/core/runtime/tests/jsrealm.rs b/core/runtime/tests/jsrealm.rs
index d0f9b40..5dc3b1a 100644
--- a/core/runtime/tests/jsrealm.rs
+++ b/core/runtime/tests/jsrealm.rs
@@ -183,8 +183,7 @@ fn js_realm_init() {
 #[test]
 fn js_realm_init_snapshot() {
   let snapshot = {
-    let runtime =
-      JsRuntimeForSnapshot::new(Default::default(), Default::default());
+    let runtime = JsRuntimeForSnapshot::new(Default::default());
     let snap: &[u8] = &runtime.snapshot();
     Vec::from(snap).into_boxed_slice()
   };
@@ -819,13 +818,10 @@ fn es_snapshot() {
       ..Default::default()
     };
 
-    let runtime = JsRuntimeForSnapshot::new(
-      RuntimeOptions {
-        extensions: vec![extension],
-        ..Default::default()
-      },
-      Default::default(),
-    );
+    let runtime = JsRuntimeForSnapshot::new(RuntimeOptions {
+      extensions: vec![extension],
+      ..Default::default()
+    });
     runtime.snapshot()
   };
 
diff --git a/core/runtime/tests/snapshot.rs b/core/runtime/tests/snapshot.rs
index 399d7ff..2783733 100644
--- a/core/runtime/tests/snapshot.rs
+++ b/core/runtime/tests/snapshot.rs
@@ -16,8 +16,7 @@ use url::Url;
 #[test]
 fn will_snapshot() {
   let snapshot = {
-    let mut runtime =
-      JsRuntimeForSnapshot::new(Default::default(), Default::default());
+    let mut runtime = JsRuntimeForSnapshot::new(Default::default());
     runtime.execute_script_static("a.js", "a = 1 + 2").unwrap();
     runtime.snapshot()
   };
@@ -35,8 +34,7 @@ fn will_snapshot() {
 #[test]
 fn will_snapshot2() {
   let startup_data = {
-    let mut runtime =
-      JsRuntimeForSnapshot::new(Default::default(), Default::default());
+    let mut runtime = JsRuntimeForSnapshot::new(Default::default());
     runtime
       .execute_script_static("a.js", "let a = 1 + 2")
       .unwrap();
@@ -44,13 +42,10 @@ fn will_snapshot2() {
   };
 
   let snapshot = Snapshot::JustCreated(startup_data);
-  let mut runtime = JsRuntimeForSnapshot::new(
-    RuntimeOptions {
-      startup_snapshot: Some(snapshot),
-      ..Default::default()
-    },
-    Default::default(),
-  );
+  let mut runtime = JsRuntimeForSnapshot::new(RuntimeOptions {
+    startup_snapshot: Some(snapshot),
+    ..Default::default()
+  });
 
   let startup_data = {
     runtime
@@ -78,8 +73,7 @@ fn will_snapshot2() {
 #[test]
 fn test_snapshot_callbacks() {
   let snapshot = {
-    let mut runtime =
-      JsRuntimeForSnapshot::new(Default::default(), Default::default());
+    let mut runtime = JsRuntimeForSnapshot::new(Default::default());
     runtime
       .execute_script_static(
         "a.js",
@@ -113,8 +107,7 @@ fn test_snapshot_callbacks() {
 #[test]
 fn test_from_boxed_snapshot() {
   let snapshot = {
-    let mut runtime =
-      JsRuntimeForSnapshot::new(Default::default(), Default::default());
+    let mut runtime = JsRuntimeForSnapshot::new(Default::default());
     runtime.execute_script_static("a.js", "a = 1 + 2").unwrap();
     let snap: &[u8] = &runtime.snapshot();
     Vec::from(snap).into_boxed_slice()
@@ -209,17 +202,14 @@ fn es_snapshot() {
     Ok(String::from("test"))
   }
 
-  let mut runtime = JsRuntimeForSnapshot::new(
-    RuntimeOptions {
-      extensions: vec![Extension {
-        name: "test_ext",
-        ops: Cow::Borrowed(&[op_test::DECL]),
-        ..Default::default()
-      }],
+  let mut runtime = JsRuntimeForSnapshot::new(RuntimeOptions {
+    extensions: vec![Extension {
+      name: "test_ext",
+      ops: Cow::Borrowed(&[op_test::DECL]),
       ..Default::default()
-    },
-    Default::default(),
-  );
+    }],
+    ..Default::default()
+  });
 
   let specifier = crate::resolve_url("file:///0.js").unwrap();
   let source_code =
@@ -248,18 +238,15 @@ fn es_snapshot() {
 
   let snapshot = runtime.snapshot();
 
-  let mut runtime2 = JsRuntimeForSnapshot::new(
-    RuntimeOptions {
-      startup_snapshot: Some(Snapshot::JustCreated(snapshot)),
-      extensions: vec![Extension {
-        name: "test_ext",
-        ops: Cow::Borrowed(&[op_test::DECL]),
-        ..Default::default()
-      }],
+  let mut runtime2 = JsRuntimeForSnapshot::new(RuntimeOptions {
+    startup_snapshot: Some(Snapshot::JustCreated(snapshot)),
+    extensions: vec![Extension {
+      name: "test_ext",
+      ops: Cow::Borrowed(&[op_test::DECL]),
       ..Default::default()
-    },
-    Default::default(),
-  );
+    }],
+    ..Default::default()
+  });
 
   assert_module_map(&mut runtime2, &modules);
 
@@ -312,13 +299,10 @@ pub(crate) fn generic_es_snapshot_without_runtime_module_loader(
       ..Default::default()
     };
 
-    let runtime = JsRuntimeForSnapshot::new(
-      RuntimeOptions {
-        extensions: vec![extension],
-        ..Default::default()
-      },
-      Default::default(),
-    );
+    let runtime = JsRuntimeForSnapshot::new(RuntimeOptions {
+      extensions: vec![extension],
+      ..Default::default()
+    });
 
     runtime.snapshot()
   };
@@ -417,13 +401,10 @@ pub(crate) fn generic_preserve_snapshotted_modules_test(
   let loader = Rc::new(LoggingModuleLoader::new(NoopModuleLoader));
 
   let mut runtime = if test_snapshot {
-    let snapshot_runtime = JsRuntimeForSnapshot::new(
-      RuntimeOptions {
-        extensions: vec![extension],
-        ..Default::default()
-      },
-      Default::default(),
-    );
+    let snapshot_runtime = JsRuntimeForSnapshot::new(RuntimeOptions {
+      extensions: vec![extension],
+      ..Default::default()
+    });
     let startup_data = snapshot_runtime.snapshot();
 
     JsRuntime::new(RuntimeOptions {