refactor(allocator, ast_tools): generate a single CloneIn traversal via a runtime flag#24422
Merged
Merged
Conversation
github-actions
Bot
added
A-ast
Merging this PR will not alter performance
Comparing 
Footnotes
|
overlookmotel
left a comment
overlookmotel
left a comment
Member
There was a problem hiding this comment.
Idea is good but implementation is slop! Comments below.
Boshen
changed the title
Boshen
changed the title
Boshen
changed the title
Member
Author
Merge activity
|
…ia a runtime flag (#24422) ## Summary The `CloneIn` derive generated **two near-identical traversals over the whole AST** — `clone_in` (resets semantic ids to their default) and `clone_in_with_semantic_ids` (preserves them). Almost every node carries an id field, so the two bodies diverge nearly everywhere and the pair roughly doubled the clone codegen. This threads *"preserve semantic ids?"* as a **runtime `bool`** through a **single** traversal: - **`CloneIn::clone_in_impl(&self, with_semantic_ids: bool, allocator)` is the one required method.** `clone_in` / `clone_in_with_semantic_ids` are `#[inline(always)]` default wrappers that call it with `false` / `true`. Hand-written impls provide only `clone_in_impl`. - The **derive emits a single `clone_in_impl` body per type** — no per-type `clone_in` / `clone_in_with_semantic_ids` wrappers. - Semantic ids live in `Cell<NodeId>` / `Cell<Option<{Scope,Symbol,Reference}Id>>`. **`Cell<T: Copy + Default>::clone_in_impl` threads the flag**: preserve (`Cell::new(self.get())`) when `with_semantic_ids`, otherwise reset to the default (`Cell::new(T::default())` = `NodeId::DUMMY` / `None`). So the derive emits a plain `clone_in_impl(&self.node_id, …)` per id field — no per-field `if/else`. - **`Cell::clone_in_impl` is `#[inline(never)]`** so the (for some consumers dead) preserve branch isn't inlined at every id-field site — see Results. ## Results Measured `f736d87e77` → `db9f2ab41a` on this branch (release, stripped, fat-LTO): | | before | after | Δ | |---|--:|--:|--:| | `oxc_ast` generated `CloneIn` (source lines) | 7,180 | 5,581 | **−1,599 (−22%)** | | oxlint binary | 14,192,816 B | 14,126,656 B | **−66,160 B (−64.6 KiB)** | | napi transform cdylib (`--features allocator`) | 3,633,312 B | 3,633,312 B | **0 (neutral)** | The two binaries move differently because they clone differently: - **oxlint / rolldown clone *with* semantic ids** (react compiler on `TSType`; whole-`Program` clones). They already carried chunks of both traversals; merging dedups them → **−64.6 KiB**. - **napi transform / parse clone *without* ids** — `oxc_transformer` never calls `clone_in_with_semantic_ids`, so before this PR `--gc-sections` dropped that whole half. The merged traversal would force the preserve branch live at every id field (~+16 KiB); `#[inline(never)]` on the `Cell` leaf outlines it to one function per `Cell<T>`, keeping these binaries **flat**. ## Behavior & perf Behavior is identical by construction — `clone_in` resets ids (`Cell::new(None)` / `Cell::new(DUMMY)`), `clone_in_with_semantic_ids` preserves them. The runtime flag is loop-invariant and well predicted; the `#[inline(never)]` leaf adds one outlined call per id-cell on the clone path (expected negligible — worth a benchmark spot-check). Generated files were produced by `just ast`. --- 🤖 AI-assisted (Claude Code), reviewed and measured by the author.
graphite-app
Bot
force-pushed
the
clone-in-single-traversal
branch
from
July 14, 2026 15:49
db9f2ab to
f297654
Compare
graphite-app Bot
pushed a commit
that referenced
this pull request
Jul 16, 2026
…4564) Follow-on after #24422. Make cloning semantic IDs (`NodeId`, `ScopeId` etc) use branchless arithmetic. * Cloning `NodeId` now just uses a single OR operation to take into account whether to preserve the existing ID (`clone_in_with_semantic_ids`) or substitute a dummy ID (`clone_in`). * Cloning `Cell<Option<ScopeId>>` is 2 instructions (NOT + AND) on x86_64, or just 1 on aarch64 (NOT-AND). https://godbolt.org/z/1KWdc5hnb The mechanism is a type `CloneInSemanticIds` which is used as `with_semantic_ids` instead of a `bool`: ```rs #[repr(u32)] pub enum CloneInSemanticIds { With = 0, Without = u32::MAX, } ``` This is 4 bytes, rather than `bool`'s 1 byte, but that makes no difference - either way it takes 1 register to pass between functions. The choice of discriminants is what enables the cheap arithmetic. This is even smaller in terms of binary size than the previous solution of marking `Cell::clone_in_impl` as `#[inline(never)]` (OR instruction is smaller than function call), and it's more performant as it loses the function call overhead. It also keeps `Cell`'s blanket implementation of `CloneIn` generalized. Between #24422 and this PR, merging `CloneIn` with/without semantic IDs into a single path has gained us a large binary size reduction, and lost us nothing - or may even have gained us a marginal perf improvement. No visible change on our benchmarks, as we don't use `CloneIn` much, but it may well be a measurable improvement for Rolldown, which clones whole ASTs.
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
2 participants
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.
Summary
The
CloneInderive generated two near-identical traversals over the whole AST —clone_in(resets semantic ids to their default) andclone_in_with_semantic_ids(preserves them). Almost every node carries an id field, so the two bodies diverge nearly everywhere and the pair roughly doubled the clone codegen.This threads "preserve semantic ids?" as a runtime
boolthrough a single traversal:CloneIn::clone_in_impl(&self, with_semantic_ids: bool, allocator)is the one required method.clone_in/clone_in_with_semantic_idsare#[inline(always)]default wrappers that call it withfalse/true. Hand-written impls provide onlyclone_in_impl.clone_in_implbody per type — no per-typeclone_in/clone_in_with_semantic_idswrappers.Cell<NodeId>/Cell<Option<{Scope,Symbol,Reference}Id>>.Cell<T: Copy + Default>::clone_in_implthreads the flag: preserve (Cell::new(self.get())) whenwith_semantic_ids, otherwise reset to the default (Cell::new(T::default())=NodeId::DUMMY/None). So the derive emits a plainclone_in_impl(&self.node_id, …)per id field — no per-fieldif/else.Cell::clone_in_implis#[inline(never)]so the (for some consumers dead) preserve branch isn't inlined at every id-field site — see Results.Results
Measured
f736d87e77→db9f2ab41aon this branch (release, stripped, fat-LTO):oxc_astgeneratedCloneIn(source lines)--features allocator)The two binaries move differently because they clone differently:
TSType; whole-Programclones). They already carried chunks of both traversals; merging dedups them → −64.6 KiB.oxc_transformernever callsclone_in_with_semantic_ids, so before this PR--gc-sectionsdropped that whole half. The merged traversal would force the preserve branch live at every id field (~+16 KiB);#[inline(never)]on theCellleaf outlines it to one function perCell<T>, keeping these binaries flat.Behavior & perf
Behavior is identical by construction —
clone_inresets ids (Cell::new(None)/Cell::new(DUMMY)),clone_in_with_semantic_idspreserves them. The runtime flag is loop-invariant and well predicted; the#[inline(never)]leaf adds one outlined call per id-cell on the clone path (expected negligible — worth a benchmark spot-check). Generated files were produced byjust ast.🤖 AI-assisted (Claude Code), reviewed and measured by the author.