perf(JSC): O(E+N) GetModuleNamespace; iterative InnerModuleLoading#200
Merged
Conversation
- AbstractModuleRecord::getModuleNamespace: replace per-name star-graph walk (O(names × edges)) with a single BFS that records each name's unique Local/Namespace binding. A name with exactly one binding is Resolved by spec regardless of resolveSet, so only names with >1 binding or an Indirect entry fall back to resolveExportImpl. - AbstractModuleRecord::resolveExportImpl: read m_resolutionCache in Type::Query unconditionally. Only Resolved values are ever written (root or local), so a non-empty resolveSet can only turn paths to the cached binding into null, and merge(Resolved, null) = Resolved. - JSModuleLoader::innerModuleLoading: drain a worklist on the state instead of recursing through the FinishLoadingImportedModule -> ContinueModuleLoading re-entry. continueModuleLoading short-circuits already-visited modules without a function call. - ModuleGraphLoadingState: inline the trivial accessors; widen m_visitedSet to AbstractModuleRecord* so the per-edge contains check runs before dynamicDowncast. - JSModuleLoader::loadRequestedModules: memoize the GraphLoadingState capability per module (skip rejected so retry still re-walks). bench/module-loader (500 modules, 30k star edges): GetModuleNamespace + ResolveExport drop from ~48% of cycles to <1%. Roughly 2x faster end-to-end vs previous.
WalkthroughThe changes optimize ECMAScript module graph loading by refactoring from recursive call-based traversal to an iterative state-managed worklist approach, while consolidating module namespace export resolution into a single graph traversal with unified conflict detection. Changes
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
JSTests/modules/self-star-link.js: an Indirect cycle that falls back to star (JSC's IndirectFallback) makes the cached Resolved value path-dependent — C resolved as a root caches 'D', but C reached from A must yield 'E'. The original !foundStarLinks gate is correct here. The getModuleNamespace BFS doesn't depend on this read, so no perf change.
Preview Builds
|
Jarred-Sumner
added a commit
to oven-sh/bun
that referenced
this pull request
Apr 26, 2026
oven-sh/WebKit#200: - O(E+N) GetModuleNamespace via single-BFS unique-binding fast path - iterative InnerModuleLoading worklist - inline ModuleGraphLoadingState accessors - memoize LoadRequestedModules per module bench/module-loader (500 modules / 30k star edges): ~2x faster end-to-end.
The per-module WriteBarrier<JSPromise> kept one extra Promise alive for the lifetime of every module record (visible in fetch-leak's heap-stats threshold). The memoization only helps the rare case of two concurrent import()s of the same module before either fetches; the original per-state walk handles that correctly, just with a redundant traversal. Not worth a permanent +1 Promise per module.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/JavaScriptCore/runtime/JSModuleLoader.cpp`:
- Around line 841-861: The current code enqueues an already-loaded child via
state->enqueueInnerLoad(loaded) and then continues to the next sibling, which
breaks the required depth-first descent; change the logic in the block that
checks module->loadedModules() (ModuleMapKey lookup) to process cached children
immediately using an explicit stack or frame instead of enqueueing them: when
you find iter->value.m_module (loaded) and state->containsVisited(loaded) is
false, push or process that loaded AbstractModuleRecord* (the same work that
enqueueInnerLoad would trigger) before advancing to the next request so
descendant HostLoadImportedModule calls (hostLoadImportedModule) and synchronous
failures occur before siblings; preserve the state->isLoading() checks and the
existing payload creation (ModuleLoaderPayload::create) and only call
hostLoadImportedModule for requests after the cached-child stack/frame has been
drained.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 599ee7ac-30ee-40fe-a40f-a3dae38a12dc
📒 Files selected for processing (2)
Source/JavaScriptCore/runtime/AbstractModuleRecord.cppSource/JavaScriptCore/runtime/JSModuleLoader.cpp
Comment on lines
+841
to
+861
| if (!loadedModulesEmpty) { | ||
| if (auto iter = module->loadedModules().find(ModuleMapKey { request.m_specifier.impl(), request.type() }); iter != module->loadedModules().end()) { | ||
| // 2.d.ii.1. Perform InnerModuleLoading(state, record.[[Module]]). | ||
| AbstractModuleRecord* loaded = iter->value.m_module.get(); | ||
| if (state->containsVisited(loaded)) | ||
| state->setPendingModulesCount(state->pendingModulesCount() - 1); | ||
| else | ||
| state->enqueueInnerLoad(loaded); | ||
| if (!state->isLoading()) | ||
| break; | ||
| continue; | ||
| } | ||
| } | ||
| // 2.d.iii. Else, | ||
| // 2.d.iii.1. Perform HostLoadImportedModule(module, request, state.[[HostDefined]], state). | ||
| // The payload only carries `state`; one instance is sufficient for every | ||
| // request originating from this drain. Async loads stash it in their | ||
| // ModuleLoadingContext, but they all want the same state back. | ||
| if (!payload) | ||
| payload = ModuleLoaderPayload::create(vm, state); | ||
| JSPromise* promise = hostLoadImportedModule(globalObject, cyclic, request, payload, state->scriptFetcher(), true); |
There was a problem hiding this comment.
Keep [[LoadedModules]] descent depth-first.
This path now enqueues an already-loaded child and keeps iterating later sibling requests. Step 2.d.ii.1 used to descend into that child immediately, so this reorders descendant HostLoadImportedModule calls and synchronous failures relative to later siblings. A graph like [A_loaded, B] can now start B before a synchronous failure in A_loaded's subtree is observed, which changes host side effects and potentially first-error precedence. Use an explicit stack/frame so cached children are processed before advancing to the next sibling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Source/JavaScriptCore/runtime/JSModuleLoader.cpp` around lines 841 - 861, The
current code enqueues an already-loaded child via
state->enqueueInnerLoad(loaded) and then continues to the next sibling, which
breaks the required depth-first descent; change the logic in the block that
checks module->loadedModules() (ModuleMapKey lookup) to process cached children
immediately using an explicit stack or frame instead of enqueueing them: when
you find iter->value.m_module (loaded) and state->containsVisited(loaded) is
false, push or process that loaded AbstractModuleRecord* (the same work that
enqueueInnerLoad would trigger) before advancing to the next request so
descendant HostLoadImportedModule calls (hostLoadImportedModule) and synchronous
failures occur before siblings; preserve the state->isLoading() checks and the
existing payload creation (ModuleLoaderPayload::create) and only call
hostLoadImportedModule for requests after the cached-child stack/frame has been
drained.
Jarred-Sumner
added a commit
to oven-sh/bun
that referenced
this pull request
Apr 26, 2026
Jarred-Sumner
added a commit
to oven-sh/bun
that referenced
this pull request
Apr 26, 2026
Companion to oven-sh/WebKit#200. Adds correctness coverage for the new JSC `GetModuleNamespace` single-BFS fast path: - deep `export * from` chain — every transitive binding is present - two siblings with the same local — name is excluded (ambiguous), but a local in the importing module shadows correctly - indirect re-export through a star chain — still resolves via the slow path These pass on both the current and patched JSC (the optimization preserves observable behavior). Bench impact from the WebKit change (500 modules, 30k star edges, local release without LTO): - before: ~694ms - after: ~320ms - node 24: ~525ms, deno 2.7: ~157ms Separately found while auditing: a fresh `import()` that reaches a module whose previous load failed resolves on Bun (all versions back to 1.3.11) but rejects on Node/Deno — pre-existing, not touched by this PR. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This was referenced Apr 27, 2026
xhjkl
pushed a commit
to xhjkl/bun
that referenced
this pull request
May 14, 2026
…#29729) Companion to oven-sh/WebKit#200. Adds correctness coverage for the new JSC `GetModuleNamespace` single-BFS fast path: - deep `export * from` chain — every transitive binding is present - two siblings with the same local — name is excluded (ambiguous), but a local in the importing module shadows correctly - indirect re-export through a star chain — still resolves via the slow path These pass on both the current and patched JSC (the optimization preserves observable behavior). Bench impact from the WebKit change (500 modules, 30k star edges, local release without LTO): - before: ~694ms - after: ~320ms - node 24: ~525ms, deno 2.7: ~157ms Separately found while auditing: a fresh `import()` that reaches a module whose previous load failed resolves on Bun (all versions back to 1.3.11) but rejects on Node/Deno — pre-existing, not touched by this PR. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
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
1 participant
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.
perf(JSC): O(E+N) GetModuleNamespace; iterative InnerModuleLoading
walk (O(names × edges)) with a single BFS that records each name's
unique Local/Namespace binding. A name with exactly one binding is
Resolved by spec regardless of resolveSet, so only names with >1
binding or an Indirect entry fall back to resolveExportImpl.
Type::Query unconditionally. Only Resolved values are ever written
(root or local), so a non-empty resolveSet can only turn paths to the
cached binding into null, and merge(Resolved, null) = Resolved.
instead of recursing through the FinishLoadingImportedModule ->
ContinueModuleLoading re-entry. continueModuleLoading short-circuits
already-visited modules without a function call.
m_visitedSet to AbstractModuleRecord* so the per-edge contains check
runs before dynamicDowncast.
capability per module (skip rejected so retry still re-walks).
bench/module-loader (500 modules, 30k star edges):
GetModuleNamespace + ResolveExport drop from ~48% of cycles to <1%.
Roughly 2x faster end-to-end vs previous.
bench/module-loader (500 modules, 30k star edges)
Bun test/js/bun/resolve + test/js/node/module suites unchanged vs baseline.