From 61e9b71e4c0d321cde6bc34fd98105a7631711e5 Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Wed, 24 Jul 2024 17:36:19 +0200 Subject: [PATCH] change from failsafe_analyse to failsafe_parse (vercel/turbo#8828) ### Description change from failsafe_analyse to failsafe_parse as ParseResult is flat and can be snapshot easily failsafe_analyze didn't work correctly as it stores nested Vcs in State which breaks GC and dropping of excessive cells since these Vcs point to stale cells that no longer exist ### Testing Instructions --- crates/turbopack-ecmascript/src/lib.rs | 111 ++++-------------- crates/turbopack-ecmascript/src/parse.rs | 1 + .../src/references/mod.rs | 6 +- .../side_effect_optimization/facade/module.rs | 4 +- .../side_effect_optimization/locals/module.rs | 2 +- .../src/tree_shake/asset.rs | 2 +- crates/turbopack-mdx/src/lib.rs | 6 +- 7 files changed, 36 insertions(+), 96 deletions(-) diff --git a/crates/turbopack-ecmascript/src/lib.rs b/crates/turbopack-ecmascript/src/lib.rs index d198702e7020e..b963ae5a7f903 100644 --- a/crates/turbopack-ecmascript/src/lib.rs +++ b/crates/turbopack-ecmascript/src/lib.rs @@ -74,7 +74,7 @@ use turbopack_core::{ FindContextFileResult, ModulePart, }, source::Source, - source_map::{GenerateSourceMap, OptionSourceMap, SourceMap}, + source_map::{GenerateSourceMap, OptionSourceMap}, }; // TODO remove this pub use turbopack_resolve::ecmascript as resolve; @@ -181,18 +181,6 @@ fn modifier() -> Vc { Vc::cell("ecmascript".into()) } -#[derive(PartialEq, Eq, Clone, TraceRawVcs)] -struct MemoizedSuccessfulAnalysis { - operation: Vc, - references: ReadRef, - local_references: ReadRef, - reexport_references: ReadRef, - evaluation_references: ReadRef, - exports: ReadRef, - async_module: ReadRef, - source_map: Option>, -} - #[derive(Clone)] pub struct EcmascriptModuleAssetBuilder { source: Vc>, @@ -256,7 +244,7 @@ pub struct EcmascriptModuleAsset { pub inner_assets: Option>, #[turbo_tasks(debug_ignore)] #[serde(skip)] - last_successful_analysis: turbo_tasks::State>, + last_successful_parse: turbo_tasks::State>>, } /// An optional [EcmascriptModuleAsset] @@ -335,7 +323,7 @@ impl EcmascriptModuleAsset { options, compile_time_info, inner_assets: None, - last_successful_analysis: Default::default(), + last_successful_parse: Default::default(), }) } @@ -357,7 +345,7 @@ impl EcmascriptModuleAsset { options, compile_time_info, inner_assets: Some(inner_assets), - last_successful_analysis: Default::default(), + last_successful_parse: Default::default(), }) } @@ -377,69 +365,24 @@ impl EcmascriptModuleAsset { } #[turbo_tasks::function] - pub async fn failsafe_analyze(self: Vc) -> Result> { - let this = self.await?; - - let result = self.analyze(); - let result_value = result.await?; - - let successful = result_value.successful; - let current_memo = MemoizedSuccessfulAnalysis { - operation: result, - // We need to store the ReadRefs since we want to keep a snapshot. - references: result_value.references.await?, - local_references: result_value.local_references.await?, - reexport_references: result_value.reexport_references.await?, - evaluation_references: result_value.evaluation_references.await?, - exports: result_value.exports.await?, - async_module: result_value.async_module.await?, - source_map: if let Some(map) = *result_value.source_map.await? { - Some(map.await?) - } else { - None - }, - }; - let state_ref; - let best_value = if successful { - ¤t_memo - } else { - state_ref = this.last_successful_analysis.get(); - state_ref.as_ref().unwrap_or(¤t_memo) - }; - let MemoizedSuccessfulAnalysis { - operation, - references, - local_references, - reexport_references, - evaluation_references, - exports, - async_module, - source_map, - } = best_value; - // It's important to connect to the last operation here to keep it active, so - // it's potentially recomputed when garbage collected - Vc::connect(*operation); - let result = AnalyzeEcmascriptModuleResult { - references: ReadRef::cell(references.clone()), - local_references: ReadRef::cell(local_references.clone()), - reexport_references: ReadRef::cell(reexport_references.clone()), - evaluation_references: ReadRef::cell(evaluation_references.clone()), - exports: ReadRef::cell(exports.clone()), - code_generation: result_value.code_generation, - async_module: ReadRef::cell(async_module.clone()), - source_map: Vc::cell(source_map.clone().map(ReadRef::cell)), - successful, - } - .cell(); - if successful { - this.last_successful_analysis.set(Some(current_memo)); - } - Ok(result) + pub fn parse(&self) -> Vc { + parse(self.source, Value::new(self.ty), self.transforms) } #[turbo_tasks::function] - pub fn parse(&self) -> Vc { - parse(self.source, Value::new(self.ty), self.transforms) + pub async fn failsafe_parse(self: Vc) -> Result> { + let real_result = self.parse(); + let real_result_value = real_result.await?; + let this = self.await?; + let result_value = if matches!(*real_result_value, ParseResult::Ok { .. }) { + this.last_successful_parse + .set(Some(real_result_value.clone())); + real_result_value + } else { + let state_ref = this.last_successful_parse.get(); + state_ref.as_ref().unwrap_or(&real_result_value).clone() + }; + Ok(ReadRef::cell(result_value)) } #[turbo_tasks::function] @@ -498,7 +441,7 @@ impl EcmascriptModuleAsset { ) -> Result> { let this = self.await?; - let parsed = parse(this.source, Value::new(this.ty), this.transforms); + let parsed = self.parse(); Ok(EcmascriptModuleContent::new_without_analysis( parsed, @@ -513,11 +456,7 @@ impl EcmascriptModuleAsset { chunking_context: Vc>, async_module_info: Option>, ) -> Result> { - let this = self.await?; - - let parsed = parse(this.source, Value::new(this.ty), this.transforms) - .resolve() - .await?; + let parsed = self.parse().resolve().await?; let analyze = self.analyze().await?; @@ -561,7 +500,7 @@ impl Module for EcmascriptModuleAsset { #[turbo_tasks::function] async fn references(self: Vc) -> Result> { - let analyze = self.failsafe_analyze().await?; + let analyze = self.analyze().await?; let references = analyze.references.await?.iter().copied().collect(); Ok(Vc::cell(references)) } @@ -593,12 +532,12 @@ impl ChunkableModule for EcmascriptModuleAsset { impl EcmascriptChunkPlaceable for EcmascriptModuleAsset { #[turbo_tasks::function] async fn get_exports(self: Vc) -> Result> { - Ok(self.failsafe_analyze().await?.exports) + Ok(self.analyze().await?.exports) } #[turbo_tasks::function] async fn get_async_module(self: Vc) -> Result> { - Ok(self.failsafe_analyze().await?.async_module) + Ok(self.analyze().await?.async_module) } } @@ -669,7 +608,7 @@ impl ChunkItem for ModuleChunkItem { #[turbo_tasks::function] async fn is_self_async(&self) -> Result> { if let Some(async_module) = *self.module.get_async_module().await? { - Ok(async_module.is_self_async(self.module.failsafe_analyze().await?.references)) + Ok(async_module.is_self_async(self.module.analyze().await?.references)) } else { Ok(Vc::cell(false)) } diff --git a/crates/turbopack-ecmascript/src/parse.rs b/crates/turbopack-ecmascript/src/parse.rs index 7fae1353e5cbd..9a7580e628a66 100644 --- a/crates/turbopack-ecmascript/src/parse.rs +++ b/crates/turbopack-ecmascript/src/parse.rs @@ -45,6 +45,7 @@ use crate::{ #[turbo_tasks::value(shared, serialization = "none", eq = "manual")] #[allow(clippy::large_enum_variant)] pub enum ParseResult { + // Note: Ok must not contain any Vc as it's snapshot by failsafe_parse Ok { #[turbo_tasks(debug_ignore, trace_ignore)] program: Program, diff --git a/crates/turbopack-ecmascript/src/references/mod.rs b/crates/turbopack-ecmascript/src/references/mod.rs index e284992e9ad58..5a4eaab2a4e67 100644 --- a/crates/turbopack-ecmascript/src/references/mod.rs +++ b/crates/turbopack-ecmascript/src/references/mod.rs @@ -102,7 +102,7 @@ use super::{ WellKnownObjectKind, }, errors, - parse::{parse, ParseResult}, + parse::ParseResult, special_cases::special_cases, utils::js_value_to_pattern, webpack::{ @@ -427,11 +427,11 @@ pub(crate) async fn analyse_ecmascript_module_internal( }; let parsed = if let Some(part) = part { - let parsed = parse(source, ty, transforms); + let parsed = module.failsafe_parse(); let split_data = split(source.ident(), source, parsed); part_of_module(split_data, part) } else { - parse(source, ty, transforms) + module.failsafe_parse() }; let ModuleTypeResult { diff --git a/crates/turbopack-ecmascript/src/side_effect_optimization/facade/module.rs b/crates/turbopack-ecmascript/src/side_effect_optimization/facade/module.rs index c2092447a8127..ba03e3357eafe 100644 --- a/crates/turbopack-ecmascript/src/side_effect_optimization/facade/module.rs +++ b/crates/turbopack-ecmascript/src/side_effect_optimization/facade/module.rs @@ -79,7 +79,7 @@ impl Module for EcmascriptModuleFacadeModule { ModulePart::Evaluation" ); }; - let result = module.failsafe_analyze().await?; + let result = module.analyze().await?; let references = result.evaluation_references; let mut references = references.await?.clone_value(); references.push(Vc::upcast(EcmascriptModulePartReference::new_part( @@ -97,7 +97,7 @@ impl Module for EcmascriptModuleFacadeModule { ModulePart::Evaluation" ); }; - let result = module.failsafe_analyze().await?; + let result = module.analyze().await?; let references = result.reexport_references; let mut references = references.await?.clone_value(); references.push(Vc::upcast(EcmascriptModulePartReference::new_part( diff --git a/crates/turbopack-ecmascript/src/side_effect_optimization/locals/module.rs b/crates/turbopack-ecmascript/src/side_effect_optimization/locals/module.rs index 39b98678c3f40..0cc104ef0b999 100644 --- a/crates/turbopack-ecmascript/src/side_effect_optimization/locals/module.rs +++ b/crates/turbopack-ecmascript/src/side_effect_optimization/locals/module.rs @@ -49,7 +49,7 @@ impl Module for EcmascriptModuleLocalsModule { #[turbo_tasks::function] async fn references(&self) -> Result> { - let result = self.module.failsafe_analyze().await?; + let result = self.module.analyze().await?; Ok(result.local_references) } } diff --git a/crates/turbopack-ecmascript/src/tree_shake/asset.rs b/crates/turbopack-ecmascript/src/tree_shake/asset.rs index c7b4b7c2d7168..63a43ea340ee7 100644 --- a/crates/turbopack-ecmascript/src/tree_shake/asset.rs +++ b/crates/turbopack-ecmascript/src/tree_shake/asset.rs @@ -50,7 +50,7 @@ impl EcmascriptModulePartAsset { #[turbo_tasks::function] pub async fn is_async_module(self: Vc) -> Result> { let this = self.await?; - let result = this.full_module.failsafe_analyze(); + let result = this.full_module.analyze(); if let Some(async_module) = *result.await?.async_module.await? { Ok(async_module.is_self_async(self.references())) diff --git a/crates/turbopack-mdx/src/lib.rs b/crates/turbopack-mdx/src/lib.rs index a02b29837926c..9468495d4a9bc 100644 --- a/crates/turbopack-mdx/src/lib.rs +++ b/crates/turbopack-mdx/src/lib.rs @@ -191,11 +191,11 @@ impl MdxModuleAsset { } #[turbo_tasks::function] - async fn failsafe_analyze(self: Vc) -> Result> { + async fn analyze(self: Vc) -> Result> { let asset = into_ecmascript_module_asset(&self).await; if let Ok(asset) = asset { - Ok(asset.failsafe_analyze()) + Ok(asset.analyze()) } else { let mut result = AnalyzeEcmascriptModuleResultBuilder::new(); result.set_successful(false); @@ -216,7 +216,7 @@ impl Module for MdxModuleAsset { #[turbo_tasks::function] async fn references(self: Vc) -> Result> { - let analyze = self.failsafe_analyze().await?; + let analyze = self.analyze().await?; Ok(analyze.references) } }