Skip to content

Commit

Permalink
chore(turbo-tasks): Remove support for local_cells (a specific mode o…
Browse files Browse the repository at this point in the history
…f local tasks we ended up not using) (#75672)

Local cells would allow us to avoid allocating real cells for data
passed to local task functions.

Unfortunately, in practice, we manually resolve most arguments passed to
local task functions, which would defeat this optimization.

I was holding onto this in the hopes that we might end up using it, but
that seems fleeting:
- We could get similar benefits by scanning for unreferenced cells at
the end of function execution using `TraceRawVcs` at the cost of a bit
of execution time. This would work even on `Vc`s that were resolved.
- For ergonomics reasons, we want to eventually rename `ResolvedVc` to
`Vc` (and `Vc` to something like `UnresolvedVc`), which will probably
lead to us resolving even more stuff, breaking this potential
optimization even more.
- Cache eviction may do a good enough job of removing cells, the bigger
enemy are the harder-to-evict tasks, which our current version of local
tasks does do a good job of reducing.

Meanwhile, it makes the code harder to follow, and could cause issues if
this codepath somehow got activated.
  • Loading branch information
bgw authored Feb 5, 2025
1 parent e8b41f6 commit a29bfa2
Show file tree
Hide file tree
Showing 20 changed files with 73 additions and 495 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,6 @@ impl UpdateOutputOperation {
cell,
})
}
Ok(Ok(RawVc::LocalCell(_, _))) => {
panic!("LocalCell must not be output of a task");
}
Ok(Ok(RawVc::LocalOutput(_, _))) => {
panic!("LocalOutput must not be output of a task");
}
Expand Down
1 change: 0 additions & 1 deletion turbopack/crates/turbo-tasks-backend/tests/local_cell.rs

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: unexpected token, expected one of: "fs", "network", "operation", "local", or "local_cells"
error: unexpected token, expected one of: "fs", "network", "operation", "local"
--> tests/function/fail_attribute_invalid_args.rs:9:25
|
9 | #[turbo_tasks::function(invalid_argument)]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: unexpected token, expected one of: "fs", "network", "operation", "local", or "local_cells"
error: unexpected token, expected one of: "fs", "network", "operation", "local"
--> tests/function/fail_attribute_invalid_args_inherent_impl.rs:14:29
|
14 | #[turbo_tasks::function(invalid_argument)]
Expand Down
24 changes: 4 additions & 20 deletions turbopack/crates/turbo-tasks-macros/src/func.rs
Original file line number Diff line number Diff line change
Expand Up @@ -756,19 +756,14 @@ pub struct FunctionArguments {
/// perform IO should not be manually annotated.
io_markers: FxHashSet<IoMarker>,
/// Should the function return an `OperationVc` instead of a `Vc`? Also ensures that all
/// arguments are `OperationValue`s. Mutually exclusive with the `local_cells` flag.
/// arguments are `OperationValue`s. Mutually exclusive with the `local` flag.
///
/// If there is an error due to this option being set, it should be reported to this span.
operation: Option<Span>,
/// Does not run the function as a real task, and instead runs it inside the parent task using
/// task-local state. The function call itself will not be cached, but cells will be created on
/// the parent task.
pub local: Option<Span>,
/// Changes the behavior of `Vc::cell` to create local cells that are not cached across task
/// executions. Cells can be converted to their non-local versions by calling `Vc::resolve`.
///
/// If there is an error due to this option being set, it should be reported to this span.
pub local_cells: Option<Span>,
}

impl Parse for FunctionArguments {
Expand Down Expand Up @@ -796,26 +791,19 @@ impl Parse for FunctionArguments {
("local", Meta::Path(_)) => {
parsed_args.local = Some(meta.span());
}
("local_cells", Meta::Path(_)) => {
parsed_args.local_cells = Some(meta.span());
}
(_, meta) => {
return Err(syn::Error::new_spanned(
meta,
"unexpected token, expected one of: \"fs\", \"network\", \"operation\", \
\"local\", or \"local_cells\"",
\"local\"",
))
}
}
}
if let (Some(_), Some(span)) = (
parsed_args.local.or(parsed_args.local_cells),
parsed_args.operation,
) {
if let (Some(_), Some(span)) = (parsed_args.local, parsed_args.operation) {
return Err(syn::Error::new(
span,
"\"operation\" is mutually exclusive with the \"local\" and \"local_cells\" \
options",
"\"operation\" is mutually exclusive with the \"local\" option",
));
}
Ok(parsed_args)
Expand Down Expand Up @@ -1101,7 +1089,6 @@ pub struct NativeFn {
pub is_method: bool,
pub filter_trait_call_args: Option<FilterTraitCallArgsTokens>,
pub local: bool,
pub local_cells: bool,
}

impl NativeFn {
Expand All @@ -1116,7 +1103,6 @@ impl NativeFn {
is_method,
filter_trait_call_args,
local,
local_cells,
} = self;

if *is_method {
Expand All @@ -1141,7 +1127,6 @@ impl NativeFn {
#function_path_string.to_owned(),
turbo_tasks::macro_helpers::FunctionMeta {
local: #local,
local_cells: #local_cells,
},
#arg_filter,
#function_path,
Expand All @@ -1156,7 +1141,6 @@ impl NativeFn {
#function_path_string.to_owned(),
turbo_tasks::macro_helpers::FunctionMeta {
local: #local,
local_cells: #local_cells,
},
#function_path,
)
Expand Down
2 changes: 0 additions & 2 deletions turbopack/crates/turbo-tasks-macros/src/function_macro.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ pub fn function(args: TokenStream, input: TokenStream) -> TokenStream {
.inspect_err(|err| errors.push(err.to_compile_error()))
.unwrap_or_default();
let local = args.local.is_some();
let local_cells = args.local_cells.is_some();

let Some(turbo_fn) = TurboFn::new(&sig, DefinitionContext::NakedFn, args) else {
return quote! {
Expand All @@ -61,7 +60,6 @@ pub fn function(args: TokenStream, input: TokenStream) -> TokenStream {
is_method: turbo_fn.is_method(),
filter_trait_call_args: None, // not a trait method
local,
local_cells,
};
let native_function_ident = get_native_function_ident(ident);
let native_function_ty = native_fn.ty();
Expand Down
4 changes: 0 additions & 4 deletions turbopack/crates/turbo-tasks-macros/src/value_impl_macro.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,6 @@ pub fn value_impl(args: TokenStream, input: TokenStream) -> TokenStream {
.inspect_err(|err| errors.push(err.to_compile_error()))
.unwrap_or_default();
let local = func_args.local.is_some();
let local_cells = func_args.local_cells.is_some();

let Some(turbo_fn) =
TurboFn::new(sig, DefinitionContext::ValueInherentImpl, func_args)
Expand All @@ -142,7 +141,6 @@ pub fn value_impl(args: TokenStream, input: TokenStream) -> TokenStream {
is_method: turbo_fn.is_method(),
filter_trait_call_args: None, // not a trait method
local,
local_cells,
};

let native_function_ident = get_inherent_impl_function_ident(ty_ident, ident);
Expand Down Expand Up @@ -225,7 +223,6 @@ pub fn value_impl(args: TokenStream, input: TokenStream) -> TokenStream {
.inspect_err(|err| errors.push(err.to_compile_error()))
.unwrap_or_default();
let local = func_args.local.is_some();
let local_cells = func_args.local_cells.is_some();

let Some(turbo_fn) =
TurboFn::new(sig, DefinitionContext::ValueTraitImpl, func_args)
Expand Down Expand Up @@ -255,7 +252,6 @@ pub fn value_impl(args: TokenStream, input: TokenStream) -> TokenStream {
is_method: turbo_fn.is_method(),
filter_trait_call_args: turbo_fn.filter_trait_call_args(),
local,
local_cells,
};

let native_function_ident =
Expand Down
10 changes: 0 additions & 10 deletions turbopack/crates/turbo-tasks-macros/src/value_macro.rs
Original file line number Diff line number Diff line change
Expand Up @@ -323,16 +323,6 @@ pub fn value(args: TokenStream, input: TokenStream) -> TokenStream {
let content = self;
turbo_tasks::ResolvedVc::cell_private(#cell_access_content)
}

/// Places a value in a task-local cell stored in the current task.
///
/// Task-local cells are stored in a task-local arena, and do not persist outside the
/// lifetime of the current task (including child tasks). Task-local cells can be resolved
/// to be converted into normal cells.
#cell_prefix fn local_cell(self) -> turbo_tasks::Vc<Self> {
let content = self;
turbo_tasks::Vc::local_cell_private(#cell_access_content)
}
};

let into = if let IntoMode::New | IntoMode::Shared = into_mode {
Expand Down
3 changes: 1 addition & 2 deletions turbopack/crates/turbo-tasks-macros/src/value_trait_macro.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,12 +121,11 @@ pub fn value_trait(args: TokenStream, input: TokenStream) -> TokenStream {
},
is_method: turbo_fn.is_method(),
filter_trait_call_args: turbo_fn.filter_trait_call_args(),
// `local` and `local_cells` are currently unsupported here because:
// `local` is currently unsupported here because:
// - The `#[turbo_tasks::function]` macro needs to be present for us to read this
// argument. (This could be fixed)
// - This only makes sense when a default implementation is present.
local: false,
local_cells: false,
};

let native_function_ident = get_trait_default_impl_function_ident(trait_ident, ident);
Expand Down
1 change: 0 additions & 1 deletion turbopack/crates/turbo-tasks-memory/tests/local_cell.rs

This file was deleted.

5 changes: 1 addition & 4 deletions turbopack/crates/turbo-tasks-testing/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use turbo_tasks::{
registry,
test_helpers::with_turbo_tasks_for_testing,
util::{SharedError, StaticOrArc},
CellId, ExecutionId, InvalidationReason, LocalTaskId, MagicAny, RawVc, ReadConsistency, TaskId,
CellId, InvalidationReason, LocalTaskId, MagicAny, RawVc, ReadConsistency, TaskId,
TaskPersistence, TraitTypeId, TurboTasksApi, TurboTasksCallApi,
};

Expand Down Expand Up @@ -57,11 +57,9 @@ impl VcStorage {
i
};
let task_id = TaskId::from(i as u32 + 1);
let execution_id = ExecutionId::from(i as u64 + 1);
handle.spawn(with_turbo_tasks_for_testing(
this.clone(),
task_id,
execution_id,
async move {
let result = AssertUnwindSafe(future).catch_unwind().await;

Expand Down Expand Up @@ -316,7 +314,6 @@ impl VcStorage {
..Default::default()
}),
TaskId::from(u32::MAX),
ExecutionId::from(u64::MAX),
f,
)
}
Expand Down
163 changes: 0 additions & 163 deletions turbopack/crates/turbo-tasks-testing/tests/local_cell.rs

This file was deleted.

3 changes: 0 additions & 3 deletions turbopack/crates/turbo-tasks-testing/tests/shrink_to_fit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,6 @@ async fn test_shrink_to_fit() -> Result<()> {
let a: Vc<Wrapper> = Vc::cell(Vec::with_capacity(100));
assert_eq!(a.await?.capacity(), 0);

let b: Vc<Wrapper> = Vc::local_cell(Vec::with_capacity(100));
assert_eq!(b.await?.capacity(), 0);

Ok(())
})
.await
Expand Down
Loading

0 comments on commit a29bfa2

Please sign in to comment.