Skip to content

Commit

Permalink
[Turbopack] fix a bunch of bugs and inefficiencies in the call graph (#…
Browse files Browse the repository at this point in the history 
…70243)

### What?

* switch remove vs change order
* take_collectibles does not need to return an Option
* avoid double Option
* fix edges set bugs
* remove collectibles from outdated collectibles
* remove unneccessary remove
  • Loading branch information
@sokra
sokra authored Sep 25, 2024
1 parent 5fd1d53 commit d5b252a
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 43 deletions.
14 changes: 7 additions & 7 deletions turbopack/crates/turbo-tasks-memory/src/edges_set.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ impl EdgesDataEntry {
| EdgesDataEntry::OutputAndCell0(type_id)
| EdgesDataEntry::ChildAndCell0(type_id)
| EdgesDataEntry::ChildOutputAndCell0(type_id),
) => *type_id == cell_id.type_id,
) => cell_id.index == 0 && *type_id == cell_id.type_id,
(entry, EdgesDataEntry::Complex(set)) => set.contains(&entry),
_ => false,
}
Expand Down Expand Up @@ -330,26 +330,26 @@ impl EdgesDataEntry {
}
_ => {}
},
EdgeEntry::Cell(_) => match self {
EdgesDataEntry::Cell0(_) => {
EdgeEntry::Cell(cell_id) if cell_id.index == 0 => match self {
EdgesDataEntry::Cell0(value_ty) if cell_id.type_id == *value_ty => {
*self = EdgesDataEntry::Empty;
return true;
}
EdgesDataEntry::OutputAndCell0(_) => {
EdgesDataEntry::OutputAndCell0(value_ty) if cell_id.type_id == *value_ty => {
*self = EdgesDataEntry::Output;
return true;
}
EdgesDataEntry::ChildAndCell0(_) => {
EdgesDataEntry::ChildAndCell0(value_ty) if cell_id.type_id == *value_ty => {
*self = EdgesDataEntry::Child;
return true;
}
EdgesDataEntry::ChildOutputAndCell0(_) => {
EdgesDataEntry::ChildOutputAndCell0(value_ty) if cell_id.type_id == *value_ty => {
*self = EdgesDataEntry::ChildAndOutput;
return true;
}
_ => {}
},
EdgeEntry::Collectibles(_) => {}
EdgeEntry::Cell(_) | EdgeEntry::Collectibles(_) => {}
}
if let EdgesDataEntry::Complex(set) = self {
if set.remove(&entry) {
Expand Down
97 changes: 62 additions & 35 deletions turbopack/crates/turbo-tasks-memory/src/task.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -268,8 +268,11 @@ struct MaybeCollectibles {

impl MaybeCollectibles {
/// Consumes the collectibles (if any) and return them.
fn take_collectibles(&mut self) -> Option<Collectibles> {
self.inner.as_mut().map(|boxed| take(&mut **boxed))
fn take_collectibles(&mut self) -> Collectibles {
self.inner
.as_mut()
.map(|boxed| take(&mut **boxed))
.unwrap_or_default()
}

/// Consumes the collectibles (if any) and return them.
Expand Down Expand Up @@ -305,6 +308,23 @@ impl MaybeCollectibles {
.or_default();
*value -= count as i32;
}

/// Removes an collectible if the count is positive.
fn remove_emit(&mut self, trait_type: TraitTypeId, value: RawVc) -> bool {
let Some(inner) = self.inner.as_mut() else {
return false;
};

let auto_hash_map::map::Entry::Occupied(mut e) = inner.entry((trait_type, value)) else {
return false;
};
let value = e.get_mut();
*value -= 1;
if *value == 0 {
e.remove();
}
true
}
}

struct InProgressState {
Expand Down Expand Up @@ -828,32 +848,32 @@ impl Task {
let outdated_children = outdated_edges.drain_children();
let outdated_collectibles = outdated_collectibles.take_collectibles();

let remove_job = if outdated_children.is_empty() {
None
} else {
state.aggregation_node.handle_lost_edges(
&aggregation_context,
&self.id,
outdated_children,
)
};

let mut change = TaskChange {
unfinished: -1,
#[cfg(feature = "track_unfinished")]
unfinished_tasks_update: vec![(self.id, -1)],
..Default::default()
};
if let Some(collectibles) = outdated_collectibles {
for ((trait_type, value), count) in collectibles.into_iter() {
change.collectibles.push((trait_type, value, -count));
}
for ((trait_type, value), count) in outdated_collectibles.into_iter() {
change.collectibles.push((trait_type, value, -count));
}
let change_job = state
.aggregation_node
.apply_change(&aggregation_context, change);
let remove_job = if outdated_children.is_empty() {
None
} else {
Some(state.aggregation_node.handle_lost_edges(
&aggregation_context,
&self.id,
outdated_children,
))
};

drop(state);
change_job.apply(&aggregation_context);
remove_job.apply(&aggregation_context);
change_job.apply(&aggregation_context);
}
aggregation_context.apply_queued_updates();
}
Expand Down Expand Up @@ -984,9 +1004,9 @@ impl Task {
for child in new_children {
outdated_edges.insert(TaskEdge::Child(child));
}
if let Some(collectibles) = outdated_collectibles {
if !outdated_collectibles.is_empty() {
let mut change = TaskChange::default();
for ((trait_type, value), count) in collectibles.into_iter() {
for ((trait_type, value), count) in outdated_collectibles.into_iter() {
change.collectibles.push((trait_type, value, -count));
}
change_job = state
Expand All @@ -1008,7 +1028,6 @@ impl Task {
outdated_edges.remove_all(&new_edges);
for child in new_children {
new_edges.insert(TaskEdge::Child(child));
outdated_edges.remove(TaskEdge::Child(child));
}
if !backend.has_gc() {
// This will stay here for longer, so make sure to not consume too
Expand All @@ -1022,38 +1041,37 @@ impl Task {
stateful,
edges: new_edges.into_list(),
};
let outdated_children = outdated_edges.drain_children();
if !outdated_children.is_empty() {
remove_job = state.aggregation_node.handle_lost_edges(
&aggregation_context,
&self.id,
outdated_children,
);
}
if !count_as_finished {
let mut change = TaskChange {
unfinished: -1,
#[cfg(feature = "track_unfinished")]
unfinished_tasks_update: vec![(self.id, -1)],
..Default::default()
};
if let Some(collectibles) = outdated_collectibles {
for ((trait_type, value), count) in collectibles.into_iter() {
change.collectibles.push((trait_type, value, -count));
}
for ((trait_type, value), count) in outdated_collectibles.into_iter() {
change.collectibles.push((trait_type, value, -count));
}
change_job = state
.aggregation_node
.apply_change(&aggregation_context, change);
} else if let Some(collectibles) = outdated_collectibles {
} else if !outdated_collectibles.is_empty() {
let mut change = TaskChange::default();
for ((trait_type, value), count) in collectibles.into_iter() {
for ((trait_type, value), count) in outdated_collectibles.into_iter() {
change.collectibles.push((trait_type, value, -count));
}
change_job = state
.aggregation_node
.apply_change(&aggregation_context, change);
}
let outdated_children = outdated_edges.drain_children();
if !outdated_children.is_empty() {
remove_job = state.aggregation_node.handle_lost_edges(
&aggregation_context,
&self.id,
outdated_children,
);
}

done_event.notify(usize::MAX);
drop(state);
self.clear_dependencies(outdated_edges, backend, turbo_tasks);
Expand All @@ -1062,8 +1080,8 @@ impl Task {
for cell in drained_cells {
cell.gc_drop(turbo_tasks);
}
change_job.apply(&aggregation_context);
remove_job.apply(&aggregation_context);
change_job.apply(&aggregation_context);
}
if let TaskType::Once(_) = self.ty {
// unset the root type, so tasks below are no longer active
Expand Down Expand Up @@ -1680,9 +1698,18 @@ impl Task {
backend: &MemoryBackend,
turbo_tasks: &dyn TurboTasksBackendApi<MemoryBackend>,
) {
let mut aggregation_context = TaskAggregationContext::new(turbo_tasks, backend);
let mut state = self.full_state_mut();
state.collectibles.emit(trait_type, collectible);
if let TaskStateType::InProgress(box InProgressState {
outdated_collectibles,
..
}) = &mut state.state_type
{
if outdated_collectibles.remove_emit(trait_type, collectible) {
return;
}
}
let mut aggregation_context = TaskAggregationContext::new(turbo_tasks, backend);
let change_job = state.aggregation_node.apply_change(
&aggregation_context,
TaskChange {
Expand Down
2 changes: 1 addition & 1 deletion turbopack/crates/turbo-tasks-memory/src/task/aggregation.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,7 @@ impl<'l> AggregationNodeGuard for TaskGuard<'l> {
for (&(trait_type_id, collectible), count) in collectibles.iter() {
change
.collectibles
.push((trait_type_id, collectible, -count));
.push((trait_type_id, collectible, -*count));
}
}
if let TaskStateType::InProgress(box InProgressState {
Expand Down

0 comments on commit d5b252a

Please sign in to comment.