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

bgw · 2024-07-26T04:36:26Z

Description

More fixes to ReadRef on top of #8845. There's a bit of a cascade of changes that needed to happen here:

ReadRef::cell should actually use VcCellMode so that it supports new/shared semantics correctly.
... however, VcCellMode needs to expose an API (raw_cell) for working with SharedReference so that we can reuse the SharedReference/Arc inside of ReadRef instead of cloning the data inside of it.
... however, CurrentCellRef needs to expose a compare_and_update method that works for SharedReference (compare_and_update_with_shared_reference).
... however, the naming conventions for the CurrentCellRef methods are a bit confusing at this point, so rename them.

Bigger Picture

This my sneaky way of introducing the raw_cell API as part of a smaller PR, since that API will be needed for converting a local Vc to a resolved Vc. That's a similar problem of needing to reuse a SharedReference along with VcCellMode semantics.

Testing Instructions

cargo nextest r -p turbo-tasks -p turbo-tasks-memory

vercel · 2024-07-26T04:36:32Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
examples-nonmonorepo	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 29, 2024 9:05pm
rust-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 29, 2024 9:05pm

8 Skipped Deployments

Name	Status	Preview	Updated (UTC)
examples-basic-web	⬜️ Ignored (Inspect)	Visit Preview	Jul 29, 2024 9:05pm
examples-designsystem-docs	⬜️ Ignored (Inspect)	Visit Preview	Jul 29, 2024 9:05pm
examples-gatsby-web	⬜️ Ignored (Inspect)	Visit Preview	Jul 29, 2024 9:05pm
examples-kitchensink-blog	⬜️ Ignored (Inspect)	Visit Preview	Jul 29, 2024 9:05pm
examples-native-web	⬜️ Ignored (Inspect)	Visit Preview	Jul 29, 2024 9:05pm
examples-svelte-web	⬜️ Ignored (Inspect)	Visit Preview	Jul 29, 2024 9:05pm
examples-tailwind-web	⬜️ Ignored (Inspect)	Visit Preview	Jul 29, 2024 9:05pm
examples-vite-web	⬜️ Ignored (Inspect)	Visit Preview	Jul 29, 2024 9:05pm

bgw · 2024-07-26T04:36:42Z

Implement remainder of local Vcs: casting and get_task_id #8871
Respect VcCellMode in TraitRef::cell #8870
Implement resolution for local Vcs #8826
Minimal implementation of local Vcs #8780
Make ReadRef using VcCellMode semantics, add VcCellMode::raw_cell API #8847 👈
Fix ReadRef<T>::cell when T != T::Read::Repr #8845 : 1 other dependent PR (#8846 )
Add/move tests for Vc generics #8843
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @bgw and the rest of your teammates on Graphite

github-actions · 2024-07-26T04:38:55Z

🟢 Turbopack Benchmark CI successful 🟢

Thanks

github-actions · 2024-07-26T04:43:01Z

✅ This change can build next-swc

github-actions · 2024-07-26T04:46:26Z

⚠️ CI failed ⚠️

The following steps have failed in CI:

Turbopack Rust tests (mac/win, non-blocking)

See workflow summary for details

bgw · 2024-07-26T19:20:01Z

crates/turbo-tasks/src/read_ref.rs

    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,
        }
    }


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.

bgw · 2024-07-26T19:27:48Z

crates/turbo-tasks/src/vc/cast.rs

-        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()?))
-            },
-        )


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.

bgw · 2024-07-26T19:38:07Z

crates/turbo-tasks/src/vc/cell_mode.rs

+    /// 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;


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).

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

When is this the case?

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)

bgw · 2024-07-26T19:40:08Z

crates/turbo-tasks/src/trait_ref.rs

        // 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()
    }


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

arlyon · 2024-07-31T10:01:41Z

crates/turbo-tasks/src/manager.rs

+    /// 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


Perhaps it is useful to have the reciprocal comment?

bgw · 2024-08-02T21:20:40Z

Migrated to the next.js repo: vercel/next.js#68467

…e::raw_cell` API (#68467) *This is a migrated PR. This was in the turbo repository before the next.js merge.* **Migrated From:** vercel/turborepo#8847 ## Description More fixes to `ReadRef` on top of vercel/turborepo#8845. There's a bit of a cascade of changes that needed to happen here: - `ReadRef::cell` should actually use `VcCellMode` so that it supports new/shared semantics correctly. - ... however, `VcCellMode` needs to expose an API (`raw_cell`) for working with `SharedReference` so that we can reuse the `SharedReference`/`Arc` inside of `ReadRef` instead of cloning the data inside of it. - ... however, `CurrentCellRef` needs to expose a `compare_and_update` method that works for `SharedReference` (`compare_and_update_with_shared_reference`). - ... however, the naming conventions for the `CurrentCellRef` methods are a bit confusing at this point, so rename them. ## Bigger Picture This my sneaky way of introducing the `raw_cell` API as part of a smaller PR, since that API will be needed for converting a local `Vc` to a resolved `Vc`. That's a similar problem of needing to reuse a `SharedReference` along with `VcCellMode` semantics. ## Testing Instructions ``` cargo nextest r -p turbo-tasks -p turbo-tasks-memory ```

turbo-orchestrator bot added created-by: turbopack labels Jul 26, 2024

bgw mentioned this pull request Jul 26, 2024

Add/move tests for Vc generics #8843

Merged

This was referenced Jul 26, 2024

Fix ReadRef<T>::cell when T != T::Read::Repr #8845

Merged

Redundant PR, please ignore #8846

Closed

vercel bot deployed to Preview – examples-nonmonorepo July 26, 2024 04:36 View deployment

vercel bot deployed to Preview – rust-docs July 26, 2024 04:38 View deployment

bgw force-pushed the bgw/cell-mode-raw branch from 5cfd7a9 to fb383de Compare July 26, 2024 04:39

bgw changed the base branch from bgw/read-ref-cell-mode to bgw/fix-read-ref-cell July 26, 2024 04:39

vercel bot deployed to Preview – examples-nonmonorepo July 26, 2024 04:39 View deployment

vercel bot deployed to Preview – rust-docs July 26, 2024 04:41 View deployment

bgw changed the title ~~More ReadRef fixes using VcCellMode::raw_cell API~~ Make ReadRef using VcCellMode semantics, add VcCellMode::raw_cell API Jul 26, 2024

bgw force-pushed the bgw/cell-mode-raw branch from fb383de to b322676 Compare July 26, 2024 19:15

vercel bot deployed to Preview – examples-nonmonorepo July 26, 2024 19:15 View deployment

vercel bot deployed to Preview – rust-docs July 26, 2024 19:17 View deployment

bgw force-pushed the bgw/cell-mode-raw branch from b322676 to 2070b13 Compare July 26, 2024 19:36

vercel bot deployed to Preview – examples-nonmonorepo July 26, 2024 19:36 View deployment

vercel bot deployed to Preview – rust-docs July 26, 2024 19:38 View deployment

bgw commented Jul 26, 2024

View reviewed changes

bgw marked this pull request as ready for review July 26, 2024 19:43

bgw requested a review from a team as a code owner July 26, 2024 19:43

bgw requested review from sokra and wbinnssmith July 26, 2024 19:45

This was referenced Jul 26, 2024

Minimal implementation of local Vcs #8780

Closed

Implement resolution for local Vcs #8826

Closed

bgw force-pushed the bgw/fix-read-ref-cell branch from 83869a1 to 6bb3574 Compare July 29, 2024 17:54

bgw force-pushed the bgw/cell-mode-raw branch from 2070b13 to 372724e Compare July 29, 2024 17:54

vercel bot deployed to Preview – examples-nonmonorepo July 29, 2024 17:54 View deployment

vercel bot deployed to Preview – rust-docs July 29, 2024 17:56 View deployment

bgw force-pushed the bgw/fix-read-ref-cell branch from 6bb3574 to 88a3dea Compare July 29, 2024 20:41

Base automatically changed from bgw/fix-read-ref-cell to main July 29, 2024 20:51

More ReadRef fixes using VcCellMode::raw_cell API

8de8c62

bgw force-pushed the bgw/cell-mode-raw branch from 372724e to 8de8c62 Compare July 29, 2024 21:02

vercel bot deployed to Preview – examples-nonmonorepo July 29, 2024 21:03 View deployment

bgw mentioned this pull request Jul 29, 2024

Respect VcCellMode in TraitRef::cell #8870

Closed

vercel bot deployed to Preview – rust-docs July 29, 2024 21:05 View deployment

bgw mentioned this pull request Jul 30, 2024

Implement remainder of local Vcs: casting and get_task_id #8871

Closed

arlyon reviewed Jul 31, 2024

View reviewed changes

sokra approved these changes Aug 1, 2024

View reviewed changes

bgw mentioned this pull request Aug 2, 2024

[turbopack] Make ReadRef using VcCellMode semantics, add VcCellMode::raw_cell API vercel/next.js#68467

Merged

bgw closed this Aug 2, 2024

bgw deleted the bgw/cell-mode-raw branch August 4, 2024 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bgw commented Jul 26, 2024 •

edited

Loading

vercel bot commented Jul 26, 2024 •

edited

Loading

bgw commented Jul 26, 2024 •

edited

Loading

github-actions bot commented Jul 26, 2024 •

edited

Loading

github-actions bot commented Jul 26, 2024

github-actions bot commented Jul 26, 2024 •

edited

Loading

bgw Jul 26, 2024

bgw Jul 26, 2024

bgw Jul 26, 2024

arlyon Jul 31, 2024

bgw Jul 31, 2024 •

edited

Loading

bgw Jul 26, 2024 •

edited

Loading

arlyon Jul 31, 2024

bgw commented Aug 2, 2024

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

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

Conversation

bgw commented Jul 26, 2024 • edited Loading

Description

Bigger Picture

Testing Instructions

vercel bot commented Jul 26, 2024 • edited Loading

bgw commented Jul 26, 2024 • edited Loading

github-actions bot commented Jul 26, 2024 • edited Loading

🟢 Turbopack Benchmark CI successful 🟢

github-actions bot commented Jul 26, 2024

github-actions bot commented Jul 26, 2024 • edited Loading

⚠️ CI failed ⚠️

bgw Jul 26, 2024

Choose a reason for hiding this comment

bgw Jul 26, 2024

Choose a reason for hiding this comment

bgw Jul 26, 2024

Choose a reason for hiding this comment

arlyon Jul 31, 2024

Choose a reason for hiding this comment

bgw Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

bgw Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

arlyon Jul 31, 2024

Choose a reason for hiding this comment

bgw commented Aug 2, 2024

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

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

bgw commented Jul 26, 2024 •

edited

Loading

vercel bot commented Jul 26, 2024 •

edited

Loading

bgw commented Jul 26, 2024 •

edited

Loading

github-actions bot commented Jul 26, 2024 •

edited

Loading

github-actions bot commented Jul 26, 2024 •

edited

Loading

bgw Jul 31, 2024 •

edited

Loading

bgw Jul 26, 2024 •

edited

Loading