Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix equal checks for cells and output #8721

Merged
merged 2 commits into from
Jul 15, 2024
Merged

fix equal checks for cells and output #8721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b836432
Check for equal return value when assigning output
sokra Jul 9, 2024
f590275
avoid equal check for types that have no valid equal check
sokra Jul 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 11 additions & 13 deletions crates/turbo-tasks-memory/src/output.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,17 @@ impl Output {

pub fn link(&mut self, target: RawVc, turbo_tasks: &dyn TurboTasksBackendApi<MemoryBackend>) {
debug_assert!(*self != target);
self.assign(OutputContent::Link(target), turbo_tasks)
if let OutputContent::Link(old_target) = &self.content {
if *old_target == target {
// unchanged
return;
Comment on lines +59 to +62
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can early return when returning the same value again.

Previously this notified all dependent tasks, which is unnecessary.

}
}
self.content = OutputContent::Link(target);
// notify
if !self.dependent_tasks.is_empty() {
turbo_tasks.schedule_notify_tasks_set(&take(&mut self.dependent_tasks));
}
}

pub fn error(&mut self, error: Error, turbo_tasks: &dyn TurboTasksBackendApi<MemoryBackend>) {
Expand All @@ -79,18 +89,6 @@ impl Output {
}
}

pub fn assign(
&mut self,
content: OutputContent,
turbo_tasks: &dyn TurboTasksBackendApi<MemoryBackend>,
) {
self.content = content;
// notify
if !self.dependent_tasks.is_empty() {
turbo_tasks.schedule_notify_tasks_set(&take(&mut self.dependent_tasks));
}
}

pub fn gc_drop(self, turbo_tasks: &dyn TurboTasksBackendApi<MemoryBackend>) {
// notify
if !self.dependent_tasks.is_empty() {
Expand Down
2 changes: 1 addition & 1 deletion crates/turbopack-core/src/source_map/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ pub trait GenerateSourceMap {
/// The distinction between the source map spec's [sourcemap::Index] and our
/// [SourceMap::Sectioned] is whether the sections are represented with Vcs
/// pointers.
#[turbo_tasks::value(shared)]
#[turbo_tasks::value(shared, cell = "new")]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cell = "new" changes the behavior of .cell() to no longer perform an == check and just always override the cell.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we determine which cells should always be overridden?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you don't want cells to ever match using equality? Which I imagine should be never unless you've got another bug (e.g. inconsistent cell creation order?).

I agree this seems weird and like we're masking bigger problems.

Copy link
Member Author

@sokra sokra Jul 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you don't want cells to ever match using equality?

  • some types have very expensive equal checks and are very unlikely to be equal (e. g. the parsed AST or the SourceMap)
  • some types don't have a equality defined
  • some types define equality in a way that it never matches

But in general you want to check equality for early stop bubbling invaliations...

pub enum SourceMap {
/// A decoded source map contains no Vcs.
Decoded(#[turbo_tasks(trace_ignore)] InnerSourceMap),
Expand Down
32 changes: 10 additions & 22 deletions crates/turbopack-css/src/process.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ impl<'i, 'o> StyleSheetLike<'i, 'o> {
#[turbo_tasks::value(transparent)]
pub struct UnresolvedUrlReferences(pub Vec<(String, Vc<UrlAssetReference>)>);

#[turbo_tasks::value(shared, serialization = "none", eq = "manual")]
#[turbo_tasks::value(shared, serialization = "none", eq = "manual", cell = "new")]
pub enum ParseCssResult {
Ok {
#[turbo_tasks(debug_ignore, trace_ignore)]
Expand All @@ -296,13 +296,7 @@ pub enum ParseCssResult {
NotFound,
}

impl PartialEq for ParseCssResult {
fn eq(&self, _: &Self) -> bool {
false
}
}

#[turbo_tasks::value(shared, serialization = "none", eq = "manual")]
#[turbo_tasks::value(shared, serialization = "none", eq = "manual", cell = "new")]
pub enum CssWithPlaceholderResult {
Ok {
parse_result: Vc<ParseCssResult>,
Expand All @@ -324,12 +318,6 @@ pub enum CssWithPlaceholderResult {
NotFound,
}

impl PartialEq for CssWithPlaceholderResult {
fn eq(&self, _: &Self) -> bool {
false
}
}

#[turbo_tasks::value(shared, serialization = "none", eq = "manual")]
pub enum FinalCssResult {
Ok {
Expand Down Expand Up @@ -390,10 +378,10 @@ pub async fn process_css_with_placeholder(
url_references: *url_references,
placeholders: HashMap::new(),
}
.into())
.cell())
}
ParseCssResult::Unparseable => Ok(CssWithPlaceholderResult::Unparseable.into()),
ParseCssResult::NotFound => Ok(CssWithPlaceholderResult::NotFound.into()),
ParseCssResult::Unparseable => Ok(CssWithPlaceholderResult::Unparseable.cell()),
ParseCssResult::NotFound => Ok(CssWithPlaceholderResult::NotFound.cell()),
}
}

Expand Down Expand Up @@ -602,7 +590,7 @@ async fn process_content(
}
.cell()
.emit();
return Ok(ParseCssResult::Unparseable.into());
return Ok(ParseCssResult::Unparseable.cell());
}

_ => {
Expand All @@ -629,7 +617,7 @@ async fn process_content(
}
.cell()
.emit();
return Ok(ParseCssResult::Unparseable.into());
return Ok(ParseCssResult::Unparseable.cell());
}
}
})
Expand Down Expand Up @@ -669,12 +657,12 @@ async fn process_content(
Ok(v) => v,
Err(err) => {
err.to_diagnostics(&handler).emit();
return Ok(ParseCssResult::Unparseable.into());
return Ok(ParseCssResult::Unparseable.cell());
}
};

if handler.has_errors() {
return Ok(ParseCssResult::Unparseable.into());
return Ok(ParseCssResult::Unparseable.cell());
}

if matches!(ty, CssModuleAssetType::Module) {
Expand Down Expand Up @@ -722,7 +710,7 @@ async fn process_content(
url_references: Vc::cell(url_references),
options: config,
}
.into())
.cell())
}

/// Visitor that lints wrong css module usage.
Expand Down
Loading