Skip to content

perf(turbopack): Detect more immutable tasks #80423

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.

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

Merged
merged 5 commits into from
Jun 17, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 9 additions & 7 deletions turbopack/crates/turbo-tasks-backend/src/backend/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1622,7 +1622,7 @@ impl<B: BackingStorage> TurboTasksBackendInner<B> {
// new_children list now.
AggregationUpdateQueue::run(
AggregationUpdateJob::DecreaseActiveCounts {
task_ids: new_children.into_iter().collect(),
task_ids: new_children.into_keys().collect(),
},
&mut ctx,
);
Expand Down Expand Up @@ -1670,10 +1670,12 @@ impl<B: BackingStorage> TurboTasksBackendInner<B> {
let mut old_edges = Vec::new();

let has_children = !new_children.is_empty();
let has_mutable_children =
has_children && new_children.values().any(|is_immutable| !*is_immutable);

// If the task is not stateful and has no children, it does not have a way to be invalidated
// and we can mark it as immutable.
if !stateful && !has_children {
// If the task is not stateful and has no mutable children, it does not have a way to be
// invalidated and we can mark it as immutable.
if !stateful && !has_mutable_children {
task.mark_as_immutable();
}

Expand All @@ -1686,7 +1688,7 @@ impl<B: BackingStorage> TurboTasksBackendInner<B> {
if has_children {
old_edges.extend(
iter_many!(task, Child { task } => task)
.filter(|task| !new_children.remove(task))
.filter(|task| new_children.remove(task).is_none())
.map(OutdatedEdge::Child),
);
} else {
Expand Down Expand Up @@ -1717,7 +1719,7 @@ impl<B: BackingStorage> TurboTasksBackendInner<B> {
}),
);
}
if self.should_track_dependencies() {
if !task.is_immutable() && self.should_track_dependencies() {
old_edges.extend(iter_many!(task, OutdatedCellDependency { target } => OutdatedEdge::CellDependency(target)));
old_edges.extend(iter_many!(task, OutdatedOutputDependency { target } => OutdatedEdge::OutputDependency(target)));
old_edges.extend(
Expand Down Expand Up @@ -1783,7 +1785,7 @@ impl<B: BackingStorage> TurboTasksBackendInner<B> {
// that. (We already filtered out the old children from that list)
AggregationUpdateQueue::run(
AggregationUpdateJob::DecreaseActiveCounts {
task_ids: new_children.into_iter().collect(),
task_ids: new_children.into_keys().collect(),
},
&mut ctx,
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,10 @@ impl ConnectChildOperation {
};

// Quick skip if the child was already connected before
if !new_children.insert(child_task_id) {
if new_children.insert(child_task_id, is_immutable).is_some() {
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic here needs to be adjusted after changing from FxHashSet to FxHashMap. With HashMap.insert(), the return value is Some(old_value) when a key already exists (indicating replacement), not false as with HashSet.insert().

To maintain the original behavior of skipping when a child is already connected, the condition should be:

if new_children.contains_key(&child_task_id) {
    return;
}
new_children.insert(child_task_id, is_immutable);

Or alternatively:

if let Some(_) = new_children.insert(child_task_id, is_immutable) {
    return;
}

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made it use only a static flag to know the perf difference of using a dynamic flag using codspeed.

See #80506

return;
}

if parent_task.has_key(&CachedDataItemKey::Child {
task: child_task_id,
}) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use rustc_hash::FxHashSet;
use rustc_hash::FxHashMap;
use smallvec::SmallVec;
use turbo_tasks::TaskId;

Expand All @@ -14,7 +14,7 @@ use crate::{
pub fn connect_children(
parent_task_id: TaskId,
parent_task: &mut impl TaskGuard,
new_children: FxHashSet<TaskId>,
new_children: FxHashMap<TaskId, bool>,
queue: &mut AggregationUpdateQueue,
has_active_count: bool,
should_track_activeness: bool,
Expand All @@ -25,14 +25,14 @@ pub fn connect_children(

let parent_aggregation = get_aggregation_number(parent_task);

for &new_child in new_children.iter() {
for &new_child in new_children.keys() {
parent_task.add_new(CachedDataItem::Child {
task: new_child,
value: (),
});
}

let new_follower_ids: SmallVec<_> = new_children.iter().copied().collect();
let new_follower_ids: SmallVec<_> = new_children.keys().copied().collect();

let aggregating_node = is_aggregating_node(parent_aggregation);
let upper_ids = (!aggregating_node).then(|| get_uppers(&*parent_task));
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::{cmp::max, num::NonZeroU32};

use rustc_hash::FxHashSet;
use rustc_hash::FxHashMap;
use turbo_tasks::TaskId;

use crate::backend::{
Expand All @@ -15,7 +15,7 @@ const AGGREGATION_NUMBER_BUFFER_SPACE: u32 = 3;
pub fn prepare_new_children(
parent_task_id: TaskId,
parent_task: &mut impl TaskGuard,
new_children: &FxHashSet<TaskId>,
new_children: &FxHashMap<TaskId, bool>,
queue: &mut AggregationUpdateQueue,
) {
if new_children.is_empty() {
Expand Down Expand Up @@ -48,7 +48,7 @@ pub fn prepare_new_children(
if !is_aggregating_node(future_parent_aggregation) {
let child_base_aggregation_number =
future_parent_aggregation + AGGREGATION_NUMBER_BUFFER_SPACE;
for &new_child in new_children.iter() {
for &new_child in new_children.keys() {
queue.push(AggregationUpdateJob::UpdateAggregationNumber {
task_id: new_child,
base_aggregation_number: child_base_aggregation_number,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ impl UpdateOutputOperation {
return;
}
let children = if ctx.should_track_children() {
new_children.iter().copied().collect()
new_children.keys().copied().collect()
} else {
Default::default()
};
Expand Down
6 changes: 4 additions & 2 deletions turbopack/crates/turbo-tasks-backend/src/data.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::cmp::Ordering;

use rustc_hash::FxHashSet;
use rustc_hash::FxHashMap;
use serde::{Deserialize, Serialize};
use turbo_tasks::{
CellId, KeyValuePair, SessionId, TaskId, TraitTypeId, TypedSharedReference, ValueTypeId,
Expand Down Expand Up @@ -315,7 +315,9 @@ pub struct InProgressStateInner {
pub done_event: Event,
/// Children that should be connected to the task and have their active_count decremented
/// once the task completes.
pub new_children: FxHashSet<TaskId>,
///
/// The bool value is `is_immutable` of the child task.
pub new_children: FxHashMap<TaskId, bool>,
}

#[derive(Debug)]
Expand Down
Loading