Skip to content

require(esm): a macro whose file is already in the graph no longer spins, and a shared module is fetched once (WebKit bump for oven-sh/WebKit#675) - #42915

Open
robobun wants to merge 4 commits into
robobun/464ab2b6/macro-under-require-esm-spinfrom
robobun/75afb400/macro-file-in-require-graph
Open

robobun wants to merge 4 commits into
robobun/464ab2b6/macro-under-require-esm-spinfrom
robobun/75afb400/macro-file-in-require-graph

Conversation

@robobun

@robobun robobun commented Sep 16, 2026 •

Copy link
Copy Markdown
Collaborator

Stacked on #42778 (the base is its branch). WEBKIT_VERSION is a preview tag of oven-sh/WebKit#675. Move it to the merged autobuild-<sha> before this lands.

Problem

  • require() of an ES module still spins when a module in its graph calls a macro and the graph already holds the macro's file (the two test.todo cases of macros: do not spin when a macro runs beneath require() of an ES module #42778). Bun 1.3.13 runs these.
  • The macro's entry module does await import(<macro file>), and Macro::init waits for it. What settles the file's fetch or load promise is parked in the require()'s VM::m_synchronousModuleQueue, which cannot drain before the macro returns.
  • Without a macro, the parked fetch makes Bun transpile a module that two siblings import twice, and a nested require() of a just-fetched sibling throws require() async module ... is unsupported.

Fix

Background

  • A macro runs while a file is transpiled. For a dependency of a require()d ES module that is on the main thread, inside the loader's fetch hook.
  • m_synchronousModuleQueue is Bun's addition to JSC for require(esm). The loader's promise reactions go there, and only that require() drains it.
  • A registry entry has a fetch promise (source), a module promise (record) and a load promise (record plus its imports).
Notes

WEBKIT_VERSION is autobuild-preview-pr-675-d953c3bd: 65513e295c plus oven-sh/WebKit#675. main has moved to a newer WebKit since (c28156899e, built with LLVM 23). The preview stays on 65513e295c while the base branch is on the LLVM 21 toolchain, and both move when #42778 is rebased. The binary size check compares with main, so it reports bun-freebsd-aarch64 1.6 MB over for that WebKit difference alone. The pin commit carries [skip size check] for that reason.

What each test needs (debug build with ASAN, base = this branch without the new pin):

test base with the pin
with modules that the require() already started to load ASSERTION FAILED: module->loadedModules().size() <= loadedModulesCountBefore + 1 pass
with the macro's file imported earlier by the same module spins pass
a macro that fails, same shape spins pass
with the macro's file imported earlier by an ancestor spins pass
with two macros from a file that an ancestor imported earlier spins pass
with a macro from a builtin module that the same module imported earlier spins pass
with a macro's file that calls a macro itself spins pass
with a module that calls a macro, requested earlier, that the macro's module imports ReferenceError: two is not defined pass
a module that two siblings import runs its macro once 2 2 1 1
require-esm-fetched-once: a module that two siblings import is loaded once onLoad sees it twice once
require-esm-fetched-once: require() of an ES module that the enclosing graph has just fetched require() async module "leaf.mjs" is unsupported pass
require-esm-fetched-once: import() of a CommonJS module that the enclosing graph has just fetched the exports are read twice once

The three tests in test/js/bun/resolve/require-esm-fetched-once.test.ts do not use a macro and do not need #42778. They fail on main (1.4.3) and pass on 1.3.13. If the pin should land before #42778, say so and I split it out.

The macro-mode transpile. A macro's module is transpiled with is_macro_runtime: macro calls in it are not expanded. On the base, a second request for a module that the require() had already transpiled fetched it again. From a macro's module that second transpile was the macro-mode one, and it replaced the source that the program runs: a module with its own macro call then failed with ReferenceError. With the new pin the loader uses the source that is already there.

Still open (one test.todo, and see oven-sh/WebKit#675):

  • A require() inside a macro of a module that the outer require() has started to load reports it as an async module. A require() is a graph load of its own, and it links on the module's load promise, which the outer require()'s parked reactions fulfill. 1.3.13 runs this.
  • A macro whose file is still in flight on the transpiler thread for an earlier import() spins. That completion goes to the regular event loop, which the macro's wait does not run.
  • macros: run every macro on one dedicated VM thread #40059 runs every macro on its own thread and removes the main-thread macro path. It makes the macro tests here pass by construction, and the three require-esm-fetched-once tests still need the engine change.

#33180 is not touched. A first version of the engine change fetched again from the top-level loadModule, which also made require() of a module that is in flight on another thread work. oven-sh/WebKit#662 does exactly that, so it is not here. The tests of #37185 still fail with this pin.

Self-review. A first review found the overlap with oven-sh/WebKit#662 and #396 and the macro-mode transpile problem, and the engine change was rebuilt around them. A second review of an intermediate version found that dropping the pipeFrom job changed the job order of plain require(esm) (false async-module errors, a changed CommonJS evaluation order). The final version keeps the job order. A differential run of 2,800 random module graphs against the base shows no graph that is worse and three that are fixed.

Suites run on the fixed engine (local build, see oven-sh/WebKit#675 for how): all of test/bundler/transpiler/macro-test.test.ts, test/js/bun/resolve/, test/js/node/module/, test/js/bun/plugin/plugins.test.ts, test/js/bun/test/mock/, test/cli/run/require-cache.test.ts (without the RSS leak tests), test/regression/issue/24387.test.ts, 30493.test.ts. load the same empty JS file 2000 times times out on this machine on the base too.


[policy-decision:webkit] gate passed · iteration 0 · 3 files touched

passes on PR (with fix)
Test-only change.

Debug/ASAN (expected pass):
$ bun bd test 'test/bundler/transpiler/macro-test.test.ts' 'test/js/bun/resolve/require-esm-fetched-once.test.ts'
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/bundler/transpiler/macro-test.test.ts test/js/bun/resolve/require-esm-fetched-once.test.ts
bun test v1.4.3 (09bb54630)

test/bundler/transpiler/macro-test.test.ts:
[macro] call escapeHTML
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call symbolKeys
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call escape
[macro] call addStrings
[macro] call addStrings
[macro] call addStrings
[macro] call addStrings
[macro] call addStrings
[macro] call addStrings
[macro] call addStrings
[macro] call addStrings
[macro] call addStrings
[macro] call addStrings
[macro] call addStrings
[macro] call addStrings
[macro] call addStrings
[macro] call addStrings
[macro] call addStrings
[macro] call addStrings
[macro] call addStringsUTF16
[macro] call addStringsUTF16
[macro] call addStringsUTF16
[macro] call addStringsUTF16
[macro] call addStringsUTF16
[macro] call addStringsUTF16
[macro] call addStringsUTF16
[macro] call addStringsUTF16
[macro] call addStringsUTF16
[macro] call addStringsUTF16
[macro] call addStringsUTF16
[macro] call addStringsUTF16
[macro] call addStringsUTF16
[macro] call addStringsUTF16
[macro] call addStringsUTF16
[macro] call addStringsUTF16
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call identity
[macro] call default
[macro] call default
[macro] call identity
[macro] call identity
[macro] call escape
[macro] call i
... (truncated)
Exit: 0
diff hotspot
scripts/build/deps/webkit.ts                       |   2 +-
 test/bundler/transpiler/macro-test.test.ts         | 156 +++++++++++++++++++--
 .../bun/resolve/require-esm-fetched-once.test.ts   |  93 ++++++++++++
 3 files changed, 241 insertions(+), 10 deletions(-)

gate history · 2 passed · 0 rejected · iteration 0

evidence per changed file
file                                                  reads  edits  tests
scripts/build/deps/webkit.ts                              1      0     27
test/bundler/transpiler/macro-test.test.ts                3      7     25
test/js/bun/resolve/require-esm-fetched-once.test.ts      1      2     14

A synchronous module load uses a fetch that the host already delivered, and
import() beneath it does not wait for another load's promise. The tag is a
preview build of the open PR. It has to move to the autobuild-<sha> of the
merge commit before this lands.
@robobun

robobun commented Sep 16, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status

@robobun

robobun commented Sep 16, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 5:28 AM PT - Sep 16th, 2026

❌ @robobun, your commit 6142e89 has 1 failures in Build #116512 (All Failures):

  • 📦 Binary size — 1 over 0.50 MB
  • targetthis build canary: main #116425
    sizeΔ
    bun-darwin-aarch6459.95 MB60.48 MB-549.9 KB
    bun-darwin-x6465.83 MB66.19 MB-368.6 KB
    bun-linux-aarch6476.43 MB76.37 MB+64.1 KB
    bun-linux-x6476.38 MB76.84 MB-471.9 KB
    bun-linux-aarch64-musl69.63 MB69.32 MB+320.0 KB
    bun-linux-x64-musl70.51 MB70.81 MB-316.0 KB
    bun-linux-aarch64-android83.40 MB83.15 MB+256.4 KB
    bun-linux-x64-android85.83 MB86.20 MB-375.9 KB
    bun-freebsd-x6487.94 MB88.28 MB-348.0 KB
    ❌ bun-freebsd-aarch6491.28 MB89.67 MB+1.62 MB
    bun-windows-x6482.57 MB83.49 MB-938.0 KB
    bun-windows-aarch6472.27 MB72.59 MB-326.0 KB

    Add [skip size check] to the commit message if this increase is intentional.


🧪   To try this PR locally:

bunx bun-pr 42915

That installs a local version of the PR into your bun-42915 executable, so you can run:

bun-42915 --bun

@claude claude Bot 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.

Warning

This review may be incomplete: the analysis reached its time or budget limit before every step could run, so its coverage is partial.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Comment thread scripts/build/deps/webkit.ts Outdated
Comment thread test/bundler/transpiler/macro-test.test.ts
Comment thread test/js/bun/resolve/require-esm-fetched-once.test.ts
Comment thread test/bundler/transpiler/macro-test.test.ts
@robobun

robobun commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator Author

Answers to the review:

  • WEBKIT_VERSION (red): correct, and intended for now. The preview tag is there so that CI runs against the engine change while [JSC] Synchronous module load: use a fetch the host already delivered, and do not make import() wait for another load's promise WebKit#675 is open. The first line of the PR body says that the pin has to move to the merged autobuild-<sha> before this lands. I do that when Credits & License moved to bottom (professional) #675 merges.
  • The failing-macro test: changed in the next push. The macro now prints a line before it throws, and the test asserts that line, so a macro that never ran does not pass.
  • The second transpile in require() of a just-fetched ES module: that one is Bun's own. fetchCommonJSModule transpiles the file before it asks the loader, for every require() that finds an entry below Fetched. It happened on the base too, before the false async-module error. Fix spurious "require() async module" TypeError when require() hits an ESM module the import graph is still fetching #37185 works on that function. This PR does not touch src/, so I left it and did not assert a load count in that test.
  • The reversed import order (the file with the macro first, then the macro's own file, which calls a macro itself): it fails the same way on 1.3.13, on the base and here (Error importing macro), because the macro's module is transpiled without macro expansion and its own macro call has no import. It is not part of this change. The comment in the test describes the order it tests: the require() has transpiled the file before the macro asks for it.

The next push also moves the pin to a new preview build: oven-sh/WebKit#675 got one more commit from its review (a top-level load of a just-fetched module built the module record twice). require-esm-fetched-once.test.ts gets a test for it.

oven-sh/WebKit#675 got one more commit: a top-level load of a module that the
enclosing require(esm) has just fetched continues from the host's fetch promise,
so the module record is built once.

The size check compares with the latest build of main, which pins a newer
WebKit (c28156899e, built with LLVM 23). This preview is 65513e295c plus the
fix, because the base branch is still on the LLVM 21 toolchain. That difference
puts bun-freebsd-aarch64 1.6 MB over, and it goes away when the pin moves to the
merged commit.

@claude claude Bot 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.

Code review found no issues

No high-confidence issues detected in this change.

@robobun

robobun commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator Author

This review has no findings to act on. The four threads from the first review have replies and are resolved. What remains before merge is in the first line of the PR body: oven-sh/WebKit#675 has to land, and then the pin moves to its autobuild-<sha>.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant