From 75ab6b04205413fc1a28446f279ab7ce99b5bf45 Mon Sep 17 00:00:00 2001 From: Benjamin Woodruff Date: Fri, 24 Jan 2025 11:41:07 -0800 Subject: [PATCH] chore(turbo-tasks): Clean up consistency/untracked APIs around local outputs and cells (#75171) We only read data owned by the the currently-executing task when reading from a local Vc, so: - A separate "untracked" API doesn't make sense. There's never any tracking needed for reading local outputs or cells. (would we mark the current task as dependent on itself?) - Allowing a `consistency` argument isn't useful because you can't strongly consistently await your own task. - We don't need to call `notify_scheduled_tasks`, we're not adding any new task dependencies. --- .../crates/turbo-tasks-testing/src/lib.rs | 10 ---- turbopack/crates/turbo-tasks/src/manager.rs | 40 +++++----------- turbopack/crates/turbo-tasks/src/raw_vc.rs | 48 ++++++------------- 3 files changed, 25 insertions(+), 73 deletions(-) diff --git a/turbopack/crates/turbo-tasks-testing/src/lib.rs b/turbopack/crates/turbo-tasks-testing/src/lib.rs index 56cb55b068c2c..38368e2a5e54a 100644 --- a/turbopack/crates/turbo-tasks-testing/src/lib.rs +++ b/turbopack/crates/turbo-tasks-testing/src/lib.rs @@ -228,19 +228,9 @@ impl TurboTasksApi for VcStorage { } fn try_read_local_output( - &self, - parent_task_id: TaskId, - local_task_id: LocalTaskId, - consistency: ReadConsistency, - ) -> Result> { - self.try_read_local_output_untracked(parent_task_id, local_task_id, consistency) - } - - fn try_read_local_output_untracked( &self, _parent_task_id: TaskId, _local_task_id: LocalTaskId, - _consistency: ReadConsistency, ) -> Result> { unimplemented!() } diff --git a/turbopack/crates/turbo-tasks/src/manager.rs b/turbopack/crates/turbo-tasks/src/manager.rs index 039fd8c04aa24..d4bcfb6508435 100644 --- a/turbopack/crates/turbo-tasks/src/manager.rs +++ b/turbopack/crates/turbo-tasks/src/manager.rs @@ -137,20 +137,13 @@ pub trait TurboTasksApi: TurboTasksCallApi + Sync + Send { index: CellId, ) -> Result>; + /// This does not accept a consistency argument, as you cannot control consistency of a read of + /// an operation owned by your own task. Strongly consistent reads are only allowed on + /// `OperationVc`s, which should never be local tasks. fn try_read_local_output( &self, parent_task_id: TaskId, local_task_id: LocalTaskId, - consistency: ReadConsistency, - ) -> Result>; - - /// INVALIDATION: Be careful with this, it will not track dependencies, so - /// using it could break cache invalidation. - fn try_read_local_output_untracked( - &self, - parent_task_id: TaskId, - local_task_id: LocalTaskId, - consistency: ReadConsistency, ) -> Result>; fn read_task_collectibles(&self, task: TaskId, trait_id: TraitTypeId) -> TaskCollectiblesMap; @@ -335,7 +328,7 @@ pub enum TaskPersistence { LocalCells, } -#[derive(Clone, Copy, Eq, PartialEq)] +#[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum ReadConsistency { /// The default behavior for most APIs. Reads are faster, but may return stale values, which /// may later trigger re-computation. @@ -1323,26 +1316,16 @@ impl TurboTasksApi for TurboTasks { &self, parent_task_id: TaskId, local_task_id: LocalTaskId, - consistency: ReadConsistency, - ) -> Result> { - // we don't currently support reading a local output outside of it's own task, so - // tracked/untracked is currently irrelevant - self.try_read_local_output_untracked(parent_task_id, local_task_id, consistency) - } - - /// INVALIDATION: Be careful with this, it will not track dependencies, so - /// using it could break cache invalidation. - fn try_read_local_output_untracked( - &self, - parent_task_id: TaskId, - local_task_id: LocalTaskId, - // we don't currently support reading a local output outside of it's own task, so - // consistency is currently irrelevant - _consistency: ReadConsistency, ) -> Result> { CURRENT_GLOBAL_TASK_STATE.with(|gts| { let gts_read = gts.read().unwrap(); + + // Local Vcs are local to their parent task, and do not exist outside of it. This is + // weakly enforced at compile time using the `NonLocalValue` marker trait. This + // assertion exists to handle any potential escapes that the compile-time checks cannot + // capture. gts_read.assert_task_id(parent_task_id); + match gts_read.get_local_task(local_task_id) { LocalTask::Scheduled { done_event } => Ok(Err(done_event.listen())), LocalTask::Done { output } => Ok(Ok(output.as_read_result()?)), @@ -2051,10 +2034,9 @@ pub(crate) async fn read_local_output( this: &dyn TurboTasksApi, parent_task_id: TaskId, local_task_id: LocalTaskId, - consistency: ReadConsistency, ) -> Result { loop { - match this.try_read_local_output(parent_task_id, local_task_id, consistency)? { + match this.try_read_local_output(parent_task_id, local_task_id)? { Ok(raw_vc) => return Ok(raw_vc), Err(event_listener) => event_listener.await, } diff --git a/turbopack/crates/turbo-tasks/src/raw_vc.rs b/turbopack/crates/turbo-tasks/src/raw_vc.rs index 7e07d1a51f17f..ac0ef750d8fd2 100644 --- a/turbopack/crates/turbo-tasks/src/raw_vc.rs +++ b/turbopack/crates/turbo-tasks/src/raw_vc.rs @@ -178,10 +178,9 @@ impl RawVc { } } RawVc::LocalOutput(task_id, local_cell_id) => { - current = - read_local_output(&*tt, task_id, local_cell_id, ReadConsistency::Eventual) - .await - .map_err(|source| ResolveTypeError::TaskError { source })?; + current = read_local_output(&*tt, task_id, local_cell_id) + .await + .map_err(|source| ResolveTypeError::TaskError { source })?; } RawVc::LocalCell(execution_id, local_cell_id) => { let shared_reference = read_local_cell(execution_id, local_cell_id); @@ -214,24 +213,22 @@ impl RawVc { let tt = turbo_tasks(); let mut current = self; let mut notified = false; - let mut lazily_notify = || { - if !notified { - tt.notify_scheduled_tasks(); - notified = true; - } - }; loop { match current { RawVc::TaskOutput(task) => { - lazily_notify(); + if !notified { + tt.notify_scheduled_tasks(); + notified = true; + } current = read_task_output(&*tt, task, consistency).await?; } RawVc::TaskCell(_, _) => return Ok(current), RawVc::LocalOutput(task_id, local_cell_id) => { - lazily_notify(); - current = read_local_output(&*tt, task_id, local_cell_id, consistency).await?; + debug_assert_eq!(consistency, ReadConsistency::Eventual); + current = read_local_output(&*tt, task_id, local_cell_id).await?; } RawVc::LocalCell(execution_id, local_cell_id) => { + debug_assert_eq!(consistency, ReadConsistency::Eventual); let shared_reference = read_local_cell(execution_id, local_cell_id); let value_type = get_value_type(shared_reference.0); return Ok((value_type.raw_cell)(shared_reference)); @@ -245,20 +242,10 @@ impl RawVc { pub(crate) async fn to_non_local(self) -> Result { let tt = turbo_tasks(); let mut current = self; - let mut notified = false; - let mut lazily_notify = || { - if !notified { - tt.notify_scheduled_tasks(); - notified = true; - } - }; loop { match current { RawVc::LocalOutput(task_id, local_cell_id) => { - lazily_notify(); - current = - read_local_output(&*tt, task_id, local_cell_id, ReadConsistency::Eventual) - .await?; + current = read_local_output(&*tt, task_id, local_cell_id).await?; } RawVc::LocalCell(execution_id, local_cell_id) => { let shared_reference = read_local_cell(execution_id, local_cell_id); @@ -435,18 +422,10 @@ impl Future for ReadRawVcFuture { } } RawVc::LocalOutput(task_id, local_output_id) => { - let read_result = if this.untracked { - tt.try_read_local_output_untracked( - task_id, - local_output_id, - this.consistency, - ) - } else { - tt.try_read_local_output(task_id, local_output_id, this.consistency) - }; + debug_assert_eq!(this.consistency, ReadConsistency::Eventual); + let read_result = tt.try_read_local_output(task_id, local_output_id); match read_result { Ok(Ok(vc)) => { - this.consistency = ReadConsistency::Eventual; this.current = vc; continue 'outer; } @@ -455,6 +434,7 @@ impl Future for ReadRawVcFuture { } } RawVc::LocalCell(execution_id, local_cell_id) => { + debug_assert_eq!(this.consistency, ReadConsistency::Eventual); return Poll::Ready( Ok(read_local_cell(execution_id, local_cell_id).into()), );