Repository navigation
Conversation
The pass nested its types by value. When a closure returns a local, and the local is later assigned a value that holds the closure, the type of the local holds its last type several times. The type multiplied on each of the 10 passes of the fixpoint and each join cloned it. The nested types are now entries of a table with one entry per distinct type. A join of two entries that makes no ref id is computed once. The result of the pass is the same.
|
Updated 6:20 PM PT - Sep 19th, 2026
✅ @robobun, your commit 778aaa124b219b5f1e6ad882682b5e18ba4ef00c passed in 🧪 To try this PR locally: bunx bun-pr 43559That installs a local version of the PR into your bun-43559 --bun |
|
Status: ready for review. How I reproduced it:
The new test |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe validator now uses interned ref-access type IDs for joins, storage, propagation, and validation. Two bundler tests cover ref identity joins and recursive closure type propagation. ChangesRef access validation
Suggested reviewers: Priority: ⬆️ High 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the memo's equivalence to the old by-value joins: join(a, a) is an identity in every arm of join_ref_access_types/join_ref_access_ref_types (so the a == b shortcut is sound); the only next_ref_id calls reachable from inside a join go through interner.next_ref_id, so ref_ids_made cannot miss a fresh id; Structure.value is only ever interned as Ref/RefValue/Structure (the None/Guard/Nullable cases set RefAccessType::None instead), so interner.join on two values falls through to join_ref_access_ref_types as before; and ty_equal_key maps tyEqual nested entries to the same ty_equal_ids slot, so Env::set's changed-detection matches the removed custom PartialEq.
Extended reasoning...
The single posted finding is a pre-existing, partly-fixed performance bound (joins that mint a ref id are not memoized, so cycle + merged-refs inputs still grow), which the PR description already acknowledges. To add something concrete rather than restate it, I traced the four properties the correctness of the interner depends on: the self-join shortcut, the ref-id counter covering every id-creating site inside a join, the invariant that Structure.value entries are always ref-shaped so the nested interner.join reduces to the old join_ref_access_ref_types, and the ty_equal_key induction that replaces the old tyEqual PartialEq. None of these showed a behavioral divergence from the base. The change is a non-trivial rework of a fixpoint pass with a derived equality replacing hand-written tyEqual semantics, so a human look remains worthwhile, but I found nothing beyond the inline comment.
The table held each type twice: in a Vec, and as the key of a HashMap. An ArrayHashMap keeps its keys in insertion order, so the index of a key is the id of the type, and its value is the first tyEqual entry. An input that defeats the reuse of joins (a join that makes a ref id) now takes 57 MB. It took 111 MB before this commit and 99 MB on main.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I checked the core soundness questions of the interner on this push and found nothing wrong: the joins memo is only written when ref_ids_made did not move, so an id-making join (two different Refs, or Ref vs structure, at any nesting depth) is recomputed and makes fresh ids each pass exactly as before; Structure.value: Some(intern(value)) is equivalent to the old value.to_ref_type().map(Box::new) because the None | Guard | Nullable arm above already excludes the only cases where to_ref_type returns None; and the keys()[id] / values()[id] indexing in get and ty_equal_key cannot go out of range because every nested id is interned before the type that holds it. The timeout: 30_000 on the new spawn test was also examined and left alone since it is a hang guard paired with a signalCode assertion, not a wait.
Extended reasoning...
This run's inline finding is a naming/consistency nit (raw ArrayHashMap instead of the crate's IndexMap wrapper), which is not a reason to hold the PR on its own. The change itself is a non-trivial rewrite of a fixpoint analysis in the React Compiler port (type interning plus join memoization with a ref-id-creation escape hatch), so I did not approve; a human should still read the memo/ty_equal semantics against upstream. The points listed in the body are the concrete concerns I traced through the code and ruled out: memo write gating on ref_ids_made, the Structure.value equivalence given the enclosing match arms, the interner index invariant, and the spawn timeout that the ruled-out list already covered.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
This branch has not been deployed
1 participant
Problem
bun build --react-compilertakes 6 s for a 12 line component and 94 s for a 30 line one. The time is injoin_ref_access_typesand the derivedcloneofRefAccessType(src/react_compiler/validation/validate_no_ref_access_in_render.rs).letlocal, and the local is later assigned a value that holds the closure. Its type then holds its own previous type several times.Box). The type multiplies on each of the 10 passes, and every join clones it.Fix
RefAccessTypeInterner(anArrayHashMap), one per distinct type. The repro makes 30 entries.test/bundler/transpiler/react-compiler.test.ts(one times out on 1.4.3), that file, andreact-compiler-fixtures.test.ts. Self-reviewed: 9 concerns raised, 8 addressed (Notes).Background
ref.currentduring render. It runs at most 10 passes over a map from identifier toRefAccessType. If the map does not settle, the component stays as written, as upstream.Structuretype describes an object or a function:valuejoins what the object holds,fn_type.return_typeis what the function returns.if (ref.current == null)guard allows which write, so such a join is not reused.Notes
Repro (from the report, 6 s on 1.4.3,
bun build slow.jsx --react-compiler --target=browser --external '*'):Growth. Nodes in the largest type after each pass, counted in Babel, which has the same types: 7, 27, 91, 297, 959, 3,087, 9,927, 31,913, 102,583, 329,739, and 3.5 million nodes over the whole map. Structurally distinct nodes in the whole map after the same passes: 5, 10, 13, 16, 19, 22, 25, 28, 31, 34.
Timings (release builds, linux x64, CPU time of one
bun build, peak RSS in brackets):useCallbackremovedCompof the new testconst a1 = [ref]; const a2 = [a1]; ..., 1,000 statements, last one usedThe nested array row is the cubic case that the Notes of #43230 describe. The last row is the common case. The difference there is inside the noise of the machine (minimum 2.71 s against 2.44 s). The table allocates nothing until a function or an object with ref content is seen.
Scaling. The cycle of the new test with k objects in it, this PR: k = 2, 4, 8, 16, 32, 64, 128 take 21, 16, 19, 21, 23, 36, 97 ms. Main gives no result in 300 s for k = 2 (7.5 GB then).
Upstream. Before this PR the file was the same as
compiler/crates/react_compiler_validation/src/validate_no_ref_access_in_render.rsat the ported commit, except for imports. TypeScript upstream shares nested types by reference, butjoinRefAccessTypesandtyEqualstill walk them in full, so Babel grows the same way: 1.1 s and 17.7 s above, andJavaScript heap out of memoryonCompof the new test. Babel 1.0.0 does not reach this pass on the first repro, because it throwsThis value cannot be modified(a hook argument is reassigned) before it. The port records that error and continues, as newer upstream does. WithuseCallbackremoved, both reportRef type environment did not converge.Why the result is the same.
join(a, a)isain the old code too: each rule returns its left operand when the ids are equal. That is thea == bshortcut.next_ref_id()only for two refs with different ids, or for a ref and a structure. Every other join is a function of its two operands, so the table can return the earlier result. A join that made an id is computed again each time and makes new ids each time, as before. So the sequence ofnext_ref_id()calls is the same as before.tyEqualignores the id of aRefand of aRefValue. Each entry stores the first entry that istyEqualto it, found through a key in which nested indexes are replaced by those first entries. Two types aretyEqualif and only if their keys are equal..clone()on the nowCopytypes, the two join functions,RefAccessRefType).clippy::clone_on_copyis allowed for this module.How I checked it.
Debugoutput), the return type, thechangedflag, and the type of every identifier with ref ids renamed in order of first use. 2,781 functions ofreact-compiler-fixtures.test.ts(52 with ref errors), 25 ofreact-compiler.test.tsand 387 of generated components: no difference. The old side is too slow for 12 more generated components (over 120 s each on a debug build).Tests.
react-compiler ref validation does not multiply the type of a local that holds a closure over itself: on 1.4.3 the build needs more than 200 s, so the test fails by its timeout (or by the 30 s kill switch of the spawn when the test timeout is longer). With this PR it takes 0.3 s on a debug build.react-compiler/GuardOnARefFromAJoinOfTwoRefspasses on main and with this PR. It covers the rule that a join that makes a ref id is not reused: on a build that reuses every join,TwoRefscompiles and the test fails.Not fixed here. A join that makes a ref id is not reused, because the old code gives each such join its own ids, and a later guard can tell them apart (the second test). A component that has the cycle above and also merges two different refs inside it still grows, but each step is cheaper than before:
const w = () => () => [props.c ? r1 : r2]in one objectwone object further from the localconst a0 = [c ? r1 : r2]; const a1 = [a0]; ..., 800 statements (compiles)None of the 4,600 generated components is in this group. I did not add a budget that ends the pass early. A budget also stops inputs that settle after large joins (the last row compiles on main and here), so it changes results, and the repo rule is to not cap a count to hide growth. The growth comes from the rule that each such join makes new ids, which upstream has too. A table of such joins that is cleared at each pass would make a pass cheap. But two joins of one pair would then share their new ids inside a pass, which main does not do: the errors of a rejected component can differ, and the side by side check no longer holds. That can be a follow-up.
The table.
bun_collections::ArrayHashMap<RefAccessType, RefAccessTypeId>keeps its keys in insertion order. The index of a key is the id of the type, and the value is the firsttyEqualentry. It uses the global allocator, not the arena of the crate'sIndexMap, because an arena keeps every outgrown buffer of a table that grows. An entry costs about 65 bytes. A first version with aVecplus aHashMapcost about 150 bytes and took more memory than main on the first and last row.Self-review. Three reviewers read the diff for a change of result, for the rules of
DESIGN.mdandREVIEW.md, and for the tests. None found a change of result. Raised and addressed: the diff rewrote upstream text without need (.clone(), a loop indestructure, a derive), now restored.RefAccessTypeInterner::next_ref_idhad noNot in upstreammarker, and a re-port that calls the freenext_ref_id()there would silently reuse joins that make ids. No test covered that rule (second test added, checked against a build without the rule). The first test had no kill switch and did not assertsignalCode. The random components had no guard on a merged or extracted ref and nowhileloop (22 hand-written inputs added to the comparison). Numbers for the common case and for inputs that defeat the reuse were missing (tables above). Names and comments (ref_ids_made,ty_equal_first, the test name). Raised and not addressed: report the growth to upstream, whose Rust port and TypeScript have it too.Related. #42807 ends the pass when a nested fixpoint does not converge, and leaves out the growth inside the 10 passes of the top level. This PR is that case. The two PRs change adjacent hunks of the Rust file and both append tests to the same test file, so the second one to land needs a rebase. #43230 adds a stack check to the first lines of two functions of the file.
[human-review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 2
evidence per changed file