Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughChangesChunk splitting
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change prevents empty code-splitting chunks for re-export-only files while preserving emitted code for wrapped or side-effectful modules. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, implementation, expected behavior, edge cases, and verification results. It does not use the exact template headings, but it provides the required information and is substantially complete. Comment |
|
Updated 12:53 AM PT - Aug 28th, 2026
❌ @robobun, your commit 469d433 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 40595That installs a local version of the PR into your bun-40595 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Status: ready for review. Reproduced with the build from the report ( Current shape (2ed8679): tree shaking sets a per-file bit when a live part prints code. CI on 469d433 (build 107469): every test lane that ran is green for this diff. The one red job is darwin aarch64, where the step failed outside the test runner (agent setup, also failing on main). The other entries are flakes that passed on retry. No bundler test fails. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
do not loop through every statement in every file to answer if it prints code. do something smarter.
|
Reworked in 8dabdd3: no scan at chunk time anymore. Tree shaking already visits each part exactly once when it becomes live (
|
…thing
With --splitting, a file whose only live parts are re-exports of unwrapped
bundled files (export { x } from "./impl.js", export * from, export * as,
or an import plus export {}) still got its own chunk when its set of entry
points was unique. Those statements print nothing: the importers bind to
the re-exported file directly. The chunk held only the banner, and nothing
imported it.
compute_chunks now counts a live part as contributing code only when one
of its statements survives convert_stmts_for_chunk: imports and re-exports
of unwrapped bundled files, unused import records and export clauses are
dropped at print time. The wrapper part of a wrapped file counts, since it
prints the __esm / __commonJS closure.
… chunk Drop the pass over every reachable file's parts. compute_chunks now asks file_prints_code only for a file whose chunk key has no chunk yet; a file whose key already has one joins it as before. The check walks the file's live parts through the bitset iterator and stops at the first part that prints.
Instead of scanning a file's parts when it would start a chunk, set a per-file bit in mark_part_live_step as each part becomes live: one tag check on the part's single statement, skipped once the file's bit is set, and only with code splitting. compute_chunks reads the bit and otherwise goes back to the #40581 flow: a file without it joins no chunk.
1c161ef to
9ff5baf
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request has now been reviewed several times and this review found new issues. Before patching these one by one, step back: would one root-cause fix close several of them? Is the pull request's scope growing with each push? Prefer root-cause fixes, keep scope fixed, and note out-of-scope improvements as follow-ups.
…prints The walk that orders a chunk's parts and imports starts from the chunk's files, so a file whose live parts print nothing must still join the chunk: an import() target that only re-exports otherwise loads its targets in chunk-index order instead of source order, and inside a shared chunk the files it re-exports could print in a different order. compute_chunks therefore puts every live file in its chunk again and records per chunk whether any file prints code; a code-splitting chunk where none does is skipped when the chunks are sorted, before any index is handed out.
This branch has not been deployed
2 participants
Problem
--splitting, a file whose only live parts are re-exports of unwrapped bundled files (export { x } from "./impl.js",export * from,export * as ns from, or an import plusexport {}) still got its own chunk when its set of entry points was unique. The chunk held only// @bunand the banner, and nothing imported it. A large split build in the wild ended up with 5 such chunks, each compiled to bytecode for nothing.convert_stmts_for_chunk(src/bundler/linker_context/convertStmtsForChunk.rs) drops the statement, because the importers are bound to the target's symbols directly.Fix
mark_part_live_step(src/bundler/LinkerContext.rs) runs once per part that becomes live. If the file's bit in the newLinkerGraph::files_with_codeis not set yet, it classifies the part: a wrapped file prints its__esm/__commonJSclosure, and otherwisepart_prints_codechecks the tag of the part's single statement (with tree shaking, every top-level statement is its own part). One tag check per live part, nothing per dead part, nothing once the bit is set, nothing without code splitting.part_prints_codemirrors whatconvert_stmts_for_chunkdrops outright: animport,export … fromorexport *of a bundled file withwrap == None, an import record the barrel optimization marked unused, and anexport {}clause.export * fromcounts as printing when the record calls__reExport. Anything else counts as printing.compute_chunks(src/bundler/linker_context/computeChunks.rs) puts every live file in its chunk, as before bundler: don't create a code-splitting chunk for a file with no live parts #40581, and records per chunk whether any of its files prints code. A code-splitting chunk where none does is skipped when the chunks are sorted, before any index is handed out. The per-file pass bundler: don't create a code-splitting chunk for a file with no live parts #40581 added is gone.init_x()orrequire_x()and ownsimport_x) counts as printing. Keeping every file in its chunk matters for order: the walk that orders a chunk's parts and imports starts from the chunk's files, so animport()target that only re-exports must stay in its own chunk to load its targets in source order.test/bundler/bundler_splitting.test.ts(NoChunkForReExportOnlyFilesis the report's repro, 5 output files;NoChunkForExportStarOrClauseOnlyFiles;ReExportOnlyEntryKeepsImportOrderandReExportOnlyFileKeepsChunkOrderfor the order). Alsoesbuild/splitting,esbuild/dce,esbuild/default,esbuild/importstar*,bundler_barrel,bundler_compile_splitting,bundler_edgecase,bundler_html,bun-build-api,metafile,bundler_cjs2esm,bundler_regressions,standalone,css-modules,bake/dev-and-prod(1043 tests).Background
compute_chunkskeys a chunk by the set of entry points that reach a file (entry_bits). Every live JS file with that key joins the chunk. A file with a unique key gets a chunk of its own.export { x } from "./impl.js"is kept for the second reason. When bundling, the linker resolves the re-export at link time: an importer ofxbinds toimpl.js's symbol, so the statement itself prints nothing.WrapKind::CjsorEsm) is one whose body runs lazily inside__commonJS(...)or__esm(...). An import of it prints arequire_x()orinit_x()call, so a re-export of a wrapped file does print code.Notes
Output of the report's repro with this change (
--splitting --target=bun --minify --banner):entry.js,entry-[hash].js(impl.js),a-*.js,b-*.js,e-*.js. Program output unchanged:hi a A/hi b/E.Earlier revisions computed the answer in
compute_chunks(a pass over every reachable file's parts, then a lazy scan when a file would start a chunk) and kept such files out of every chunk. The scan moved into tree shaking per review. Keeping the files out of their chunks changed evaluation order in two cases (a review finding): animport()target that only re-exports loaded its targets in chunk-index order, and inside a shared chunk a re-exported file kept only for its side effects could print after a sibling. Both are covered by the two order tests; the first fails on the previous revision.Probes run by hand against the debug build, all with correct program output:
module.exports = {...}; if (cond) module.exports.x = 1): the barrel's chunk is kept and exportsimport_impl.export * froma wrapped CommonJS file: kept, prints__reExport(...).require()d elsewhere (--target=node, so it is__esm-wrapped): kept, printsinit_wrapped(), and the importers carry the bareimport "./chunk".import "./side.js"; export {}as the only statements: no chunk;side.jsstill runs.The post-print alternative (drop a chunk whose pieces are empty and remove the
import "./chunk"statements that point at it) was not taken: chunk indices are baked intounique_key,cross_chunk_imports,css_chunksand the output file list by then, and the imports are already printed text.Two tests in
bun-build-api.test.ts(bytecode: function record on an encoder page boundary,bytecode: repeated builds don't retain the generated code) time out at the 5 s default on this debug build and pass with--timeout 120000;Bun.build can be called thousands of timespasses alone and times out under the load of 22 other files. None use splitting.compile/HelloWorldWithProcessVersionsBunfails on a debug build becauseprocess.versions.bunis1.4.1-debug; it passes with the release binary. None is related to this change.[review] gate passed · iteration 3 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 1 rejected · iteration 3
evidence per changed file