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.

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

Make ReadRef using VcCellMode semantics, add VcCellMode::raw_cell API #8847

Closed
wants to merge 1 commit into from
Closed
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
15 changes: 7 additions & 8 deletions crates/turbo-tasks/src/backend.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use std::{
any::Any,
borrow::Cow,
fmt::{self, Debug, Display, Write},
future::Future,
Expand All @@ -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 {
Expand Down Expand Up @@ -347,11 +347,14 @@ impl Display for CellContent {
}

impl TypedCellContent {
pub fn cast<T: Any + VcValueType>(self) -> Result<ReadRef<T>> {
pub fn cast<T: VcValueType>(self) -> Result<ReadRef<T>> {
let data = self.1 .0.ok_or_else(|| anyhow!("Cell is empty"))?;
let data = data
.downcast()
.downcast::<<T::Read as VcRead<T>>::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))
}

Expand All @@ -374,10 +377,6 @@ impl TypedCellContent {
)
}

pub fn try_cast<T: Any + VcValueType>(self) -> Option<ReadRef<T>> {
Some(ReadRef::new_arc(self.1 .0?.downcast().ok()?))
}

pub fn into_untyped(self) -> CellContent {
self.1
}
Expand Down
2 changes: 1 addition & 1 deletion crates/turbo-tasks/src/completion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down
143 changes: 115 additions & 28 deletions crates/turbo-tasks/src/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1522,57 +1522,144 @@ pub struct CurrentCellRef {
index: CellId,
}

type VcReadRepr<T> = <<T as VcValueType>::Read as VcRead<T>>::Repr;

impl CurrentCellRef {
pub fn conditional_update_shared<
T: VcValueType + 'static,
F: FnOnce(Option<&T>) -> Option<T>,
>(
/// Updates the cell if the given `functor` returns a value.
pub fn conditional_update<T>(&self, functor: impl FnOnce(Option<&T>) -> Option<T>)
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::<VcReadRepr<T>>())
.map(|content| <T::Read as VcRead<T>>::repr_to_value_ref(content));
let new_value = functor(old_ref)?;
Some(SharedReference::new(triomphe::Arc::new(
<T::Read as VcRead<T>>::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<SharedReference>,
) {
let tt = turbo_tasks();
let content = tt
.read_own_task_cell(self.current_task, self.index)
.ok()
.and_then(|v| v.try_cast::<T>());
let update =
functor(content.as_deref().map(|content| {
<<T as VcValueType>::Read as VcRead<T>>::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<T: PartialEq + VcValueType + 'static>(&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.
///
/// Take this example of a custom equality implementation on a transparent
/// wrapper type:
///
/// ```
/// #[turbo_tasks::value(transparent, eq = "manual")]
/// struct Wrapper(Vec<u32>);
///
/// 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 {}
/// ```
///
/// Comparisons of `Vc<Wrapper>` used when updating the cell will use
/// `Wrapper`'s custom equality implementation, rather than the one
/// provided by the target (`Vec<u32>`) 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<T>(&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_content)
Some(new_value)
});
}

pub fn update_shared<T: VcValueType + 'static>(&self, new_content: T) {
/// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it is useful to have the reciprocal comment?

/// [`CurrentCellRef::compare_and_update`].
///
/// The [`SharedReference`] is expected to use the `<T::Read as
/// VcRead<T>>::Repr` type for its representation of the value.
pub fn compare_and_update_with_shared_reference<T>(&self, new_shared_reference: SharedReference)
where
T: VcValueType + PartialEq,
{
fn extract_sr_value<T: VcValueType>(sr: &SharedReference) -> &T {
<T::Read as VcRead<T>>::repr_to_value_ref(
sr.0.downcast_ref::<VcReadRepr<T>>()
.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_shared_reference)
});
}

/// Unconditionally updates the content of the cell.
pub fn update<T>(&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(
<T::Read as VcRead<T>>::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 `<T::Read as
/// VcRead<T>>::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
};
Expand Down
11 changes: 6 additions & 5 deletions crates/turbo-tasks/src/read_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down Expand Up @@ -244,14 +244,15 @@ where
/// reference.
pub fn cell(read_ref: ReadRef<T>) -> Vc<T> {
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::<T, <T::Read as VcRead<T>>::Repr>(read_ref.0)
}));
};
Vc {
node: local_cell.into(),
node: <T::CellMode as VcCellMode<T>>::raw_cell(
SharedReference::new(value).typed(type_id),
),
_t: PhantomData,
}
}
Comment on lines 245 to 258
Copy link
Member Author

Choose a reason for hiding this comment

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

Essentially the problem here was that ReadRef can't/shouldn't implement cell itself. It must defer to VcCellMode's implementation, as only that trait's implementation knows how to correctly construct a cell.

Expand Down
2 changes: 1 addition & 1 deletion crates/turbo-tasks/src/trait_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Comment on lines 102 to 108
Copy link
Member Author

@bgw bgw Jul 26, 2024

Choose a reason for hiding this comment

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

TraitRef::cell has similar problems to ReadRef::cell here, and needs to also call VcCellMode::raw_cell, but that's a bit more complicated as I need a mapping of type ids to raw_vc function pointers. I'll do that in a subsequent PR.

Edit: Fixed in #8870

Expand Down
18 changes: 3 additions & 15 deletions crates/turbo-tasks/src/vc/cast.rs
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -27,19 +27,7 @@ where
type Output = ReadRef<T>;

fn cast(content: TypedCellContent) -> Result<Self::Output> {
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<ReadRef<<T::Read as VcRead<T>>::Repr>>,
Self::Output,
>(&ManuallyDrop::new(content.cast()?))
},
)
content.cast()
Comment on lines -30 to -42
Copy link
Member Author

Choose a reason for hiding this comment

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

It turns out that this transmute wasn't entirely correct. You can't do a straight transmute between two ReadRef<T>s because it contains a triomphe::Arc<T>, and the vtable half of triomphe::Arc<T>'s fat pointer is different depending on the type of T.

You need to use something like unchecked_sidecast_triomphe_arc so that the fat pointer is constructed correctly. I modified the TypedCellContent::cast method in this PR to do the cast of Arc for us before constructing the ReadRef.

}
}

Expand Down
51 changes: 41 additions & 10 deletions crates/turbo-tasks/src/vc/cell_mode.rs
Original file line number Diff line number Diff line change
@@ -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<T> = <<T as VcValueType>::Read as VcRead<T>>::Target;
type VcReadRepr<T> = <<T as VcValueType>::Read as VcRead<T>>::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<T>
where
T: VcValueType,
{
fn cell(value: <T::Read as VcRead<T>>::Target) -> Vc<T>;
/// Create a new cell.
fn cell(value: VcReadTarget<T>) -> Vc<T>;

/// 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;
}
Comment on lines +20 to +22
Copy link
Member Author

Choose a reason for hiding this comment

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

Local Vc resolution needs this VcCellMode::raw_cell method too because we need to be able to construct a new cell inside of RawVc where we don't have compile-time information about the concrete type T (but we do have the type id).

Copy link
Contributor

Choose a reason for hiding this comment

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

In some cases, we will re-use the shared value.

When is this the case?

Copy link
Member Author

@bgw bgw Jul 31, 2024

Choose a reason for hiding this comment

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

When constructing a Vc from a ReadRef or when converting a local Vc to a global resolved Vc we already have a SharedReference or triomphe::Arc, so we can re-use that value and avoid any need to clone the underlying value.

(I'll update the comment in the code here to provide this context)


/// Mode that always updates the cell's content.
Expand All @@ -22,14 +31,21 @@ impl<T> VcCellMode<T> for VcCellNewMode<T>
where
T: VcValueType,
{
fn cell(inner: <T::Read as VcRead<T>>::Target) -> Vc<T> {
fn cell(inner: VcReadTarget<T>) -> Vc<T> {
let cell = find_cell_by_type(T::get_value_type_id());
cell.update_shared(<T::Read as VcRead<T>>::target_to_repr(inner));
cell.update(<T::Read as VcRead<T>>::target_to_value(inner));
Vc {
node: cell.into(),
_t: PhantomData,
}
}

fn raw_cell(content: TypedSharedReference) -> RawVc {
debug_assert_repr::<T>(&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
Expand All @@ -40,15 +56,30 @@ pub struct VcCellSharedMode<T> {

impl<T> VcCellMode<T> for VcCellSharedMode<T>
where
T: VcValueType,
<T::Read as VcRead<T>>::Repr: PartialEq,
T: VcValueType + PartialEq,
{
fn cell(inner: <T::Read as VcRead<T>>::Target) -> Vc<T> {
fn cell(inner: VcReadTarget<T>) -> Vc<T> {
let cell = find_cell_by_type(T::get_value_type_id());
cell.compare_and_update_shared(<T::Read as VcRead<T>>::target_to_repr(inner));
cell.compare_and_update(<T::Read as VcRead<T>>::target_to_value(inner));
Vc {
node: cell.into(),
_t: PhantomData,
}
}

fn raw_cell(content: TypedSharedReference) -> RawVc {
debug_assert_repr::<T>(&content);
let cell = find_cell_by_type(content.0);
cell.compare_and_update_with_shared_reference::<T>(content.1);
cell.into()
}
}

fn debug_assert_repr<T: VcValueType>(content: &TypedSharedReference) {
debug_assert!(
(*content.1 .0).is::<VcReadRepr<T>>(),
"SharedReference for type {} must use representation type {}",
type_name::<T>(),
type_name::<VcReadRepr<T>>(),
);
}
3 changes: 1 addition & 2 deletions crates/turbo-tasks/src/vc/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down
Loading
Loading