Repository navigation
[JSC] Share one ScriptFetchParameters per type instead of allocating one per module request - #561
Conversation
…one per module request ScriptFetchParameters for the built-in types (None/JavaScript/WebAssembly/JSON/Text) carries nothing but the one-byte type, yet every import statement's ModuleRequest, every loadModule()/requestImportModule() and every export entry that names a module allocated its own RefCounted object and kept it for the life of the module record. In a large module graph (Claude Code: ~800 modules) that is ~2 MB of immortal 32-byte allocations. create(Type) now returns a process-wide immortal instance per type; createUnique(Type) keeps the old behaviour for the callers that attach per-request state (WebCore's ModuleFetchParameters subclass constructs directly; a HostDefined type string and the Bun import-attributes map still get their own object, and the one site that called setAttributes() on a freshly created object now always creates a unique one). The class becomes ThreadSafeRefCounted since module records in every VM (workers included) ref the shared instances.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it introduces process-wide shared ScriptFetchParameters instances referenced across VMs and switches the class to ThreadSafeRefCounted, a human look at the shared-state invariants would still be worthwhile.
What was reviewed:
create(Type)lazy init:std::call_once+LazyNeverDestroyedis thread-safe;HostDefined(index 5) falls through tocreateUniqueand no caller passes it here anyway.- Only mutation path is
setAttributes()at JSModuleLoader.cpp:503; the new guard ensures shared per-type instances are never mutated (HostDefined params are always freshly allocated viacreate(String)). - Other
create(Type)callers (JSModuleLoader.cpp:360/385, Completion.cpp:254) only readtype(), andgetRegisteredMayBeNullkeys on type value not pointer identity. - WebCore's
ModuleFetchParameterssubclass has its owncreate()and constructs directly, so it's unaffected by the base-class factory change.
Extended reasoning...
Overview
This PR converts ScriptFetchParameters::create(Type) from a per-call allocation into a lookup of one of five process-wide immortal instances (one per built-in Type), stored in a LazyNeverDestroyed<std::array<RefPtr, 5>> initialized under std::call_once. A new createUnique(Type) preserves the old allocate-per-call behaviour. The class moves from RefCounted to ThreadSafeRefCounted so module records in multiple VMs/workers can safely ref the shared instances. The one Bun-additions mutation site (setAttributes in JSModuleLoader::importModule) is updated to allocate a fresh unique instance whenever it would otherwise mutate a shared per-type instance. A new ScriptFetchParameters.cpp translation unit is added to Sources.txt.
Security risks
None identified. There is no user-controlled input flowing into the static array index (it's an internal enum), no auth/crypto/permission code, and the ThreadSafeRefCounted change strictly increases safety over RefCounted. The shared instances carry only a one-byte type and empty default members; the only mutator (setAttributes) is guarded so it never touches a shared instance.
Level of scrutiny
Moderate. The diff is small and follows established WTF idioms, but it introduces process-global shared state in the module loader — a core runtime path exercised by every import — and relies on the invariant that no current or future caller mutates a ScriptFetchParameters obtained from create(Type). The guard in importModule uses type() != HostDefined as a proxy for "this is a shared instance", which is correct today but is an indirect check worth a maintainer's eye. /Source/JavaScriptCore is also listed in .github/CODEOWNERS (inherited from upstream), which nudges toward a human sign-off.
Other factors
I traced every ScriptFetchParameters::create( caller and the sole setAttributes call site: none read pointer identity, and the registry key is (specifier, type) by value. WebCore's ModuleFetchParameters subclass shadows create with its own factory and is unaffected. The PR author reports the JSTests module suite and import-attributes stress tests pass unchanged. Nothing blocks merging from a correctness standpoint that I can see; deferring purely because shared cross-VM state in the module loader merits a second pair of eyes.
|
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 (4)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughChangesScript Fetch Parameter Lifecycle
Merge Risk: ⚪ Minimal · up to Built-in script fetch parameter types now reuse shared thread-safe instances while attribute-bearing imports retain unique mutable parameters. The current change has no identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides a clear rationale, implementation summary, scope, and verification results. It does not follow the repository template because it omits the bug title and Bugzilla URL, the reviewer line, and the required changed-file and function list. Resolution Add the associated Bugzilla title and URL, include the required
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
|
… target's return value for the caller's realm Pins WEBKIT_VERSION at the preview build of oven-sh/WebKit#590. A callable returned by a wrapped function whose target is not a plain JSFunction (a callable Proxy, or a built-in such as the realm's own Function) was wrapped for the target's realm instead of the caller's. The caller could read the other realm's Function constructor off the wrapper's prototype and reach that realm's global object, in both directions. The range from 2e2aa2290fac also contains oven-sh/WebKit#561, #566 and #568, already merged on oven-sh/WebKit main.
1 participant
ScriptFetchParametersfor the built-in types (None / JavaScript / WebAssembly / JSON / Text) carries nothing but a one-byteType, yet every import statement'sModuleRequest, everyloadModule()/requestImportModule(), and (in Bun) every typed export entry didadoptRef(*new ScriptFetchParameters(type))and kept it for the life of the module record. A mimalloc heap profile of an idle Claude Code process (~800 modules) shows ~2.1 MB of these immortal 32-byte objects.What
ScriptFetchParameters::create(Type)returns a process-wide immortal instance per built-in type (lazily created once);createUnique(Type)keeps the allocate-per-call behaviour.ModuleFetchParameterssubclass constructs directly (unchanged),create(const String& hostDefinedImportType)is unchanged, and the one Bun-additions site inJSModuleLoader::requestImportModulethat callssetAttributes()now always does so on acreateUnique()object (it previously could only have been a fresh one anyway).RefCounted→ThreadSafeRefCounted: module records in every VM (workers too) ref the shared instances.Verification
JSTests/modules/*and the import-attributes stress tests give identical results to the parent commit when run under the bare shell (102 pass; the same 8 need the harness either way). Static + dynamic import withwith { type: "json" }works. In Bun/Claude Code theScriptFetchParametersline disappears from the idle heap profile (numbers in the Bun PR).