Conversation
|
@robobun adopt |
|
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 (1)
Included review availability: Your plan includes up to 5 reviews per rolling hour; 1 remains after this review. WalkthroughChangesThe subspace initialization API now accepts direct client and server IsoSubspace initialization
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the SKILL.md nit, I verified the refactor is behavior-preserving: reading getCustomHeapCellType/heap.*HeapCellType before taking heapData.lock() is safe (those members are immutable after JSHeapData construction); all slot members live on the base DOMClientIsoSubspaces/DOMIsoSubspaces so the pointer-to-member applies correctly to the Extended* instances; the "T"_s name matches what ISO_SUBSPACE_INIT(..., T) stringized before (the literal token T); and no old-form 4-lambda call sites remain in src/.
Extended reasoning...
The only finding is a stale documentation snippet. The core change — extracting the IsoSubspace-creation slow path into one shared NEVER_INLINE function keyed by pointer-to-member slots — reads as a faithful mechanical hoist. I checked the four things a reviewer would worry about on this path (lock ordering vs. the heapCellType read, pointer-to-member base/derived compatibility, the constructor argument equivalence with ISO_SUBSPACE_INIT, and completeness of the call-site sweep) and each held. Deferring rather than approving because this is 129 files on the GC allocation hot path and warrants a human sign-off on the equivalence claim.
There was a problem hiding this comment.
I re-reviewed after d8806a4 and didn't find any issues — the SKILL.md nit is addressed and a grep confirms no old-form subspaceForImpl call sites remain in src/. Given this refactors the IsoSubspace creation path that every Bun-side JSC class allocates through, a maintainer look before merge is still worthwhile.
What was reviewed:
subspaceForImplSlowpreserves the old inline path's behavior: same lock, same server-slot null check, same two slot writes, sameoutputConstraintSpacesappend.- The per-type
static_assert, cell-type selection, andvisitOutputConstraintscomparison stay compile-time in the inline template;getCustomHeapCellTypenow runs before the lock, which is fine (it just returns a reference to an eagerly-constructedJSHeapDatamember). - Spot-checked ~20 call sites including the
UseCustomHeapCellType::Yesones (NodeVMGlobalObject, NapiHandleScopeImpl, BakeGlobalObject, ZigGlobalObject, NativePromiseContext) — all pass the same member slots the old lambdas touched.
Extended reasoning...
Overview
This PR de-duplicates the IsoSubspace creation slow path across all ~250 subspaceFor<T> instantiations. WebCore::subspaceForImpl in BunClientData.h is refactored: instead of four templated lambdas (get/set client, get/set server), callers pass two pointer-to-member slots (&DOMClientIsoSubspaces::m_clientSubspaceForFoo, &DOMIsoSubspaces::m_subspaceForFoo). The inline template keeps only the fast path (load client slot → return if set) and the compile-time per-type work (static_assert, cell-type selection, visitOutputConstraints comparison); the once-per-(VM,type) creation path moves to a single NEVER_INLINE subspaceForImplSlow in BunClientData.cpp. 127 hand-written call sites plus the generate-classes.ts / generate-jssink.ts templates and the implementing-jsc-classes-cpp skill snippet are updated to the new form. Net binary-size win is 64–240 KB per target.
Security risks
None. No user input is involved; this is internal GC-allocator plumbing. The pointer-to-member arguments are compile-time constants at every call site.
Level of scrutiny
High. subspaceFor<T> is what JSC calls on every allocation of every Bun-defined JS class; the slow path constructs the type's IsoSubspace and registers it for output constraints. A regression here would surface as allocation failures, wrong-destructor calls, or missed GC visits across the whole runtime. That said, the actual logic change is ~25 lines in two files; the other 128 files are a mechanical signature update that would fail to compile if wrong.
I verified behavior preservation line-by-line against the old body: the lock is taken at the same point relative to the server-slot read/write; the server subspace is created only when the slot is empty and appended to outputConstraintSpaces under the same predicate; the client subspace is always freshly created and stored (matching the old setClient on every miss). The IsoSubspace name argument is now the literal "T", which is what ISO_SUBSPACE_INIT(heap, cellType, T) stringized when T was the template parameter — a diagnostic label only. sizeof(T) and T::numberOfLowerTierPreciseCells are passed explicitly instead of via the macro. Moving the getCustomHeapCellType call outside the lock is safe: it dereferences an eagerly-constructed IsoHeapCellType member of JSHeapData, not the locked subspaces() map.
Other factors
CI passed 179/179 on the first run and the PR description lists debug-build verification across workers, node:vm, streams, sqlite, crypto, and web-globals — all subsystems that exercise distinct IsoSubspace slots including several UseCustomHeapCellType::Yes cases. My earlier nit (stale SKILL.md snippet) was addressed in d8806a4; a grep confirms no old-form call sites remain in src/. I'm deferring rather than approving only because this sits directly on the GC allocation path for every class in the runtime, which per the review guidelines warrants a maintainer sign-off even for a mechanical refactor.
Guards the invariant this change introduces: the creation path exists in one out-of-line place, and new classes reach it through subspaceForImpl. The source-lints workflow now also triggers on the C++ and codegen files the lint reads.
There was a problem hiding this comment.
I reviewed this PR and found no bugs; the earlier SKILL.md nit was addressed in d8806a4. Because it rewrites WebCore::subspaceForImpl — the hook JSC calls on every allocation of every Bun-defined cell type — a maintainer look is still worthwhile before merge.
What was reviewed:
subspaceForImplSlowreproduces the exactISO_SUBSPACE_INITargs (name"T"_s,vm.heap, cell type,sizeof(T),numberOfLowerTierPreciseCells), same lock, same output-constraint registration, same two slot writes.- Pointer-to-member slot types work: all
m_clientSubspaceFor*/m_subspaceFor*members live on theDOMClientIsoSubspaces/DOMIsoSubspacesbase (theExtended*subclasses are empty), soDOMClientIsoSubspaces::*applied to anExtendedDOMClientIsoSubspaces&is well-defined. getCustomHeapCellTypenow runs before the heap lock instead of under it — it only returns a reference to aJSHeapDatamember, so no ordering concern.- Spot-checked ~20 call sites plus both codegen templates for slot-name mismatches; the compiler enforces the rest.
Extended reasoning...
Overview
This PR refactors WebCore::subspaceForImpl in src/jsc/bindings/BunClientData.{h,cpp}: the once-per-(VM,type) IsoSubspace creation path moves out of the ALWAYS_INLINE template into a single NEVER_INLINE function subspaceForImplSlow, and the four getter/setter lambdas each caller passed are replaced by two pointer-to-data-member arguments. 127 hand-written call sites, both codegen templates (generate-classes.ts, generate-jssink.ts), and the implementing-jsc-classes-cpp skill snippet are updated to the new form. A new source-lint test asserts IsoSubspace construction stays confined to BunClientData.cpp, and source-lints.yml is widened to run on .h/.cpp/codegen/** changes. CI reports 64-240 KB smaller stripped binaries per target.
Security risks
None identified. No user input reaches this path; it is purely allocation-infrastructure plumbing. The pointer-to-member offsets are compile-time constants and the type system prevents passing a slot from the wrong struct.
Level of scrutiny
High. subspaceFor<T> is the static hook JSC calls on every allocation of every Bun-defined cell type; a wrong cell-type, wrong size, or missed output-constraint registration here is a type-confusion or UAF vector across the whole runtime. That is exactly the "most-blocked category" in REVIEW.md's native memory-safety section, and src/jsc/bindings/ is CODEOWNER-gated. The refactor itself is small and I believe behavior-preserving — the fast path is unchanged, and the slow path is a straight extraction with the per-type facts (sizeof(T), numberOfLowerTierPreciseCells, visitOutputConstraints comparison, cell-type selection) still computed in the templated inline part and passed as plain values — but a maintainer familiar with JSC's IsoSubspace invariants should confirm.
Other factors
- The 127 call-site edits are mechanical and compiler-enforced: a mistyped member name or mismatched slot pair fails to compile. I spot-checked a range including the
UseCustomHeapCellType::Yessites (NativePromiseContext,NodeVMGlobalObject,NapiHandleScopeImpl,ZigGlobalObject,BakeGlobalObject) — those keep their custom-heap-cell-type lambda as the fourth argument, unchanged. - The one semantic timing shift I noticed —
getCustomHeapCellType(heapData)now runs beforeLocker locker { heapData.lock() }rather than under it — is safe: those callbacks returnserver.m_heapCellTypeForX, a reference to a member constructed inJSHeapData::JSHeapDataand never mutated. - The
"T"_ssubspace name is not a regression: the oldISO_SUBSPACE_INIT(heap, cellType, T)stringized the template parameter name, so the name was already the literal"T"for every type routed through this helper. - My earlier inline nit (stale SKILL.md snippet) was addressed in d8806a4 and the thread is resolved. First CI run on the full change was 179/179 green.
…e construction (#39833) Follow-up to #39770, which moved the IsoSubspace creation path out of `WebCore::subspaceForImpl` into one out-of-line `subspaceForImplSlow`. #39503 did the same thing and is closed as superseded. These are the two pieces of it that still apply. ### Problem - `.claude/skills/implementing-jsc-classes-cpp/SKILL.md:30-35` still shows the four-lambda `subspaceForImpl` call. That signature no longer exists, so the snippet does not compile when copied. - Nothing stops a new class, or a codegen template, from constructing its own `IsoSubspace` again. The compiler does not enforce that invariant, and a template that does it pays for it once per generated class. ### Fix - The skill snippet now shows the `BUN_SUBSPACE_SLOTS` form that every caller on main uses (for example `src/jsc/bindings/AsyncContextFrame.h:39`), and says where the subspaces are created. - `test/internal/source-lints/iso-subspace-creation.test.ts` scans `src/**/*.{h,cpp}` and `src/codegen/**/*.ts` for `ISO_SUBSPACE_INIT`, `makeUnique<IsoSubspace>` or `new IsoSubspace` outside `src/jsc/bindings/BunClientData.cpp`. It also asserts that file still constructs one, so the lint cannot pass on an empty scan. - `source-lints.yml` now also triggers on `src/**/*.h`, `src/**/*.cpp` and `src/codegen/**`, the files this lint reads (the README in that directory asks for this). `src/codegen/**` replaces the narrower `src/codegen/class-definitions.ts` entry. - Verified: the lint passes on main (`bun bd test` and the released bun). It fails on the tree before #39770, listing the four constructions in `BunClientData.h`, and it fails when a construction is added to a header or to `generate-jssink.ts`, naming the file and line. ### Background - Every JS class with C++ fields has a `subspaceFor<T>` hook that JSC calls on each allocation of `T`. The creation of the subspace behind it (lock, construct, register, fill the two slots) is about 640 bytes of code and runs once per type. Inlined per class it cost about 160 KB of the linux-x64 binary, which is what #39770 removed. - `test/internal/source-lints/` holds grep-style lints over `src/`. They run on GitHub Actions against a released bun and are excluded from the Buildkite shards, so they only run on PRs that touch the paths listed in the workflow. <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 1 · 3 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/internal/source-lints/iso-subspace-creation.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/internal/source-lints/iso-subspace-creation.test.ts bun test v1.4.0 (6e906e4) test/internal/source-lints/iso-subspace-creation.test.ts: (pass) IsoSubspaces are only constructed in BunClientData.cpp [1035.72ms] 1 pass 0 fail 3 expect() calls Ran 1 test across 1 file. [4.46s] Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` .../skills/implementing-jsc-classes-cpp/SKILL.md | 8 +-- .github/workflows/source-lints.yml | 8 ++- .../source-lints/iso-subspace-creation.test.ts | 67 ++++++++++++++++++++++ 3 files changed, 76 insertions(+), 7 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests .claude/skills/implementing-jsc-classes-cpp/SKILL.md 2 2 0 .github/workflows/source-lints.yml 1 1 0 test/internal/source-lints/iso-subspace-creation.test.ts 4 8 0 ``` </details> <!-- robobun:evidence:end -->
2 participants
Problem
WebCore::subspaceForImplinsrc/jsc/bindings/BunClientData.hisALWAYS_INLINE, so its IsoSubspace creation path (take the heap lock, pick the HeapCellType, construct theIsoSubspace, register it for output constraints, construct the per-VMGCClient::IsoSubspace, store both) is inlined into everysubspaceFor<T>in the binary.sizeof(T)and two constants.Fix
The creation path is now one
NEVER_INLINEfunction,subspaceForImplSlowinBunClientData.cpp, taking those per-type values as arguments. The slot to fill is passed as a pointer to member ofDOMClientIsoSubspaces/DOMIsoSubspaces, replacing the four getter and setter lambdas each caller used to pass.subspaceForImplkeeps only the per-allocation fast path inline: load the client data, load the slot, return it if set, otherwise call the slow path. Thestatic_asserton destructibility and thevisitOutputConstraintscheck still happen per type at compile time, in the inline part.Same behavior as before: same subspace per type, same lock, same two slot writes. The
IsoSubspacename stays"T", which is whatISO_SUBSPACE_INIT(heap, cellType, T)stringized whenTwas a template parameter.Every caller is updated (129 files: the hand-written bindings plus the
generate-classes.tsandgenerate-jssink.tstemplates); a grep for the old lambda form finds nothing left insrc/. Theimplementing-jsc-classes-cppskill snippet that showed the old call form is updated too.Stripped binary size, this PR vs its base commit (main build of dc59d3e), from CI's binary-size step:
Test:
test/internal/source-lints/iso-subspace-creation.test.tsgrepssrc/**/*.{h,cpp}andsrc/codegen/**/*.tsfor anything constructing anIsoSubspace(ISO_SUBSPACE_INIT,makeUnique<IsoSubspace>,new IsoSubspace) outsideBunClientData.cpp. It fails on the base commit (the four inlined constructions inBunClientData.h) and passes here, and keeps a future class from growing its own copy of the creation path.source-lints.ymlnow also triggers on the C++ and codegen files it reads.Verified on a debug build: workers, node:vm, Headers, streams, mock functions, bun:sqlite, node:sqlite, node:crypto (hmac, ecdh, x509), FormData, URLSearchParams, MessageChannel, AbortSignal and web-globals tests pass. The generated
ZigGeneratedClasses.huses the new form.Background
IsoSubspace(a size-segregated heap region per type, so a freed slot can only be reused by the same type).JSC::IsoSubspaceis the per-heap object;GCClient::IsoSubspaceis the per-VM allocator handle onto it. Bun creates both lazily the first time a type is allocated.DOMIsoSubspacesandDOMClientIsoSubspacesare structs with onestd::unique_ptrslot per type (the heap-side and VM-side objects respectively).subspaceFor<T>is the static hook JSC calls on every allocation ofTto find the right subspace, which is why its inline body matters and its creation path does not.&DOMIsoSubspaces::m_subspaceForFoo) is a compile-time offset;obj.*slotreads that field. Passing it lets one out-of-line function fill any slot without a per-type lambda.[review] gate passed · iteration 1 · 132 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file