Repository: vercel/next.js
Pull request: Turbopack: Implement `TaskInput` for `ReadRef`, use a `ReadRef` input for `AssetIdent::new`

@lukesandberg and I noticed this opportunity.

### Hypothesis

`AssetIdent::new` is a dummy constructor function used for caching value with unique `Vc`s. It is called frequently. The `AssetIdent` struct is large and contains three different `Vec`s, so it's likely non-trivial to clone.

- When an uncached turbo task function is called, its argument is boxed and cloned once to be used as cache key. There's two copies of the argument at this point: A cache key, and a local copy for the about-to-be-executed function.
- This function immediately constructs a cell, which moves its local copy of the argument into a `triomphe::Arc`, preserving it.

So we end up with two persistent copies of the task input. This doesn't matter when the struct is small and cheap to clone, but it does matter here.

The solution is to wrap `AssetIdent` in a `ReadRef` before passing it to a `turbo_tasks::function`. `ReadRef` does not impact equality (defers to inner value), it's cheap to clone, and `ReadRef::cell` provides a fast-path that re-uses the inner `triomphe::Arc` for the newly constructed cell:

https://github.com/vercel/next.js/blob/5cf762235a1e9f02443f45a69a7afd1880317f77/turbopack/crates/turbo-tasks/src/read_ref.rs#L247-L262

### Tradeoffs

If there's a cache hit, this approach constructs and throws away a temporary `Arc`, that the original implementation wouldn't. So in cases where there's a very high cache hit rate, or where the `Clone` would've been very cheap, this may come with a CPU time tradeoff. That doesn't seem to be the case here (cache hits are about 30%).

### Rough Benchmark

Does not save a measurable amount of memory

```
cargo run --release -p next-build-test -- generate ~/shadcn-ui/apps/v4/ > ~/shadcn-ui/apps/v4/project_options.json
cd ~/shadcn-ui/apps/v4/
pnpm i
command time -v ~/next.js/target/release/next-build-test run sequential 1 1 '/sink'
```

maxrss (kbytes) before (5 runs):
```
2355284
2325432
2320960
2281920
2385740
```

maxrss (kbytes) after (5 runs):
```
2361084
2323468
2364040
2353300
2388904
```

This shows it's slightly worse, but only 1%, so it's easily within margin-of-error:

<img src="https://app.graphite.dev/user-attachments/assets/a1a7e1c8-bc05-4262-91d8-c3590f5a1253.png" width=500>

Closes PACK-5393

Changed files:
filename: turbopack/crates/turbo-tasks-macros/src/value_macro.rs
patch: @@ -207,7 +207,7 @@ impl Parse for ValueArguments {
 }
 
 pub fn value(args: TokenStream, input: TokenStream) -> TokenStream {
-    let mut item = parse_macro_input!(input as Item);
+    let item = parse_macro_input!(input as Item);
     let ValueArguments {
         serialization_mode,
         into_mode,
@@ -217,13 +217,21 @@ pub fn value(args: TokenStream, input: TokenStream) -> TokenStream {
         operation,
     } = parse_macro_input!(args as ValueArguments);
 
+    let mut struct_attributes = vec![quote! {
+        #[derive(
+            turbo_tasks::ShrinkToFit,
+            turbo_tasks::trace::TraceRawVcs,
+            turbo_tasks::NonLocalValue,
+        )]
+        #[shrink_to_fit(crate = "turbo_tasks::macro_helpers::shrink_to_fit")]
+    }];
+
     let mut inner_type = None;
     if transparent {
         if let Item::Struct(ItemStruct {
-            attrs,
             fields: Fields::Unnamed(FieldsUnnamed { unnamed, .. }),
             ..
-        }) = &mut item
+        }) = &item
             && unnamed.len() == 1
         {
             let field = unnamed.iter().next().unwrap();
@@ -251,7 +259,7 @@ pub fn value(args: TokenStream, input: TokenStream) -> TokenStream {
                  [`{inner_type_string}`].",
             );
 
-            attrs.push(parse_quote! {
+            struct_attributes.push(parse_quote! {
                 #[doc = #doc_str]
             });
         }
@@ -343,22 +351,21 @@ pub fn value(args: TokenStream, input: TokenStream) -> TokenStream {
         quote! {}
     };
 
-    let mut struct_attributes = vec![quote! {
-        #[derive(
-            turbo_tasks::ShrinkToFit,
-            turbo_tasks::trace::TraceRawVcs,
-            turbo_tasks::NonLocalValue,
-        )]
-        #[shrink_to_fit(crate = "turbo_tasks::macro_helpers::shrink_to_fit")]
-    }];
     match serialization_mode {
-        SerializationMode::Auto => struct_attributes.push(quote! {
-            #[derive(
-                turbo_tasks::macro_helpers::serde::Serialize,
-                turbo_tasks::macro_helpers::serde::Deserialize,
-            )]
-            #[serde(crate = "turbo_tasks::macro_helpers::serde")]
-        }),
+        SerializationMode::Auto => {
+            struct_attributes.push(quote! {
+                #[derive(
+                    turbo_tasks::macro_helpers::serde::Serialize,
+                    turbo_tasks::macro_helpers::serde::Deserialize,
+                )]
+                #[serde(crate = "turbo_tasks::macro_helpers::serde")]
+            });
+            if transparent {
+                struct_attributes.push(quote! {
+                    #[serde(transparent)]
+                });
+            }
+        }
         SerializationMode::None | SerializationMode::Custom => {}
     };
     if inner_type.is_some() {

filename: turbopack/crates/turbo-tasks/src/read_ref.rs
patch: @@ -1,8 +1,10 @@
 use std::{
-    fmt::{Debug, Display},
-    hash::Hash,
+    cmp::Ordering,
+    fmt::{self, Debug, Display},
+    hash::{Hash, Hasher},
     marker::PhantomData,
     mem::transmute_copy,
+    ops::Deref,
 };
 
 use serde::{Deserialize, Serialize};
@@ -31,7 +33,7 @@ impl<T> Clone for ReadRef<T> {
     }
 }
 
-impl<T> std::ops::Deref for ReadRef<T>
+impl<T> Deref for ReadRef<T>
 where
     T: VcValueType,
 {
@@ -47,28 +49,26 @@ where
     T: VcValueType,
     VcReadTarget<T>: Display,
 {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         Display::fmt(&**self, f)
     }
 }
 
 impl<T> Debug for ReadRef<T>
 where
-    T: VcValueType,
-    VcReadTarget<T>: Debug,
+    T: Debug,
 {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        Debug::fmt(&**self, f)
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        Self::as_raw_ref(self).fmt(f)
     }
 }
 
 impl<T> TraceRawVcs for ReadRef<T>
 where
-    T: VcValueType,
-    VcReadTarget<T>: TraceRawVcs,
+    T: TraceRawVcs,
 {
     fn trace_raw_vcs(&self, trace_context: &mut TraceRawVcsContext) {
-        (**self).trace_raw_vcs(trace_context);
+        Self::as_raw_ref(self).trace_raw_vcs(trace_context);
     }
 }
 
@@ -85,48 +85,39 @@ where
 
 impl<T> PartialEq for ReadRef<T>
 where
-    T: VcValueType,
-    VcReadTarget<T>: PartialEq,
+    T: PartialEq,
 {
     fn eq(&self, other: &Self) -> bool {
-        PartialEq::eq(&**self, &**other)
+        Self::as_raw_ref(self).eq(Self::as_raw_ref(other))
     }
 }
 
-impl<T> Eq for ReadRef<T>
-where
-    T: VcValueType,
-    VcReadTarget<T>: Eq,
-{
-}
+impl<T> Eq for ReadRef<T> where T: Eq {}
 
 impl<T> PartialOrd for ReadRef<T>
 where
-    T: VcValueType,
-    VcReadTarget<T>: PartialOrd,
+    T: PartialOrd,
 {
-    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
-        PartialOrd::partial_cmp(&**self, &**other)
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Self::as_raw_ref(self).partial_cmp(Self::as_raw_ref(other))
     }
 }
 
 impl<T> Ord for ReadRef<T>
 where
-    T: VcValueType,
-    VcReadTarget<T>: Ord,
+    T: Ord,
 {
-    fn cmp(&self, other: &Self) -> std::cmp::Ordering {
-        Ord::cmp(&**self, &**other)
+    fn cmp(&self, other: &Self) -> Ordering {
+        Self::as_raw_ref(self).cmp(Self::as_raw_ref(other))
     }
 }
 
 impl<T> Hash for ReadRef<T>
 where
-    T: VcValueType,
-    VcReadTarget<T>: Hash,
+    T: Hash,
 {
-    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
-        Hash::hash(&**self, state)
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        Self::as_raw_ref(self).hash(state)
     }
 }
 
@@ -198,14 +189,13 @@ where
 
 impl<T> Serialize for ReadRef<T>
 where
-    T: VcValueType,
-    VcReadTarget<T>: Serialize,
+    T: Serialize,
 {
     fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
     where
         S: serde::Serializer,
     {
-        (**self).serialize(serializer)
+        Self::as_raw_ref(self).serialize(serializer)
     }
 }
 
@@ -231,6 +221,12 @@ impl<T> ReadRef<T> {
         Self(arc)
     }
 
+    /// Returns the reference to `&T`, rather than `<<T as VcValueType>::Read as VcRead<T>>::Target`
+    /// (the behavior of [`Deref`]).
+    pub fn as_raw_ref(this: &ReadRef<T>) -> &T {
+        &this.0
+    }
+
     pub fn ptr_eq(&self, other: &ReadRef<T>) -> bool {
         triomphe::Arc::ptr_eq(&self.0, &other.0)
     }

filename: turbopack/crates/turbo-tasks/src/task/task_input.rs
patch: @@ -13,7 +13,7 @@ use serde::{Deserialize, Serialize};
 use turbo_rcstr::RcStr;
 
 use crate::{
-    MagicAny, ResolvedVc, TaskId, TransientInstance, TransientValue, ValueTypeId, Vc,
+    MagicAny, ReadRef, ResolvedVc, TaskId, TransientInstance, TransientValue, ValueTypeId, Vc,
     trace::TraceRawVcs,
 };
 
@@ -112,6 +112,25 @@ where
     }
 }
 
+impl<T> TaskInput for ReadRef<T>
+where
+    T: TaskInput,
+{
+    fn is_resolved(&self) -> bool {
+        Self::as_raw_ref(self).is_resolved()
+    }
+
+    fn is_transient(&self) -> bool {
+        Self::as_raw_ref(self).is_transient()
+    }
+
+    async fn resolve_input(&self) -> Result<Self> {
+        Ok(ReadRef::new_owned(
+            Box::pin(Self::as_raw_ref(self).resolve_input()).await?,
+        ))
+    }
+}
+
 impl<T> TaskInput for Option<T>
 where
     T: TaskInput,

filename: turbopack/crates/turbopack-core/src/ident.rs
patch: @@ -5,7 +5,9 @@ use once_cell::sync::Lazy;
 use regex::Regex;
 use serde::{Deserialize, Serialize};
 use turbo_rcstr::RcStr;
-use turbo_tasks::{NonLocalValue, ResolvedVc, TaskInput, ValueToString, Vc, trace::TraceRawVcs};
+use turbo_tasks::{
+    NonLocalValue, ReadRef, ResolvedVc, TaskInput, ValueToString, Vc, trace::TraceRawVcs,
+};
 use turbo_tasks_fs::FileSystemPath;
 use turbo_tasks_hash::{DeterministicHash, Xxh3Hash64Hasher, encode_hex, hash_xxh3_hash64};
 
@@ -81,6 +83,10 @@ pub struct AssetIdent {
 }
 
 impl AssetIdent {
+    pub fn new(ident: AssetIdent) -> Vc<Self> {
+        AssetIdent::new_inner(ReadRef::new_owned(ident))
+    }
+
     pub fn add_modifier(&mut self, modifier: RcStr) {
         debug_assert!(!modifier.is_empty(), "modifiers cannot be empty.");
         self.modifiers.push(modifier);
@@ -98,72 +104,10 @@ impl AssetIdent {
     }
 }
 
-#[turbo_tasks::value_impl]
-impl ValueToString for AssetIdent {
-    #[turbo_tasks::function]
-    async fn to_string(&self) -> Result<Vc<RcStr>> {
-        let mut s = self.path.value_to_string().owned().await?.into_owned();
-
-        // The query string is either empty or non-empty starting with `?` so we can just concat
-        s.push_str(&self.query);
-        // ditto for fragment
-        s.push_str(&self.fragment);
-
-        if !self.assets.is_empty() {
-            s.push_str(" {");
-
-            for (i, (key, asset)) in self.assets.iter().enumerate() {
-                if i > 0 {
-                    s.push(',');
-                }
-
-                let asset_str = asset.to_string().await?;
-                write!(s, " {key} => {asset_str:?}")?;
-            }
-
-            s.push_str(" }");
-        }
-
-        if let Some(layer) = &self.layer {
-            write!(s, " [{}]", layer.name)?;
-        }
-
-        if !self.modifiers.is_empty() {
-            s.push_str(" (");
-
-            for (i, modifier) in self.modifiers.iter().enumerate() {
-                if i > 0 {
-                    s.push_str(", ");
-                }
-
-                s.push_str(modifier);
-            }
-
-            s.push(')');
-        }
-
-        if let Some(content_type) = &self.content_type {
-            write!(s, " <{content_type}>")?;
-        }
-
-        if !self.parts.is_empty() {
-            for part in self.parts.iter() {
-                if !matches!(part, ModulePart::Facade) {
-                    // facade is not included in ident as switching between facade and non-facade
-                    // shouldn't change the ident
-                    write!(s, " <{part}>")?;
-                }
-            }
-        }
-
-        Ok(Vc::cell(s.into()))
-    }
-}
-
 #[turbo_tasks::value_impl]
 impl AssetIdent {
     #[turbo_tasks::function]
-    pub fn new(ident: AssetIdent) -> Vc<Self> {
+    fn new_inner(ident: ReadRef<AssetIdent>) -> Vc<Self> {
         debug_assert!(
             ident.query.is_empty() || ident.query.starts_with("?"),
             "query should be empty or start with a `?`"
@@ -172,7 +116,7 @@ impl AssetIdent {
             ident.fragment.is_empty() || ident.fragment.starts_with("#"),
             "query should be empty or start with a `?`"
         );
-        ident.cell()
+        ReadRef::cell(ident)
     }
 
     /// Creates an [AssetIdent] from a [FileSystemPath]
@@ -427,6 +371,68 @@ impl AssetIdent {
     }
 }
 
+#[turbo_tasks::value_impl]
+impl ValueToString for AssetIdent {
+    #[turbo_tasks::function]
+    async fn to_string(&self) -> Result<Vc<RcStr>> {
+        let mut s = self.path.value_to_string().owned().await?.into_owned();
+
+        // The query string is either empty or non-empty starting with `?` so we can just concat
+        s.push_str(&self.query);
+        // ditto for fragment
+        s.push_str(&self.fragment);
+
+        if !self.assets.is_empty() {
+            s.push_str(" {");
+
+            for (i, (key, asset)) in self.assets.iter().enumerate() {
+                if i > 0 {
+                    s.push(',');
+                }
+
+                let asset_str = asset.to_string().await?;
+                write!(s, " {key} => {asset_str:?}")?;
+            }
+
+            s.push_str(" }");
+        }
+
+        if let Some(layer) = &self.layer {
+            write!(s, " [{}]", layer.name)?;
+        }
+
+        if !self.modifiers.is_empty() {
+            s.push_str(" (");
+
+            for (i, modifier) in self.modifiers.iter().enumerate() {
+                if i > 0 {
+                    s.push_str(", ");
+                }
+
+                s.push_str(modifier);
+            }
+
+            s.push(')');
+        }
+
+        if let Some(content_type) = &self.content_type {
+            write!(s, " <{content_type}>")?;
+        }
+
+        if !self.parts.is_empty() {
+            for part in self.parts.iter() {
+                if !matches!(part, ModulePart::Facade) {
+                    // facade is not included in ident as switching between facade and non-facade
+                    // shouldn't change the ident
+                    write!(s, " <{part}>")?;
+                }
+            }
+        }
+
+        Ok(Vc::cell(s.into()))
+    }
+}
+
 fn clean_separators(s: &str) -> String {
     static SEPARATOR_REGEX: Lazy<Regex> = Lazy::new(|| Regex::new(r"[/#?]").unwrap());
     SEPARATOR_REGEX.replace_all(s, "_").to_string()