Skip to content

perf(external): drop the runtime import from chunks that never wrap - #10517

Merged
IWANABETHATGUY merged 4 commits into
mainfrom
perf/external-toesm-runtime-edge
Jul 31, 2026
Merged

perf(external): drop the runtime import from chunks that never wrap#10517
IWANABETHATGUY merged 4 commits into
mainfrom
perf/external-toesm-runtime-edge

Conversation

@IWANABETHATGUY

@IWANABETHATGUY IWANABETHATGUY commented Jul 29, 2026

Copy link
Copy Markdown
Member

Stacked on #10516 — review that one first.

A chunk that does not wrap an external still emitted a useless import of the runtime chunk:

Object.defineProperty(exports, Symbol.toStringTag, { value: "Module" });
require("./rolldown-runtime.js");        // nothing is used from it
let node_https = require("node:https");
exports.foo = node_https.request;

Why it happened

Not an empty cross-chunk import. The chunk really did import __toESM, so it rendered const require_rolldown_runtime = require(...); DCE then dropped the unused binding and kept the side-effectful call. The import existed because patch_module_dependencies set reads_external_as_esm from the bundle-wide record, so a module reading only a name off the external still demanded the helper and put it in its chunk's depended symbols.

The fix

Derive that edge from the same observer set the renderers use, so a module demands __toESM only when it is an observer. Both decisions now come from one source, which is what makes narrowing safe: the chunk that wraps is exactly the chunk that depends on the runtime.

Under-supplying is the dangerous direction — a chunk that wraps without the edge references a helper it never imported and finalization panics — so an observer that is not included keeps the previous bundle-wide behaviour, matching rendering's own unattributable fallback. Chunks do not exist yet at that point, so is_included stands in for "will have a chunk".

This only became safe once #10516 made rendering observer-attributed. Narrowing this edge before that reintroduces the panic, which is why it is a separate commit rather than part of the fix below it.

Result

external_interop_only_reachable_via_entry_export shows the payoff — with only entry-a demanding the helper, the runtime stops being shared and folds into entry-a, so the separate runtime chunk disappears entirely:

// entry-a.js
let node_https = require("node:https");
node_https = __toESM(node_https);

// entry-b.js
let node_https = require("node:https");
exports.foo = node_https.request;

named-user in reexport_default_import_of_external_multi_chunk loses its bare require the same way; that fixture's _test.mjs still asserts the values executably.

Those two snapshots are the only output changes. The fixture comment and internal doc previously stated this edge had to stay bundle-wide with the bare require as an accepted cost — true when written, so both are updated.

IWANABETHATGUY commented Jul 29, 2026

Copy link
Copy Markdown
Member Author

How to use the Graphite Merge Queue

Add the label graphite: merge-when-ready to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@IWANABETHATGUY IWANABETHATGUY changed the title fix(external): demand __toESM only from modules that render it perf(external): drop the runtime import from chunks that never wrap Jul 29, 2026
@IWANABETHATGUY
IWANABETHATGUY force-pushed the perf/external-toesm-runtime-edge branch from ba2cf35 to 5f85092 Compare July 29, 2026 09:28
@IWANABETHATGUY
IWANABETHATGUY force-pushed the perf/external-toesm-runtime-edge branch from 5f85092 to 0fee37d Compare July 29, 2026 10:54
Base automatically changed from fix/10069-external-toesm-interop to main July 29, 2026 11:03
@IWANABETHATGUY
IWANABETHATGUY force-pushed the perf/external-toesm-runtime-edge branch from 0fee37d to 6a1ffa8 Compare July 30, 2026 02:27
@netlify

netlify Bot commented Jul 30, 2026

Copy link
Copy Markdown

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit 5c0bfcf
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/6a6ca3a7186c7d0007b1db37

@IWANABETHATGUY
IWANABETHATGUY marked this pull request as ready for review July 30, 2026 03:38
@codspeed-hq

codspeed-hq Bot commented Jul 30, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 7 untouched benchmarks
⏩ 10 skipped benchmarks1


Comparing perf/external-toesm-runtime-edge (5c0bfcf) with main (e273daa)

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.

@IWANABETHATGUY
IWANABETHATGUY force-pushed the perf/external-toesm-runtime-edge branch from 3d711d2 to af29a8d Compare July 31, 2026 05:26

@hyfdev hyfdev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The narrowing keeps the two decisions in agreement: a chunk that wraps an external — via an observer, an unattributable observer, or a kept default/namespace import — depends on the runtime, and a named-only chunk drops both the wrap and the import together. The same invariant holds on the dead-import and entry-export paths.

Two notes on the documented safety argument below; neither blocks merge.

— Review by @hyfdev, assisted by deepseek-v4-flash

Comment thread internal-docs/runtime-helpers/implementation.md Outdated

@hyfdev hyfdev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The observer-scoped edge matches rendering for the attributable cases this PR targets: named-only chunks drop both the wrap and the bare runtime import, while observer chunks keep __toESM with a live runtime edge. The unattributable !is_included fallback keeps the dangerous under-supply direction conservative, and the two snapshot deltas plus the multi-chunk executable assert match that contract.

The open notes on the dual wrap/edge channels and the untested safety paths still stand. One small leftover in the same function, outside the changed hunk: the entry-export block still says the recorded interop is bundle-wide; under the new predicate a named-only entry export neither wraps nor demands the helper.

— Review by @hyfdev, assisted by Grok 4.5

IWANABETHATGUY and others added 4 commits July 31, 2026 21:29
A chunk that no longer wraps an external still carried a bare

    require("./rolldown-runtime.js");

That is not an empty cross-chunk import. The chunk really did import `__toESM`,
so it rendered `const require_rolldown_runtime = require(...)`; DCE then dropped
the unused binding and kept the side-effectful call. The import existed because
`patch_module_dependencies` set `reads_external_as_esm` from the bundle-wide
record, so a module reading only a *name* off the external still demanded the
helper and put it in its chunk's depended symbols.

Derive the edge from the same observer set the renderers use, so a module demands
`__toESM` only when it is an observer. Both decisions now come from one source,
which is what makes narrowing safe: the chunk that wraps is exactly the chunk
that depends on the runtime. Under-supplying remains the dangerous direction — a
chunk that wraps without the edge references a helper it never imported and
finalization panics — so an observer that is not included keeps the old
bundle-wide behaviour, matching rendering's own unattributable fallback. Chunks
do not exist yet here, so `is_included` stands in for "will have a chunk".

`external_interop_only_reachable_via_entry_export` is the guard for that panic,
and it now shows the payoff: with only `entry-a` demanding the helper the runtime
stops being shared and folds into `entry-a`, so the separate runtime chunk
disappears and `entry-b` is left with just its own `require`. `named-user` in the
multi-chunk fixture loses its bare `require` the same way; its `_test.mjs` still
asserts the values.

The fixture comment and the internal doc claimed this edge had to stay
bundle-wide, with the bare `require` as the accepted cost. Both are updated: the
cost is gone, and the reason it can be narrowed is that the wrap and the edge are
no longer derived independently.

Claude-Session: https://claude.ai/code/session_01NsgZ6r2G3wbVm4ZfJNJbVn
Checking an external's observer set for a non-included observer inside
the per-reference closure rescans the whole set for every module
referencing a popular external, making dependency patching quadratic.
The answer only depends on the external, so hoist it into a set built
in one pass before the parallel walk; the closure is back to O(1)
lookups. Also drop has_interop_use_for, dead since the closure switched
to reading the observer map directly.

Claude-Session: https://claude.ai/code/session_01Fm7X9sdW3kKWvZTKCf4N1J
Review of #10517 pointed out that "a module demands `__toESM` only when it
is an observer" understates where the wrap can come from, and that the two
directions the safety argument rests on were untested.

The wrap arrives through two channels, and each carries its own edge:

- Recorded observations: `chunk_recorded_external_interop` wraps the chunks
  observers landed in, and `patch_module_dependencies` narrows the edge to
  those same observers — including the entry-export references, which reach
  neither the statement walk nor any `named_imports`.
- Statically written imports: `chunk_external_interop_modes` also wraps on
  the chunk's own `named_imports` with no liveness filter, so a default or
  namespace import whose binding nothing reads wraps while recording no
  observer. `reference_needed_symbols` and `compute_chunk_imports` read the
  same `specifier_needs_interop` predicate to supply its edge.

So the invariant is pairwise rather than one source answering both
questions, which is what keeps a future "register `ToEsm` only for live
imports" refactor from reopening the under-supply panic. Both new fixtures
were red/green checked by disabling the channel each one covers; the
`barrel` hop in the entry-export one is load-bearing, since a direct shim
dependency would supply the edge through `needs_inherit_to_esm_runtime`
instead and pin nothing.

Claude-Session: https://claude.ai/code/session_01S3RQeYwhJmEu4eLYkanvm1
@IWANABETHATGUY
IWANABETHATGUY force-pushed the perf/external-toesm-runtime-edge branch from af29a8d to 5c0bfcf Compare July 31, 2026 13:31
@IWANABETHATGUY
IWANABETHATGUY merged commit 347cb3b into main Jul 31, 2026
34 of 35 checks passed
@IWANABETHATGUY
IWANABETHATGUY deleted the perf/external-toesm-runtime-edge branch July 31, 2026 13:39
@rolldown-guard rolldown-guard Bot mentioned this pull request Aug 3, 2026
shulaoda added a commit that referenced this pull request Aug 3, 2026
## [1.2.2] - 2026-08-03

### 🚀 Features

- code-splitting: support inlining of shared deps in dynamic entries (#10526) by @nicolo-ribaudo
- plugin: time the user callbacks configured on the options (#10509) by @IWANABETHATGUY

### 🐛 Bug Fixes

- dev: force `cleanDir` off in dev mode (#10579) by @shulaoda
- dev: keep `?rolldown-lazy` proxy ids out of user `resolveId` hooks (#10580) by @btea
- initialize re-exported wrapped ESM modules at the entry chunk (#10567) by @hyfdev
- link: resolve circular star reexports as null (#10445) by @Nic-Polumeyv
- transform: transformer gate skips newer syntax that oxc can lower (#10564) by @sekyungk
- code-splitting: materialize consumer-local barrel routing when the order-wrap plan is empty (#10544) by @hyfdev
- plugin: measure plugin hooks in JavaScript, report them from Rust (#10508) by @IWANABETHATGUY
- plugin: include external dynamic imports in chunk metadata (#10557) by @hyfdev
- plugin: report plugin timings after closeBundle has run (#10521) by @IWANABETHATGUY
- browser: avoid using node builtin modules (#10542) by @sapphi-red

### 🚜 Refactor

- plugin: give the build's own clocks their own type (#10520) by @shulaoda

### 📚 Documentation

- correct stale `buildStart` comment in `ScanStageCache::merge` (#10577) by @shulaoda
- update outdated pluginutils references (#10574) by @dogledogle

### ⚡ Performance

- external: drop the runtime import from chunks that never wrap (#10517) by @IWANABETHATGUY
- plugin: clock the build only when it is being measured (#10558) by @IWANABETHATGUY

### 🧪 Testing

- vite-tests: drop the dead `@rolldown/pluginutils` pnpm override (#10575) by @IWANABETHATGUY
- transform: group the transform-target fixtures and say why each exists (#10569) by @IWANABETHATGUY
- dev: fix hmr-hot-update-hook-vite fixture on Windows (#10565) by @h-a-n-a
- dev: match Vite's renamed full-reload log (#10547) by @shulaoda

### ⚙️ Miscellaneous Tasks

- deps: update napi (#10576) by @renovate[bot]
- deps: bump `@napi-rs/wasm-runtime` to 1.2.2 (#10570) by @shulaoda
- deps: update npm packages (#10583) by @renovate[bot]
- deps: update dependency rolldown-plugin-dts to ^0.28.0 (#10588) by @renovate[bot]
- deps: update taiki-e/install-action action to v2.85.5 (#10587) by @renovate[bot]
- deps: update codspeedhq/action action to v5 (#10584) by @renovate[bot]
- deps: update github actions (#10582) by @renovate[bot]
- deps: update rust crates (#10581) by @renovate[bot]
- deps: update dependency vite-plus to v0.2.7 (#10568) by @renovate[bot]
- deps: update dependency rolldown-plugin-dts to v0.27.14 (#10531) by @renovate[bot]
- deps: update pnpm to v11.17.0 (#10571) by @renovate[bot]
- deps: update @napi-rs/wasm-runtime to 1.2.1 (#10545) by @sapphi-red

### ❤️ New Contributors

* @sekyungk made their first contribution in [#10564](#10564)
* @nicolo-ribaudo made their first contribution in [#10526](#10526)

Co-authored-by: shulaoda <165626830+shulaoda@users.noreply.github.com>
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.

2 participants