From 3298ca06f58db3b0ed75c3853e5b603ef8bd5f4d Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sat, 13 Jan 2024 20:56:56 -0500 Subject: [PATCH] fix(jsr): ensure proper storage of ahead of time module infos (#21918) This was incorrectly being stored so the AOT cache for JSR specifiers wasn't being used. I added a wrapper type to help prevent the API being used incorrectly. --- cli/cache/mod.rs | 5 ++- cli/cache/module_info.rs | 75 +++++++++++++++++++++++++++++++++------- 2 files changed, 66 insertions(+), 14 deletions(-) diff --git a/cli/cache/mod.rs b/cli/cache/mod.rs index 87ad23b10c..13efc24ff6 100644 --- a/cli/cache/mod.rs +++ b/cli/cache/mod.rs @@ -96,6 +96,8 @@ pub type LocalLspHttpCache = deno_cache_dir::LocalLspHttpCache; pub use deno_cache_dir::HttpCache; +use self::module_info::ModuleInfoCacheSourceHash; + /// A "wrapper" for the FileFetcher and DiskCache for the Deno CLI that provides /// a concise interface to the DENO_DIR when building module graphs. pub struct FetchCacher { @@ -280,10 +282,11 @@ impl Loader for FetchCacher { source: &str, module_info: &deno_graph::ModuleInfo, ) { + let source_hash = ModuleInfoCacheSourceHash::from_source(source.as_bytes()); let result = self.module_info_cache.set_module_info( specifier, MediaType::from_specifier(specifier), - source, + &source_hash, module_info, ); if let Err(err) = result { diff --git a/cli/cache/module_info.rs b/cli/cache/module_info.rs index 4a80774f66..063809f849 100644 --- a/cli/cache/module_info.rs +++ b/cli/cache/module_info.rs @@ -39,6 +39,22 @@ pub static MODULE_INFO_CACHE_DB: CacheDBConfiguration = CacheDBConfiguration { on_failure: CacheFailure::InMemory, }; +pub struct ModuleInfoCacheSourceHash(String); + +impl ModuleInfoCacheSourceHash { + pub fn new(hash: u64) -> Self { + Self(hash.to_string()) + } + + pub fn from_source(source: &[u8]) -> Self { + Self::new(FastInsecureHasher::hash(source)) + } + + pub fn as_str(&self) -> &str { + &self.0 + } +} + /// A cache of `deno_graph::ModuleInfo` objects. Using this leads to a considerable /// performance improvement because when it exists we can skip parsing a module for /// deno_graph. @@ -68,7 +84,7 @@ impl ModuleInfoCache { &self, specifier: &ModuleSpecifier, media_type: MediaType, - expected_source_hash: &str, + expected_source_hash: &ModuleInfoCacheSourceHash, ) -> Result, AnyError> { let query = SELECT_MODULE_INFO; let res = self.conn.query_row( @@ -76,7 +92,7 @@ impl ModuleInfoCache { params![ &specifier.as_str(), serialize_media_type(media_type), - &expected_source_hash, + expected_source_hash.as_str(), ], |row| { let module_info: String = row.get(0)?; @@ -91,7 +107,7 @@ impl ModuleInfoCache { &self, specifier: &ModuleSpecifier, media_type: MediaType, - source_hash: &str, + source_hash: &ModuleInfoCacheSourceHash, module_info: &ModuleInfo, ) -> Result<(), AnyError> { let sql = " @@ -104,7 +120,7 @@ impl ModuleInfoCache { params![ specifier.as_str(), serialize_media_type(media_type), - &source_hash, + source_hash.as_str(), &serde_json::to_string(&module_info)?, ], )?; @@ -135,7 +151,7 @@ impl<'a> deno_graph::ModuleAnalyzer for ModuleInfoCacheModuleAnalyzer<'a> { media_type: MediaType, ) -> Result { // attempt to load from the cache - let source_hash = FastInsecureHasher::hash(source.as_bytes()).to_string(); + let source_hash = ModuleInfoCacheSourceHash::from_source(source.as_bytes()); match self.module_info_cache.get_module_info( specifier, media_type, @@ -214,7 +230,11 @@ mod test { ModuleSpecifier::parse("https://localhost/mod2.ts").unwrap(); assert_eq!( cache - .get_module_info(&specifier1, MediaType::JavaScript, "1") + .get_module_info( + &specifier1, + MediaType::JavaScript, + &ModuleInfoCacheSourceHash::new(1) + ) .unwrap(), None ); @@ -234,31 +254,52 @@ mod test { text: "test".to_string(), }); cache - .set_module_info(&specifier1, MediaType::JavaScript, "1", &module_info) + .set_module_info( + &specifier1, + MediaType::JavaScript, + &ModuleInfoCacheSourceHash::new(1), + &module_info, + ) .unwrap(); assert_eq!( cache - .get_module_info(&specifier1, MediaType::JavaScript, "1") + .get_module_info( + &specifier1, + MediaType::JavaScript, + &ModuleInfoCacheSourceHash::new(1) + ) .unwrap(), Some(module_info.clone()) ); assert_eq!( cache - .get_module_info(&specifier2, MediaType::JavaScript, "1") + .get_module_info( + &specifier2, + MediaType::JavaScript, + &ModuleInfoCacheSourceHash::new(1) + ) .unwrap(), None, ); // different media type assert_eq!( cache - .get_module_info(&specifier1, MediaType::TypeScript, "1") + .get_module_info( + &specifier1, + MediaType::TypeScript, + &ModuleInfoCacheSourceHash::new(1) + ) .unwrap(), None, ); // different source hash assert_eq!( cache - .get_module_info(&specifier1, MediaType::JavaScript, "2") + .get_module_info( + &specifier1, + MediaType::JavaScript, + &ModuleInfoCacheSourceHash::new(2) + ) .unwrap(), None, ); @@ -269,7 +310,11 @@ mod test { // should get it assert_eq!( cache - .get_module_info(&specifier1, MediaType::JavaScript, "1") + .get_module_info( + &specifier1, + MediaType::JavaScript, + &ModuleInfoCacheSourceHash::new(1) + ) .unwrap(), Some(module_info) ); @@ -280,7 +325,11 @@ mod test { // should no longer exist assert_eq!( cache - .get_module_info(&specifier1, MediaType::JavaScript, "1") + .get_module_info( + &specifier1, + MediaType::JavaScript, + &ModuleInfoCacheSourceHash::new(1) + ) .unwrap(), None, );