[turbo-tasks]: Manually devirtualize our internal calling conventions#93910
Draft
lukesandberg wants to merge 12 commits into
Draft
[turbo-tasks]: Manually devirtualize our internal calling conventions#93910lukesandberg wants to merge 12 commits into
lukesandberg wants to merge 12 commits into
Conversation
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
Tests PassedCommit: 7fffcd9 |
lukesandberg
changed the title
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Additional Suggestions:
- Type mismatch in
eviction.rscauses undefined behavior: raw pointer cast toProdHandleConcrete(parameterized onProdBackingStorage = Either<TurboBackingStorage, NoopBackingStorage>) but the actualArcholdsTurboTasks<TurboTasksBackend<TurboBackingStorage>>— a different monomorphization.
- Missing
static_handlefeature onturbo-tasks-backenddev-dependency causes test panics withunreachable!()infrom_static_raw().
This crate is the future home of the #[no_mangle] pub extern "Rust"
fn providers that implement the dispatch surface declared (later in
this phase) in turbo-tasks/src/handle.rs.
For now it is a skeleton: lib.rs is empty (just a doc comment) and
the crate exposes two optional features:
- prod : pulls in turbo-tasks-backend (will emit __tt_prod_*
providers).
- test : pulls in turbo-tasks-testing (will emit __tt_test_*
providers).
Building with neither feature compiles to an empty lib, so 'cargo
check --workspace' works without flags. Binaries and tests that
actually consume the dispatch must enable the appropriate feature(s).
Adds the tagged-pointer dispatch infrastructure that replaces the
Arc<dyn TurboTasksApi> task-local. Only one method (`invalidate`) is on
the dispatch surface so far — subsequent commits will add the rest.
In turbo-tasks/src/handle.rs:
* TurboTasksHandle { tag: HandleTag, ptr: NonNull<()> } — the tagged
pointer that will replace Arc<dyn TurboTasksApi> in TURBO_TASKS.
* unsafe extern "Rust" forward declarations for __tt_prod_invalidate
/ __tt_test_invalidate.
* impl TurboTasksHandle { fn invalidate(...) { match self.tag { ... } } }
— direct dispatch over the tag; both arms are extern "Rust" calls
that thin LTO will inline cross-crate.
* Clone / Drop dispatch through __tt_<arm>_clone_arc / drop_arc so the
underlying Arc refcount stays correct.
* Uses macro_metavar_expr_concat (nightly, stable on our toolchain) to
auto-generate the per-arm symbol names from a single method list.
In turbo-tasks-handle/src/lib.rs:
* ProdHandleConcrete type alias matches what next-napi-bindings uses.
* #[no_mangle] pub extern "Rust" fn __tt_prod_invalidate + clone/drop
providers for the prod arm; mirror for the test arm (VcStorage).
* from_prod / from_test constructors that convert an Arc<...> into
a TurboTasksHandle by Arc::into_raw.
No call sites are switched yet; TURBO_TASKS still holds Arc<dyn
TurboTasksApi>. CI green for all feature combinations of
turbo-tasks-handle (none, prod, test, prod+test).
…d list
Adds the remaining ~21 methods of the TurboTasksApi + TurboTasksCallApi
trait surface to the tagged-pointer dispatch:
TurboTasksCallApi:
dynamic_call, native_call, trait_call, send_compilation_event,
get_task_name
TurboTasksApi:
invalidate (already wired), invalidate_with_reason,
invalidate_serialization, try_read_task_output, try_read_task_cell,
try_read_local_output, read_task_collectibles, emit_collectible,
unemit_collectible, unemit_collectibles, try_read_own_task_cell,
read_own_task_cell, update_own_task_cell, mark_own_task_as_finished,
connect_task, spawn_detached_for_testing,
subscribe_to_compilation_events, is_tracking_dependencies
The following remain OFF the dispatch surface, per the plan:
run, run_once, run_once_with_reason, start_once_process,
stop_and_wait, task_statistics
Every caller of these already has a concrete TurboTasks<B> in scope
(via direct construction in #[tokio::test] / benches / fuzz / the napi
top-level run), so they don't need extern "Rust" dispatch.
Also switches the provider macro from UFCS `<Concrete as
TurboTasksApi>::` to method-call syntax `tt.`, which
resolves both supertrait (TurboTasksCallApi) and subtrait
(TurboTasksApi) methods uniformly.
Cleans up Arc clone/drop to use Arc::increment_strong_count /
decrement_strong_count instead of the from_raw -> clone -> forget
dance. Cleaner and avoids the sketchy debug_assert on pointer
identity.
All four feature combinations of turbo-tasks-handle compile:
none, prod, test, prod+test. Workspace check passes.
The thread-local will soon hold a TurboTasksHandle instead of an
Arc<dyn TurboTasksApi>, so turbo_tasks_weak() needs a parallel
TurboTasksWeakHandle type — the existing Arc::downgrade(arc) path
breaks once the task-local stops holding an Arc.
Adds:
* pub struct TurboTasksWeakHandle { tag, ptr } — mirrors
TurboTasksHandle but holds a Weak<concrete>::into_raw() pointer.
* TurboTasksHandle::downgrade(&self) -> TurboTasksWeakHandle.
* TurboTasksWeakHandle::upgrade(&self) -> Option<TurboTasksHandle>.
* Per-arm extern "Rust" symbols: __tt_<arm>_downgrade, _upgrade,
_clone_weak, _drop_weak. Providers in turbo-tasks-handle round-
trip through Weak::from_raw/Weak::into_raw and Arc::downgrade /
Weak::upgrade.
The plan suggested deferring weak-handle work to a later phase if
benches showed scope-spawning didn't matter. After auditing the
weak-handle consumers (turbo-tasks-fs's filesystem watcher, four
sites — all real and load-bearing), it's cleaner to build the weak
handle now alongside the strong handle than to leave a half-Weak<dyn>
hybrid behind.
No call sites switched yet.
…TurboTasksHandle (WIP)
This is the main flip. Workspace 'cargo check' is green; building test
binaries hits a linkage issue (see end of message).
Core change:
* task_local! { TURBO_TASKS: TurboTasksHandle } in manager.rs (was
Arc<dyn TurboTasksApi>).
* Public accessors turbo_tasks(), try_turbo_tasks(), with_turbo_tasks,
turbo_tasks_weak(), turbo_tasks_scope, turbo_tasks_future_scope,
with_turbo_tasks_for_testing, and the free run/run_once/
run_once_with_reason helpers now return/take TurboTasksHandle.
* TurboTasksWeakHandle parallels TurboTasksHandle for the rare weak
case (used by the fs watcher).
Method dispatch surface expanded:
* run, run_once, run_once_with_reason, start_once_process,
stop_and_wait, task_statistics are now also on the dispatch surface
— the test harness in turbo-tasks-testing type-erases its
TestInstance.tt and calls turbo_tasks::run_once() on it, so 'off
the dispatch surface' didn't hold for these methods after all.
* task_statistics returns &TaskStatisticsApi; the extern provider
returns *const TaskStatisticsApi and the handle wrapper re-binds
the lifetime to &self.
* provide_prod_trait! / provide_test_trait! variants force UFCS for
methods whose inherent signature on TurboTasks<B> shadows the
trait method's signature (the run/stop family).
Internal API signature changes:
* Invalidator::invalidate, invalidate_with_reason now take
&TurboTasksHandle (was &dyn TurboTasksApi).
* read_task_output, read_local_output internal helpers take
&TurboTasksHandle. wait_task_completion's own loop is inlined.
* TurboTaskContextError.turbo_tasks field changed from
Arc<dyn TurboTasksCallApi> to TurboTasksHandle.
* Backend operation::Context::turbo_tasks() returns TurboTasksHandle
(uses thread-local).
Construction sites:
* TurboTasks::make_handle(self: Arc<Self>) -> TurboTasksHandle
inherent method on TurboTasks<B> and on VcStorage.
* make_handle_from_ref(&self) for ergonomic use inside TurboTasks's
own methods that need to scope into themselves.
Downstream updates:
* turbo-tasks-fs DiskFileSystemInner.turbo_tasks Weak<dyn...> →
TurboTasksWeakHandle.
* turbo-tasks-fs watcher signatures changed.
* turbopack-dev-server, turbopack-cli, turbopack-cli's serve()
signature take TurboTasksHandle; UpdateServer::run too.
* turbo-tasks-testing TestInstance.tt and helpers use handles.
Cargo deps:
* next-napi-bindings dev-deps turbo-tasks-handle [features = 'prod'].
* turbo-tasks-backend dev-deps turbo-tasks-handle [features =
'prod', 'test'] for tests + benches.
** KNOWN ISSUE — TEST BUILD LINKAGE **
Test binaries fail to link because cargo doesn't pull
'libturbo_tasks_handle.rlib' into the test binary's link command even
though it's declared as a dev-dep. Investigation needed:
- Likely cause: cargo skips a dev-dep that's never referenced by name
in the test sources.
- Quick workaround: add 'extern crate turbo_tasks_handle;' to each
test file that lives in turbo-tasks-backend/tests/. ~30 files.
- Better solution: probably move providers to a separate
turbo-tasks-prod-handle / turbo-tasks-test-handle pair of crates
that consumers reference explicitly, or use a build-script trick
to force linkage.
The cdylib (next-napi-bindings) does link correctly because cdylib
linking includes all declared deps.
…sting The turbo-tasks-handle crate added in phase 1 created an awkward cycle: it needed turbo-tasks-backend (for ProdHandleConcrete) and turbo-tasks-testing (for VcStorage) as feature-gated deps, which made adding it as a dep of turbo-tasks-backend or turbo-tasks-testing impossible (cycle). The result was per-binary opt-in via dev-deps, which doesn't compose: every test/bench/example crate would need to explicitly enable features on a separate handle crate. This commit collapses turbo-tasks-handle out of existence: * The __tt_prod_* providers move into turbo-tasks-backend/src/ handle_providers.rs. turbo-tasks-backend already owns TurboTasksBackend and the prod handle concrete type. * The __tt_test_* providers move into turbo-tasks-testing/src/ handle_providers.rs. turbo-tasks-testing already owns VcStorage. * The dispatch contract (TurboTasksHandle, HandleTag, forward decls, forwarder macros) stays in turbo-tasks/src/handle.rs. * Two new Cargo features on turbo-tasks: prod_handle, test_handle. Each gates its arm's HandleTag variant, extern decls, and match arms. Single-variant builds (only prod or only test) collapse to a direct call; both-variant builds get cmp + b.ne + direct call. * turbo-tasks-backend deps on turbo-tasks [prod_handle]. turbo-tasks-testing deps on turbo-tasks [test_handle]. * TurboTasks::make_handle is gated behind feature = 'prod_handle'; VcStorage::make_handle is in turbo-tasks-testing so unconditional. Test binaries linking the providers still has a wrinkle: when a crate's test binary unifies test_handle on via dev-deps but the test code doesn't 'use turbo_tasks_testing', cargo doesn't pull libturbo_tasks_testing.rlib into the link command. The fix is 'extern crate turbo_tasks_testing;' in the affected files. So far applied to: - turbo-tasks-backend/src/lib.rs (#[cfg(test)]) - turbo-tasks-backend/tests/eviction.rs - turbo-tasks-backend/benches/mod.rs Still pending: a handful of other workspace crates that build tests without using turbo_tasks_testing directly. Diagnosed but not yet fixed (turbopack-cli, turbopack-tracing, etc.).
The retry/retry_async functions are generic Future utilities with no dependency on turbo-tasks. They previously lived in turbo-tasks-testing where they pulled the rest of that crate (and its test_handle feature activation) into anything that transitively needed retry. Specifically, turbopack-cli dev-depped turbopack-bench which depped turbo-tasks-testing solely for retry(). That dependency triggered turbo-tasks feature unification that enabled test_handle, generating extern decls for __tt_test_* in turbopack-cli's lib test binary, which then failed to link because cargo doesn't pull the unused turbo-tasks-testing rlib into the binary's link command. Moving the two retry helpers to turbopack-bench (their sole user) breaks that chain. turbopack-cli's lib tests now build cleanly.
When 'turbo-tasks' is built standalone with neither 'prod_handle' nor 'test_handle' active, 'HandleTag' previously became a zero-variant enum which is invalid under '#[repr(u8)]' — and downstream consumers (e.g. 'turbo-tasks-bytes') that only declare value types would fail to compile because TurboTasks::make_handle requires 'HandleTag::Prod'. This commit: - Adds 'HandleTag::Unreachable = 255', present only when neither feature is active. The variant is never constructed by consumer code; it just keeps the enum inhabited. - Adds '_ => unreachable!()' arms to every dispatch 'match' (TurboTasksHandle's forwarder macro, task_statistics, Clone, Drop, downgrade, and the same trio for TurboTasksWeakHandle), feature-gated to only exist when no real arm exists. - Ungates TurboTasks::make_handle / make_handle_from_ref so they always exist; in the no-features build they construct a handle with the Unreachable tag (any dispatch through it panics at runtime, but the binary builds). - Adds 'compile_error!' if a build activates 'test_handle' but not 'prod_handle' AND tries to call TurboTasks::make_handle — that combination is structurally inconsistent. The workspace 'cargo check' is green. 'cargo build --workspace --tests --benches' still has linker errors in crates that don't directly dep on turbo-tasks-backend or -testing but get their features unified on by other workspace members. Fixing those needs 'extern crate' anchors in the affected crates (next commit).
Renames the dispatch arms by mechanism (Static/Dynamic) rather than by intended user (Prod/Test). VcStorage from turbo-tasks-testing uses the Dynamic arm because we don't want to wire its concrete type into the static dispatch surface, not because dynamic dispatch is inherently a "test" concept. Also restructures features so static_handle is explicit opt-in (activated by turbo-tasks-backend's static_handle feature, which the napi binding enables) rather than being feature-unified workspace-wide. This avoids the linker error where test binaries that don't directly link turbo-tasks-backend would otherwise see unresolved __tt_static_* extern symbols. Drops the unused read_task_output free function.
The `__tt_static_*` providers in turbo-tasks-backend cast the opaque `*const ()` receiver to `&TurboTasks<TurboTasksBackend<Either<TurboBackingStorage, NoopBackingStorage>>>`. Tests and benches were constructing `TurboTasks::new(TurboTasksBackend::new(_, TurboBackingStorage))` (no Either wrapper), so the resulting `Arc<TurboTasks<_>>` had a different concrete type than what the providers expected — reinterpreting the pointer was undefined behavior and segfaulted under optimization. Adds `ProdBackingStorage` (the Either-wrapped type) and small `prod_backing_storage_turbo` / `prod_backing_storage_noop` helpers in turbo-tasks-backend, then updates every test_config.trs and the three bench harnesses to use them. Also gates the backend's own tests/benches to activate the `static_handle` feature on themselves via a dev-dep self-reference, so `TurboTasksHandle::from_static_raw` is wired up to the actual extern providers instead of the no-feature unreachable! stub.
…nd self-dev-dep `turbo_tasks()` returns a `TurboTasksHandle` by value, not a smart pointer that can be deref'd. The new caller in client.rs needs `&turbo_tasks()` not `&*turbo_tasks()` — bind to a local first so the temporary's drop order is explicit. Also reverts the self-dev-dep on turbo-tasks-backend that activated `static_handle` for its own tests/benches. That activation propagated across the workspace via Cargo feature unification and broke test bins that don't directly link `turbo-tasks-backend` (unresolved `__tt_static_*` extern symbols). Backend tests now panic at runtime without `static_handle`; will reactivate via per-crate dev-deps in a follow-up so only test bins that actually link the backend turn it on.
lukesandberg
force-pushed
the
devirtualize_by_hand_since_rustc_cannot_do_it
branch
from
2edc638 to
f02ec68
Compare
Contributor
Stats from current PR✅ No significant changes detected📊 All Metrics📖 Metrics GlossaryDev Server Metrics:
Build Metrics:
Change Thresholds:
⚡ Dev Server
📦 Dev Server (Webpack) (Legacy)📦 Dev Server (Webpack)
⚡ Production Builds
📦 Production Builds (Webpack) (Legacy)📦 Production Builds (Webpack)
📦 Bundle SizesBundle Sizes⚡ TurbopackClient Main Bundles
Server Middleware
Build DetailsBuild Manifests
📦 WebpackClient Main Bundles
Polyfills
Pages
Server Edge SSR
Middleware
Build DetailsBuild Manifests
Build Cache
🔄 Shared (bundler-independent)Runtimes
📎 Tarball URLCommit: f02ec68 |
Resolves the bench-job panic without forcing `static_handle` on workspace-wide (which would spread `__tt_static_*` extern decls into test bins that don't link `turbo-tasks-backend`). When `static_handle` is on (napi binding), `make_handle` returns the Static handle that dispatches through `extern "Rust"` symbols — devirtualized under thin LTO, as before. When `static_handle` is off but `dynamic_handle` is on (workspace tests/benches, which transitively pull in `turbo-tasks-testing`), `make_handle` upcasts the `Arc<TurboTasks<B>>` to `Arc<dyn TurboTasksApi>` and returns a Dynamic handle. One vtable indirection per dispatched call — slower than static, but correct, and only the test/bench path pays it. The third arm (neither feature on) is a runtime panic stub so the generic `impl<B> TurboTasks<B>` block still compiles for crates that only depend on `turbo-tasks` for types.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.