-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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<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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Essentially the problem here was that |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Edit: Fixed in #8870 |
||
|
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 | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 You need to use something like |
||
} | ||
} | ||
|
||
|
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Local Vc resolution needs this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When is this the case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When constructing a (I'll update the comment in the code here to provide this context) |
||
|
||
/// Mode that always updates the cell's content. | ||
|
@@ -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 | ||
|
@@ -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>>(), | ||
); | ||
} |
There was a problem hiding this comment.
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?