Skip to content

Commit 10e70b6

Browse files
bgwbgw
authored
Turbopack: Implement TaskInput for ReadRef, use a ReadRef input for AssetIdent::new (#83356)
@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>
1 parent 295c516 commit 10e70b6

File tree

4 files changed

+148
-120
lines changed

4 files changed

+148
-120
lines changed

‎turbopack/crates/turbo-tasks-macros/src/value_macro.rs

Lines changed: 26 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,7 @@ impl Parse for ValueArguments {
207207
}
208208

209209
pub fn value(args: TokenStream, input: TokenStream) -> TokenStream {
210-
let mut item = parse_macro_input!(input as Item);
210+
let item = parse_macro_input!(input as Item);
211211
let ValueArguments {
212212
serialization_mode,
213213
into_mode,
@@ -217,13 +217,21 @@ pub fn value(args: TokenStream, input: TokenStream) -> TokenStream {
217217
operation,
218218
} = parse_macro_input!(args as ValueArguments);
219219

220+
let mut struct_attributes = vec![quote! {
221+
#[derive(
222+
turbo_tasks::ShrinkToFit,
223+
turbo_tasks::trace::TraceRawVcs,
224+
turbo_tasks::NonLocalValue,
225+
)]
226+
#[shrink_to_fit(crate = "turbo_tasks::macro_helpers::shrink_to_fit")]
227+
}];
228+
220229
let mut inner_type = None;
221230
if transparent {
222231
if let Item::Struct(ItemStruct {
223-
attrs,
224232
fields: Fields::Unnamed(FieldsUnnamed { unnamed, .. }),
225233
..
226-
}) = &mut item
234+
}) = &item
227235
&& unnamed.len() == 1
228236
{
229237
let field = unnamed.iter().next().unwrap();
@@ -251,7 +259,7 @@ pub fn value(args: TokenStream, input: TokenStream) -> TokenStream {
251259
[`{inner_type_string}`].",
252260
);
253261

254-
attrs.push(parse_quote! {
262+
struct_attributes.push(parse_quote! {
255263
#[doc = #doc_str]
256264
});
257265
}
@@ -343,22 +351,21 @@ pub fn value(args: TokenStream, input: TokenStream) -> TokenStream {
343351
quote! {}
344352
};
345353

346-
let mut struct_attributes = vec![quote! {
347-
#[derive(
348-
turbo_tasks::ShrinkToFit,
349-
turbo_tasks::trace::TraceRawVcs,
350-
turbo_tasks::NonLocalValue,
351-
)]
352-
#[shrink_to_fit(crate = "turbo_tasks::macro_helpers::shrink_to_fit")]
353-
}];
354354
match serialization_mode {
355-
SerializationMode::Auto => struct_attributes.push(quote! {
356-
#[derive(
357-
turbo_tasks::macro_helpers::serde::Serialize,
358-
turbo_tasks::macro_helpers::serde::Deserialize,
359-
)]
360-
#[serde(crate = "turbo_tasks::macro_helpers::serde")]
361-
}),
355+
SerializationMode::Auto => {
356+
struct_attributes.push(quote! {
357+
#[derive(
358+
turbo_tasks::macro_helpers::serde::Serialize,
359+
turbo_tasks::macro_helpers::serde::Deserialize,
360+
)]
361+
#[serde(crate = "turbo_tasks::macro_helpers::serde")]
362+
});
363+
if transparent {
364+
struct_attributes.push(quote! {
365+
#[serde(transparent)]
366+
});
367+
}
368+
}
362369
SerializationMode::None | SerializationMode::Custom => {}
363370
};
364371
if inner_type.is_some() {

‎turbopack/crates/turbo-tasks/src/read_ref.rs

Lines changed: 31 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
use std::{
2-
fmt::{Debug, Display},
3-
hash::Hash,
2+
cmp::Ordering,
3+
fmt::{self, Debug, Display},
4+
hash::{Hash, Hasher},
45
marker::PhantomData,
56
mem::transmute_copy,
7+
ops::Deref,
68
};
79

810
use serde::{Deserialize, Serialize};
@@ -31,7 +33,7 @@ impl<T> Clone for ReadRef<T> {
3133
}
3234
}
3335

34-
impl<T> std::ops::Deref for ReadRef<T>
36+
impl<T> Deref for ReadRef<T>
3537
where
3638
T: VcValueType,
3739
{
@@ -47,28 +49,26 @@ where
4749
T: VcValueType,
4850
VcReadTarget<T>: Display,
4951
{
50-
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
52+
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
5153
Display::fmt(&**self, f)
5254
}
5355
}
5456

5557
impl<T> Debug for ReadRef<T>
5658
where
57-
T: VcValueType,
58-
VcReadTarget<T>: Debug,
59+
T: Debug,
5960
{
60-
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
61-
Debug::fmt(&**self, f)
61+
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
62+
Self::as_raw_ref(self).fmt(f)
6263
}
6364
}
6465

6566
impl<T> TraceRawVcs for ReadRef<T>
6667
where
67-
T: VcValueType,
68-
VcReadTarget<T>: TraceRawVcs,
68+
T: TraceRawVcs,
6969
{
7070
fn trace_raw_vcs(&self, trace_context: &mut TraceRawVcsContext) {
71-
(**self).trace_raw_vcs(trace_context);
71+
Self::as_raw_ref(self).trace_raw_vcs(trace_context);
7272
}
7373
}
7474

@@ -85,48 +85,39 @@ where
8585

8686
impl<T> PartialEq for ReadRef<T>
8787
where
88-
T: VcValueType,
89-
VcReadTarget<T>: PartialEq,
88+
T: PartialEq,
9089
{
9190
fn eq(&self, other: &Self) -> bool {
92-
PartialEq::eq(&**self, &**other)
91+
Self::as_raw_ref(self).eq(Self::as_raw_ref(other))
9392
}
9493
}
9594

96-
impl<T> Eq for ReadRef<T>
97-
where
98-
T: VcValueType,
99-
VcReadTarget<T>: Eq,
100-
{
101-
}
95+
impl<T> Eq for ReadRef<T> where T: Eq {}
10296

10397
impl<T> PartialOrd for ReadRef<T>
10498
where
105-
T: VcValueType,
106-
VcReadTarget<T>: PartialOrd,
99+
T: PartialOrd,
107100
{
108-
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
109-
PartialOrd::partial_cmp(&**self, &**other)
101+
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
102+
Self::as_raw_ref(self).partial_cmp(Self::as_raw_ref(other))
110103
}
111104
}
112105

113106
impl<T> Ord for ReadRef<T>
114107
where
115-
T: VcValueType,
116-
VcReadTarget<T>: Ord,
108+
T: Ord,
117109
{
118-
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
119-
Ord::cmp(&**self, &**other)
110+
fn cmp(&self, other: &Self) -> Ordering {
111+
Self::as_raw_ref(self).cmp(Self::as_raw_ref(other))
120112
}
121113
}
122114

123115
impl<T> Hash for ReadRef<T>
124116
where
125-
T: VcValueType,
126-
VcReadTarget<T>: Hash,
117+
T: Hash,
127118
{
128-
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
129-
Hash::hash(&**self, state)
119+
fn hash<H: Hasher>(&self, state: &mut H) {
120+
Self::as_raw_ref(self).hash(state)
130121
}
131122
}
132123

@@ -198,14 +189,13 @@ where
198189

199190
impl<T> Serialize for ReadRef<T>
200191
where
201-
T: VcValueType,
202-
VcReadTarget<T>: Serialize,
192+
T: Serialize,
203193
{
204194
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
205195
where
206196
S: serde::Serializer,
207197
{
208-
(**self).serialize(serializer)
198+
Self::as_raw_ref(self).serialize(serializer)
209199
}
210200
}
211201

@@ -231,6 +221,12 @@ impl<T> ReadRef<T> {
231221
Self(arc)
232222
}
233223

224+
/// Returns the reference to `&T`, rather than `<<T as VcValueType>::Read as VcRead<T>>::Target`
225+
/// (the behavior of [`Deref`]).
226+
pub fn as_raw_ref(this: &ReadRef<T>) -> &T {
227+
&this.0
228+
}
229+
234230
pub fn ptr_eq(&self, other: &ReadRef<T>) -> bool {
235231
triomphe::Arc::ptr_eq(&self.0, &other.0)
236232
}

‎turbopack/crates/turbo-tasks/src/task/task_input.rs

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ use serde::{Deserialize, Serialize};
1313
use turbo_rcstr::RcStr;
1414

1515
use crate::{
16-
MagicAny, ResolvedVc, TaskId, TransientInstance, TransientValue, ValueTypeId, Vc,
16+
MagicAny, ReadRef, ResolvedVc, TaskId, TransientInstance, TransientValue, ValueTypeId, Vc,
1717
trace::TraceRawVcs,
1818
};
1919

@@ -112,6 +112,25 @@ where
112112
}
113113
}
114114

115+
impl<T> TaskInput for ReadRef<T>
116+
where
117+
T: TaskInput,
118+
{
119+
fn is_resolved(&self) -> bool {
120+
Self::as_raw_ref(self).is_resolved()
121+
}
122+
123+
fn is_transient(&self) -> bool {
124+
Self::as_raw_ref(self).is_transient()
125+
}
126+
127+
async fn resolve_input(&self) -> Result<Self> {
128+
Ok(ReadRef::new_owned(
129+
Box::pin(Self::as_raw_ref(self).resolve_input()).await?,
130+
))
131+
}
132+
}
133+
115134
impl<T> TaskInput for Option<T>
116135
where
117136
T: TaskInput,

0 commit comments

Comments
 (0)