next.js
0f38c522 - Turbopack: simplify asset ident constructors (#93213)

Commit
132 days ago
Turbopack: simplify asset ident constructors (#93213) ### What? Removes the per-method turbo-task constructors on `AssetIdent` (`from_path`, `with_query`, `with_fragment`, `with_modifier`, `with_part`, `with_path`, `with_layer`, `with_content_type`, `with_asset`, `rename_as`, and `path`). Each of those was its own cached task that returned a small projection or a one-field-changed copy. They are now plain Rust builder methods on the owned value, with a single `into_vc()` at the end of the chain that goes through the existing cached `new_inner` constructor. Call sites that previously chained `Vc` methods now look like: ```rust module .ident() .owned() .await? .with_modifier(rcstr!("async loader")) .into_vc() ``` ### Why? These constructors were tiny "projection" turbo-tasks that paid the cost of a task lookup, cell allocation, and dependency tracking but whose cache layer didn't meaningfully prevent recomputation. The trade-off is invalidation semantics: - **Before:** a caller doing `module.ident().path()` depended on the cached `path()` projection. If the source `AssetIdent` changed but its `.path` field was unchanged (e.g. a new modifier was added), `path()` re-ran, returned the same `FileSystemPath` cell, and the caller did not re-run. - **After:** the same caller does `module.ident().await?.path` and depends directly on the `AssetIdent` cell. Any change to the ident (modifier, query, layer, …) invalidates the caller, even if the path is unchanged. In practice this is rarely a real loss: when an ident changes, the `Module` typically changes too, and the dependent task was going to re-run anyway. `new_inner` already deduplicates structurally-equal idents, so the wrappers were paying overhead per call without buying meaningful invalidation isolation. Measured on a `vercel-site` build via `NEXT_TURBOPACK_TASK_STATISTICS` and `turbopack/scripts/analyze_cache_effectiveness.py`: | Task | canary (hits / misses) | this branch | | --------------------------------- | ---------------------- | ----------- | | `AssetIdent::path` | 778,273 / 98,018 | removed | | `AssetIdent::with_modifier` | 27,895 / 22,801 | removed | | `AssetIdent::from_path` | 2,954 / 29,650 | removed | | `AssetIdent::with_part` | 2 / 5,440 | removed | | `AssetIdent::with_layer` | 7 / 4,356 | removed | | `AssetIdent::rename_as` | 4,969 / 2,269 | removed | | `AssetIdent::with_query` | 0 / 521 | removed | | `AssetIdent::with_content_type` | 0 / 79 | removed | | `AssetIdent::new_inner` | 628 / 129,777 | 29,213 / 120,650 | Aggregate over the whole build: - Total cached tasks: 1,300 → 1,292 - Total task invocations: 39,361,186 → 38,208,036 (~1.15M fewer lookups) - Total cache misses: 6,812,198 → 6,639,937 (~172k fewer) - Overall hit rate: 82.7% → 82.6% (essentially unchanged) `new_inner` absorbs the construction work that used to be split across the wrappers. Four upstream tasks gained +519 cache hits each (`EsmAssetReference::resolve_reference`, `ReferencedAsset::from_resolve_result`, `NextServerUtilityModule::ident`, `NodeJsChunkingContext::chunk_item_id_strategy`); no task gained any new misses. ### How? - `AssetIdent::from_path` and the `with_*` methods are now plain `&mut self`/`self`-by-value builder methods on the struct itself, not `#[turbo_tasks::function]`s. - A new `AssetIdent::into_vc(self)` finalizes the builder by going through the still-cached `new_inner`. - `AssetIdent::path()` is removed; callers use `.path` on an owned `AssetIdent`. - All call sites across `turbopack-*` and `next-*` crates are updated. Most go from `ident.with_modifier(m)` (returning `Vc`) to `ident.owned().await?.with_modifier(m).into_vc()`. - A follow-up commit removes a few `.clone()`s introduced in the conversion that aren't needed once lifetimes are bound to a local. ### Follow-ups (out of scope) While migrating call sites, two pre-existing entry builders surfaced as candidates for cleanup. Not addressed here, but worth noting: - `get_app_page_entry` (`crates/next-core/src/next_app/app_page_entry.rs`) replaces the *content* of the source returned by `load_next_js_template` (prefixing imports onto `result.build()`) but reuses the template's `ident` with a `?page=...` query suffix as a disambiguator. The new `VirtualSource` ends up with content from one place and an ident chain pointing at another. A cleaner shape would be to mint a fresh ident from the page path, since the caller already knows what it's building. - `create_page_ssr_entry_module` (`crates/next-pages/page_entry.rs`) has the same shape on the instrumentation-conflict branch: it appends `export const register = hoist(...)` to the template content and constructs a `VirtualSource` with the original `source.ident()` unchanged. Lower-frequency than the app-page case (fires at most once per build), but the ident still misrepresents the constructed content. <!-- NEXT_JS_LLM_PR -->
Author
Parents
Loading