Skip to content

fix(code-splitting): re-derive chunk exec order after the runtime sweep#10334

Merged
graphite-app[bot] merged 1 commit into
mainfrom
codex/flag-off-runtime-sweep-chunk-order
Jul 19, 2026
Merged

fix(code-splitting): re-derive chunk exec order after the runtime sweep#10334
graphite-app[bot] merged 1 commit into
mainfrom
codex/flag-off-runtime-sweep-chunk-order

Conversation

@hyfdev

@hyfdev hyfdev commented Jul 17, 2026

Copy link
Copy Markdown
Member

Summary

Flag-off builds could emit cross-chunk imports — and therefore run side effects — in a different order than main, on the default path. When the post-chunking runtime sweep drops the runtime module out of a shared chunk it co-hosts with user modules, that chunk was left with an execution order keyed on the removed runtime (exec order 0) instead of its first surviving module, so it sorted ahead of chunks it should have followed. This restores main's ordering by re-deriving chunk execution orders after the sweep.

Verified mechanism

  • Before feat(code-splitting): wrap strict execution order modules on demand #10104 (1d86ffe06), sweep_unused_runtime_module ran in generate_chunks before chunk exec-order assignment (there was a comment requiring it). feat(code-splitting): wrap strict execution order modules on demand #10104 moved the sweep into finalize_chunk_plan, which runs after assignment, so the regression is live on main today.
  • The Common-chunk comparator keys on modules[0]'s exec order; sort_chunk_modules always makes the runtime (module exec order 0) modules[0]. So a chunk hosting the runtime keys on 0.
  • When the walk-back (find_entry_level_external_module) invalidates the runtime demand (the [Bug]: rolldown/runtime is false-positively injected #9374 star-re-export-to-external shape) and the sweep strips the runtime from a still-live shared chunk, the chunk keeps exec_order derived from the now-absent runtime. rebuild_sorted_chunk_idx_vec(true) only re-sorts the stale values; renumber_live_chunks is strict-only. That stale key feeds both sorted_chunk_idx_vec and the cross-chunk import sort (compute_cross_chunk_links.rs:110/124), so an entry emits import "./C.js" before a chunk that should run first.

Fix

Chunk exec orders must already be assigned before order analysis runs (the on-demand actual-order simulation sorts sibling chunks by exec_order), so assignment cannot simply move after the sweep. Instead:

  • Extract the assignment loop into assign_chunk_exec_orders (code_splitting.rs); keep the provisional call where it was in generate_chunks.
  • Re-run it in finalize_chunk_plan when the sweep actually removed the runtime from a chunk — the same condition that already triggered rebuild_sorted_chunk_idx_vec(true) — then rebuild the sorted vec.

Final ordering contract (documented on assign_chunk_exec_orders). Flag-off, analyze_execution_order returns None, so no lowering and no renumber_live_chunks run; the re-run is the sole authority and reproduces exactly what main produced by sweeping before assignment. Under strict, the sweep block only runs when required_runtime_helpers() is empty (no order wrapper demands the runtime); renumber_live_chunks (strict-only, after lowering) merely compacts existing exec_order values to close tombstone gaps and never re-derives them from modules[0]. The re-run happens after lowering, so whichever of the two touches exec_order last leaves every live chunk both densely numbered and keyed on its correct lead module — they never contend.

Behavior / snapshot impact

  • New fixture only. topics/runtime/sweep_shared_chunk_exec_order — three entries; re.js (export * from 'external') makes tree-shaking include the runtime for __reExport, and the runtime folds into re.js's shared {entry_a, entry_b} chunk. The walk-back flattens the star re-export to a chunk-level export * from "external", so the post-order sweep drops the runtime from that still-live shared chunk. entry_a also imports the earlier side-effect chunk s_ac.js (shared {entry_a, entry_c}).
  • Main-parity proof. The fixture was built against three states. Pre-feat(code-splitting): wrap strict execution order modules on demand #10104 main (ce98ae798) emits import "./s_ac.js" before import "./re.js"; current main emits import "./re.js" first (the runtime-hosting chunk keeping stale exec order 0) — a real side-effect-order change on the default path; this branch restores the pre-feat(code-splitting): wrap strict execution order modules on demand #10104 order. The committed snapshot is therefore the pre-regression output, not an expectation written to match the fix. Reverting only the two source files onto this branch reproduces the failure, so the fixture discriminates.
  • No existing snapshot changes. No other fixture failed under INSTA_UPDATE=no, which is what a changed snapshot would do, so no existing snapshot changed.

Validation

  • cargo test -p rolldown — 1877 passed here against 1876 on main, the difference being exactly the new fixture. Both runs share the same 5 environmental failures (cjs_module_lexer_compat ×3 + npm_packages/util_deprecate need pnpm install; test262_module_code needs the submodule). Run with INSTA_UPDATE=no: .insta.yaml sets update: 'always', so a local run without it rewrites snapshots in place and reports success regardless — worth knowing for anyone reproducing the revert-check above.
  • cargo fmt --all --check — clean.
  • cargo clippy -p rolldown --all-targets -- -D warnings — clean.

Follow-up to #10104; addresses item 3 of #10104 (comment)

@hyfdev
hyfdev force-pushed the codex/flag-off-runtime-sweep-chunk-order branch from 39bb381 to 4957c8a Compare July 17, 2026 08:18
@hyfdev
hyfdev force-pushed the codex/flag-off-runtime-sweep-chunk-order branch from 4957c8a to cbe7f25 Compare July 18, 2026 05:35
@hyfdev
hyfdev changed the base branch from codex/strict-flag-off-init-pins to main July 18, 2026 05:36
@hyfdev
hyfdev force-pushed the codex/flag-off-runtime-sweep-chunk-order branch from cbe7f25 to f8ec80d Compare July 18, 2026 05:48
@netlify

netlify Bot commented Jul 18, 2026

Copy link
Copy Markdown

Deploy Preview for rolldown-rs ready!

Name Link
🔨 Latest commit ab2a338
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/6a5cec81f192aa000803a2c4
😎 Deploy Preview https://deploy-preview-10334--rolldown-rs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@hyfdev
hyfdev force-pushed the codex/flag-off-runtime-sweep-chunk-order branch 3 times, most recently from 40e86d8 to 0f69617 Compare July 19, 2026 12:49
@hyfdev
hyfdev marked this pull request as ready for review July 19, 2026 12:51
Copilot AI review requested due to automatic review settings July 19, 2026 12:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@codspeed-hq

codspeed-hq Bot commented Jul 19, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 7 untouched benchmarks
⏩ 10 skipped benchmarks1


Comparing codex/flag-off-runtime-sweep-chunk-order (0f69617) with main (75d8860)

Open in CodSpeed

Footnotes

  1. 10 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

shulaoda commented Jul 19, 2026

Copy link
Copy Markdown
Member

Merge activity

  • Jul 19, 3:20 PM UTC: The merge label 'graphite: merge-when-ready' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Jul 19, 3:20 PM UTC: shulaoda added this pull request to the Graphite merge queue.
  • Jul 19, 3:30 PM UTC: Merged by the Graphite merge queue.

…ep (#10334)

## Summary

Flag-off builds could emit cross-chunk imports — and therefore run side effects — in a different order than `main`, on the default path. When the post-chunking runtime sweep drops the runtime module out of a shared chunk it co-hosts with user modules, that chunk was left with an execution order keyed on the removed runtime (exec order `0`) instead of its first surviving module, so it sorted ahead of chunks it should have followed. This restores `main`'s ordering by re-deriving chunk execution orders after the sweep.

## Verified mechanism

- Before #10104 (`1d86ffe06`), `sweep_unused_runtime_module` ran in `generate_chunks` **before** chunk exec-order assignment (there was a comment requiring it). #10104 moved the sweep into `finalize_chunk_plan`, which runs **after** assignment, so the regression is live on `main` today.
- The Common-chunk comparator keys on `modules[0]`'s exec order; `sort_chunk_modules` always makes the runtime (module exec order `0`) `modules[0]`. So a chunk hosting the runtime keys on `0`.
- When the walk-back (`find_entry_level_external_module`) invalidates the runtime demand (the #9374 star-re-export-to-external shape) and the sweep strips the runtime from a still-live shared chunk, the chunk keeps `exec_order` derived from the now-absent runtime. `rebuild_sorted_chunk_idx_vec(true)` only re-sorts the stale values; `renumber_live_chunks` is strict-only. That stale key feeds both `sorted_chunk_idx_vec` and the cross-chunk import sort (`compute_cross_chunk_links.rs:110/124`), so an entry emits `import "./C.js"` before a chunk that should run first.

## Fix

Chunk exec orders must already be assigned before order analysis runs (the on-demand actual-order simulation sorts sibling chunks by `exec_order`), so assignment cannot simply move after the sweep. Instead:

- Extract the assignment loop into `assign_chunk_exec_orders` (`code_splitting.rs`); keep the provisional call where it was in `generate_chunks`.
- Re-run it in `finalize_chunk_plan` when the sweep actually removed the runtime from a chunk — the same condition that already triggered `rebuild_sorted_chunk_idx_vec(true)` — then rebuild the sorted vec.

**Final ordering contract (documented on `assign_chunk_exec_orders`).** Flag-off, `analyze_execution_order` returns `None`, so no lowering and no `renumber_live_chunks` run; the re-run is the sole authority and reproduces exactly what `main` produced by sweeping before assignment. Under strict, the sweep block only runs when `required_runtime_helpers()` is empty (no order wrapper demands the runtime); `renumber_live_chunks` (strict-only, after lowering) merely *compacts* existing `exec_order` values to close tombstone gaps and never re-derives them from `modules[0]`. The re-run happens after lowering, so whichever of the two touches `exec_order` last leaves every live chunk both densely numbered and keyed on its correct lead module — they never contend.

## Behavior / snapshot impact

- **New fixture only.** `topics/runtime/sweep_shared_chunk_exec_order` — three entries; `re.js` (`export * from 'external'`) makes tree-shaking include the runtime for `__reExport`, and the runtime folds into `re.js`'s shared `{entry_a, entry_b}` chunk. The walk-back flattens the star re-export to a chunk-level `export * from "external"`, so the post-order sweep drops the runtime from that still-live shared chunk. `entry_a` also imports the earlier side-effect chunk `s_ac.js` (shared `{entry_a, entry_c}`).
- **Main-parity proof.** The fixture was built against three states. Pre-#10104 `main` (`ce98ae798`) emits `import "./s_ac.js"` before `import "./re.js"`; current `main` emits `import "./re.js"` first (the runtime-hosting chunk keeping stale exec order `0`) — a real side-effect-order change on the default path; this branch restores the pre-#10104 order. The committed snapshot is therefore the pre-regression output, not an expectation written to match the fix. Reverting only the two source files onto this branch reproduces the failure, so the fixture discriminates.
- **No existing snapshot changes.** No other fixture failed under `INSTA_UPDATE=no`, which is what a changed snapshot would do, so no existing snapshot changed.

## Validation

- `cargo test -p rolldown` — 1877 passed here against 1876 on `main`, the difference being exactly the new fixture. Both runs share the same 5 environmental failures (`cjs_module_lexer_compat` ×3 + `npm_packages/util_deprecate` need `pnpm install`; `test262_module_code` needs the submodule). Run with `INSTA_UPDATE=no`: `.insta.yaml` sets `update: 'always'`, so a local run without it rewrites snapshots in place and reports success regardless — worth knowing for anyone reproducing the revert-check above.
- `cargo fmt --all --check` — clean.
- `cargo clippy -p rolldown --all-targets -- -D warnings` — clean.

---

Follow-up to #10104; addresses item 3 of #10104 (comment)
@graphite-app
graphite-app Bot force-pushed the codex/flag-off-runtime-sweep-chunk-order branch from 0f69617 to ab2a338 Compare July 19, 2026 15:25
@graphite-app
graphite-app Bot merged commit ab2a338 into main Jul 19, 2026
34 checks passed
@graphite-app
graphite-app Bot deleted the codex/flag-off-runtime-sweep-chunk-order branch July 19, 2026 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants