From b322676a8886df32397593a7ca2bcff9311c85bc Mon Sep 17 00:00:00 2001 From: Benjamin Woodruff Date: Thu, 25 Jul 2024 18:33:27 -0700 Subject: [PATCH] More ReadRef fixes using `VcCellMode::raw_cell` API --- crates/turbo-tasks/src/backend.rs | 15 +- crates/turbo-tasks/src/completion.rs | 2 +- crates/turbo-tasks/src/manager.rs | 140 ++++++++++++++---- crates/turbo-tasks/src/read_ref.rs | 11 +- crates/turbo-tasks/src/trait_ref.rs | 2 +- crates/turbo-tasks/src/vc/cast.rs | 18 +-- crates/turbo-tasks/src/vc/cell_mode.rs | 51 +++++-- crates/turbo-tasks/src/vc/mod.rs | 3 +- crates/turbo-tasks/src/vc/read.rs | 40 ++++- crates/turbopack-node/src/evaluate.rs | 4 +- .../turbopack-node/src/render/render_proxy.rs | 4 +- .../src/render/render_static.rs | 4 +- 12 files changed, 211 insertions(+), 83 deletions(-) diff --git a/crates/turbo-tasks/src/backend.rs b/crates/turbo-tasks/src/backend.rs index bb1572bc3a1de8..8377535dff8406 100644 --- a/crates/turbo-tasks/src/backend.rs +++ b/crates/turbo-tasks/src/backend.rs @@ -1,5 +1,4 @@ use std::{ - any::Any, borrow::Cow, fmt::{self, Debug, Display, Write}, future::Future, @@ -22,8 +21,9 @@ use crate::{ raw_vc::CellId, registry, trait_helpers::{get_trait_method, has_trait, traits}, + triomphe_utils::unchecked_sidecast_triomphe_arc, FunctionId, RawVc, ReadRef, SharedReference, TaskId, TaskIdProvider, TaskIdSet, TraitRef, - TraitTypeId, ValueTypeId, VcValueTrait, VcValueType, + TraitTypeId, ValueTypeId, VcRead, VcValueTrait, VcValueType, }; pub enum TaskType { @@ -347,11 +347,14 @@ impl Display for CellContent { } impl TypedCellContent { - pub fn cast(self) -> Result> { + pub fn cast(self) -> Result> { let data = self.1 .0.ok_or_else(|| anyhow!("Cell is empty"))?; let data = data - .downcast() + .downcast::<>::Repr>() .map_err(|_err| anyhow!("Unexpected type in cell"))?; + // SAFETY: `T` and `T::Read::Repr` must have equivalent memory representations, + // guaranteed by the unsafe implementation of `VcValueType`. + let data = unsafe { unchecked_sidecast_triomphe_arc(data) }; Ok(ReadRef::new_arc(data)) } @@ -374,10 +377,6 @@ impl TypedCellContent { ) } - pub fn try_cast(self) -> Option> { - Some(ReadRef::new_arc(self.1 .0?.downcast().ok()?)) - } - pub fn into_untyped(self) -> CellContent { self.1 } diff --git a/crates/turbo-tasks/src/completion.rs b/crates/turbo-tasks/src/completion.rs index cbd7501a7fab03..71afe672f296a5 100644 --- a/crates/turbo-tasks/src/completion.rs +++ b/crates/turbo-tasks/src/completion.rs @@ -36,7 +36,7 @@ impl Completion { // only updates the cell when it is empty (Completion::cell opted-out of // that via `#[turbo_tasks::value(cell = "new")]`) let cell = turbo_tasks::macro_helpers::find_cell_by_type(*COMPLETION_VALUE_TYPE_ID); - cell.conditional_update_shared(|old| old.is_none().then_some(Completion)); + cell.conditional_update(|old| old.is_none().then_some(Completion)); let raw: RawVc = cell.into(); raw.into() } diff --git a/crates/turbo-tasks/src/manager.rs b/crates/turbo-tasks/src/manager.rs index 5c0c2f3a5c35c0..8d5ab6319a2eff 100644 --- a/crates/turbo-tasks/src/manager.rs +++ b/crates/turbo-tasks/src/manager.rs @@ -1522,57 +1522,141 @@ pub struct CurrentCellRef { index: CellId, } +type VcReadRepr = <::Read as VcRead>::Repr; + impl CurrentCellRef { - pub fn conditional_update_shared< - T: VcValueType + 'static, - F: FnOnce(Option<&T>) -> Option, - >( + /// Updates the cell if the given `functor` returns a value. + pub fn conditional_update(&self, functor: impl FnOnce(Option<&T>) -> Option) + where + T: VcValueType, + { + self.conditional_update_with_shared_reference(|old_shared_reference| { + let old_ref = old_shared_reference + .and_then(|sr| sr.0.downcast_ref::>()) + .map(|content| >::repr_to_value_ref(content)); + let new_value = functor(old_ref)?; + Some(SharedReference::new(triomphe::Arc::new( + >::value_to_repr(new_value), + ))) + }) + } + + /// Updates the cell if the given `functor` returns a `SharedReference`. + pub fn conditional_update_with_shared_reference( &self, - functor: F, + functor: impl FnOnce(Option<&SharedReference>) -> Option, ) { let tt = turbo_tasks(); - let content = tt - .read_own_task_cell(self.current_task, self.index) - .ok() - .and_then(|v| v.try_cast::()); - let update = - functor(content.as_deref().map(|content| { - <::Read as VcRead>::target_to_value_ref(content) - })); + let cell_content = tt.read_own_task_cell(self.current_task, self.index).ok(); + let update = functor(cell_content.as_ref().and_then(|cc| cc.1 .0.as_ref())); if let Some(update) = update { - tt.update_own_task_cell( - self.current_task, - self.index, - CellContent(Some(SharedReference::new(triomphe::Arc::new(update)))), - ) + tt.update_own_task_cell(self.current_task, self.index, CellContent(Some(update))) } } - pub fn compare_and_update_shared(&self, new_content: T) { - self.conditional_update_shared(|old_content| { - if let Some(old_content) = old_content { - if PartialEq::eq(&new_content, old_content) { + /// Replace the current cell's content with `new_value` if the current + /// content is not equal by value with the existing content. + /// + /// The comparison happens using the value itself, not the `VcRead::Target` + /// of that value. This means for + /// + /// ``` + /// #[turbo_tasks::value(transparent, eq = "manual")] + /// struct Wrapper(Vec); + /// + /// impl PartialEq for Wrapper { + /// fn eq(&self, other: Wrapper) { + /// // Example: order doesn't matter for equality + /// let (mut this, mut other) = (self.clone(), other.clone()); + /// this.sort_unstable(); + /// other.sort_unstable(); + /// this == other + /// } + /// } + /// + /// impl Eq for Wrapper {} + /// ``` + /// + /// That comparisons of `Vc` used when updating the cell will use + /// `Wrapper`'s custom equality implementation, rather than the one + /// provided by the target type. + /// + /// However, in most cases, the default derived implementation of + /// `PartialEq` is used which just forwards to the inner value's + /// `PartialEq`. + pub fn compare_and_update(&self, new_value: T) + where + T: PartialEq + VcValueType, + { + self.conditional_update(|old_value| { + if let Some(old_value) = old_value { + if old_value == &new_value { + return None; + } + } + Some(new_value) + }); + } + + /// Replace the current cell's content with `new_shared_reference` if the + /// current content is not equal by value with the existing content. + /// + /// If you already have a `SharedReference`, this is a faster version of + /// [`CurrentCellRef::compare_and_update`]. + /// + /// The [`SharedReference`] is expected to use the `>::Repr` type for its representation of the value. + pub fn compare_and_update_with_shared_reference(&self, new_shared_reference: SharedReference) + where + T: VcValueType + PartialEq, + { + fn extract_sr_value(sr: &SharedReference) -> &T { + >::repr_to_value_ref( + sr.0.downcast_ref::>() + .expect("cannot update SharedReference of different type"), + ) + } + self.conditional_update_with_shared_reference(|old_sr| { + if let Some(old_sr) = old_sr { + let old_value: &T = extract_sr_value(old_sr); + let new_value = extract_sr_value(&new_shared_reference); + if old_value == new_value { return None; } } - Some(new_content) + Some(new_shared_reference) }); } - pub fn update_shared(&self, new_content: T) { + /// Unconditionally updates the content of the cell. + pub fn update(&self, new_value: T) + where + T: VcValueType, + { let tt = turbo_tasks(); tt.update_own_task_cell( self.current_task, self.index, - CellContent(Some(SharedReference::new(triomphe::Arc::new(new_content)))), + CellContent(Some(SharedReference::new(triomphe::Arc::new( + >::value_to_repr(new_value), + )))), ) } - pub fn update_shared_reference(&self, shared_ref: SharedReference) { + /// A faster version of [`Self::update`] if you already have a + /// [`SharedReference`]. + /// + /// If the passed-in [`SharedReference`] is the same as the existing cell's + /// by identity, no update is performed. + /// + /// The [`SharedReference`] is expected to use the `>::Repr` type for its representation of the value. + pub fn update_with_shared_reference(&self, shared_ref: SharedReference) { let tt = turbo_tasks(); let content = tt.read_own_task_cell(self.current_task, self.index).ok(); - let update = if let Some(TypedCellContent(_, CellContent(shared_ref_exp))) = content { - shared_ref_exp.as_ref().ne(&Some(&shared_ref)) + let update = if let Some(TypedCellContent(_, CellContent(Some(shared_ref_exp)))) = content { + // pointer equality (not value equality) + shared_ref_exp != shared_ref } else { true }; diff --git a/crates/turbo-tasks/src/read_ref.rs b/crates/turbo-tasks/src/read_ref.rs index 847532639b2e83..b5525a29b4dc06 100644 --- a/crates/turbo-tasks/src/read_ref.rs +++ b/crates/turbo-tasks/src/read_ref.rs @@ -10,9 +10,9 @@ use turbo_tasks_hash::DeterministicHash; use crate::{ debug::{ValueDebugFormat, ValueDebugFormatString}, - macro_helpers::find_cell_by_type, trace::{TraceRawVcs, TraceRawVcsContext}, triomphe_utils::unchecked_sidecast_triomphe_arc, + vc::VcCellMode, SharedReference, Vc, VcRead, VcValueType, }; @@ -244,14 +244,15 @@ where /// reference. pub fn cell(read_ref: ReadRef) -> Vc { let type_id = T::get_value_type_id(); - let local_cell = find_cell_by_type(type_id); // SAFETY: `T` and `T::Read::Repr` must have equivalent memory representations, // guaranteed by the unsafe implementation of `VcValueType`. - local_cell.update_shared_reference(SharedReference::new(unsafe { + let value = unsafe { unchecked_sidecast_triomphe_arc::>::Repr>(read_ref.0) - })); + }; Vc { - node: local_cell.into(), + node: >::raw_cell( + SharedReference::new(value).typed(type_id), + ), _t: PhantomData, } } diff --git a/crates/turbo-tasks/src/trait_ref.rs b/crates/turbo-tasks/src/trait_ref.rs index 43efa4af502964..ddb35820435e2e 100644 --- a/crates/turbo-tasks/src/trait_ref.rs +++ b/crates/turbo-tasks/src/trait_ref.rs @@ -102,7 +102,7 @@ where // See Safety clause above. let TypedSharedReference(ty, shared_ref) = trait_ref.shared_reference; let local_cell = find_cell_by_type(ty); - local_cell.update_shared_reference(shared_ref); + local_cell.update_with_shared_reference(shared_ref); let raw_vc: RawVc = local_cell.into(); raw_vc.into() } diff --git a/crates/turbo-tasks/src/vc/cast.rs b/crates/turbo-tasks/src/vc/cast.rs index 38325d377c74ef..c3816f26917eb3 100644 --- a/crates/turbo-tasks/src/vc/cast.rs +++ b/crates/turbo-tasks/src/vc/cast.rs @@ -1,8 +1,8 @@ -use std::{marker::PhantomData, mem::ManuallyDrop}; +use std::marker::PhantomData; use anyhow::Result; -use crate::{backend::TypedCellContent, ReadRef, TraitRef, VcRead, VcValueTrait, VcValueType}; +use crate::{backend::TypedCellContent, ReadRef, TraitRef, VcValueTrait, VcValueType}; /// Trait defined to share behavior between values and traits within /// [`ReadRawVcFuture`][crate::ReadRawVcFuture]. See [`VcValueTypeCast`] and @@ -27,19 +27,7 @@ where type Output = ReadRef; fn cast(content: TypedCellContent) -> Result { - Ok( - // Safety: the `VcValueType` implementor must guarantee that both `T` and - // `Repr` are #[repr(transparent)]. - unsafe { - // Downcast the cell content to the expected representation type, then - // transmute it to the expected type. - // See https://users.rust-lang.org/t/transmute-doesnt-work-on-generic-types/87272/9 - std::mem::transmute_copy::< - ManuallyDrop>::Repr>>, - Self::Output, - >(&ManuallyDrop::new(content.cast()?)) - }, - ) + content.cast() } } diff --git a/crates/turbo-tasks/src/vc/cell_mode.rs b/crates/turbo-tasks/src/vc/cell_mode.rs index 8c50017bfa263f..3aaec56d4587c6 100644 --- a/crates/turbo-tasks/src/vc/cell_mode.rs +++ b/crates/turbo-tasks/src/vc/cell_mode.rs @@ -1,16 +1,25 @@ -use std::marker::PhantomData; +use std::{any::type_name, marker::PhantomData}; use super::{read::VcRead, traits::VcValueType}; -use crate::{manager::find_cell_by_type, Vc}; +use crate::{manager::find_cell_by_type, task::shared_reference::TypedSharedReference, RawVc, Vc}; -/// Trait that controls the behavior of `Vc::cell` on a value type basis. +type VcReadTarget = <::Read as VcRead>::Target; +type VcReadRepr = <::Read as VcRead>::Repr; + +/// Trait that controls the behavior of [`Vc::cell`] based on the value type's +/// [`VcValueType::CellMode`]. /// /// This trait must remain sealed within this crate. pub trait VcCellMode where T: VcValueType, { - fn cell(value: >::Target) -> Vc; + /// Create a new cell. + fn cell(value: VcReadTarget) -> Vc; + + /// Create a type-erased `RawVc` cell given a pre-existing type-erased + /// `SharedReference`. In some cases, we will re-use the shared value. + fn raw_cell(value: TypedSharedReference) -> RawVc; } /// Mode that always updates the cell's content. @@ -22,14 +31,21 @@ impl VcCellMode for VcCellNewMode where T: VcValueType, { - fn cell(inner: >::Target) -> Vc { + fn cell(inner: VcReadTarget) -> Vc { let cell = find_cell_by_type(T::get_value_type_id()); - cell.update_shared(>::target_to_repr(inner)); + cell.update(>::target_to_value(inner)); Vc { node: cell.into(), _t: PhantomData, } } + + fn raw_cell(content: TypedSharedReference) -> RawVc { + debug_assert_repr::(&content); + let cell = find_cell_by_type(content.0); + cell.update_with_shared_reference(content.1); + cell.into() + } } /// Mode that compares the cell's content with the new value and only updates @@ -40,15 +56,30 @@ pub struct VcCellSharedMode { impl VcCellMode for VcCellSharedMode where - T: VcValueType, - >::Repr: PartialEq, + T: VcValueType + PartialEq, { - fn cell(inner: >::Target) -> Vc { + fn cell(inner: VcReadTarget) -> Vc { let cell = find_cell_by_type(T::get_value_type_id()); - cell.compare_and_update_shared(>::target_to_repr(inner)); + cell.compare_and_update(>::target_to_value(inner)); Vc { node: cell.into(), _t: PhantomData, } } + + fn raw_cell(content: TypedSharedReference) -> RawVc { + debug_assert_repr::(&content); + let cell = find_cell_by_type(content.0); + cell.compare_and_update_with_shared_reference::(content.1); + cell.into() + } +} + +fn debug_assert_repr(content: &TypedSharedReference) { + debug_assert!( + (*content.1 .0).is::>(), + "SharedReference for type {} must use representation type {}", + type_name::(), + type_name::>(), + ); } diff --git a/crates/turbo-tasks/src/vc/mod.rs b/crates/turbo-tasks/src/vc/mod.rs index 19f106eb545d87..5f31ef8da38858 100644 --- a/crates/turbo-tasks/src/vc/mod.rs +++ b/crates/turbo-tasks/src/vc/mod.rs @@ -16,10 +16,9 @@ use anyhow::Result; use auto_hash_map::AutoSet; use serde::{Deserialize, Serialize}; -use self::cell_mode::VcCellMode; pub use self::{ cast::{VcCast, VcValueTraitCast, VcValueTypeCast}, - cell_mode::{VcCellNewMode, VcCellSharedMode}, + cell_mode::{VcCellMode, VcCellNewMode, VcCellSharedMode}, default::ValueDefault, read::{ReadVcFuture, VcDefaultRead, VcRead, VcTransparentRead}, resolved::{ResolvedValue, ResolvedVc}, diff --git a/crates/turbo-tasks/src/vc/read.rs b/crates/turbo-tasks/src/vc/read.rs index e1c7c6034d2ce8..6c7e06d49c347d 100644 --- a/crates/turbo-tasks/src/vc/read.rs +++ b/crates/turbo-tasks/src/vc/read.rs @@ -39,11 +39,17 @@ where /// Convert a reference to a value to a reference to the target type. fn value_to_target_ref(value: &T) -> &Self::Target; - /// Convert the target type to the repr. - fn target_to_repr(target: Self::Target) -> Self::Repr; + /// Convert the value type to the repr. + fn value_to_repr(value: T) -> Self::Repr; + + /// Convert the target type to the value. + fn target_to_value(target: Self::Target) -> T; /// Convert a reference to a target type to a reference to a value. fn target_to_value_ref(target: &Self::Target) -> &T; + + /// Convert a reference to a repr type to a reference to a value. + fn repr_to_value_ref(repr: &Self::Repr) -> &T; } /// Representation for standard `#[turbo_tasks::value]`, where a read return a @@ -63,13 +69,21 @@ where value } - fn target_to_repr(target: Self::Target) -> T { + fn value_to_repr(value: T) -> Self::Repr { + value + } + + fn target_to_value(target: Self::Target) -> T { target } fn target_to_value_ref(target: &Self::Target) -> &T { target } + + fn repr_to_value_ref(repr: &Self::Repr) -> &T { + repr + } } /// Representation for `#[turbo_tasks::value(transparent)]` types, where reads @@ -98,12 +112,17 @@ where } } - fn target_to_repr(target: Self::Target) -> Self::Repr { + fn value_to_repr(value: T) -> Self::Repr { // Safety: see `Self::value_to_target` above. unsafe { - std::mem::transmute_copy::, Self::Repr>(&ManuallyDrop::new( - target, - )) + std::mem::transmute_copy::, Self::Repr>(&ManuallyDrop::new(value)) + } + } + + fn target_to_value(target: Self::Target) -> T { + // Safety: see `Self::value_to_target` above. + unsafe { + std::mem::transmute_copy::, T>(&ManuallyDrop::new(target)) } } @@ -113,6 +132,13 @@ where std::mem::transmute_copy::, &T>(&ManuallyDrop::new(target)) } } + + fn repr_to_value_ref(repr: &Self::Repr) -> &T { + // Safety: see `Self::value_to_target` above. + unsafe { + std::mem::transmute_copy::, &T>(&ManuallyDrop::new(repr)) + } + } } pub struct ReadVcFuture> diff --git a/crates/turbopack-node/src/evaluate.rs b/crates/turbopack-node/src/evaluate.rs index 505755b12c711d..91c0723c2a993b 100644 --- a/crates/turbopack-node/src/evaluate.rs +++ b/crates/turbopack-node/src/evaluate.rs @@ -242,7 +242,7 @@ pub fn custom_evaluate(evaluate_context: impl EvaluateContext) -> Vc Vc Vc { // We initialize the cell with a stream that is open, but has no values. // The first [render_stream_internal] pipe call will pick up that stream. let (sender, receiver) = unbounded(); - cell.update_shared(RenderStream(Stream::new_open(vec![], Box::new(receiver)))); + cell.update(RenderStream(Stream::new_open(vec![], Box::new(receiver)))); let initial = Mutex::new(Some(sender)); // run the evaluation as side effect @@ -194,7 +194,7 @@ fn render_stream(options: RenderStreamOptions) -> Vc { // In cases when only [render_stream_internal] is (re)executed, we need to // update the old stream with a new value. let (sender, receiver) = unbounded(); - cell.update_shared(RenderStream(Stream::new_open(vec![], Box::new(receiver)))); + cell.update(RenderStream(Stream::new_open(vec![], Box::new(receiver)))); sender } }), diff --git a/crates/turbopack-node/src/render/render_static.rs b/crates/turbopack-node/src/render/render_static.rs index 05e3a161f63799..5835cd634a90a2 100644 --- a/crates/turbopack-node/src/render/render_static.rs +++ b/crates/turbopack-node/src/render/render_static.rs @@ -231,7 +231,7 @@ fn render_stream(options: RenderStreamOptions) -> Vc { // We initialize the cell with a stream that is open, but has no values. // The first [render_stream_internal] pipe call will pick up that stream. let (sender, receiver) = unbounded(); - cell.update_shared(RenderStream(Stream::new_open(vec![], Box::new(receiver)))); + cell.update(RenderStream(Stream::new_open(vec![], Box::new(receiver)))); let initial = Mutex::new(Some(sender)); // run the evaluation as side effect @@ -245,7 +245,7 @@ fn render_stream(options: RenderStreamOptions) -> Vc { // In cases when only [render_stream_internal] is (re)executed, we need to // update the old stream with a new value. let (sender, receiver) = unbounded(); - cell.update_shared(RenderStream(Stream::new_open(vec![], Box::new(receiver)))); + cell.update(RenderStream(Stream::new_open(vec![], Box::new(receiver)))); sender } }),