From 6fce23c54ec619168eee096fc7bf801d0cec0cb6 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 8 Aug 2024 11:41:30 +0200 Subject: [PATCH] perf: skip saving to emit cache after first failure (#24896) --- cli/cache/emit.rs | 22 +++++++++++++++------- cli/cache/mod.rs | 4 ++-- cli/emit.rs | 4 ++-- cli/factory.rs | 6 +++--- cli/graph_util.rs | 4 ++-- tests/integration/run_tests.rs | 17 +++++++++++++++++ 6 files changed, 41 insertions(+), 16 deletions(-) diff --git a/cli/cache/emit.rs b/cli/cache/emit.rs index 1d903fbba6..fcec9e84b2 100644 --- a/cli/cache/emit.rs +++ b/cli/cache/emit.rs @@ -6,6 +6,7 @@ use deno_ast::ModuleSpecifier; use deno_core::anyhow::anyhow; use deno_core::error::AnyError; use deno_core::serde_json; +use deno_core::unsync::sync::AtomicFlag; use serde::Deserialize; use serde::Serialize; @@ -19,10 +20,10 @@ struct EmitMetadata { } /// The cache that stores previously emitted files. -#[derive(Clone)] pub struct EmitCache { disk_cache: DiskCache, cli_version: &'static str, + emit_failed_flag: AtomicFlag, } impl EmitCache { @@ -30,6 +31,7 @@ impl EmitCache { Self { disk_cache, cli_version: crate::version::deno(), + emit_failed_flag: Default::default(), } } @@ -87,12 +89,10 @@ impl EmitCache { code: &[u8], ) { if let Err(err) = self.set_emit_code_result(specifier, source_hash, code) { - // should never error here, but if it ever does don't fail - if cfg!(debug_assertions) { - panic!("Error saving emit data ({specifier}): {err}"); - } else { - log::debug!("Error saving emit data({}): {}", specifier, err); - } + // might error in cases such as a readonly file system + log::debug!("Error saving emit data ({}): {}", specifier, err); + // assume the cache can't be written to and disable caching to it + self.emit_failed_flag.raise(); } } @@ -102,6 +102,11 @@ impl EmitCache { source_hash: u64, code: &[u8], ) -> Result<(), AnyError> { + if self.emit_failed_flag.is_raised() { + log::debug!("Skipped emit cache save of {}", specifier); + return Ok(()); + } + let meta_filename = self .get_meta_filename(specifier) .ok_or_else(|| anyhow!("Could not get meta filename."))?; @@ -161,6 +166,7 @@ mod test { let cache = EmitCache { disk_cache: disk_cache.clone(), cli_version: "1.0.0", + emit_failed_flag: Default::default(), }; let to_string = |bytes: Vec| -> String { String::from_utf8(bytes).unwrap() }; @@ -192,6 +198,7 @@ mod test { let cache = EmitCache { disk_cache: disk_cache.clone(), cli_version: "2.0.0", + emit_failed_flag: Default::default(), }; assert_eq!(cache.get_emit_code(&specifier1, 10), None); cache.set_emit_code(&specifier1, 5, emit_code1.as_bytes()); @@ -200,6 +207,7 @@ mod test { let cache = EmitCache { disk_cache, cli_version: "2.0.0", + emit_failed_flag: Default::default(), }; assert_eq!( cache.get_emit_code(&specifier1, 5).map(to_string), diff --git a/cli/cache/mod.rs b/cli/cache/mod.rs index 3430f74f70..772d2359d3 100644 --- a/cli/cache/mod.rs +++ b/cli/cache/mod.rs @@ -106,7 +106,7 @@ pub use deno_cache_dir::HttpCache; /// 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 { - emit_cache: EmitCache, + emit_cache: Arc, file_fetcher: Arc, file_header_overrides: HashMap>, global_http_cache: Arc, @@ -118,7 +118,7 @@ pub struct FetchCacher { impl FetchCacher { pub fn new( - emit_cache: EmitCache, + emit_cache: Arc, file_fetcher: Arc, file_header_overrides: HashMap>, global_http_cache: Arc, diff --git a/cli/emit.rs b/cli/emit.rs index 98ee59fab9..61397d1ab7 100644 --- a/cli/emit.rs +++ b/cli/emit.rs @@ -18,7 +18,7 @@ use deno_graph::ModuleGraph; use std::sync::Arc; pub struct Emitter { - emit_cache: EmitCache, + emit_cache: Arc, parsed_source_cache: Arc, transpile_and_emit_options: Arc<(deno_ast::TranspileOptions, deno_ast::EmitOptions)>, @@ -28,7 +28,7 @@ pub struct Emitter { impl Emitter { pub fn new( - emit_cache: EmitCache, + emit_cache: Arc, parsed_source_cache: Arc, transpile_options: deno_ast::TranspileOptions, emit_options: deno_ast::EmitOptions, diff --git a/cli/factory.rs b/cli/factory.rs index 90dc6dd2ee..ed288b22f7 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -164,7 +164,7 @@ struct CliFactoryServices { global_http_cache: Deferred>, http_cache: Deferred>, http_client_provider: Deferred>, - emit_cache: Deferred, + emit_cache: Deferred>, emitter: Deferred>, fs: Deferred>, main_graph_container: Deferred>, @@ -492,9 +492,9 @@ impl CliFactory { .get_or_init(|| maybe_file_watcher_reporter) } - pub fn emit_cache(&self) -> Result<&EmitCache, AnyError> { + pub fn emit_cache(&self) -> Result<&Arc, AnyError> { self.services.emit_cache.get_or_try_init(|| { - Ok(EmitCache::new(self.deno_dir()?.gen_cache.clone())) + Ok(Arc::new(EmitCache::new(self.deno_dir()?.gen_cache.clone()))) }) } diff --git a/cli/graph_util.rs b/cli/graph_util.rs index 77d086fa81..647307bd91 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -364,7 +364,7 @@ pub struct ModuleGraphBuilder { parsed_source_cache: Arc, lockfile: Option>, maybe_file_watcher_reporter: Option, - emit_cache: cache::EmitCache, + emit_cache: Arc, file_fetcher: Arc, global_http_cache: Arc, } @@ -381,7 +381,7 @@ impl ModuleGraphBuilder { parsed_source_cache: Arc, lockfile: Option>, maybe_file_watcher_reporter: Option, - emit_cache: cache::EmitCache, + emit_cache: Arc, file_fetcher: Arc, global_http_cache: Arc, ) -> Self { diff --git a/tests/integration/run_tests.rs b/tests/integration/run_tests.rs index 9252de2e7e..44199c631f 100644 --- a/tests/integration/run_tests.rs +++ b/tests/integration/run_tests.rs @@ -5342,3 +5342,20 @@ async fn listen_tls_alpn_fail() { let status = child.wait().unwrap(); assert!(status.success()); } + +// Couldn't get the directory readonly on windows on the CI +// so gave up because this being tested on unix is good enough +#[cfg(unix)] +#[test] +fn emit_failed_readonly_file_system() { + let context = TestContextBuilder::default().use_temp_cwd().build(); + context.deno_dir().path().canonicalize().make_dir_readonly(); + let temp_dir = context.temp_dir().path().canonicalize(); + temp_dir.join("main.ts").write("import './other.ts';"); + temp_dir.join("other.ts").write("console.log('hi');"); + let output = context + .new_command() + .args("run --log-level=debug main.ts") + .run(); + output.assert_matches_text("[WILDCARD]Error saving emit data ([WILDLINE]main.ts)[WILDCARD]Skipped emit cache save of [WILDLINE]other.ts[WILDCARD]hi[WILDCARD]"); +}