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

[Turbopack] fix incorrect task access for Persistent Caching #73549

Merged
merged 3 commits into from
Dec 5, 2024
Merged

[Turbopack] fix incorrect task access for Persistent Caching #73549

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b9e146a
fix incorrect task access
sokra Dec 5, 2024
bad85da
change to debug_assertions
sokra Dec 5, 2024
d142544
add conditional compilation
sokra Dec 5, 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
2 changes: 1 addition & 1 deletion turbopack/crates/turbo-tasks-backend/src/backend/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1592,7 +1592,7 @@ impl<B: BackingStorage> TurboTasksBackendInner<B> {
let mut ctx = self.execute_context(turbo_tasks);
let mut collectibles = AutoMap::default();
{
let mut task = ctx.task(task_id, TaskDataCategory::Data);
let mut task = ctx.task(task_id, TaskDataCategory::All);
// Ensure it's an root node
loop {
let aggregation_number = get_aggregation_number(&task);
Expand Down
2 changes: 1 addition & 1 deletion turbopack/crates/turbo-tasks-backend/src/backend/operation/connect_child.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ impl ConnectChildOperation {
drop(parent_task);

{
let mut task = ctx.task(child_task_id, TaskDataCategory::Data);
let mut task = ctx.task(child_task_id, TaskDataCategory::Meta);
if !task.has_key(&CachedDataItemKey::Output {}) {
let description = ctx.get_task_desc_fn(child_task_id);
let should_schedule = task.add(CachedDataItem::new_scheduled(description));
Expand Down
47 changes: 47 additions & 0 deletions turbopack/crates/turbo-tasks-backend/src/backend/operation/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,8 @@ where
task,
task_id,
backend: self.backend,
#[cfg(debug_assertions)]
category,
}
}

Expand Down Expand Up @@ -258,11 +260,15 @@ where
task: task1,
task_id: task_id1,
backend: self.backend,
#[cfg(debug_assertions)]
category,
},
TaskGuardImpl {
task: task2,
task_id: task_id2,
backend: self.backend,
#[cfg(debug_assertions)]
category,
},
)
}
Expand Down Expand Up @@ -385,6 +391,37 @@ struct TaskGuardImpl<'a, B: BackingStorage> {
task_id: TaskId,
task: StorageWriteGuard<'a, TaskId, CachedDataItem>,
backend: &'a TurboTasksBackendInner<B>,
#[cfg(debug_assertions)]
category: TaskDataCategory,
}

impl<B: BackingStorage> TaskGuardImpl<'_, B> {
/// Verify that the task guard restored the correct category
/// before accessing the data.
#[inline]
fn check_access(&self, category: TaskDataCategory) {
Copy link
Contributor

Choose a reason for hiding this comment

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

unused variable: `category`

{
match category {
TaskDataCategory::All => {
// This category is used for non-persisted data
}
TaskDataCategory::Data => {
#[cfg(debug_assertions)]
debug_assert!(
self.category == TaskDataCategory::Data
|| self.category == TaskDataCategory::All
);
}
TaskDataCategory::Meta => {
#[cfg(debug_assertions)]
debug_assert!(
self.category == TaskDataCategory::Meta
|| self.category == TaskDataCategory::All
);
}
}
}
}
}

impl<B: BackingStorage> Debug for TaskGuardImpl<'_, B> {
Expand All @@ -408,6 +445,7 @@ impl<B: BackingStorage> TaskGuard for TaskGuardImpl<'_, B> {

#[must_use]
fn add(&mut self, item: CachedDataItem) -> bool {
self.check_access(item.category());
if !self.backend.should_persist() || self.task_id.is_transient() || !item.is_persistent() {
self.task.add(item)
} else if self.task.add(item.clone()) {
Expand All @@ -424,11 +462,13 @@ impl<B: BackingStorage> TaskGuard for TaskGuardImpl<'_, B> {
}

fn add_new(&mut self, item: CachedDataItem) {
self.check_access(item.category());
let added = self.add(item);
assert!(added, "Item already exists");
}

fn insert(&mut self, item: CachedDataItem) -> Option<CachedDataItemValue> {
self.check_access(item.category());
let (key, value) = item.into_key_and_value();
if !self.backend.should_persist() || self.task_id.is_transient() || !key.is_persistent() {
self.task
Expand Down Expand Up @@ -472,6 +512,7 @@ impl<B: BackingStorage> TaskGuard for TaskGuardImpl<'_, B> {
key: &CachedDataItemKey,
update: impl FnOnce(Option<CachedDataItemValue>) -> Option<CachedDataItemValue>,
) {
self.check_access(key.category());
if !self.backend.should_persist() || self.task_id.is_transient() || !key.is_persistent() {
self.task.update(key, update);
return;
Expand All @@ -480,6 +521,8 @@ impl<B: BackingStorage> TaskGuard for TaskGuardImpl<'_, B> {
task,
task_id,
backend,
#[cfg(debug_assertions)]
category: _,
} = self;
let mut add_persisting_item = false;
task.update(key, |old| {
Expand Down Expand Up @@ -519,6 +562,7 @@ impl<B: BackingStorage> TaskGuard for TaskGuardImpl<'_, B> {
}

fn remove(&mut self, key: &CachedDataItemKey) -> Option<CachedDataItemValue> {
self.check_access(key.category());
let old_value = self.task.remove(key);
if let Some(value) = old_value {
if self.backend.should_persist()
Expand All @@ -545,14 +589,17 @@ impl<B: BackingStorage> TaskGuard for TaskGuardImpl<'_, B> {
}

fn get(&self, key: &CachedDataItemKey) -> Option<&CachedDataItemValue> {
self.check_access(key.category());
self.task.get(key)
}

fn get_mut(&mut self, key: &CachedDataItemKey) -> Option<&mut CachedDataItemValue> {
self.check_access(key.category());
self.task.get_mut(key)
}

fn has_key(&self, key: &CachedDataItemKey) -> bool {
self.check_access(key.category());
self.task.has_key(key)
}

Expand Down
4 changes: 2 additions & 2 deletions turbopack/crates/turbo-tasks-backend/src/backend/operation/update_output.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ impl UpdateOutputOperation {
output: Result<Result<RawVc>, Option<Cow<'static, str>>>,
mut ctx: impl ExecuteContext,
) {
let mut task = ctx.task(task_id, TaskDataCategory::Data);
let mut task = ctx.task(task_id, TaskDataCategory::Meta);
if let Some(InProgressState::InProgress { stale: true, .. }) = get!(task, InProgress) {
// Skip updating the output when the task is stale
return;
Expand Down Expand Up @@ -173,7 +173,7 @@ impl Operation for UpdateOutputOperation {
ref mut queue,
} => {
if let Some(child_id) = children.pop() {
let mut child_task = ctx.task(child_id, TaskDataCategory::Data);
let mut child_task = ctx.task(child_id, TaskDataCategory::Meta);
if !child_task.has_key(&CachedDataItemKey::Output {}) {
make_task_dirty_internal(
&mut child_task,
Expand Down
97 changes: 67 additions & 30 deletions turbopack/crates/turbo-tasks-backend/src/data.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,42 @@ impl CachedDataItem {
listener,
)
}

pub fn category(&self) -> TaskDataCategory {
match self {
Self::Collectible { .. }
| Self::Child { .. }
| Self::ChildrenCount { .. }
| Self::CellData { .. }
| Self::CellTypeMaxIndex { .. }
| Self::OutputDependency { .. }
| Self::CellDependency { .. }
| Self::CollectiblesDependency { .. }
| Self::OutputDependent { .. }
| Self::CellDependent { .. }
| Self::CollectiblesDependent { .. } => TaskDataCategory::Data,

Self::Output { .. }
| Self::AggregationNumber { .. }
| Self::Dirty { .. }
| Self::Follower { .. }
| Self::Upper { .. }
| Self::PersistentUpperCount { .. }
| Self::AggregatedDirtyContainer { .. }
| Self::AggregatedCollectible { .. }
| Self::AggregatedDirtyContainerCount { .. } => TaskDataCategory::Meta,

Self::OutdatedCollectible { .. }
| Self::OutdatedOutputDependency { .. }
| Self::OutdatedCellDependency { .. }
| Self::OutdatedCollectiblesDependency { .. }
| Self::OutdatedChild { .. }
| Self::InProgressCell { .. }
| Self::InProgress { .. }
| Self::Error { .. }
| Self::AggregateRoot { .. } => TaskDataCategory::All,
}
}
}

impl CachedDataItemKey {
Expand Down Expand Up @@ -543,36 +579,37 @@ impl CachedDataItemKey {

pub fn category(&self) -> TaskDataCategory {
match self {
CachedDataItemKey::Collectible { .. }
| CachedDataItemKey::Child { .. }
| CachedDataItemKey::ChildrenCount { .. }
| CachedDataItemKey::CellData { .. }
| CachedDataItemKey::CellTypeMaxIndex { .. }
| CachedDataItemKey::OutputDependency { .. }
| CachedDataItemKey::CellDependency { .. }
| CachedDataItemKey::CollectiblesDependency { .. }
| CachedDataItemKey::OutputDependent { .. }
| CachedDataItemKey::CellDependent { .. }
| CachedDataItemKey::CollectiblesDependent { .. }
| CachedDataItemKey::InProgress { .. }
| CachedDataItemKey::InProgressCell { .. }
| CachedDataItemKey::OutdatedCollectible { .. }
| CachedDataItemKey::OutdatedOutputDependency { .. }
| CachedDataItemKey::OutdatedCellDependency { .. }
| CachedDataItemKey::OutdatedCollectiblesDependency { .. }
| CachedDataItemKey::OutdatedChild { .. }
| CachedDataItemKey::Error { .. } => TaskDataCategory::Data,

CachedDataItemKey::Output { .. }
| CachedDataItemKey::AggregationNumber { .. }
| CachedDataItemKey::Dirty { .. }
| CachedDataItemKey::Follower { .. }
| CachedDataItemKey::Upper { .. }
| CachedDataItemKey::PersistentUpperCount { .. }
| CachedDataItemKey::AggregatedDirtyContainer { .. }
| CachedDataItemKey::AggregatedCollectible { .. }
| CachedDataItemKey::AggregatedDirtyContainerCount { .. }
| CachedDataItemKey::AggregateRoot { .. } => TaskDataCategory::Meta,
Self::Collectible { .. }
| Self::Child { .. }
| Self::ChildrenCount { .. }
| Self::CellData { .. }
| Self::CellTypeMaxIndex { .. }
| Self::OutputDependency { .. }
| Self::CellDependency { .. }
| Self::CollectiblesDependency { .. }
| Self::OutputDependent { .. }
| Self::CellDependent { .. }
| Self::CollectiblesDependent { .. } => TaskDataCategory::Data,

Self::Output { .. }
| Self::AggregationNumber { .. }
| Self::Dirty { .. }
| Self::Follower { .. }
| Self::Upper { .. }
| Self::PersistentUpperCount { .. }
| Self::AggregatedDirtyContainer { .. }
| Self::AggregatedCollectible { .. }
| Self::AggregatedDirtyContainerCount { .. } => TaskDataCategory::Meta,

Self::OutdatedCollectible { .. }
| Self::OutdatedOutputDependency { .. }
| Self::OutdatedCellDependency { .. }
| Self::OutdatedCollectiblesDependency { .. }
| Self::OutdatedChild { .. }
| Self::InProgressCell { .. }
| Self::InProgress { .. }
| Self::Error { .. }
| Self::AggregateRoot { .. } => TaskDataCategory::All,
}
}
}
Expand Down
Loading