Repository navigation
[JSC] Stop a heap's local Collector threads when the heap shuts down (cherry-pick WebKit/WebKit#77844) - #882
Conversation
https://bugs.webkit.org/show_bug.cgi?id=328418 rdar://190202961 Reviewed by Marcus Plutowski. Heap::shutDown() waits for the Heap's Leave request to be processed by the Collector. However, this can happen before the collector thread has returned from m_stopIfNecessaryTimer->scheduleSoon(): 1. The collector thread is conducting a collection and calls Heap::stopTheMutator(). The mutator holds heap access, so instead of stopping it, the collector thread hands it the conn by setting mutatorHasConnBit. 2. The mutator thread, inside waitForCollector(), sees it has the conn, finishes the collection and processes its own Leave request. 3. The collector thread hasn't yet returned from m_stopIfNecessaryTimer->scheduleSoon(), which uses Heap state. 4. The mutator thread continues through ~VM, which unregisters the VM from JSRunLoopTimer::Manager. ~Heap then destroys m_stopIfNecessaryTimer before m_localCollector, so ~Collector joins the collector thread too late. 5. The collector thread accesses the freed m_stopIfNecessaryTimer, or hits the RELEASE_ASSERT in JSRunLoopTimer::Manager::scheduleTimer() since the VM has unregistered. Before 323479@main, Heap::shutDown() joined the collector thread before tearing anything down. Restore that: Collector::shutDown() stops the collect-continuously thread and the collector thread, and Heap::shutDown() calls it for a local Collector right after leaving. ~Collector now asserts that the threads are already stopped instead of stopping them itself. This will need to be fixed for the --useGlobalGC=true case too, but that depends on how stopping the mutators of several heaps will work, so it will come later. For now, this fixes the default --useGlobalGC=false case. Test: JSTests/stress/many-agents-teardown.js * JSTests/stress/many-agents-teardown.js: Added. (i.setTimeout): * Source/JavaScriptCore/heap/Collector.cpp: (JSC::Collector::~Collector): (JSC::Collector::shutDown): (JSC::Collector::stopCollectingContinuously): * Source/JavaScriptCore/heap/Collector.h: * Source/JavaScriptCore/heap/Heap.cpp: (JSC::Heap::shutDown): Canonical link: https://commits.webkit.org/324477@main
|
Status: ready for review. How I reproduced it (release
The full option list and every number are in the Notes of the description. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Preview build of ea3d81d: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe change adds explicit collector shutdown handling to heap shutdown and checks collector state during destruction. It also adds a stress test that starts agents and schedules timers during continuous collection. ChangesCollector teardown
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly identifies the primary change: stopping a heap's local Collector threads during heap shutdown. It is specific and related to the changeset. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Description check | |
The pull request description is complete and relevant. It identifies the Bugzilla issue, explains the race and fix, lists the affected files and test, documents validation results, and notes known lim… |
Full details: Description check
-
Explanation
The pull request description is complete and relevant. It identifies the Bugzilla issue, explains the race and fix, lists the affected files and test, documents validation results, and notes known limitations. It does not use the exact template line "Reviewed by NOBODY (OOPS!)", but this non-critical omission does not prevent review.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 path_filters to narrow the review scope.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Cherry-pick of WebKit#77844 (324477@main, https://bugs.webkit.org/show_bug.cgi?id=328418), landed upstream on 2026-10-22. Author and message are unchanged. Close it if a sync brings the commit first.
Problem
jscaborts when a VM dies while its collector thread is inHeap::stopTheMutator()(heap/Heap.cpp:1989):RELEASE_ASSERT(iter != m_mapping.end())atruntime/JSRunLoopTimer.cpp:150.stress/waitasync-wait-infinity-notify-half-multi-workers.js.dfg-eager-no-cjit-validate: 27 of 10,800 runs of the lane'sjscofmain, 36 at once. The test legs gate no release.Heap::shutDown()to~Collector, after~VMunregisters the VM.Fix
Heap::shutDown()calls the newCollector::shutDown()right after the heap leaves its local Collector, before anything is finalized.~Collectorasserts that both threads are stopped.jscof this branch: 0 of 10,800. With a sleep in the hand-off: 0 of 864 (main: 87).Background
StopIfNecessaryTimer: the late call.Downsides
--useGlobalGC=true(off by default, unfinished upstream). Its collector thread is shared.memoryHog!). Without theHeap.cpphunk, 14 of 23 agent tests abort in~Collector..text+352 bytes in the LTO lane.Notes
Why now, and when it is redundant. Upstream reviewed and landed the fix on 2026-10-22, as 324477@main (
eaf0efa57b26). The nextbun/upgrade-to-*sync brings the same commit. The last two syncs merged on 2026-10-15 (#814) and 2026-10-21 (#860). Until the next one,mainhas the race (since #814, still at 7961839), and it makes test legs red for changes that do not touch it (run 39351346694 ofmain274ebb3). This is a request to merge on review. The commit is the landed one, so the sync merges cleanly over it. If a sync that has 324477@main merges first, this pull request is redundant: close it.Reach. Only the fork's
jsctest legs under load show the crash: 0.06 to 0.25% of the runs of the CI test indfg-eager-no-cjit-validate, with three processes per core (2 of 3,240, 8 of 10,800 and 27 of 10,800). Those legs do not gate a release. Bun 1.4.4 (2026-10-22) pins WebKit d6375c7, which has the race, so it is the first Bun release with it. Bun was not measured. From the code, default Bun does not reach it:destroyVM()runs a synchronous full collection right before the lastderef(), so the collector thread has no collection to start at that point unlessBUN_JSC_collectContinuouslyis set.Self-review. Three concerns survived. All are about the request, none about the code.
49909f2bfcc0) was reviewed and landed while this was measured. The commit here is the landed one. Its added and removed lines are those of the draft.The port.
git amof upstream's patch onmain(7d0c8a7, then rebased on 5cfcac0 and 7961839): no conflict, offsets of at most 21 lines. The added and removed lines are upstream's, byte for byte, and the message is the one of the landed commit.JSTests/stress/many-agents-teardown.jsis upstream's file, unchanged. No existing test is edited, soJSTests/BUN-TEST-DIFFERENCES.mdhas no new entry. Nothing is fenced withUSE(BUN_JSC_ADDITIONS).Reproduce (release
jsc, Linux x64, 12 cores. Line numbers are those ofmain7961839. The builds that were measured are of 7d0c8a7, before the sync #860, whereHeap.cpp:1989is line 1960. The rebased commit was built and checked again: last table row and "After the rebase".) InJSTests/stress, with the options of the modedfg-eager-no-cjit-validate:The test starts 20 agents. Each agent is a VM on its own thread, with a collector thread and, in this mode, a thread that requests a collection every millisecond. One process alone never failed (0 of 10). With 36 processes at once:
bin/jscofbun-webkit-linux-amd64-ltoformain7d0c8a7main7d0c8a7bin/jscofbun-webkit-linux-amd64-ltoformain7961839bin/jscofbun-webkit-linux-amd64-ltofor this branch (autobuild-preview-pr-882-ea3d81da)The last two rows ran back to back on the same machine. All 8 cores of the local build and all 27 of the lane's build of
mainhave this stack, on the collector thread of one agent:In the same cores the agent threads are in
~VM(VM.cpp:734)-> ~Heap -> ~Collector, that is afterJSRunLoopTimer::Manager::unregisterVM(VM.cpp:689).The sequence on
main(C is the collector thread of a VM, M its mutator)Heap::stopTheMutator()gives M the conn: the compare-exchange atHeap.cpp:1987setsmutatorHasConnBit. C has not run line 1989 yet.VM::~VM -> Heap::shutDown()and waits for its Leave (waitForMembershipChange). It sees the conn, runs the whole collection, and applies its own Leave inCollector::runNotRunningPhase.Heap::shutDown()returns.~VMunregisters the VM (VM.cpp:689). Then~Heapdestroysm_stopIfNecessaryTimer(Heap.h:992) beforem_localCollector(Heap.h:943), whose destructor is the first thing that joins C.Refif~Heapgot further.Before 323479@main
Heap::shutDown()stopped both threads itself before it finalized anything.A wider window. For the numbers below, both builds got three lines that are not in this change: a sleep, read from an environment variable, between
Heap.cpp:1987and:1989. With it the crash onmainis the nullRef: SIGSEGV atStopIfNecessaryTimer.cpp:48,this == nullptr, while the mutator of that VM is in~Collector -> stopThread() -> join().mainjsc --destroy-vm --collectContinuously=true --collectContinuouslyPeriodMS=0.05 -e 'var e=Date.now()+3; while(Date.now()<e){}'stress/many-agents-teardown.jswith its own optionsAfter the rebase on the sync #860 (no function of this change differs between the two bases): the stress runner on the agent files, 261 pass and 0 fail. With the sleep: 0 of 600 for the one-VM program at 30 ms, 0 of 120 for the CI test at 50 ms.
The new test in this fork.
many-agents-teardown.jsis//@ memoryHog!. The lanes pass--memory-limited, so the runner printsSkipping stress/many-agents-teardown.js. Without the sleep it does not tell the two builds apart on this machine:main0 of 160, this change 0 of 160 (40 alone, 120 at 8 at once).A lost hunk shows in CI.
~Collectornow hasRELEASE_ASSERT(!m_collectContinuouslyThread)andRELEASE_ASSERT(m_threadShouldStop). A build of this change without theHeap.cpphunk: 14 of the 23JSTests/stressfiles that start an agent exit 134 in one run each with default options (Collector::~Collector <- Heap::~Heap <- VM::~VM). So a later sync that drops the call inHeap::shutDown()fails in every mode.Agent tests in all modes.
run-jsc-stress-tests --memory-limitedon the 24 files that start an agent, 17 modes:mainand on this change.mainhas both failures at the same rate, so they are not from this change:waitasync-promise-timeout-finite-gc.js.no-cjit-validate-phaseshits the shell's timeout. The main thread spins onwaiterListSize()after its only agent is gone. Alternating batches, 24 at once:main18 of 3,600, this change 15 of 3,600.intl-datetimeformat-language-change-mid-construction.js.default:cacheable resolution pinned at ... across 25 turns.main6 of 960, this change 3 of 960.RELEASE_ASSERT(!m_threadShouldStop)inCollector::requestCollection, or at the one inHeap::finishRelinquishingConn. The collector is now stopped while finalizers run, as it was before 323479@main.Cost (Release builds of 7d0c8a7 with and without the change, x64, no LTO.
strace,perfandvalgrindare not on this machine, so the counts are fromgdb.)mainCollector::stopThread,stopCollectingContinuously,AutomaticThread::tryStopAutomaticThread::join,Thread::waitForCompletion(a VM that never collected)--collectContinuously=trueHeap::shutDown()up to finalization (jsc --destroy-vm -e '', single-stepped, 3 runs)~Collector()(varies by run)~Collectordid. It now happens before the finalizers, not after them..textofjsc: 48,104,087 to 48,104,439 bytes (+352) in the LTO lane (main7961839 and the preview build of this branch). Without LTO: 43,813,544 to 43,813,864 (+320).Heap::shutDown()527 to 585 bytes,~Collector()1,208 to 1,394, newCollector::shutDown()28,stopCollectingContinuously()376 to 386.objdump, addresses removed):Collector::runNotRunningPhaseandJSRunLoopTimer::Manager::scheduleTimerhave no differing instruction.Heap::stopTheMutatordiffers in five__LINE__constants of its RELEASE_ASSERTs.main, 0.611 s (0.553 to 0.679) with the change. With--collectContinuously=1: 1.308 s (1.110 to 1.451) and 1.248 s (1.081 to 1.505).Designs that were weighed and set aside:
Collector::m_threadLockheld across the compare-exchange andscheduleSoon(). Every Leave is applied under that lock, so it also covers--useGlobalGC. A prototype had 0 crashes of 240 with the sleep. It adds a lock pair to each hand-off of a live VM and a lock order fromm_threadLockinto the timer locks.Refto the timer taken before the compare-exchange, andJSRunLoopTimer::Manager::scheduleTimerreturning for an unregistered VM. Not built. The late call still happens, and the assert atJSRunLoopTimer.cpp:150is gone for every other timer.A test-only option that sleeps in the hand-off would make a regression test that fails every time on
main. It was left out: it is a load and a branch on every hand-off, in code that upstream owns.Not fixed, not changed
--useGlobalGC=true: one shared Collector that is never destroyed, so there is no thread to join. Upstream's message says that this comes later. It is off by default and asserts with two heaps.stress/sampling-profiler-line-column.js.ftl-eager, the other test that fails now and then on the same lane, is fixed by [JSC] A VM with a sampling profiler builds the PC to CodeOrigin maps that the profiler reads #804. With 36 processes at once: 8 of 360 onmain, 0 of 2,520 with the preview build of [JSC] A VM with a sampling profiler builds the PC to CodeOrigin maps that the profiler reads #804.