Skip to content

Commit

Permalink
change from failsafe_analyse to failsafe_parse (#8828)
Browse files Browse the repository at this point in the history
### 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

<!--
  Give a quick description of steps to test your changes.
-->
  • Loading branch information
sokra authored Jul 24, 2024
1 parent 00039b9 commit 962fdb0
Show file tree
Hide file tree
Showing 7 changed files with 36 additions and 96 deletions.
111 changes: 25 additions & 86 deletions crates/turbopack-ecmascript/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -181,18 +181,6 @@ fn modifier() -> Vc<RcStr> {
Vc::cell("ecmascript".into())
}

#[derive(PartialEq, Eq, Clone, TraceRawVcs)]
struct MemoizedSuccessfulAnalysis {
operation: Vc<AnalyzeEcmascriptModuleResult>,
references: ReadRef<ModuleReferences>,
local_references: ReadRef<ModuleReferences>,
reexport_references: ReadRef<ModuleReferences>,
evaluation_references: ReadRef<ModuleReferences>,
exports: ReadRef<EcmascriptExports>,
async_module: ReadRef<OptionAsyncModule>,
source_map: Option<ReadRef<SourceMap>>,
}

#[derive(Clone)]
pub struct EcmascriptModuleAssetBuilder {
source: Vc<Box<dyn Source>>,
Expand Down Expand Up @@ -256,7 +244,7 @@ pub struct EcmascriptModuleAsset {
pub inner_assets: Option<Vc<InnerAssets>>,
#[turbo_tasks(debug_ignore)]
#[serde(skip)]
last_successful_analysis: turbo_tasks::State<Option<MemoizedSuccessfulAnalysis>>,
last_successful_parse: turbo_tasks::State<Option<ReadRef<ParseResult>>>,
}

/// An optional [EcmascriptModuleAsset]
Expand Down Expand Up @@ -335,7 +323,7 @@ impl EcmascriptModuleAsset {
options,
compile_time_info,
inner_assets: None,
last_successful_analysis: Default::default(),
last_successful_parse: Default::default(),
})
}

Expand All @@ -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(),
})
}

Expand All @@ -377,69 +365,24 @@ impl EcmascriptModuleAsset {
}

#[turbo_tasks::function]
pub async fn failsafe_analyze(self: Vc<Self>) -> Result<Vc<AnalyzeEcmascriptModuleResult>> {
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 {
&current_memo
} else {
state_ref = this.last_successful_analysis.get();
state_ref.as_ref().unwrap_or(&current_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<ParseResult> {
parse(self.source, Value::new(self.ty), self.transforms)
}

#[turbo_tasks::function]
pub fn parse(&self) -> Vc<ParseResult> {
parse(self.source, Value::new(self.ty), self.transforms)
pub async fn failsafe_parse(self: Vc<Self>) -> Result<Vc<ParseResult>> {
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]
Expand Down Expand Up @@ -498,7 +441,7 @@ impl EcmascriptModuleAsset {
) -> Result<Vc<EcmascriptModuleContent>> {
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,
Expand All @@ -513,11 +456,7 @@ impl EcmascriptModuleAsset {
chunking_context: Vc<Box<dyn ChunkingContext>>,
async_module_info: Option<Vc<AsyncModuleInfo>>,
) -> Result<Vc<EcmascriptModuleContent>> {
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?;

Expand Down Expand Up @@ -561,7 +500,7 @@ impl Module for EcmascriptModuleAsset {

#[turbo_tasks::function]
async fn references(self: Vc<Self>) -> Result<Vc<ModuleReferences>> {
let analyze = self.failsafe_analyze().await?;
let analyze = self.analyze().await?;
let references = analyze.references.await?.iter().copied().collect();
Ok(Vc::cell(references))
}
Expand Down Expand Up @@ -593,12 +532,12 @@ impl ChunkableModule for EcmascriptModuleAsset {
impl EcmascriptChunkPlaceable for EcmascriptModuleAsset {
#[turbo_tasks::function]
async fn get_exports(self: Vc<Self>) -> Result<Vc<EcmascriptExports>> {
Ok(self.failsafe_analyze().await?.exports)
Ok(self.analyze().await?.exports)
}

#[turbo_tasks::function]
async fn get_async_module(self: Vc<Self>) -> Result<Vc<OptionAsyncModule>> {
Ok(self.failsafe_analyze().await?.async_module)
Ok(self.analyze().await?.async_module)
}
}

Expand Down Expand Up @@ -669,7 +608,7 @@ impl ChunkItem for ModuleChunkItem {
#[turbo_tasks::function]
async fn is_self_async(&self) -> Result<Vc<bool>> {
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))
}
Expand Down
1 change: 1 addition & 0 deletions crates/turbopack-ecmascript/src/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
6 changes: 3 additions & 3 deletions crates/turbopack-ecmascript/src/references/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ use super::{
WellKnownObjectKind,
},
errors,
parse::{parse, ParseResult},
parse::ParseResult,
special_cases::special_cases,
utils::js_value_to_pattern,
webpack::{
Expand Down Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ impl Module for EcmascriptModuleLocalsModule {

#[turbo_tasks::function]
async fn references(&self) -> Result<Vc<ModuleReferences>> {
let result = self.module.failsafe_analyze().await?;
let result = self.module.analyze().await?;
Ok(result.local_references)
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/turbopack-ecmascript/src/tree_shake/asset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ impl EcmascriptModulePartAsset {
#[turbo_tasks::function]
pub async fn is_async_module(self: Vc<Self>) -> Result<Vc<bool>> {
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()))
Expand Down
6 changes: 3 additions & 3 deletions crates/turbopack-mdx/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,11 +191,11 @@ impl MdxModuleAsset {
}

#[turbo_tasks::function]
async fn failsafe_analyze(self: Vc<Self>) -> Result<Vc<AnalyzeEcmascriptModuleResult>> {
async fn analyze(self: Vc<Self>) -> Result<Vc<AnalyzeEcmascriptModuleResult>> {
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);
Expand All @@ -216,7 +216,7 @@ impl Module for MdxModuleAsset {

#[turbo_tasks::function]
async fn references(self: Vc<Self>) -> Result<Vc<ModuleReferences>> {
let analyze = self.failsafe_analyze().await?;
let analyze = self.analyze().await?;
Ok(analyze.references)
}
}
Expand Down

0 comments on commit 962fdb0

Please sign in to comment.