Skip to content

Commit

Permalink
fix(jsr): ensure proper storage of ahead of time module infos (denola…
Browse files Browse the repository at this point in the history
…nd#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.
  • Loading branch information
dsherret committed Jan 14, 2024
1 parent 248fb9c commit 3298ca0
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 14 deletions.
5 changes: 4 additions & 1 deletion cli/cache/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ pub type LocalLspHttpCache =
deno_cache_dir::LocalLspHttpCache<RealDenoCacheEnv>;
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 {
Expand Down Expand Up @@ -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 {
Expand Down
75 changes: 62 additions & 13 deletions cli/cache/module_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -68,15 +84,15 @@ impl ModuleInfoCache {
&self,
specifier: &ModuleSpecifier,
media_type: MediaType,
expected_source_hash: &str,
expected_source_hash: &ModuleInfoCacheSourceHash,
) -> Result<Option<ModuleInfo>, AnyError> {
let query = SELECT_MODULE_INFO;
let res = self.conn.query_row(
query,
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)?;
Expand All @@ -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 = "
Expand All @@ -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)?,
],
)?;
Expand Down Expand Up @@ -135,7 +151,7 @@ impl<'a> deno_graph::ModuleAnalyzer for ModuleInfoCacheModuleAnalyzer<'a> {
media_type: MediaType,
) -> Result<ModuleInfo, deno_ast::Diagnostic> {
// 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,
Expand Down Expand Up @@ -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
);
Expand All @@ -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,
);
Expand All @@ -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)
);
Expand All @@ -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,
);
Expand Down

0 comments on commit 3298ca0

Please sign in to comment.