Repository navigation
Conversation
mark_file_live_for_tree_shaking and mark_part_live_for_tree_shaking were mutually recursive, one frame per edge in the import graph. A linear chain of a few thousand modules is enough to overflow the stack under debug+ASAN, and release builds have the same bound at a larger depth. The pass only flips bits in files_live/parts_live and never reads a bit it just wrote to decide anything else, so processing order is irrelevant to the fixpoint. Replace the recursion with a single LIFO Vec on TreeShakeCtx: each step pushes its successors instead of calling, and the driver drains the queue. Successors already marked live are skipped before the push to keep the worklist bounded by the edge count. With the recursion gone the per-iteration index re-borrows (which only existed to satisfy borrowck across the &mut self calls) become plain iterators again.
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 10:23 PM PT - Jul 17th, 2026
❌ @robobun, your commit 41fadd1 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34541That installs a local version of the PR into your bun-34541 --bun |
|
whats the release build perf diff? |
There was a problem hiding this comment.
Beyond the inline finding on mark_file_reachable_for_code_splitting, I also checked: the now-private mark_part_live_step (formerly mark_part_live_for_tree_shaking) has no other callers; the debug_assert!(ctx.worklist.is_empty()) holds across the entry-point loop since each drain runs to completion; and the LIFO order change (deps before file in mark_part_live_step) is safe because the pass is monotonic bit-setting — the final files_live/parts_live fixpoint is order-independent.
Extended reasoning...
The worklist conversion itself looks correct — the pass only flips bits and re-checks the live bit at the top of each step, so duplicate worklist entries no-op on second pop and the fixpoint is unchanged. The iterator simplifications are sound now that there is no recursive &mut ctx call inside the loops. The one substantive gap is the sibling mark_file_reachable_for_code_splitting (called unconditionally right after this pass over the same graph) still self-recursing per edge, which the inline comment covers.
|
Release build, hyperfine (same base commit, only
So within noise. The liveness pass is a bitset walk over the module graph; on three.js×100 the whole pass is sub-millisecond and the wall clock is parse/print. The worklist adds one The more interesting number: on release, a 30000-module chain bundles on both; a 60000-module chain segfaults on both. With the tree-shake recursion gone the next overflow is |
…itting Runs unconditionally right after the tree-shaking pass over the same graph and self-recursed once per edge the same way. Only per-node state is the entry-point bit (monotone) and the min distance (relaxed on every shorter revisit), so the fixpoint is order-independent; drive it off a LIFO Vec<(source_index, distance)> on CodeSplitCtx.
There was a problem hiding this comment.
The mark_file_reachable_for_code_splitting sibling from my earlier comment is now converted (2fc45fe), and this pass found no further issues — deferring to a human on the fixpoint/order-independence reasoning since it's a ~200-line control-flow rewrite of the tree-shaking core.
What was reviewed:
- Fixpoint equivalence: both step functions still early-return on already-set
files_live/parts_livebits, so duplicate worklist pushes are harmless; no loop body reads state that a deferred (formerly-inline) recursive call would have written. distances[]relaxation in the code-splitting pass converges to the same minima under LIFO order via thetraverse_againguard.mark_part_live_for_tree_shakinghad no callers outside this file, so the rename to a privatemark_part_live_stepis safe.- The new
parts.iter()borrow is over&'r [PartList](immutable), so holding it acrossctx.worklist.pushis sound — the old index-reborrow contortions were only needed for the recursive&mut selfcall.
Extended reasoning...
Overview
Converts three mutually-/self-recursive graph walks in LinkerContext::tree_shaking_and_code_splitting (mark_file_live_for_tree_shaking, mark_part_live_for_tree_shaking, mark_file_reachable_for_code_splitting) to explicit LIFO worklists on the pass context structs, fixing a stack overflow on deep linear import chains. ~200 lines changed in src/bundler/LinkerContext.rs plus a 3000-module regression test in bundler_edgecase.test.ts. My earlier review flagged that the code-splitting pass was left recursive; that was addressed in 2fc45fe and the remaining recursive walks (ReachableFileVisitor::visit etc.) are explicitly scoped out with a stated reason (load-bearing DFS-postorder output).
Security risks
None. Pure bundler-internal graph traversal over already-parsed module metadata; no user-controlled input reaches new code paths, no allocation sized from untrusted data beyond what already existed.
Level of scrutiny
High. This is an algorithmic rewrite of the bundler's liveness and reachability passes — the phase that decides which code survives into the output. A silent divergence from the recursive semantics would manifest as incorrectly tree-shaken code or wrong chunk assignment, which the byte-identical-output verification on three.js and --splitting chains addresses but doesn't prove for all graph shapes. The order-independence argument (monotone bitsets; distance relaxation on shorter revisit) is sound as far as I can trace, but it's the kind of reasoning a maintainer should sign off on.
Other factors
Jarred-Sumner engaged asking about release-build perf and got hyperfine numbers (within noise). The bundler test suites listed in the description were run clean. The debug_assert!(ctx.worklist.is_empty()) at each public entry guards against a partially-drained worklist leaking across calls. The pre-push is_set filters bound the worklist by edge count rather than letting it grow with duplicate pushes, and the in-step is_set early-returns make any duplicates that do slip through (same target pushed twice before either is popped) harmless.
|
Opened #34554 which cherry-picks this tree-shaking worklist as its first commit and converts the remaining per-edge-recursive walks over the JS import graph ( |
|
CI build 74969: the bundler changes are green on every lane (277+ passed, including the new |
What does this PR do?
The three graph walks in
LinkerContext::tree_shaking_and_code_splittingwere recursive, one call frame per edge in the import graph. A linear chain of a few thousand modules is enough to overflow the stack:Repro (debug build, 3000-module chain where each imports the next):
Release builds survive longer chains but hit the same bound eventually.
Fix
mark_file_live_for_tree_shaking/mark_part_live_for_tree_shaking: the pass only flips bits infiles_live/parts_liveand never reads a bit it just wrote to decide anything else in the same step, so processing order is irrelevant to the fixpoint. Replace the mutual recursion with a single LIFOVec<TreeShakeWork>onTreeShakeCtx: each step pushes its successors instead of calling, and the public entry drains the queue. Successors already marked live are skipped before the push to keep the worklist bounded by the edge count.mark_file_reachable_for_code_splitting: runs unconditionally right after, over the same graph, and self-recursed the same way. Per-node state is the entry-point bit (monotone) and the min distance (relaxed on every shorter revisit), so the fixpoint is again order-independent. Same LIFO treatment with aVec<(source_index, distance)>onCodeSplitCtx.With the recursion gone the per-iteration index re-borrows (which only existed to satisfy borrowck across the recursive
&mut selfcalls) become plain iterators again.Scope
This covers the
tree_shaking_and_code_splittingphase. The bundler has several other per-edge-recursive graph walks (ReachableFileVisitor::visitinbundle_v2.rsis the next to overflow at ~7000 modules under debug+ASAN, thenFindImportedPartsVisitor::visit,validate_tla, etc.) that are left out of this PR because they build DFS-postorder lists whose output order is load-bearing, so they need an explicit-stack conversion rather than a flat worklist. Those are tracked separately.How did you verify your code works?
edgecase/DeepImportChainTreeShakinginbundler_edgecase.test.tsbundles a 3000-deep linear import chain and runs the output (prints3000). Crashes with the ASAN trace above on main, passes with this change.--splitting.LinkerContext.rsswapped): three.js x10 157ms vs 175ms, three.js x100--minify1.076s vs 1.073s, 10000-module chain 812ms vs 889ms. All within noise; the pass is a bitset walk that is sub-millisecond on three.js.bun bd testonbundler_edgecase,esbuild/dce,esbuild/default,esbuild/splitting,esbuild/importstar,esbuild/css,bundler_html,bundler_cjs,bundler_cjs2esm,bundler_barrel,bundler_splitting: 0 new failures.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bundler_edgecase.test.ts