[JSC] JSModuleLoader::clearAll(): pin prelinked edges in one pass - #627
[JSC] JSModuleLoader::clearAll(): pin prelinked edges in one pass#627dylan-conway wants to merge 1 commit into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs — the new bulk-forget path produces the same set of pinned edges as the old per-slot loop, just without the O(n²) importer scan. Leaving final sign-off to a human since this is JSC runtime module-resolution bookkeeping and /Source/JavaScriptCore is CODEOWNERS-gated.
What was reviewed:
- Verified the hoisted
pinPrelinkedEdges(importer, onlyTarget)is byte-for-byte the old lambda body, withthis->access replacing the[&]capture. - Traced the null-key fast path: pinning runs before any
slot.clear()/m_prelinkedRecordRemoved.quickSet(), soprelinkedRecordForResolution(target)sees the same populated table the old first iteration did; every edge (a→b) with both slots populated is pinned exactly once, matching the old order-dependent walk. - Confirmed the non-null-key branch is unchanged aside from dropping the now-dead
!keyOrNullForAlldisjunct.
Extended reasoning...
Overview
The PR touches two files in Source/JavaScriptCore/runtime/: JSModuleLoader.cpp and JSModuleLoader.h. It hoists a local lambda into a public overload pinPrelinkedEdges(AbstractModuleRecord*, uint32_t) and adds a dedicated fast path in forgetPrelinkedRecordsWithKey(nullptr) (the clearAll() case) that pins every importer's outgoing edges in one pass while the table is still fully populated, then clears and marks each slot. The single-key removeEntry path and forgetPrelinkedRecord(uint32_t) are semantically unchanged. All of this lives inside the USE(BUN_JSC_ADDITIONS) block.
Security risks
None identified. This is internal bookkeeping over an already-trusted in-memory module graph; there is no parsing, no untrusted input, no auth/crypto/permissions surface. The only correctness-adjacent risk would be dropping a graph edge (leaving a module unable to resolve an import after clearAll()), and I traced the old vs. new edge sets to be identical: previously each edge was pinned either when its importer left (outgoing pass) or when its target left first (incoming pass); now every populated importer pins all outgoing edges before anything is cleared, which yields exactly the same set. Slots already marked in m_prelinkedRecordRemoved before the call still return null from prelinkedRecordForResolution in both versions.
Level of scrutiny
Moderate. The diff is small (~40 lines) and mechanical, but it sits in JSC's module loader where a missed edge would surface as a hard-to-reproduce "binding resolves to undefined after registry clear" bug. /Source/JavaScriptCore is listed in .github/CODEOWNERS (@ WebKit/jsc-reviewers), and while that file is an upstream artifact in Bun's fork and explicitly says contributors don't "own" components, the guideline to defer on CODEOWNERS-covered paths applies. A reviewer familiar with the prelinked-graph invariants should confirm the equivalence argument.
Other factors
The PR description reports manual end-to-end testing (a --compile --bytecode program exercising live bindings, namespace imports, re-exports, and dynamic import across clearAll()) with identical output before/after, plus a profile showing the hot symbol gone. There's no automated test added, which is typical for this fork's performance changes. The header addition is a plain non-exported method declaration alongside the existing overload — no ABI or JS_EXPORT_PRIVATE concerns.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour. WalkthroughJSModuleLoader adds importer-scoped prelinked edge pinning. Module-index pinning delegates to the new overload. Clearing all prelinked records pins importer edges before removing slots. ChangesPrelinked edge pinning
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The updated clear-all path preserves prelinked edges before registry removal without introducing an identified merge risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the performance issue, implementation change, preserved behavior, and testing. It does not follow the repository template because it omits the Bugzilla bug title and link, review status, and changed-file or function list.
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
Preview Builds
|
forgetPrelinkedRecordsWithKey(nullptr) called pinPrelinkedEdges(i) for every registered slot, and each call walked the request lists of all the other registered records looking for edges into i: O(records x edges) for a registry that is being dropped as a whole. When every slot leaves, first pin each record's edges to all of its registered targets, while the table is still complete, then clear the slots as before: O(edges). The records end up with the same [[LoadedModules]] entries (every edge between two registered records, none into slots already removed). Removing the records of a single key is unchanged. pinPrelinkedEdgesOf(importer, onlyTarget) is the former local lambda.
5fe89a3 to
31c9967
Compare
1 participant
JSModuleLoader::clearAll()drops a loader's whole registry. With a prelinked module graph it first has to move the graph edges that the loader's table was holding into the records' own[[LoadedModules]], so records that are still referenced keep resolving their imports.It did that through
forgetPrelinkedRecordsWithKey(nullptr), which calledpinPrelinkedEdges(i)for every registered slot; each of those calls walks the request lists of all the other registered records looking for edges intoi. That is O(records × edges) for a singleclearAll().This makes the clear-everything case one pass: pin each registered record's edges to all of its registered targets while the table is still complete, then clear the slots — O(edges). The end state is the same as before: every edge between two registered records is in the importer's
[[LoadedModules]], and none are added for slots that had already been removed.removeEntry(key)(records of one key) is unchanged, andpinPrelinkedEdgesOf(importer, onlyTarget)is the former local lambda; the clearing loop itself is untouched apart from skipping the per-slot pin when everything is being cleared.Testing
Built Bun against this branch. A
--compile --bytecode(with splitting) program that loads the same modules into several loaders, clears them while functions from their records are still referenced, and then keeps calling those functions (liveletbindings,import * as ns,export * fromre-exports, dynamicimport()) prints the same output before and after this change, and a profile of repeated load/clearAll()cycles over a ~600-module graph no longer showspinPrelinkedEdges(previously the top self-time symbol on that thread).