compile: pre-register embedded modules without allocating loader promises - #40674
Merged
Merged
Conversation
…ises With WebKit 86e19b8edd7c, ModuleRegistryEntry::provideModule() records the source and module record without creating the fetch/module/load promises, and markLoaded() tells the loader the record's [[LoadedModules]] is complete; the loader materializes a settled promise only if some later load asks for one. Use those instead of building a resolved load promise per module. For a ~600-chunk compiled app this removes ~1,900 JSPromise allocations from startup (2,986 -> 1,118 during --help); behaviour and host-hook counts are unchanged.
Contributor
|
Warning Review limit reached
On-demand reviews are free for the next 24 days. After that, they cost $0.25 per reviewed file. Or wait 27 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Your 89 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
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 (2)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughThe PR updates the WebKit revision and changes standalone module closure registration to use loaded registry entries, the updated ChangesWebKit revision
Standalone module registration
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change reduces unnecessary promise allocations during compiled-module registration while preserving on-demand loading behavior; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
Collaborator
|
Updated 6:09 PM PT - Aug 27th, 2026
@Jarred-Sumner, your commit c486188 is building: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Beyond the inline nit, I checked the removed RETURN_IF_EXCEPTION after entry->provideModule(...) — the signature moved from globalObject to vm, which is JSC's convention for a non-throwing call, so dropping the check is consistent with the upstream API change. Also grepped the repo for the old WebKit SHA — no other occurrences to update.
Extended reasoning...
The dropped exception check is the kind of hunk REVIEW.md's "exception checks after every call that can enter JS" rule would flag on a skim, so it's worth recording that it was looked at: provideModule now takes VM& rather than JSGlobalObject*, which in JSC signals the call cannot throw, and the PR description states it no longer allocates promises (the previous throwing path). The old-SHA grep and the isLoaded()/markLoaded() swaps are mechanical; the WebKit-side half of this change (oven-sh/WebKit@ 86e19b8e) and its prebuilt artifacts weren't verified here.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/jsc/bindings/ZigGlobalObject.cpp— nit: stale comment — function header still says "loadPromise settled" but the new code calls markLoaded() and no longer creates/settles a loadPromiseExtended reasoning...
The doc comment above registerStandaloneClosure() describes the old mechanism ("[[LoadedModules]] filled and loadPromise settled"); after this change the loop calls entry->markLoaded() and never allocates or fulfills a loadPromise (one is only materialized lazily by JSC if a top-level load asks). Relative to base this is newly inaccurate — a future reader may re-add a setLoadPromise/fulfill to "restore" the documented behavior, undoing the allocation win. Fix: reword to "[[LoadedModules]] filled and the entry marked loaded" (or similar) so the comment matches markLoaded() semantics.
Verification: nit — The doc comment at src/jsc/bindings/ZigGlobalObject.cpp:3855-3858 still reads "so they are marked loaded outright — [[LoadedModules]] filled and loadPromise settled". On the base branch this was accurate: the trailing loop did
JSPromise::create+loaded->fulfill+entry->setLoadPromise(vm, loaded). After this diff the trailing loop is now… | nit — The header comment at…
… module evaluation without a promise per import edge
…ettling a load promise No-Verification-Needed: comment-only change.
This was referenced Aug 28, 2026
Open
Jarred-Sumner
pushed a commit
that referenced
this pull request
Aug 28, 2026
### Problem - Bun's WebKit pin `f5deafe090` is 343 upstream commits behind `6b879687ee` (2026-08-28), 77 of them in JavaScriptCore, WTF or bmalloc. oven-sh/WebKit#528 merges that range into the fork. - Upstream `c8fdf5eb81` adds a `const String& referrer` parameter to `GlobalObjectMethodTable::moduleLoaderFetch`. Bun's three implementations have the old shape and do not compile against the merged engine. - The range fixes three engine bugs Bun users can hit: a crash on a deeply nested destructuring pattern (`752ab90072`), a DFG misfold of a property in a non-reified static table (`33876268a1`; `process`, `Buffer` and `Event` use such tables), and DCE deleting the overflow check of checked Int32 arithmetic (`9f3eea6f46`). ### Fix - `WEBKIT_VERSION` is `1817c3c37f5dfc004a33dafac9f6adcf1a9641a4`, the merge commit of oven-sh/WebKit#528 on the fork's main (release `autobuild-1817c3c37f5dfc004a33dafac9f6adcf1a9641a4`, 42 tarballs, one per platform and flavor). Its tree is identical to the preview build `autobuild-preview-pr-528-41528be5` that CI ran against. - `GlobalObject::moduleLoaderFetch`, `StandaloneGlobalObject::moduleLoaderFetch` and `bakeModuleLoaderFetch` take the new `referrer` parameter and ignore it. This is the only Bun source change the range needs. - Verified: `test/js/bun/jsc/webkit-upgrade-6b879687ee.test.ts` pins the three fixes. At the current pin the first two crash the child process and the third prints seven `true`s. Eighteen more suites (3,719 tests) pass on a debug + ASAN build against the merged tree (notes). ### Background - Bun links a prebuilt JavaScriptCore from the oven-sh/WebKit release named in `scripts/build/deps/webkit.ts`. A fork pull request publishes a prerelease tagged `autobuild-preview-pr-<N>-<sha8>`. - `GlobalObjectMethodTable` holds the embedder hooks of a JSC global object. `moduleLoaderFetch` loads a module's source. WebCore uses `referrer` for the `Referer` header. - The DFG is JSC's optimizing JIT. An Absence PropertyCondition is its proof that a structure lacks a property. A static property table holds a class's built-in properties until the first access reifies them, so a structure can own a property its property table does not list. <details><summary>Notes</summary> - Conflict resolutions, the per-commit review of the upstream range (API and ABI changes, behavior changes, performance, build) and the `jsc` shell verification are in oven-sh/WebKit#528. - Behavior changes in the range that are visible from JavaScript: the three fixes pinned by the new test; a Wasm validator fix for `try`/`catch` result types that let `ref.test`/`ref.cast` be folded away on a narrower fallthrough type (`8f229fb729`); a use after free when a Proxy `apply` trap frees a `CallLinkInfo` from inside a polymorphic call (`7920db18a5`); a use after free when one thread grows a shared `WebAssembly.Memory` while another frees a sibling memory (`4002a938c8`); OMG inlining a SIMD callee into a non-SIMD function corrupting results (`9a17cd1100`); DFG fixes for a Uint32Array load typed Int32 (`8283d4f127`), a stale abstract value after folding `GetScope` (`cb05a6083a`), unpinned range proofs in integer range optimization (`03a07e4200`), a B3 select specialization leaving a dangling CSE entry (`1d5c10e2f9`) and a BackwardsGraph that dropped a back-edge source so LICM hoisted a control-dependent load (`7d867192b7`). A function whose bytecode metadata would overflow 32-bit offsets now fails with an out of memory error instead of corrupting memory (`97df94ead0`). - Performance changes of note: DFG `VariableEventStream` and FTL `OSRExitDescriptor` values become byte streams (upstream: 66.6 MB to 17.8 MB and 86 MB to 4.8 MB on JetStream3); `JSString::isDefinitelyAtom()` lets value profiling and the JITs skip dereferencing the StringImpl; `op_instanceof` metadata is linked so the LLInt caches `Symbol.hasInstance`; object rest destructuring clones through `objectCloneFast`; Temporal chinese and dangi calendar month walks are memoized (66x); write barriers compare against zero with `test`/`cbz`; GC `stopAllocating` sets and clears newly-allocated bits per word; the Wasm type registry is swept once per GC cycle instead of per module destruction; `table.size` and externref `table.get` are inlined in BBQ and OMG. - API changes Bun compiles against without change: `WTF::Expected` is now `std::expected` (Bun's `ExceptionOr`, `JSDOMConvertResult` and `CallbackResult` satisfy its constraints); `ScopedLambda` is non-copyable and takes a lambda directly; `ThreadSafeWeakPtr` loses its tagging parameter and locks on copy (Bun's `MessagePort` and `BroadcastChannel` registries); `VM::ClientData` gains a virtual `reconcileWeakReferencesAtGCEnd` with a default body; `ErrorInstance::finishCreation(VM&, StackTraceCapturePolicy)` becomes `finishCreationForEmbedderError`. - On Apple silicon, `d9d2fbd881` changes how JSC counts performance cores (M5 Pro/Max report a "Super" level), which sizes `numberOfGCMarkers` and the low-core DFG/FTL threshold scaling. - The fork keeps its own `YarrJIT.cpp`. Upstream's two Yarr commits in the range (lookbehinds with quantified groups and with lookaheads in the JIT) are ports of what the fork already compiles; their four tests pass on the fork's JIT and interpreter. - Suites run on the debug + ASAN build against the merged tree: `test/js/bun/jsc`, `bun/jsc-stress`, `node/events`, `node/util`, `node/vm`, `node/module`, `bun/resolve`, `node/worker_threads`, `bun/wasm`, `web/url`, `web/atomics`, `web/temporal`, `web/intl`, `web/workers`, `node/string_decoder`, `bundler/bundler_compile`, `bundler/bun-build-api`, `bundler/bun-build-compile`: 3,719 pass. The failures, all checked: 5 s or per-test timeouts under debug + ASAN that pass with a longer timeout (DOMJIT warm-up loops, `parseArgs` stress, `util.inspect`, the `vm.Script` leak check, worker termination, the `bun-build-api` bytecode tests, `bun-build-compile`'s `--compile --bytecode` cases; the compiled-executable aliasing check was run by hand and keeps 12.7 MB of instruction streams out of anonymous memory); tests that fail the same way on a debug + ASAN build of main at the current pin (`compile/HelloWorldWithProcessVersionsBun`, the worker "message flood" timing check, and the ASAN-only `terminate() while dns.lookup()` test, which fails on a LeakSanitizer report for a `node_fs_binding::Binding` that main already leaks, tracked separately); and one cascade from those (the cross-process `structured-clone` child). - #40674 moved the pin to `f5deafe090` and landed first; this branch is rebased on it. CI ran at the preview pin `autobuild-preview-pr-528-41528be5` (same tree as the merge commit) before the pin moved to the merge commit's release. </details> <!-- robobun:evidence:begin --> --- **[decide:webkit]** gate passed · iteration 2 · 6 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: BUILD FAILED (no junit output) $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/bun/jsc/webkit-upgrade-6b879687ee.test.ts" ninja: Entering directory `/workspace/bun/build/debug' [1/167] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 243 extern-C blocks audited [2/167] gen JSSink.{cpp,h,lut.h,rs} generated_jssink.rs: 7 sinks, 84 exported symbols Generating /workspace/bun/build/debug/codegen/JSSink.lut.h from /workspace/bun/build/debug/codegen/JSSink.lut.txt [3/167] gen ZigGeneratedClasses.{cpp,h,rs} Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts - ResolveMessage (15 fields) - BuildMessage (10 fields) Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts - Archive (4 fields, 1 class fields) Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts - ResourceUsage (8 fields) - Subprocess (20 fields) Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts - CronJob (5 fields) Found 3 classes from /workspace/bun/src/runtime/api/filesystem_router.classes.ts - FileSystem ... (truncated) release without fix: all passed bun test v1.4.1-canary.1 (b443464) test/js/bun/jsc/webkit-upgrade-6b879687ee.test.ts: (pass) WebKit 6b879687ee upgrade > an Absence PropertyCondition consults non-reified static property tables (33876268a1) [16.06ms] (pass) WebKit 6b879687ee upgrade > checked arithmetic keeps its overflow and negative zero checks under DCE (9f3eea6f46) [25.47ms] (pass) WebKit 6b879687ee upgrade > a destructuring pattern nested too deep does not crash bytecode generation (752ab90072) [56.02ms] 3 pass 0 fail 13 expect() calls Ran 3 tests across 1 file. [195.00ms] __F:0:S:0 ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" "test/js/bun/jsc/webkit-upgrade-6b879687ee.test.ts" bun test v1.4.1 (65362b5) test/js/bun/jsc/webkit-upgrade-6b879687ee.test.ts: (pass) WebKit 6b879687ee upgrade > a destructuring pattern nested too deep does not crash bytecode generation (752ab90072) [403.08ms] (pass) WebKit 6b879687ee upgrade > an Absence PropertyCondition consults non-reified static property tables (33876268a1) [455.66ms] (pass) WebKit 6b879687ee upgrade > checked arithmetic keeps its overflow and negative zero checks under DCE (9f3eea6f46) [1330.17ms] 3 pass 0 fail 13 expect() calls Ran 3 tests across 1 file. [3.50s] __F:0:S:0 release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision 19abbf1 features baseline 23 deps, 131 codegen, 1172 objects in 715ms ninja: Entering directory `/workspace/bun/build/release' [1/145] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 243 extern-C blocks audited [2/145] gen ZigGeneratedClasses.{cpp,h,rs} Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts - ResolveMessage (15 fields) - BuildMessage (10 fields) Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts - Archive (4 fields, 1 class fields) Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts - ResourceUsage (8 fields) - Subprocess (20 fields) Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts - CronJob (5 fields) Found 3 classes from /workspace/bun/src/runtime/api/filesystem_router.classes.ts - FileSystemRouter (5 fields) - FrameworkFileSystemRouter (2 fields) - MatchedRoute (8 fields) Found 1 classes from /workspace/ ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` scripts/build/deps/webkit.ts | 2 +- .../verify-baseline-static/allowlist-aarch64.txt | 3 +- src/jsc/bindings/ZigGlobalObject.cpp | 10 +- src/jsc/bindings/ZigGlobalObject.h | 4 +- src/runtime/bake/BakeGlobalObject.cpp | 6 +- test/js/bun/jsc/webkit-upgrade-6b879687ee.test.ts | 168 +++++++++++++++++++++ 6 files changed, 181 insertions(+), 12 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 2 <details><summary>evidence per changed file</summary> ``` file reads edits tests scripts/build/deps/webkit.ts 0 0 0 scripts/verify-baseline-static/allowlist-aarch64.txt 0 0 0 src/jsc/bindings/ZigGlobalObject.cpp 0 0 0 src/jsc/bindings/ZigGlobalObject.h 0 0 0 src/runtime/bake/BakeGlobalObject.cpp 0 0 0 test/js/bun/jsc/webkit-upgrade-6b879687ee.test.ts 1 1 0 ``` </details> <!-- robobun:evidence:end -->
This was referenced Sep 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Follow-up to #40643. When a compiled executable pre-registers its embedded module graph, each module got a fetch promise, a module promise and a resolved load promise that nothing ever awaits (the graph walk reads
[[LoadedModules]]/record()directly). With oven-sh/WebKit@f5deafe090cf:ModuleRegistryEntry::provideModule(vm, record)stores the record without allocating any promise or reaction (and no extra GC field);ensureFetchPromise()/ensureModulePromise()hand back ones already settled with the record if a later top-level load asks, andmoduleLoadTopSettledtreats a record result as already provided.AbstractModuleRecord::evaluateModuleSyncevaluates aSyntheticModuleRecord(Bun'snode:*/bun:*builtins) directly instead of wrappingundefinedin a fresh promise — InnerModuleEvaluation hits that once per import edge to a builtin.markLoaded()states that the record's[[LoadedModules]]is complete;hostLoadImportedModuletreats such an entry as loaded and only materializes a load promise (loadedPromise()) for the edge that actually needs one (in practice: each root).Bun's
registerStandaloneClosurenow calls those two instead of building promises itself.claude --help(2.1.250,--compile --bytecode --splitting)JSPromise::createcallsThe remaining promise allocations are ~10 per root (JSC's top-level
loadModule/ evaluate chain × 61 roots here); nothing is per module or per import edge anymore.How did you verify your code works?
test/bundler/bundler_compile_splitting.test.ts(incl. the host-hook-count assertions added in #40643),bun-build-compile.test.ts,bundler_compile.test.ts; drove compiled apps by hand (static/dynamic imports, cycles through the entry, builtins, Worker,import.meta.resolve, missing embedded path, CJS + bytecode); built the Claude Code CLI with this branch and comparedBUN_JSC_dumpModuleLoadingState/ promise / microtask counts againstmain.