Repository: denoland/deno_core
Pull request #753 (2024-05-23 19:05:32)

refactor: pass along hash through eval context code cache callbacks
Follow-up to https://github.com/denoland/deno_core/pull/752


Patch:
diff --git a/core/lib.rs b/core/lib.rs
index 5434462..727f9a7 100644
--- a/core/lib.rs
+++ b/core/lib.rs
@@ -104,6 +104,7 @@ pub use crate::module_specifier::resolve_import;
 pub use crate::module_specifier::resolve_path;
 pub use crate::module_specifier::resolve_url;
 pub use crate::module_specifier::resolve_url_or_path;
+pub use crate::module_specifier::specifier_has_uri_scheme;
 pub use crate::module_specifier::ModuleResolutionError;
 pub use crate::module_specifier::ModuleSpecifier;
 pub use crate::modules::CustomModuleEvaluationKind;
@@ -117,12 +118,12 @@ pub use crate::modules::ModuleLoader;
 pub use crate::modules::ModuleName;
 pub use crate::modules::ModuleSource;
 pub use crate::modules::ModuleSourceCode;
-pub use crate::modules::ModuleSourceCodeCache;
 pub use crate::modules::ModuleSourceFuture;
 pub use crate::modules::ModuleType;
 pub use crate::modules::NoopModuleLoader;
 pub use crate::modules::RequestedModuleType;
 pub use crate::modules::ResolutionKind;
+pub use crate::modules::SourceCodeCacheInfo;
 pub use crate::modules::StaticModuleLoader;
 pub use crate::modules::ValidateImportAttributesCb;
 pub use crate::normalize_path::normalize_path;
diff --git a/core/module_specifier.rs b/core/module_specifier.rs
index a251666..30cdf2d 100644
--- a/core/module_specifier.rs
+++ b/core/module_specifier.rs
@@ -143,10 +143,10 @@ pub fn resolve_path(
 /// We additionally require the scheme to be at least 2 characters long,
 /// because otherwise a windows path like c:/foo would be treated as a URL,
 /// while no schemes with a one-letter name actually exist.
-fn specifier_has_uri_scheme(specifier: &str) -> bool {
+pub fn specifier_has_uri_scheme(specifier: &str) -> bool {
   let mut chars = specifier.chars();
   let mut len = 0usize;
-  // THe first character must be a letter.
+  // The first character must be a letter.
   match chars.next() {
     Some(c) if c.is_ascii_alphabetic() => len += 1,
     _ => return false,
diff --git a/core/modules/mod.rs b/core/modules/mod.rs
index 8e52efc..7309b93 100644
--- a/core/modules/mod.rs
+++ b/core/modules/mod.rs
@@ -202,10 +202,14 @@ pub type CustomModuleEvaluationCb = Box<
   ) -> Result<CustomModuleEvaluationKind, AnyError>,
 >;
 
+/// A callback to get the code cache for a script.
+/// (specifier, code) -> ...
 pub type EvalContextGetCodeCacheCb =
-  Box<dyn Fn(&str) -> Result<Option<Cow<'static, [u8]>>, AnyError>>;
+  Box<dyn Fn(&Url, &v8::String) -> Result<SourceCodeCacheInfo, AnyError>>;
 
-pub type EvalContextCodeCacheReadyCb = Box<dyn Fn(&str, &[u8])>;
+/// Callback when the code cache is ready.
+/// (specifier, hash, data) -> ()
+pub type EvalContextCodeCacheReadyCb = Box<dyn Fn(Url, u64, &[u8])>;
 
 pub enum CustomModuleEvaluationKind {
   /// This evaluation results in a single, "synthetic" module.
@@ -338,7 +342,7 @@ impl ModuleType {
 }
 
 #[derive(Debug)]
-pub struct ModuleSourceCodeCache {
+pub struct SourceCodeCacheInfo {
   pub hash: u64,
   pub data: Option<Cow<'static, [u8]>>,
 }
@@ -363,7 +367,7 @@ pub struct ModuleSourceCodeCache {
 pub struct ModuleSource {
   pub code: ModuleSourceCode,
   pub module_type: ModuleType,
-  pub code_cache: Option<ModuleSourceCodeCache>,
+  pub code_cache: Option<SourceCodeCacheInfo>,
   module_url_specified: ModuleName,
   /// If the module was found somewhere other than the specified address, this will be [`Some`].
   module_url_found: Option<ModuleName>,
@@ -375,7 +379,7 @@ impl ModuleSource {
     module_type: impl Into<ModuleType>,
     code: ModuleSourceCode,
     specifier: &ModuleSpecifier,
-    code_cache: Option<ModuleSourceCodeCache>,
+    code_cache: Option<SourceCodeCacheInfo>,
   ) -> Self {
     let module_url_specified = specifier.as_ref().to_owned().into();
     Self {
@@ -394,7 +398,7 @@ impl ModuleSource {
     code: ModuleSourceCode,
     specifier: &ModuleSpecifier,
     specifier_found: &ModuleSpecifier,
-    code_cache: Option<ModuleSourceCodeCache>,
+    code_cache: Option<SourceCodeCacheInfo>,
   ) -> Self {
     let module_url_found = if specifier == specifier_found {
       None
@@ -428,7 +432,7 @@ impl ModuleSource {
     code: &'static str,
     specified: impl AsRef<str>,
     found: impl AsRef<str>,
-    code_cache: Option<ModuleSourceCodeCache>,
+    code_cache: Option<SourceCodeCacheInfo>,
   ) -> Self {
     let specified = specified.as_ref().to_string();
     let found = found.as_ref().to_string();
diff --git a/core/ops_builtin_v8.rs b/core/ops_builtin_v8.rs
index d3419d0..0eff1e0 100644
--- a/core/ops_builtin_v8.rs
+++ b/core/ops_builtin_v8.rs
@@ -272,19 +272,20 @@ pub fn op_eval_context<'a>(
   let tc_scope = &mut v8::TryCatch::new(scope);
   let source = v8::Local::<v8::String>::try_from(source)
     .map_err(|_| type_error("Invalid source"))?;
-  let specifier = resolve_url(&specifier)?.to_string();
-  let specifier_v8 = v8::String::new(tc_scope, &specifier).unwrap();
+  let specifier = resolve_url(&specifier)?;
+  let specifier_v8 = v8::String::new(tc_scope, specifier.as_str()).unwrap();
   let origin = script_origin(tc_scope, specifier_v8);
 
-  let (script, try_store_code_cache) = state
+  let (maybe_script, maybe_code_cache_hash) = state
     .eval_context_get_code_cache_cb
     .as_ref()
-    .and_then(|cb| {
-      cb(&specifier).unwrap().map(|code_cache| {
+    .map(|cb| {
+      let code_cache = cb(&specifier, &source).unwrap();
+      if let Some(code_cache_data) = &code_cache.data {
         let mut source = v8::script_compiler::Source::new_with_cached_data(
           source,
           Some(&origin),
-          v8::CachedData::new(&code_cache),
+          v8::CachedData::new(code_cache_data),
         );
         let script = v8::script_compiler::compile(
           tc_scope,
@@ -297,12 +298,19 @@ pub fn op_eval_context<'a>(
           Some(cached_data) => cached_data.rejected(),
           _ => true,
         };
-        (script, rejected)
-      })
+        let maybe_code_cache_hash = if rejected {
+          Some(code_cache.hash) // recreate the cache
+        } else {
+          None
+        };
+        (Some(script), maybe_code_cache_hash)
+      } else {
+        (None, Some(code_cache.hash))
+      }
     })
-    .unwrap_or_else(|| {
-      (v8::Script::compile(tc_scope, source, Some(&origin)), true)
-    });
+    .unwrap_or_else(|| (None, None));
+  let script = maybe_script
+    .unwrap_or_else(|| v8::Script::compile(tc_scope, source, Some(&origin)));
 
   let null = v8::null(tc_scope);
   let script = match script {
@@ -322,13 +330,13 @@ pub fn op_eval_context<'a>(
     }
   };
 
-  if try_store_code_cache {
+  if let Some(code_cache_hash) = maybe_code_cache_hash {
     if let Some(cb) = state.eval_context_code_cache_ready_cb.as_ref() {
       let unbound_script = script.get_unbound_script(tc_scope);
       let code_cache = unbound_script.create_code_cache().ok_or_else(|| {
         type_error("Unable to get code cache from unbound module script")
       })?;
-      cb(&specifier, &code_cache);
+      cb(specifier, code_cache_hash, &code_cache);
     }
   }
 
diff --git a/core/runtime/jsruntime.rs b/core/runtime/jsruntime.rs
index 7caf85e..8768c3f 100644
--- a/core/runtime/jsruntime.rs
+++ b/core/runtime/jsruntime.rs
@@ -536,11 +536,6 @@ pub struct RuntimeOptions {
   /// embedders might implement loading WASM or test modules.
   pub custom_module_evaluation_cb: Option<CustomModuleEvaluationCb>,
 
-  // Controls whether V8 code cache is enabled. Code cache can be applied
-  // to ES modules (loaded through `ModuleLoader`) and to scripts evaluated
-  // through `Deno.core.evalContext`.
-  pub enable_code_cache: bool,
-
   /// Callbacks to retrieve and store code cache for scripts evaluated
   /// through evalContext.
   pub eval_context_code_cache_cbs:
@@ -688,14 +683,10 @@ impl JsRuntime {
     let waker = op_state.waker.clone();
     let op_state = Rc::new(RefCell::new(op_state));
     let (eval_context_get_code_cache_cb, eval_context_set_code_cache_cb) =
-      if options.enable_code_cache {
-        options
-          .eval_context_code_cache_cbs
-          .map(|cbs| (Some(cbs.0), Some(cbs.1)))
-          .unwrap_or_default()
-      } else {
-        (None, None)
-      };
+      options
+        .eval_context_code_cache_cbs
+        .map(|cbs| (Some(cbs.0), Some(cbs.1)))
+        .unwrap_or_default();
     let state_rc = Rc::new(JsRuntimeState {
       source_mapper: RefCell::new(source_mapper),
       shared_array_buffer_store: options.shared_array_buffer_store,


Test patch:
diff --git a/core/modules/tests.rs b/core/modules/tests.rs
index 1b3e49c..5054cec 100644
--- a/core/modules/tests.rs
+++ b/core/modules/tests.rs
@@ -15,8 +15,8 @@ use crate::modules::ModuleError;
 use crate::modules::ModuleInfo;
 use crate::modules::ModuleRequest;
 use crate::modules::ModuleSourceCode;
-use crate::modules::ModuleSourceCodeCache;
 use crate::modules::RequestedModuleType;
+use crate::modules::SourceCodeCacheInfo;
 use crate::resolve_import;
 use crate::resolve_url;
 use crate::runtime::JsRuntime;
@@ -226,7 +226,7 @@ impl Future for DelayedSourceCodeFuture {
           src.0,
           inner.url.as_str(),
           src.1,
-          Some(ModuleSourceCodeCache {
+          Some(SourceCodeCacheInfo {
             hash,
             data: inner
               .code_cache
@@ -1730,7 +1730,6 @@ fn test_load_with_code_cache() {
     let updated_code_cache = loader.updated_code_cache.clone();
     let mut runtime = JsRuntime::new(RuntimeOptions {
       module_loader: Some(loader),
-      enable_code_cache: true,
       ..Default::default()
     });
     let spec = resolve_url("file:///a.js").unwrap();
@@ -1774,7 +1773,6 @@ fn test_load_with_code_cache() {
     let updated_code_cache = loader.updated_code_cache.clone();
     let mut runtime = JsRuntime::new(RuntimeOptions {
       module_loader: Some(loader),
-      enable_code_cache: true,
       ..Default::default()
     });
     let spec = resolve_url("file:///a.js").unwrap();
@@ -1811,7 +1809,6 @@ fn test_load_with_code_cache() {
     let updated_code_cache = loader.updated_code_cache.clone();
     let mut runtime = JsRuntime::new(RuntimeOptions {
       module_loader: Some(loader),
-      enable_code_cache: true,
       ..Default::default()
     });
     let spec = resolve_url("file:///a.js").unwrap();
diff --git a/core/runtime/tests/misc.rs b/core/runtime/tests/misc.rs
index 1ab7ed0..ba94cc2 100644
--- a/core/runtime/tests/misc.rs
+++ b/core/runtime/tests/misc.rs
@@ -1315,17 +1315,21 @@ fn eval_context_with_code_cache() {
   let code_cache = {
     let updated_code_cache = Arc::new(Mutex::new(HashMap::new()));
 
-    let get_code_cache_cb = Box::new(|_: &str| Ok(None));
+    let get_code_cache_cb = Box::new(|_: &Url, source: &v8::String| {
+      Ok(SourceCodeCacheInfo {
+        data: None,
+        hash: hash_source(source),
+      })
+    });
 
     let updated_code_cache_clone = updated_code_cache.clone();
     let set_code_cache_cb =
-      Box::new(move |specifier: &str, code_cache: &[u8]| {
+      Box::new(move |specifier: Url, _hash: u64, code_cache: &[u8]| {
         let mut c = updated_code_cache_clone.lock();
-        c.insert(specifier.to_string(), code_cache.to_vec());
+        c.insert(specifier, code_cache.to_vec());
       });
 
     let mut runtime = JsRuntime::new(RuntimeOptions {
-      enable_code_cache: true,
       eval_context_code_cache_cbs: Some((get_code_cache_cb, set_code_cache_cb)),
       ..Default::default()
     });
@@ -1337,7 +1341,7 @@ fn eval_context_with_code_cache() {
       .unwrap();
 
     let c = updated_code_cache.lock();
-    let mut keys = c.keys().collect::<Vec<_>>();
+    let mut keys = c.keys().map(|s| s.as_str()).collect::<Vec<_>>();
     keys.sort();
     assert_eq!(keys, vec!["file:///foo.js",]);
     c.clone()
@@ -1348,23 +1352,24 @@ fn eval_context_with_code_cache() {
     let updated_code_cache = Arc::new(Mutex::new(HashMap::new()));
 
     let code_cache_clone = code_cache.clone();
-    let get_code_cache_cb = Box::new(move |specifier: &str| {
-      Ok(
-        code_cache_clone
-          .get(specifier)
-          .map(|code_cache| Cow::Owned(code_cache.clone())),
-      )
-    });
+    let get_code_cache_cb =
+      Box::new(move |specifier: &Url, source: &v8::String| {
+        Ok(SourceCodeCacheInfo {
+          data: code_cache_clone
+            .get(specifier)
+            .map(|code_cache| Cow::Owned(code_cache.clone())),
+          hash: hash_source(source),
+        })
+      });
 
     let updated_code_cache_clone = updated_code_cache.clone();
     let set_code_cache_cb =
-      Box::new(move |specifier: &str, code_cache: &[u8]| {
+      Box::new(move |specifier: Url, _hash: u64, code_cache: &[u8]| {
         let mut c = updated_code_cache_clone.lock();
-        c.insert(specifier.to_string(), code_cache.to_vec());
+        c.insert(specifier, code_cache.to_vec());
       });
 
     let mut runtime = JsRuntime::new(RuntimeOptions {
-      enable_code_cache: true,
       eval_context_code_cache_cbs: Some((get_code_cache_cb, set_code_cache_cb)),
       ..Default::default()
     });
@@ -1380,3 +1385,11 @@ fn eval_context_with_code_cache() {
     assert!(c.is_empty());
   }
 }
+
+fn hash_source(source: &v8::String) -> u64 {
+  use std::hash::Hash;
+  use std::hash::Hasher;
+  let mut hasher = twox_hash::XxHash64::default();
+  source.hash(&mut hasher);
+  hasher.finish()
+}