Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 26 days. After that, they cost $0.25 per reviewed file. Or wait 6 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (14)
Comment |
|
Updated 11:11 PM PT - Aug 25th, 2026
✅ @robobun, your commit 6e3d3fe2496229a808ad365b03453a04de0f14ef passed in 🧪 To try this PR locally: bunx bun-pr 37842That installs a local version of the PR into your bun-37842 --bun |
![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. Since it swaps the GC-root storage backing every bun_jsc::Strong, depends on an unlanded WebKit bump (CI is expected red), and carries a measured eden-pause trade-off at high handle counts, a maintainer sign-off is still warranted.
What was reviewed:
Strong.rsslot read/write/clear vs.JSC::Strong::set()semantics — plain store is correct (StrongSet is scanned unconditionally, no barrier).- Teardown ordering:
is_shutting_downguard still needed since StrongSet dies in ~VM beforedeinit_runtime_state. - Grepped for lingering
StrongRootBlock/StrongRefImpl/Bun__StrongRef__setreferences (none) and build-file entries. heapStats/getProtectedObjectsnow delegate to JSC's own walkers; test asserts both see 5000 armed timers and 0 after clear.
Extended reasoning...
Overview
This PR removes the custom StrongRootBlock GC-cell mechanism (added in #35849) that backed bun_jsc::Strong handles, and reverts to using JSC's own strong-handle storage — now JSC::StrongSet after upstream refactor ff64aee116d4 deleted HandleSet. Net -380 lines. Touches: Strong.rs (direct slot read/write, drops Bun__StrongRef__set FFI), StrongRef.{cpp,h} (allocate/deallocate via vm.heap.strongSet()), deletes StrongRootBlock.{h,cpp} + iso-subspace entries + the "Srb" marking constraint + JSVMClientData block-list state, BunJSCModule.h (heapStats/getProtectedObjects back to JSC's own walkers), root.h include swap, and comment-only edits in JSRef.rs/Response.rs.
Security risks
None. No user-controlled input paths; this is internal GC rooting machinery.
Level of scrutiny
High. This is the mechanism that keeps every bun_jsc::Strong-held JSValue alive across GC — timers, sockets, promises, etc. A mistake here is a use-after-GC or a leak affecting the whole runtime. Additionally:
- The PR depends on an unlanded WebKit bump (oven-sh/WebKit#404);
<JavaScriptCore/StrongSet.h>does not exist against the currently-pinned WebKit, so CI cannot validate it yet. Merging before the bump would break the build. - It carries a deliberate performance trade-off the author measured: eden STW at 1M live handles goes from ~3.8 ms to ~10.1 ms (roughly the pre-#35849
HandleSetcost). Below ~100k it's noise. This was requested by a maintainer, but the regression/simplification trade should be acknowledged by a human on the record.
Other factors
- The no-barrier store in
Impl::setmirrorsJSC::Strong::set()and is correct because the "Sh" constraint scans every StrongSet slot on every fixpoint; the PR comments this accurately. - The
is_shutting_downearly-return inImpl::destroyis retained with an updated rationale (StrongSet freed in ~VM phase C beforedeinit_runtime_statephase E) — matches the #31990 crash class. ExternTraits<Bun::StrongRef>::ExternTypechanged fromBun::StrongRefImpl*toJSC::JSValue*, consistent with the newusing StrongRef = std::unique_ptr<JSC::JSValue, StrongRefDeleter>; the Rustadoptside treats it as an opaqueNonNull<Impl>either way.- I grepped the whole repo for
StrongRootBlock,m_strongRootBlock*,StrongRefImpl, andBun__StrongRef__set— no remaining references, including build files. - The updated test asserts
protectedObjectTypeCounts.Timeout,protectedObjectCount, andgetProtectedObjects()all report 5000 armed timers and 0 after clear, and that noStrong*cell type appears inobjectTypeCounts— it fails on main (spareStrongRootBlockretained) and covers the observable contract this PR restores.
Given the WebKit dependency gating CI and the GC-critical surface, this should land with maintainer eyes rather than automated approval.
|
Status: ready for review, blocked on the WebKit bump. CI on this PR fails in every build lane with Additional verification since the description was written, using a local
|
26e085d to
f9d69d5
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// slot backing `Strong` lives in the VM's `JSC::StrongSet` and must be | ||
| /// dropped on the JS thread. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Strong must be dropped on the JS thread (the slot belongs to the VM's | ||
| // `JSC::StrongSet`, which is only touched under the JSLock). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Set a new value for the strong reference. The slot already exists, so | ||
| /// `_global` is only taken to keep the signature interchangeable with | ||
| /// [`Optional::set`], which needs it to allocate one. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Opaque FFI handle: points at the `JSC::JSValue` slot that | ||
| /// `Bun__StrongRef__new` allocated in the VM's `JSC::StrongSet` (the same | ||
| /// storage `JSC::Strong<>` uses); see StrongRef.cpp. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// The slot holds exactly the `EncodedJSValue` bits (`JSC::JSValue` is one | ||
| /// 64-bit word), which is what [`JSValue`] is on this side. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Plain store, like `JSC::Strong::set()`: the GC's strong-handle | ||
| /// constraint scans every slot of the set, so there is no barrier to run. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The slot belongs to the VM's StrongSet, which ~VM frees (teardown | ||
| // phase C, see `VirtualMachine::teardown`), while runtime state that | ||
| // still owns `Strong`s is torn down after that (`deinit_runtime_state`, | ||
| // phase E). `is_shutting_down` is set before teardown starts, and from | ||
| // then on the slot simply dies with the set: the final collection is | ||
| // followed by ~VM's lastChanceToFinalize, so nothing it roots outlives | ||
| // the VM. The Rust VM TLS outlives ~VM. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // leak until that VM's teardown. StrongSet::deallocate requires | ||
| // the JSLock, so it cannot be called from here. Flag in debug so | ||
| // the owning wrapper can queue the drop back to the JS thread. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `JSGlobalObject` is an opaque `UnsafeCell`-backed ZST handle, so | ||
| // `&JSGlobalObject` is ABI-identical to a non-null `*const T`. `new` hands out | ||
| // a slot inside the VM's StrongSet (no heap allocation of its own); `delete` | ||
| // returns it and so stays `unsafe fn`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Plain store, like JSC::Strong::set(): the "Sh" marking constraint scans | ||
| // every StrongSet slot, so there is no barrier to run. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // One slot in the VM's JSC::StrongSet, the storage JSC::Strong<> itself uses. | ||
| // The slot pointer is the handle: bun_jsc::Strong (Strong.rs) reads and writes | ||
| // the JSValue through it directly and only comes back here to release it. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
f9d69d5 to
736c3df
Compare
| /// `JsRef` is `!Send + !Sync` (transitively via `JSValue` and `Strong`): a | ||
| /// `Strong` must be dropped on the JS thread. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// The `JSC::JSValue` slot that `Bun__StrongRef__new` allocated in the VM's | ||
| /// `JSC::StrongSet`; see StrongRef.cpp. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// A plain store, like `JSC::Strong::set()`: the GC scans every slot of | ||
| /// the set, so there is no write barrier. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `VirtualMachine::teardown` destroys the JSC VM, and the StrongSet | ||
| // with it, before it drops the runtime state that still owns | ||
| // `Strong`s; once shutdown has begun the slot dies with the set. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Off the JS thread (an `unsafe impl Send` wrapper dropped us on | ||
| // a pool thread). StrongSet::deallocate needs the JSLock, so the | ||
| // slot leaks until the VM's teardown; the wrapper should queue | ||
| // the drop back to the JS thread instead. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `&JSGlobalObject` is ABI-identical to a non-null `*const T` (opaque ZST | ||
| // handle), so `new` is a safe fn; `delete` consumes the slot and stays unsafe. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The handle behind bun_jsc::Strong (Strong.rs): a slot in the VM's | ||
| // JSC::StrongSet, which Rust reads and writes directly. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
Rebased onto main now that the WebKit bump with the StrongSet refactor (#39371) has landed; the one conflict was the |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it rewires bun_jsc::Strong's GC-rooting backing store and carries a measured eden-pause trade-off that was requested via Slack, a maintainer sign-off would still be worthwhile.
What was reviewed:
Impl::get/set/clearnow read/write theJSC::JSValueslot directly — RustJSValueis#[repr(transparent)]over an 8-byte word, layout-identical to the 64-bit C++JSC::JSValue, so the raw slot access is ABI-correct and no write barrier is needed sinceStrongSetscans every slot.- Grepped for remaining
StrongRootBlock/Bun__StrongRef__set/StrongRefImplreferences — none; all consumers (iso-subspace tables,BunJSCModule.h,ExternTraits.h,JSVMClientData) are updated in this diff. - The
is_shutting_downguard inImpl::destroyis retained, so the #31990 teardown-after-~VM path is still covered.
Extended reasoning...
Overview
Removes the custom StrongRootBlock GC-cell machinery (added in #35849) and backs bun_jsc::Strong with JSC's new per-heap StrongSet (upstream ff64aee116d4, shipped in the WebKit pin since #39371). Net -398 lines: deletes StrongRootBlock.{h,cpp}, its iso-subspace entries, the JSVMClientData list/cursor/structure fields, and the "Srb" marking constraint; simplifies StrongRef.cpp to a two-function wrapper over StrongSet::allocate/deallocate; drops Bun__StrongRef__set so Strong.rs reads/writes the slot directly; reverts heapStats()/getProtectedObjects() to JSC's own walkers; updates one test and a handful of comments.
Security risks
None. This is internal GC-root plumbing with no user-controlled input, parsing, or trust boundary. The relevant risk class is memory safety (use-after-free if a rooted value is not visited), not security in the auth/injection sense.
Level of scrutiny
High. bun_jsc::Strong is the primary mechanism by which native code keeps JS values alive across GC — every timer, socket, promise callback, etc. depends on it. A missed root here is a runtime-wide UAF. The change is well-argued (mirrors JSC::Strong::set()'s barrier-free store, since StrongSet::visitAggregate scans every slot on every collection) and extensively tested under ASAN and JSC-Debug assertions, but this is exactly the category where a maintainer should confirm the reasoning independently.
Other factors
- The PR documents a deliberate performance trade-off: eden STW pause at 1M live handles goes from ~3.8 ms to ~10.1 ms (roughly the pre-#35849
HandleSetnumber), while per-handle throughput improves ~15–20%. Jarred requested the change, so they should confirm the trade-off is acceptable. - I verified Rust
JSValueis#[repr(transparent)]over a 64-bit word (src/jsc/JSValue.rs:29-34), so casting theHandleSlot(JSC::JSValue*) toNonNull<JSValue>and doing raw.read()/.write()is layout-correct; the previousDecodedJSValueindirection was equivalent. - Grepped the tree for lingering
StrongRootBlock/StrongRefImpl/Bun__StrongRef__setreferences: none remain. - The comment-cop bot flagged long comments in two rounds; the author trimmed them (00:35 comment) and the current diff's comments are 1–3 lines each.
- Not covered by CODEOWNERS.
736c3df to
946c34c
Compare
|
Rebased again after #39770, which modified |
There was a problem hiding this comment.
I reviewed this PR and the automated bug hunt found no issues. Given that this swaps the backing store for every bun_jsc::Strong in the runtime (core GC-rooting infrastructure) and carries a measured eden-pause trade-off at high handle counts, a maintainer sign-off would still be worthwhile.
What was checked:
- No stale references to
StrongRootBlock,StrongRefImpl,Bun__StrongRef__set, or them_strongRootBlock*fields remain anywhere insrc/. - Rust
JSValueis#[repr(transparent)]over the 64-bit encoded word, so casting the returnedJSC::JSValue*slot toNonNull<JSValue>and reading/writing it directly is layout-sound. - The
is_shutting_downearly return inImpl::destroyis retained, covering the #31990 teardown ordering. heapStats()/getProtectedObjects()revert cleanly to the JSC walkers now that the slots live inStrongSet; the updated test asserts both paths and the absence of anyStrong*cell type.
Extended reasoning...
Overview
This PR removes Bun's custom StrongRootBlock GC-cell implementation (added in #35849 to avoid O(N) HandleSet scans on eden collections) and re-backs bun_jsc::Strong with JSC's new upstream StrongSet, which replaced HandleSet in ff64aee116d4. Net -398 lines. Bun__StrongRef__new/delete become thin wrappers over StrongSet::allocate/deallocate; Bun__StrongRef__set is deleted and Rust writes the slot directly (no write barrier, matching JSC::Strong::set()). The iso-subspace entries, JSVMClientData list/cursor/structure fields, and the "Srb" marking constraint are all removed. heapStats() and getProtectedObjects() drop their block-walk merge and go back to the plain JSC accessors.
Security risks
None. This is internal GC-root plumbing with no user-controlled input, parsing, auth, or network surface.
Level of scrutiny
High. bun_jsc::Strong is the mechanism by which timers, sockets, SQL connections, fetch bodies, and dozens of other native objects keep their JS wrappers alive. A mistake here is a use-after-free or a leak across the whole runtime. Two claims in particular deserve a maintainer's eyes:
- No write barrier on
set: the PR asserts (correctly per upstreamJSC::Strong::set()) thatStrongSetslots need no barrier because the set is scanned unconditionally on every collection. This is the whole reasonStrongRootBlockexisted — its slots wereWriteBarrier<Unknown>precisely so eden could skip old-gen blocks. Dropping the barrier is only sound because the backing store changed; someone who knows JSC's collector should confirm. - Perf trade-off: eden STW at 1M live handles goes from ~3.8 ms to ~10.1 ms (roughly the pre-#35849 number). Below ~100k handles it's noise, and per-handle throughput improves. Whether that regression at the extreme is acceptable is a product call the requester (Jarred) presumably already made, but it should be acknowledged on the record.
Other factors
- The change was explicitly requested by a maintainer, and the PR description is unusually thorough (benchmarks, ASAN + JSC-Debug verification, worker-teardown check for the #31990 shape).
- I grepped for every deleted symbol across
src/— no stragglers; the build-system references toStrongRootBlock.cppwould fail the build if any remained. - The Rust ↔ C++ ABI is sound:
JSC::HandleSlotisJSValue*, and Rust'sJSValueis#[repr(transparent)]over the same 8-byte encoded word, so the direct.read()/.write()inImpl::get/setis layout-correct. Strong::setnow ignores itsglobalparameter (renamed_global); call sites are unchanged so this is a harmless compat shim.- The comment-cop bot flagged long comments on an earlier revision; the author trimmed them in the final push and the current diff's comments are concise.
- The updated test in
timer-gc-roots.test.tsfails on main (clearedObjectTypes: ["StrongRootBlock"]) and passes with the change, satisfying the fails-without-fix rule.
Given the blast radius and the explicit perf trade-off, this warrants a human approval rather than an automated one.
|
Ready for review at 946c34c: every build lane in CI compiles this head and 170+ jobs have passed so far with no failures. One note for anyone looking at automated with/without-change rebuilds of this PR: this change deletes |
946c34c to
62c84f9
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since it swaps the backing store for every bun_jsc::Strong (core GC rooting) and accepts a measured eden-pause regression at high handle counts, a human sign-off on the trade-off and the StrongSet interaction would still be worthwhile.
What was reviewed:
Bun__StrongRef__new/deleteagainstStrongSet::allocate/deallocate; direct slot read/write from Rust matchesJSC::Strong::set()(no barrier, set is scanned unconditionally) and RustJSValueis#[repr(transparent)]over the 64-bit encoded word so the raw slot store is ABI-correct.- No remaining references to
StrongRootBlock,StrongRefImpl, orBun__StrongRef__setanywhere in the tree (build files included);ExternTraits<Bun::StrongRef>updated to the newJSC::JSValue*handle type. - The retained
is_shutting_downearly return inImpl::destroystill covers the #31990 teardown ordering with StrongSet as the store; the off-JS-thread branch leaks the slot rather than touchingStrongSet::deallocatewithout the lock. heapStats()/getProtectedObjects()reverted to JSC's own walkers, and the updatedtimer-gc-roots.test.tsasserts both see the armed timers and that noStrong*cell type remains.
Extended reasoning...
Overview
This PR removes Bun's custom StrongRootBlock GC-cell backing for bun_jsc::Strong and returns to JSC's own strong-handle storage, now JSC::StrongSet (upstream ff64aee116d4, which replaced the old HandleSet). The handle is once again the raw JSC::JSValue* slot; Rust reads/writes it directly and only crosses FFI to allocate/release. Net -398 lines: two source files deleted, the "Srb" marking constraint and four JSVMClientData members removed, iso-subspace table entries dropped, and heapStats()/getProtectedObjects() reverted to JSC's own walkers. Comment-only edits in JSRef.rs and Response.rs.
Security risks
None. This is internal GC-root plumbing with no user-facing surface, no parsing of untrusted input, and no auth/crypto involvement.
Level of scrutiny
High. bun_jsc::Strong is the primitive that keeps timers, sockets, promise reactions and dozens of other native objects alive across GC; a mistake here is a runtime-wide UAF or leak. The PR also knowingly gives up the generational skip StrongRootBlock provided (eden STW at 1M live handles goes from ~3.8 ms to ~10.1 ms per the PR's own measurements) in exchange for less code and cheaper allocation. That is a maintainer-level trade-off — the description says Jarred requested it, so Jarred (or another maintainer) confirming the numbers and the direction is the right approval path, not an automated one.
Other factors
The bug-hunting pass found nothing. I confirmed no stale references to the removed symbols remain anywhere in the repo, the Rust JSValue layout (#[repr(transparent)] over the encoded 64-bit word) makes the direct slot read/write bit-identical to JSC::JSValue on 64-bit, and the ExternTraits<Bun::StrongRef> bindgen glue was updated to the new JSC::JSValue* extern type. The updated test asserts both protectedObjectTypeCounts.Timeout and getProtectedObjects() see 5000 armed timers and none after clearing, and that no Strong* cell type appears — it fails on main and passes here. CI on the current head compiles in every lane with 170+ jobs green. All of that is reassuring, but the change sits squarely in REVIEW.md's most-blocked category (native memory safety / GC rooting) and carries a deliberate performance trade-off, so I'm deferring rather than approving.
|
CI on 62c84f9 had one red lane: |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it swaps the backing storage for every bun_jsc::Strong GC root and carries a documented eden-pause trade-off at high handle counts, a maintainer sign-off would still be worthwhile.
What was reviewed:
StrongRef.cpp/Strong.rs: slot allocate/read/write/deallocate againstStrongSet; the barrier-freeImpl::setmatchesJSC::Strong::set()'s contract that the set is scanned unconditionally.- Confirmed no remaining references to
StrongRootBlock,StrongRefImpl,Bun__StrongRef__set, or the removedm_strongRootBlock*members anywhere insrc/. ExternTraits.h/StrongRef.h:Bun::StrongRefnow wrapsJSC::JSValue*;unique_ptr::release()inconvertToExternstill hands the same pointer shape toStrong::adopt.- The retained
is_shutting_downguard inImpl::destroystill covers post-~VMdrops; the unused_globalonStrong::setwas noted as an intentional API-compat stub.
Extended reasoning...
Overview
This PR removes the custom StrongRootBlock JSCell-based backing for bun_jsc::Strong (added in #35849) and reverts to using JSC's own per-heap StrongSet, which upstream introduced in ff64aee116d4 (replacing HandleSet) and which the pinned WebKit now ships. Net −398 lines: StrongRootBlock.{h,cpp} deleted, its iso-subspace entries and JSVMClientData bookkeeping removed, the "Srb" marking constraint dropped, and BunJSCModule.h's heapStats()/getProtectedObjects() reverted to plain vm.heap.protectedObject*() calls. The Rust Strong handle is now the raw JSC::JSValue* slot; reads/writes/clears are direct pointer ops in Rust with FFI only for allocate/deallocate.
Security risks
None in the traditional sense (no user-input parsing, auth, or network surface). The risk class is memory safety: this is the mechanism by which every strong GC root in the Rust runtime pins a JS value. A bug here would be a runtime-wide UAF or leak. The specific claims audited: (1) StrongSet::allocate() returns a stable HandleSlot that lives until deallocate, so Rust's raw-pointer read/write is sound; (2) no write barrier is needed because StrongSet::visitAggregate scans every slot on every collection — the PR mirrors JSC::Strong::set()'s own plain store; (3) the is_shutting_down early return still prevents deallocating into a freed StrongSet during VM teardown.
Level of scrutiny
High. This is core GC-rooting infrastructure with FFI ABI changes (Bun__StrongRef__new now returns JSC::JSValue*, Bun__StrongRef__set is removed, Bun::StrongRef in ExternTraits.h changed extern type). It also encodes a design decision — accepting a ~2.6× eden-pause regression at 1M live handles in exchange for less code and cheaper per-handle allocation — that was requested by a maintainer in Slack but should be confirmed on the record.
Other factors
The bug-hunting system found nothing; the one candidate raised (unused _global on Strong::set) was correctly ruled out as an API-compatibility stub. Grep confirms all references to the deleted subsystem are gone. The updated timer-gc-roots.test.ts asserts protectedObjectTypeCounts, protectedObjectCount, and getProtectedObjects() all reflect StrongSet-backed handles, and asserts no Strong* cell type appears in objectTypeCounts (which fails on main). ASAN verification of worker teardown and BUN_DESTRUCT_VM_ON_EXIT=1 is documented. The one CI failure (windows-aarch64 deinitialization.test.ts) reproduces on main without this change and traces to a uSockets/libuv UAF unrelated to Strong handles. Given the blast radius, a human look is the right call even with no findings.
JSC now stores Strong<> slots in StrongBlocks owned by a per-heap StrongSet (upstream ff64aee116d4, which also deletes HandleSet). Allocate bun_jsc::Strong slots from that set instead of the StrongRootBlock cells added in #35849: Bun__StrongRef__new returns the slot, Rust reads and writes it directly (the set is scanned unconditionally, so stores need no barrier), and Bun__StrongRef__delete returns it. Bun__StrongRef__set goes away. Delete StrongRootBlock, its iso subspace, the per-VM block list on JSVMClientData and the "Srb" marking constraint. heapStats() and getProtectedObjects() go back to JSC's own protected-cell walk, which covers StrongSet slots. The AbortSignal.timeout changes from #35849 are unaffected. Requires a WebKit that contains the StrongSet refactor.
ac7f99d to
6e3d3fe
Compare
|
Rebased onto main again (head 6e3d3fe). One conflict in |
This branch has not been deployed
2 participants
What does this PR do?
Removes
StrongRootBlock(added in #35849) and backsbun_jsc::Strongwith JSC's own strong-handle storage again. Upstream JSC now keepsStrong<>slots inStrongBlocks owned by a per-heapStrongSet(upstream ff64aee116d4, which also deletedHandleSet), and the WebKit that main pins since #39371 ships it (root.halready includesStrongSet.h).Requested in Slack by @Jarred-Sumner.
Changes
StrongRef.cpp/.h:Bun__StrongRef__newallocates a slot fromvm.heap.strongSet()and returns it;Bun__StrongRef__deletecallsStrongSet::deallocate. The slot pointer is the handle, as it was before Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC #35849.Bun__StrongRef__setis gone: likeJSC::Strong::set(), a store into a slot needs no barrier (the set is scanned unconditionally), soStrong.rsreads, writes and clears the slot directly and only calls into C++ to allocate and release.StrongRootBlock.{h,cpp}, its iso subspace entries, the block list / cursor / structure onJSVMClientData, and the"Srb"marking constraint.heapStats()/getProtectedObjects()go back to JSC'sprotectedObjectTypeCounts()/forEachProtectedCell(), which walk theStrongSet, so they still report every object abun_jsc::Strongpins.is_shutting_downearly return inImpl::destroystays: teardown destroys the VM (and with it theStrongSet) beforedeinit_runtime_statedrops theStrongs it still owns (the crash class described in Release RuntimeState's JSC handles before tearing down the VM #31990), so the release must still be skipped once teardown has started.AbortSignal.timeoutlifetime fix that was also part of Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC #35849 is unaffected.Net: 77 insertions, 475 deletions.
Trade-off, measured
StrongSet::visitAggregatevisits every slot on every collection, eden included; it does not have the generational skip thatStrongRootBlockgot from being a write-barriered cell. So this trades eden pause time at very high live-handle counts for less code and cheaper allocation. Release builds, same JSC for both binaries, only the bun side differs (BUN_JSC_logGC=1, stop-the-world time summed per eden collection, 3 runs each):Strongs (armed timers)StrongRootBlock(main)The 1M number is roughly where #35849 measured the old
HandleSet(8.8 ms). Below ~100k live handles the scan is within noise.Per-handle cost (
setTimeout+clearTimeout, median of 5, two runs each):bench/snippets/set-timeout.mjs(20M)Tests
test/js/web/timers/timer-gc-roots.test.ts: the first test now checks thatheapStats().protectedObjectTypeCounts.Timeout,protectedObjectCountandgetProtectedObjects()all see 5000 armed timers and none after they are cleared, and that noStrong*cell type shows up inobjectTypeCounts. It fails on main (clearedObjectTypes: ["StrongRootBlock"], the retained spare block; also reproduced withUSE_SYSTEM_BUN=1) and passes withbun bd testagainst the pinned WebKit. The other 8 tests in the file pass unchanged.Also run with
bun bd(ASAN) against the pinned WebKit:test/js/bun/jsc/,test/js/web/timers/,test/js/web/abort/,worker-terminate-lifetime.test.ts. The only failures are ones a main build made the same way also has: the RSS-delta timer leak tests (their fixture only widens the threshold for a binary namedbun-asan; with ASAN's quarantine disabled the delta on this branch is 1.5 MB / -0.6 MB), a 30 s timeout insetInterval doesn't leak memory, and a pre-existing LeakSanitizer report fornode_fs_binding::Bindingin the dns teardown test. Worker teardown after loadingBun.SQL(the #31990 shape) andBUN_DESTRUCT_VM_ON_EXIT=1on the main thread both exit cleanly under ASAN.Notes
StrongSet.hhas the same unconditionalforEachSlotwalk, so the numbers still describe the current pin.StrongSet.hnot found); it was rebased once the bump landed. The only conflict wasBunClientData.h, where process.memoryUsage: report heapUsed from the most recent collection #39593 had addedHeapSizeAfterLastCollectionto the samenamespace Bunblock that held theStrongRootBlockforward declaration; the block is kept and only the forward declaration is removed.StrongRootBlock::subspaceForImplto useBUN_SUBSPACE_SLOTS. Resolved by deleting the file as before and removing the oneStrongRootBlockentry from each of the new-style tables; the table destructors added there still hold since each table stays a plain array of pointers.HandleSetand toBun__StrongRef__*for theStrongRootBlockpath; both halves are obsoleted by the upstream refactor plus this change and would need to be redone againstStrongSetif still wanted.setTimeout(noop, 600000)held in an array, two full GCs, then 100k-object allocation rounds to drive eden collections, parsing thep=pauses of eachEdenCollectionfromBUN_JSC_logGC=1; the throughput numbers areclearTimeout(setTimeout(...))loops of the listed shapes.