Repository navigation
Conversation
…ions
declare_symbol links every re-declaration of a name that merges with
the previous one (`enum E {} enum E {}`, top-level `var x; var x;`) from
the old symbol to the new one, so n re-declarations form a chain of
length n. The parser followed that chain from its start once per
declaration without shortening it, in
generate_closure_for_type_script_namespace_or_enum (transpile) and in
to_ast's top-level-symbol-to-parts pass (bundle), so both were O(n^2):
720 KB of merged enum blocks took 14 s to transpile and 64k `var x`
re-declarations took 6 s to bundle.
Add bun_ast::symbol::follow_symbols, the flat-table counterpart of
Map::follow with the same path compression, and use it at every place
the parser walks a link chain.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .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 change adds iterative, path-compressing symbol lookup and replaces manual link traversal in parser and import-resolution paths. Regression tests cover long merged-declaration chains in transpiler and bundler flows. ChangesSymbol chain resolution
Suggested reviewers: Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The incremental change has no identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: ready for review. Reproduced on 1.4.2 and on a debug build of main with the ledger script: n merged The regression test ( CI: 179 of 181 jobs pass and the new test passes on every lane. The two red jobs do not touch this diff:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Use empty enum blocks and 16k declarations, take the asserted output from the timed runs, and sample once when a run is slow (debug builds). Both tests now finish in about 3 s on a debug build and no longer set a per-test timeout.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 7:31 PM PT - Sep 10th, 2026
❌ @robobun, your commit 010eb4d has 2 failures in
🧪 To try this PR locally: bunx bun-pr 42242That installs a local version of the PR into your bun-42242 --bun |
There was a problem hiding this comment.
LGTM — the per-test timeouts flagged earlier are gone, and I have nothing further.
What was reviewed: follow_symbols is textbook union-find path compression via Cell::replace (returns the old link, so the compression walk advances correctly; no-link input short-circuits both loops). All five replaced call sites preserve semantics — has_link() stays true after compression so the scan_imports guard still holds, and the relocated_top_level_vars empty-ref guard is kept. The ratio-vs-same-work-baseline test design with a spent-time-capped best-of avoids absolute thresholds and matches the perf-test guidance.
Extended reasoning...
Overview
The PR adds follow_symbols in src/ast/symbol.rs — a flat-table analogue of Map::follow that walks a symbol link chain to its root and path-compresses every visited node via Cell::replace. Five hand-rolled link-following loops in src/js_parser/p.rs (record_assignment, relocated top-level vars, enum/namespace closure emission, top-level-symbol-to-parts) and one in src/js_parser/scan/scan_imports.rs are replaced with calls to it. A new test at test/js/bun/transpiler/transpiler-redeclared-symbol-chain-hang.test.ts times 16,384 merged declarations against the same number of distinctly-named ones and asserts a bounded ratio plus the expected output shape.
Security risks
None. This is a pure algorithmic-complexity fix inside the parser's symbol table; no untrusted input handling, no allocation-size arithmetic, no FFI or filesystem surface changes. The interior mutability is via Cell<Ref> on a &[Symbol] slice, which is sound and already the pattern Map::follow uses.
Level of scrutiny
Moderate — parser hot path, but the change is small and mechanical. I verified: (1) the two-loop compression is correct, including the no-link case where root == ref_ and the second loop is skipped; (2) Cell::replace returns the prior value, so the compression walk traverses the original chain rather than re-reading the just-overwritten link; (3) each replaced call site returns the same root as before — in scan_imports the walk now starts from name_ref instead of symbol.link.get(), but has_link() is checked first so the root is identical, and compression leaves has_link() true; (4) the relocated_top_level_vars empty-ref guard is preserved as an if. This matches REVIEW.md's "fix complexity at the root" and "one implementation, in the right place" (it centralizes five copies next to Map::follow).
Other factors
The earlier inline nit (explicit 90 s per-test timeouts) was addressed — both tests now use the default timeout. The test follows harness conventions (tempDir, Buffer.alloc(n, fill) over .repeat), asserts output shape before/alongside the timing ratio, and uses a same-work baseline with a spent-time-capped best-of so debug+ASAN takes one sample while release takes several. The 3×/4× thresholds sit well below the stated 10–50× pre-fix ratio, which is the shape REVIEW.md asks for in perf tests.
There was a problem hiding this comment.
LGTM — the earlier per-test timeout nit has been addressed, and the rest looks correct.
What was reviewed:
follow_symbols: two-phase find-root-then-compress viaCell::replace; preserveshas_link()on every intermediate and is a no-op write whenref_is already the root — same contract as the existingMap::follow.- All five replaced call sites resolve to the same root as the old hand-rolled loops;
scan_imports.rsstill guards onhas_link()before following, and only.linkis mutated so the borrowedsymbol.original_nameread afterward is unaffected. - Grepped
src/js_parser/for remaining.link.get()/has_link()chain walks — the leftovers inparse_entry.rsare single-hop boolean checks, not walks, so the whole class in the parser is covered. - Tests: ratio vs. same-machine distinct-name baseline with best-of sampling, no absolute-time or sleep dependence,
tempDir/Buffer.allocper harness conventions, and output-shape assertions alongside the timing.
Extended reasoning...
Overview
This PR fixes an O(n²) hang in the JS/TS parser when a symbol is redeclared many times. It adds follow_symbols in src/ast/symbol.rs — a flat-table counterpart to the existing Map::follow that walks a symbol's link chain to its root and path-compresses every visited node's Cell<Ref> link to point directly at the root. Five hand-rolled chain-following loops across src/js_parser/p.rs (record_assignment, relocated-top-level-vars, enum/namespace closure generation, top-level-symbol-to-parts) and src/js_parser/scan/scan_imports.rs are replaced with calls to this helper. A new test file benches merged-enum transpile and repeated-var bundle against a same-count distinct-name baseline and asserts the ratio stays under 3–4×.
Security risks
None. This is a purely algorithmic complexity fix inside the parser's symbol-resolution machinery. No user-controlled input reaches new indexing paths that weren't already indexed identically by the old loops; inner_index() values are parser-generated. No FFI, allocation, or trust-boundary changes.
Level of scrutiny
Moderate. The parser is a hot path, but the change is small (17 net new lines of helper, five loop replacements) and mirrors an existing, well-understood pattern already in the same file (Map::follow does the identical two-phase path compression on the linker's nested table). I verified: (1) the compression preserves the has_link() invariant since every intermediate's link is rewritten to root, never to Ref::NONE; (2) when ref_ is already the root the second loop is skipped entirely; (3) Cell::replace returns the prior link so traversal advances correctly; (4) the scan_imports.rs site still holds a shared borrow of symbol across the call, but follow_symbols only takes &[Symbol] and mutates through Cell, so there is no aliasing conflict and original_name is untouched; (5) grepping src/js_parser/ for .link.get() / has_link() shows the only remaining hits are single-hop boolean predicates in parse_entry.rs, not chain walks — the "fix the whole class" requirement is met for the parser.
Other factors
My earlier inline nit (explicit 90 000 ms per-test timeouts violating test/CLAUDE.md) has been addressed — the current test file has no timeout arguments. The tests follow harness conventions (tempDir, Buffer.alloc(n, fill) instead of .repeat, ratio-vs-baseline with best-of sampling capped by wall time so debug builds take one sample) and assert output correctness (single var E;, correct closure count, export shape) alongside the performance ratio, so they aren't purely timing-dependent. The 3×/4× thresholds against a reported 10×–50× pre-fix ratio leave adequate headroom for ASAN/debug jitter. No outstanding third-party CHANGES_REQUESTED reviews are visible in the timeline.
) ### Problem - Bundling a file that declares one top-level enum n times (`enum E {A} enum E {B} ...`) costs O(n^2) memory. 8192 blocks (100 KB of source) reach 8.6 GB RSS in `Bun.build`. With 8192 different names: 70 MB. - The cause is `compute_ts_enums_map` (`src/js_parser/p.rs:9448`). `s_enum` pushes one ref per block to `top_level_enums`. Every ref maps to the one member map that all blocks share, and the function copies that map once per ref: n maps of n entries. `LinkerGraph::load` then clones each copy. ### Fix - `compute_ts_enums_map` skips a ref whose symbol has a link. Only the newest symbol of a merged enum has no link, so each enum gets one map. - Correct because both readers of `ts_enums` look up the newest symbol only. The printer calls `symbols.follow()` first. Linker step 5 uses the ref of the named export, which is the newest symbol for `export enum E` and for `export { E }`. - 8192 blocks now take 1.1 GB. The rest is not specific to enums (see Notes). - Verified: `test/bundler/bundler_merged_enum.test.ts` (stock bun: 220 MB, bound 100 MB). Also ran `test/bundler/esbuild/{ts,dce,importstar_ts,splitting}.test.ts`. ### Background - `ts_enums` maps an enum symbol to its constant members. The linker and the printer use it to replace `E.A` in another file with the value. - Each declaration of a name gets a symbol. For `enum` + `enum`, `declare_symbol` sets `older.link = newer`, and the scope keeps the newer symbol. Readers follow the links. - All blocks of a merged enum share one `TSNamespaceMemberMap`. <details><summary>Notes</summary> Release build, linux x64, `Bun.build` of n lines `enum E{M<i>}` plus `console.log(E)`. Time and RSS after the build: | n | main | this branch | n lines `var x = x \|\| <i>;` (main and branch) | | --- | --- | --- | --- | | 2048 | 420 ms, 640 MB | 207 ms, 137 MB | | | 4096 | 1678 ms, 2203 MB | 811 ms, 312 MB | 370 ms, 391 MB | | 8192 | 9205 ms, 8588 MB | 2332 ms, 1099 MB | 1705 ms, 1461 MB | - What remains is the part dependency fan-out in the linker. Every block is a part that declares `E` and uses `E`. Step 5 gives each of the n parts a dependency on all n parts, and tree shaking visits each of them. n merged `var x = x || i;` statements cost the same on main and on this branch (last column). esbuild has the same model. This PR does not change it. - #42242 makes the link chain walks in the parser linear for the same input. It does not touch `compute_ts_enums_map`. The two changes are independent. - The new test bundles 256 blocks of 32 members (51 KB). That makes the map copies large and the part fan-out small. Peak RSS over an empty process: 220 to 250 MB before and 15 MB after in release, 320 MB before and 50 MB after in a debug ASAN build. A debug ASAN build without the fix needs 16 s for this input, so there the test fails on the default timeout and not on the bound. - The bundle of the files of `ts/EnumCrossModuleInliningMergedDeclarations` is byte-identical before and after, with and without `--minify`. - A symbol of kind `TsEnum` only gets a link from a newer `enum` of the same name in the same scope (`Scope::can_merge_symbol_kinds`). A `namespace` after an `enum` keeps the enum symbol. Symbol hoisting only links hoisted kinds. So the end of each chain is a top-level enum and is in `top_level_enums`. - Self-review: checked every writer of `Symbol::link`, every `record_export` caller and both readers of `ts_enums`. No input makes a reader look up a linked symbol. If one did, step 5 would keep the enum object (larger output) and the printer would still inline the value, so it fails safe. With the condition inverted (keep only the linked symbols) `ts/EnumCrossModuleInliningMergedDeclarations` fails its DCE check, so that test is sensitive to which symbol keeps the map. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bundler/bundler_merged_enum.test.ts bun test v1.4.3 (4ff9193) test/bundler/bundler_merged_enum.test.ts: (pass) bundler > ts/EnumCrossModuleInliningMergedDeclarations [994.67ms] killed 1 dangling process (fail) bundler > merged enum blocks do not each get a copy of the shared member table [5006.20ms] ^ this test timed out after 5000ms. 1 pass 1 fail 2 expect() calls Ran 2 tests across 1 file. [9.15s] error: script "bd" exited with code 1 __F:1:S:0 release without fix: 1 FAILED bun test v1.4.3-canary.1 (1f89da512) test/bundler/bundler_merged_enum.test.ts: (pass) bundler > ts/EnumCrossModuleInliningMergedDeclarations [30.83ms] 113 | }), 114 | emptyProcessMaxRSS(), 115 | ]); 116 | // Peak RSS over an empty process for this 51 KB input. Before the fix: 117 | // 220 MB in release, 320 MB in a debug ASAN build. After: 50 MB in debug. 118 | expect((fixtureMaxRSS - baselineMaxRSS) / 1024 / 1024).toBeLessThan(isASAN || isDebug ? 150 : 100); ^ error: expect(received).toBeLessThan(expected) Expected: < 100 Received: 222.87890625 at <anonymous> (/workspace/bun/test/bundler/bundler_merged_enum.test.ts:118:60) (fail) bundler > merged enum blocks do not each get a copy of the shared member table [276.69ms] 1 pass 1 fail 7 expect() calls Ran 2 tests across 1 file. [444.00ms] __F:1:S:0 ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bundler/bundler_merged_enum.test.ts bun test v1.4.3 (4ff9193) test/bundler/bundler_merged_enum.test.ts: (pass) bundler > ts/EnumCrossModuleInliningMergedDeclarations [911.75ms] (pass) bundler > merged enum blocks do not each get a copy of the shared member table [1756.98ms] 2 pass 0 fail 7 expect() calls Ran 2 tests across 1 file. [5.70s] __F:0:S:0 release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 690ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/21] gen JS modules (bundle-modules) Preprocess modules (8120ms) Bundle modules (50ms) Postprocesss modules (166ms) Bundle Functions (502ms) Generate Code (44ms) [8.89s] Bundled "src/js" for production 2599 kb 197 internal modules 13 native modules 50 internal functions across 16 files [1/5] cargo bun_runtime → libbun_runtime.a �[1m�[92m Compiling�[0m bun_react_compiler v0.0.0 (/workspace/bun/src/react_compiler) �[1m�[92m Compiling�[0m bun_js_parser v0.0.0 (/workspace/bun/src/js_parser) �[1m�[92m Compiling�[0m bun_resolver v0.0.0 (/workspace/bun/src/resolver) �[1m�[92m Compiling�[0m bun_ini v0.0.0 (/workspace/bun/src/ini) �[1m�[92m Compiling�[0m bun_bundler v0.0.0 (/workspace/bun/src/bundler) �[1m�[92m Compiling�[0m bun_router v0.0.0 (/workspace/bun/src/router) �[1m�[92m Compiling�[0m bun_standalone_graph v0.0.0 (/workspace/bun/src/standalone_graph) �[1m�[92m Compiling�[0m bun_transpiler v0.0.0 (/workspace/bun/src/transpiler) �[1m�[92m Compiling�[0m bun_bunfig v0.0.0 (/worksp ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/js_parser/p.rs | 6 ++ test/bundler/bundler_merged_enum.test.ts | 120 +++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/js_parser/p.rs 8 3 14 test/bundler/bundler_merged_enum.test.ts 1 2 14 ``` </details> <!-- robobun:evidence:end -->
This branch has not been deployed
2 participants
Problem
enum E {…}declarations is O(n^2): 720 KB takes 14 s on 1.4.2 (62% of samples ingenerate_closure_for_type_script_namespace_or_enum). Bundling n top-levelvar xre-declarations has the same shape: 64k take 6.2 s.declare_symbol(src/js_parser/p.rs:5193) links each merging re-declaration from the old symbol to the new one, so n re-declarations form a chain of length n.p.rs:6919andto_ast(p.rs:9268) walk that chain from its start once per declaration and never shorten it.Fix
bun_ast::symbol::follow_symbols:symbol::Map::followfor the parser's flat table. It returns the chain end and points every symbol on the way at it (path compression). All five chain walks injs_parsernow call it.linkishas_link(), a walk to the end, or one single-hop compare that cannot change (Notes). New links are only set on chain ends.Bun.buildof merged enums stays O(n^2) for a separate reason (Notes).test/js/bun/transpiler/transpiler-redeclared-symbol-chain-hang.test.ts(time ratio against distinctly named declarations: 0.7 to 1.0 with the fix, 9x on debug main, 20x to 32x on 1.4.2). Alsotest/bundler/esbuild/ts.test.tsandtest/bundler/transpiler/transpiler.test.js. Self-reviewed: 5 concerns raised, all about what this description claims, 5 addressed.Background
Ref::inner_index. When two declarations of one name merge (var x; var x;,enum E {} enum E {}), the scope keeps the newest symbol and the older one getslink = newer. ARefto an older symbol resolves by following links to the end.enum+enumsince 0.14, so its enum chains stay short.Map::follow.Notes
generate_closure_for_type_script_namespace_or_enum, the top-level-symbol-to-parts pass into_ast,record_assignment, the relocated top-levelvarpass, and the duplicate-import check inscan_imports.transformSyncof mergedenum E{A_i}x 8192 / 16384 / 32768: 202 / 802 / 5223 ms before, 18 / 35 / 82 ms after (thenamespacecontrol is 13 / 27 / 58 ms).Bun.buildofvar x=1;x 16384 / 32768 / 65536: 389 / 1623 / 6215 ms before, 17 / 29 / 68 ms after. Thevarnumbers are for a re-declared name with one reference. The linker still does work per use times per declaring part, as esbuild does.varcase was found while checking the enum report. Transpiling it was already linear. Bundling was not, because onlyto_ast's bundle-mode pass walks the chain per part. A gdb sample of the release build put the time atp.rs:9268.Bun.buildof n merged top-level enums (8192 blocks take 8.3 s and 8.6 GB RSS before and after).compute_ts_enums_map(p.rs:9450) copies the shared member map once per declaration. That has its own follow-up.declare_symbolwithKind::Other, the HMR duplicate-namespace link,Map::merge) only set a link on a symbol that has none, or run behind a logged error, so compression cannot strand a path.substitute_single_use_symbol_in_expr,symbols[ident.ref].link == ref).refthere is a nestedlet/const, which never gets a link and is never a link target, so the compare cannot change.is_module_level_or_global, a bounds-checked read-only walk over the same parser table),pm_diff_normalize.rs(capped at 64 hops) andLinkerContext(CSS-only symbols).export var x=1;followed by 200000var x=2;lines and imported from another file killsBun.buildwith SIGSEGV (stack overflow in the recursiveMap::mergeon the long chain). With this change the chain is already short when the linker runs and the build takes 167 ms. The output for that shape is still wrong (export var x; var x;drops the declarations ofxfrom the bundle). That bug is older than this PR and is tracked separately.[human-review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
self-review · 16 surviving concerns
process.cpuUsage()ratio measured on the same runs never does; the fix is a 6-line change tobench()that I verified still fails on unfixed 1.4.2.flaky-newwarnings rather than red builds: about 0.1-0.7% per lane-run on Linux, roughly 3 a day fleet-wide and 1 every 1-2 weeks on main. All three false-fails I reproduced came frombench()'s wall-clockspent < 400budget, which is cheap to fix.{ var x=1; xN }andfunction f(x, y=[x=1, xN]){ var x; xN }…var x=1;x n input. None of the four conflict textually, so nothing forces…bun build, the PR's own headline input (merged enums) stays at about 9 s….link.get()walks in src/js_parser that merge cleanly besidefollow_symbols. One of them (Bun.Transpiler: make treeShaking remove unused declarations and their imports #38352) re-creates the quadratic on…follow_symbols, not in this…compute_ts_enums_maprebuilds the shared member map once per merged block (n x M), and only the chain root's entry is ever read. Land js_parser: compress symbol link chains when following merged declarations #42242 unchanged, say in the body that the…ScopeUses::sees(renamer.rs:1246-1251, from bundler: let a nested binding keep its name unless it would capture a reference #41286). That scan has shipped since 1.4.1, takes 60/60 profile samples after the fix, and costs about 5x the parser walk this PR removes. It is a separate ledger item against the rena….linksites, one of which is the only reader that depends on the immediate link. An enforcing lint is cheap and mu…RootRefnewtype: that is not one mechanical PR and would not make the class unrepresentable. The guard this repo uses is the helper plus a source-lint, and four open PRs that hand-roll the same walk show the lint is t…named_exports[alias].ref_leaves the parser as the exporting declaration's own non-root ref.top_level_symbols_to_partsis keyed by the chain root, at the exact line this PR rewrit…42 concerns were raised and did not survive verification.