Return empty MarkedBlocks at end of collection, releasing their memory off the pause#293
Return empty MarkedBlocks at end of collection, releasing their memory off the pause#293Jarred-Sumner wants to merge 2 commits into
Conversation
…per starvation Empty MarkedBlocks are returned to the block allocator from only two places on a running heap: IncrementalSweeper's timer, and sweepSynchronously() (which normal operation never reaches). The timer path is starvable. startSweeping() runs at the end of every collection and calls scheduleTimer(), which overwrites any pending deadline (JSRunLoopTimer::Manager::scheduleTimer). A client that collects more often than the deadline pushes it back forever and the sweeper never runs, so empty blocks accumulate for the process lifetime. Fix startSweeping() to leave an already-pending deadline alone. This is an independent bug and stands on its own: a fired timer is removed from the manager's list and a completed sweep cancels it, so the guard always re-arms. Add returnEmptyBlocksAtEndOfCollection (default off) which returns empty, non-destructible blocks in the End phase, plus retainedEmptyBlocksPerDirectory (default 1) bounding what a directory that allocated during the cycle may keep as an allocation cache. This adds work to the stop-the-world pause and is therefore a measurement scaffold, not a shipping default: the intended design frees these blocks from the collector thread after the world resumes. It exists so the memory win can be measured in isolation and give the concurrent version an exact target. Measured on a sustained-churn workload with no explicit gc() (only automatic collections, so sweepSynchronously never runs): heap capacity 64.9MB -> 34.7MB, 6179 blocks returned, 32 retained by the keep-1 gate. Scope limits, stated plainly: - endMarking() sets destructibleBits() = liveBits() for NeedsDestruction directories, and only a sweep clears it, so emptyBits & ~destructibleBits is always empty there. This returns DoesNotNeedDestruction blocks only. The mask is load-bearing: these blocks are unswept, and freeing a destructible one would skip its cells' destructors. - Blocks whose weak set is not trivially destructible are skipped. ~WeakSet destroys weak blocks without running pending finalizers and without rehoming ones a live Weak<T> still points into; only the sweeper may retire those. - The call site must stay after prepareForAllocation(). What makes it safe is that reset() nulls every LocalAllocator's m_lastActiveBlock -- not the inUse claim protocol, which does not cover m_lastActiveBlock at all (MarkedBlock::Handle::stopAllocating clears the bit before it is stored). Includes a container audit for the concurrent design, which is blocked on four structures whose only guard today is that the mutator is the sole writer.
WalkthroughChangesThe change adds configurable empty-block return at the end of garbage collection, tracks allocation activity in block directories, defers block destruction until the heap phase changes, preserves pending sweeper deadlines, and adds stress coverage plus a concurrency audit. ChangesEmpty block return lifecycle
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@JSTests/stress/return-empty-blocks-at-end-of-collection.js`:
- Around line 12-22: Extend the churn coverage by adding a parallel helper
alongside churn() that retains scattered destructible cell instances, such as
Map, Set, RegExp, or TypedArray objects, while still leaving partially used
blocks. Invoke this variant in the test’s existing return-empty-block exercise
so BlockDirectory::returnEmptyBlocks covers the destructibleBits exclusion path
in addition to the weak-set case.
- Around line 58-70: Add an observable callback-invocation flag around the
FinalizationRegistry created in this test, set it from the registry callback,
and assert after gc() and the existing weak-reference liveness checks that the
flag remains false. Keep referents strongly retained so finalizers should not
run, thereby directly validating the “finalizers must not be lost” claim.
In `@Source/JavaScriptCore/runtime/OptionsList.h`:
- Line 252: Complete the description string for the
returnEmptyBlocksAtEndOfCollection option so the sentence ends with “not meant
to ship on its own” (or equivalent complete wording), while preserving the rest
of the description.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 044e9305-f192-4a45-b250-28b7074003f1
📒 Files selected for processing (9)
JSTests/stress/return-empty-blocks-at-end-of-collection.jsSource/JavaScriptCore/heap/BlockDirectory.cppSource/JavaScriptCore/heap/BlockDirectory.hSource/JavaScriptCore/heap/CONCURRENT-BLOCK-RETURN-AUDIT.mdSource/JavaScriptCore/heap/Heap.cppSource/JavaScriptCore/heap/IncrementalSweeper.cppSource/JavaScriptCore/heap/MarkedSpace.cppSource/JavaScriptCore/heap/MarkedSpace.hSource/JavaScriptCore/runtime/OptionsList.h
| // Scattered survivors keep their blocks non-empty while the dropped majority empties others, so | ||
| // both the freed and the retained path run. A contiguous survivor run would only empty whole | ||
| // blocks and never exercise the partly-used case. | ||
| function churn(round) { | ||
| const survivors = []; | ||
| for (let i = 0; i < 20000; ++i) { | ||
| const o = { round, i, payload: [i, i + 1, i + 2] }; | ||
| if (i % 10 === 0) survivors.push(o); | ||
| } | ||
| return survivors; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a destructible-cell variant to churn()-style coverage.
The PR's safety contract excludes two distinct categories from empty-block return: destructible blocks and blocks with non-trivially-destructible weak sets. This test only exercises the latter (via WeakRef/FinalizationRegistry at lines 58-70); churn() only ever allocates plain objects/arrays, which are non-destructible cells. Nothing here would catch a regression in the ~destructibleBits() exclusion check in BlockDirectory::returnEmptyBlocks.
Consider adding a parallel churn variant that allocates destructible cell types (e.g. Map, Set, RegExp, or a TypedArray) to exercise that exclusion path directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@JSTests/stress/return-empty-blocks-at-end-of-collection.js` around lines 12 -
22, Extend the churn coverage by adding a parallel helper alongside churn() that
retains scattered destructible cell instances, such as Map, Set, RegExp, or
TypedArray objects, while still leaving partially used blocks. Invoke this
variant in the test’s existing return-empty-block exercise so
BlockDirectory::returnEmptyBlocks covers the destructibleBits exclusion path in
addition to the weak-set case.
| // Weak sets pin a block out of the eligible set (only a trivially destructible one may be freed | ||
| // unswept). Exercise that path: the referents must stay live and the finalizers must not be lost. | ||
| const registry = new FinalizationRegistry(function () {}); | ||
| const weakRefs = []; | ||
| const referents = []; | ||
| for (let i = 0; i < 2000; ++i) { | ||
| const o = { i }; | ||
| referents.push(o); | ||
| weakRefs.push(new WeakRef(o)); | ||
| registry.register(o, i); | ||
| } | ||
| gc(); | ||
| for (let i = 0; i < weakRefs.length; ++i) shouldBe(weakRefs[i].deref().i, i); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Finalizer-not-lost claim is asserted nowhere.
The comment at lines 58-59 states the test exercises the invariant that "the finalizers must not be lost," but referents keeps every object strongly alive for the whole block, so no object ever becomes unreachable and no finalizer can ever run. The only assertion (line 70) checks weakRefs[i].deref().i, which validates liveness, not finalizer preservation.
Consider adding a check that the FinalizationRegistry callback was never invoked (it should stay silent, since referents remain live) to actually back the comment's claim, e.g.:
💡 Suggested addition
-const registry = new FinalizationRegistry(function () {});
+let finalized = false;
+const registry = new FinalizationRegistry(function () { finalized = true; });
const weakRefs = [];
const referents = [];
for (let i = 0; i < 2000; ++i) {
const o = { i };
referents.push(o);
weakRefs.push(new WeakRef(o));
registry.register(o, i);
}
gc();
for (let i = 0; i < weakRefs.length; ++i) shouldBe(weakRefs[i].deref().i, i);
+shouldBe(finalized, false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Weak sets pin a block out of the eligible set (only a trivially destructible one may be freed | |
| // unswept). Exercise that path: the referents must stay live and the finalizers must not be lost. | |
| const registry = new FinalizationRegistry(function () {}); | |
| const weakRefs = []; | |
| const referents = []; | |
| for (let i = 0; i < 2000; ++i) { | |
| const o = { i }; | |
| referents.push(o); | |
| weakRefs.push(new WeakRef(o)); | |
| registry.register(o, i); | |
| } | |
| gc(); | |
| for (let i = 0; i < weakRefs.length; ++i) shouldBe(weakRefs[i].deref().i, i); | |
| // Weak sets pin a block out of the eligible set (only a trivially destructible one may be freed | |
| // unswept). Exercise that path: the referents must stay live and the finalizers must not be lost. | |
| let finalized = false; | |
| const registry = new FinalizationRegistry(function () { finalized = true; }); | |
| const weakRefs = []; | |
| const referents = []; | |
| for (let i = 0; i < 2000; ++i) { | |
| const o = { i }; | |
| referents.push(o); | |
| weakRefs.push(new WeakRef(o)); | |
| registry.register(o, i); | |
| } | |
| gc(); | |
| for (let i = 0; i < weakRefs.length; ++i) shouldBe(weakRefs[i].deref().i, i); | |
| shouldBe(finalized, false); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@JSTests/stress/return-empty-blocks-at-end-of-collection.js` around lines 58 -
70, Add an observable callback-invocation flag around the FinalizationRegistry
created in this test, set it from the registry callback, and assert after gc()
and the existing weak-reference liveness checks that the flag remains false.
Keep referents strongly retained so finalizers should not run, thereby directly
validating the “finalizers must not be lost” claim.
…he pause Returning empty blocks in the End phase put the madvise inside the stop-the-world pause, which is not an acceptable trade. Split the free instead of paying it. MarkedBlock::Handle::~Handle does one thing that touches state the mutator shares -- removeBlock(), which is a bit-scan and two vector writes -- and three things that do not: ~MarkedBlock (trivial), freeAlignedMemory (thread-safe in all three AlignedMemoryAllocator subclasses), and a RESOURCE_USAGE-only counter. The second group is where the madvise is, i.e. essentially all of the cost. So do the bookkeeping in the End phase, where the mutator is already quiesced and a bit-scan is free, and queue the handle. Release the memory from destroyDeferredBlocks() once changePhase() has run resumeThePeriphery(), with the mutator running again. A queued block is unlinked from its directory, from MarkedBlockSet, and from every IsoCellSet before it is queued, so nothing can reach it and the concurrent half touches no shared state at all. This is why the containers in the audit do not need to become concurrent. It also avoids a race that sinks the walk-the-directories-after-resume approach outright: m_requests.removeFirst() (Heap.cpp:1843) runs before resumeThePeriphery() (Heap.cpp:1893), so a queued collection can begin the moment the mutator resumes, run stopThePeriphery -> stopAllocating, and repopulate every m_lastActiveBlock with a block whose inUse bit is clear -- exactly what such a freer selects on. Measured, sustained churn with no explicit gc(): heap capacity 65.2MB -> 31.1MB. Unchanged by the split, as expected: the same blocks are returned, only the release moves off the pause. ASSERT in destroyDeferredBlocks that the world is running, so that if the cost ever migrates back into the pause it fails loudly rather than silently.
Jarred-Sumner
changed the title
|
Pushed the concurrent part ( The race that kills the walk-the-directories-after-resume design
What I did instead
Every hazard in the audit lives in A queued block is unlinked from its directory, from The one new invariant is "a queued block is unreachable" — true by construction, and Results
Unchanged by the split, as it should be — the same blocks are returned, only the release moves. Test matrix green under debug+ASan with the assert live: keep-0, keep-1 + Not doneTSan has not run. It should before this lands. I'd argue the split makes it much less interesting than it would have been — the concurrent half has no shared reachable state — but "should be uninteresting" is not a TSan run, and this is the bug class that shows up as a race long before it shows up as corruption. Pause-time numbers are also not measured: this box is at load average ~244 and any timing off it would be noise. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/JavaScriptCore/heap/CONCURRENT-BLOCK-RETURN-AUDIT.md`:
- Around line 207-211: Run the TSan stress matrix covering the
deferred-destruction path before relying on the invariant asserted at the drain
point. Use the existing concurrency stress configurations and record or address
any race findings before merge.
- Around line 10-15: Specify the fenced code block language in the audit
document by changing the unlabeled fence around the block containing removeBlock
and MarkedBlock cleanup to use the appropriate cpp language identifier, while
leaving the code content unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c47c86b6-2078-480b-a6f5-f6b5824200cc
📒 Files selected for processing (8)
Source/JavaScriptCore/heap/BlockDirectory.cppSource/JavaScriptCore/heap/CONCURRENT-BLOCK-RETURN-AUDIT.mdSource/JavaScriptCore/heap/Heap.cppSource/JavaScriptCore/heap/MarkedBlock.cppSource/JavaScriptCore/heap/MarkedBlock.hSource/JavaScriptCore/heap/MarkedSpace.cppSource/JavaScriptCore/heap/MarkedSpace.hSource/JavaScriptCore/runtime/OptionsList.h
| ``` | ||
| removeBlock(this, WillDeleteBlock::Yes); // ALL the container bookkeeping. Cheap: a bitscan + vector writes. | ||
| m_block->~MarkedBlock(); // trivial | ||
| m_alignedMemoryAllocator->freeAlignedMemory(m_block); // the madvise. THE COST. Thread-safe (see below). | ||
| heap.didFreeBlock(blockSize); // a counter, RESOURCE_USAGE-only | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify the fenced block language.
The fence at Line 10 triggers markdownlint MD040; add an appropriate language such as cpp.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Source/JavaScriptCore/heap/CONCURRENT-BLOCK-RETURN-AUDIT.md` around lines 10
- 15, Specify the fenced code block language in the audit document by changing
the unlabeled fence around the block containing removeBlock and MarkedBlock
cleanup to use the appropriate cpp language identifier, while leaving the code
content unchanged.
Source: Linters/SAST tools
| Every race above is release-build-silent. TSan would be the gate, not assertions. | ||
|
|
||
| The split design pays none of this. Its only new invariant is "a deferred block is unreachable", | ||
| which is enforced by construction (it is unlinked before it is queued) and asserted at the drain | ||
| point (`ASSERT(m_deferredDestroyBlocks.isEmpty() || !heap().worldIsStopped())`). |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial
Run TSan before relying on this invariant.
The audit identifies TSan as the gate for release-build-silent races, but the PR context says it has not run. Please run the TSan stress matrix for the deferred-destruction path before merge.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Source/JavaScriptCore/heap/CONCURRENT-BLOCK-RETURN-AUDIT.md` around lines 207
- 211, Run the TSan stress matrix covering the deferred-destruction path before
relying on the invariant asserted at the drain point. Use the existing
concurrency stress configurations and record or address any race findings before
merge.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| void MarkedSpace::destroyDeferredBlocks() | ||
| { | ||
| // The whole point is that this runs off the pause. If it ever runs world-stopped, the cost it | ||
| // was written to move has moved back. | ||
| ASSERT(m_deferredDestroyBlocks.isEmpty() || !heap().worldIsStopped()); | ||
|
|
||
| // The release-the-memory half. These blocks are unlinked from every directory, from m_blocks, | ||
| // and from every IsoCellSet, so nothing can reach them and none of this touches shared state: | ||
| // ~MarkedBlock is trivial, the weak sets were required to be trivially destructible, and | ||
| // freeAlignedMemory is thread-safe in all three allocators. This is where the madvise happens, | ||
| // so this is the part worth keeping out of the stop-the-world pause. | ||
| for (MarkedBlock::Handle* block : std::exchange(m_deferredDestroyBlocks, { })) | ||
| delete block; | ||
| } |
There was a problem hiding this comment.
🟡 In ENABLE(RESOURCE_USAGE) builds, destroyDeferredBlocks() runs after changePhase() has resumed the mutator, so delete block → ~Handle() → heap.didFreeBlock() does m_blockBytesAllocated -= capacity on the collector thread while the resumed mutator's didAllocateBlock() does += capacity — and m_blockBytesAllocated is a plain size_t (Heap.h:991), so this is a lost-update race. The audit's own table row 5 flags this field, but the "none of this touches shared state" comment here overlooks that didFreeBlock() was left in the deferred half. Trivial fix: make m_blockBytesAllocated std::atomic<size_t> with relaxed ops, or move the didFreeBlock accounting into deferBlockDestruction() alongside the m_capacity decrement — worth doing before the TSan run this PR names as the landing gate.
Extended reasoning...
What the bug is. MarkedSpace::destroyDeferredBlocks() iterates the deferred block list and does delete block, which runs MarkedBlock::Handle::~Handle(). The last line of ~Handle() is heap.didFreeBlock(blockSize) (MarkedBlock.cpp:166), which under ENABLE(RESOURCE_USAGE) does m_blockBytesAllocated -= capacity (Heap.cpp:3029). m_blockBytesAllocated is declared as a plain, non-atomic size_t at Heap.h:991. Meanwhile, a mutator allocating a new block via MarkedBlock::tryCreate → new Handle(...) → heap.didAllocateBlock(blockSize) does m_blockBytesAllocated += capacity (Heap.cpp:3020). If these two non-atomic read-modify-writes overlap, one update is lost.
The code path that triggers it. runEndPhase() at Heap.cpp:1862 calls changePhase(conn, CollectorPhase::NotRunning). That reaches finishChangingPhase(), which for the End→NotRunning transition (suspended→not-suspended) calls resumeThePeriphery() and — when conn == GCConductor::Collector — also resumeTheMutator() (Heap.cpp:1900-1902). Only after changePhase() returns does runEndPhase() call m_objectSpace.destroyDeferredBlocks() (Heap.cpp:1867). So when the collector thread has the conn, the mutator is already running on its own thread by the time the deferred blocks are deleted.
Why existing code doesn't prevent it. The comment at MarkedSpace.cpp:443-447 states "nothing can reach them and none of this touches shared state", and the audit's Outcome section says the same. That claim is correct for the block itself (unlinked from every directory, from MarkedBlockSet, from every IsoCellSet), and correct for ~MarkedBlock (trivial) and freeAlignedMemory (thread-safe). But ~Handle() also calls heap.didFreeBlock(blockSize), and Heap::m_blockBytesAllocated is shared heap state that the mutator writes via didAllocateBlock. The audit doc itself identifies this hazard in its container table row 5 ("Heap::m_blockBytesAllocated | Heap.h:991 | none | Minor, RESOURCE_USAGE only"), and its own ~Handle breakdown lists didFreeBlock as "a counter, RESOURCE_USAGE-only" — but a non-atomic RMW on a shared counter is still a data race, and the split design left this call in the concurrent half.
Step-by-step proof.
- Collector-conn GC finishes marking;
runEndPhase()runs on the collector thread.returnEmptyBlocks()unlinks 100 blocks and queues them inm_deferredDestroyBlocks. changePhase(conn, CollectorPhase::NotRunning)→finishChangingPhase(): sinceconn == GCConductor::Collector, it callsresumeThePeriphery()thenresumeTheMutator(). The mutator thread wakes up and starts allocating.- Collector thread continues to Heap.cpp:1867, calls
destroyDeferredBlocks(), and startsdeleteing blocks. For each one,~Handle()reachesheap.didFreeBlock(16384). - Collector thread loads
m_blockBytesAllocated(say, 10 000 000). - Concurrently, mutator thread hits
LocalAllocator::allocateSlowCase→tryAllocateBlock→new Handle(...)→didAllocateBlock(16384): loads 10 000 000, stores 10 016 384. - Collector thread stores 10 000 000 − 16 384 = 9 983 616. The mutator's increment is lost; the counter is now 16 KB low, permanently.
Impact. Minor — this only manifests when returnEmptyBlocksAtEndOfCollection is set (off by default), ENABLE(RESOURCE_USAGE) is on, and the collector thread has the conn. The affected field is a diagnostic stats counter (exposed via blockBytesAllocated()), not a correctness input; the error is cumulative drift, not a crash. However, it is a TSan-visible race, and this PR explicitly names TSan as the gate for landing the concurrent version — so it will fire there in RESOURCE_USAGE builds. It also falsifies the "no shared state" invariant that both the code comment and the audit doc assert.
How to fix. Either of:
- Make
m_blockBytesAllocatedastd::atomic<size_t>and use relaxedfetch_add/fetch_subindidAllocateBlock/didFreeBlock— mirroring the audit's own recommendation form_capacityin row 4. - Move the
didFreeBlockaccounting into the world-stopped half: callheap().didFreeBlock(MarkedBlock::blockSize)fromdeferBlockDestruction()(alongside the existingm_capacity -=decrement), and guard the call in~Handle()on!m_isRemovedFromDirectoryso it isn't double-counted.
1 participant
What this is
Two things, one of which is a plain bug fix and one of which is deliberately not a shipping default.
1.
IncrementalSweeper::startSweepingno longer overwrites a pending deadline. This is an independent bug.startSweeping()runs at the end of every collection and callsscheduleTimer(), andJSRunLoopTimer::Manager::scheduleTimeroverwrites an existing entry's fire time (JSRunLoopTimer.cpp:157). A client that collects more often than the ~100ms deadline pushes it back forever, so the sweeper never runs and empty blocks accumulate for the process lifetime. Guarding ontimeUntilFire()fixes it. Verified this cannot wedge the sweeper permanently: a fired timer is removed from the list (JSRunLoopTimer.cpp:100,takeLast()) and a completed sweep callscancelTimer()(IncrementalSweeper.cpp:92), so the guard always re-arms.2.
returnEmptyBlocksAtEndOfCollection(default off) — a measurement scaffold. It returns empty, non-destructible blocks in the End phase. This adds work to the stop-the-world pause and is not meant to ship on. The intended design frees these blocks from the collector thread after the world resumes. This mode exists so the memory win can be measured in isolation, giving the concurrent version an exact target — any RSS delta between the two is then a bug in the concurrent path rather than an open question.retainedEmptyBlocksPerDirectory(default 1) bounds what a directory that allocated during the cycle may keep as an allocation cache. Cold directories keep nothing, so the retained set stays proportional to the working set rather than to directory count (there are ~300 directories; keep-1 across all of them would be ~5MB of permitted slack for nothing).Measurement
Sustained churn, no explicit
gc()— this matters.Heap::collectNow(Sync)callssweepSynchronously()(Heap.cpp:1337-1341) →sweepBlocks()+shrink(), which already returns every empty block. So an explicit-gc()benchmark cannot show this change doing anything; my first one measured 8 blocks and nearly fooled me. Only automatic, allocation-triggered collections reach the path.Debug + ASan build. The 32 retained is exactly keep-1 × the hot directory count, i.e. the gate behaving as designed.
Scope limits, stated up front
DoesNotNeedDestructionblocks only.endMarking()setsdestructibleBits() = liveBits()forNeedsDestructiondirectories (BlockDirectory.cpp:310) and only a sweep clears it, soemptyBits & ~destructibleBitsis always empty there.shrink()gets away with the same expression only becausesweepSynchronouslysweeps first. The mask is load-bearing, not incidental: these blocks are unswept and freeing a destructible one would skip its cells' destructors. Extending to swept-then-freed destructible blocks is follow-up work.~WeakSetdestroys weak blocks without running pending finalizers and without rehoming ones a liveWeak<T>still points into (WeakSet.cpp:34-43vsWeakSet::sweep). TheisOnList()half ofisTriviallyDestructible()is the load-bearing half —WeakSet::sweepcan emptym_blockswhile leaving the set active — so please don't delete it as redundant.prepareForAllocation(). What makes it safe is thatreset()nulls everyLocalAllocator'sm_lastActiveBlock— not theinUseclaim protocol, which does not coverm_lastActiveBlockat all:MarkedBlock::Handle::stopAllocatingclears the bit (MarkedBlock.cpp:241) before it is stored, and the tree then asserts the wholeinUsevector is empty (BlockDirectory.cpp:206-213).Container audit for the concurrent design
Source/JavaScriptCore/heap/CONCURRENT-BLOCK-RETURN-AUDIT.mdis the review artifact: every shared structure the free path touches, its readers/writers, and what each needs. Headlines:IncrementalSweeperprecedent gives us nothing. It frees with the world running, which looks like the hard case already solved — but it is aJSRunLoopTimercallback on the mutator thread. The protection is serialization by thread identity, which is exactly what a collector-thread writer destroys.assertIsMutatorOrMutatorIsStopped()is not a stale over-assertion; it is accurate, and it isASSERT_ENABLED-only, so release builds have no protection at all.removeBlockwritesm_blocks[i] = nullptrandm_freeBlockIndices.append(i)outsidem_bitvectorLock(BlockDirectory.cpp:189-190) whileaddBlockdoes all of its work inside it (:146) — including them_blocks.append()that can reallocate. It also publishes the index to the free list before clearing the bits, which is the inverse of whataddBlockasserts.IsoCellSet::addis lock-free on a hot path and really does run on parallel marker threads (GlobalExecutable.cpp:51-52);didRemoveBlockfrees theBitSetit dereferences.MarkedBlockSetis unguarded and read by conservative scanning and the sampling profiler's validation. The profiler's own thread turns out to be clean (it defers validation to the mutator underm_lock), and conservative scanning is world-stopped-only — so that reader can stay lock-free if we make the disjointness an explicit invariant.m_lastActiveBlockis a raw pointer to a not-inUseblock, so a freer selecting on~inUseBits()would free it. Fixing this structurally is a prerequisite for the concurrent version, not a detail.Testing
JSTests/stress/return-empty-blocks-at-end-of-collection.js, run in 4 configs (keep-0/keep-1,--useGenerationalGC=0,--useConcurrentGC=0,--verifyGC=1), all green on a debug+ASan build. The test drives the path with sustained churn and no explicitgc(), checks every survivor after the collection that could have freed its block, and covers the weak-set path withWeakRef/FinalizationRegistry.It is a safety test — it guards against returning a block with live cells. The memory win is proven by the measurement above, not by the test. TSan is the gate for the concurrent version and has not run here, because there is no concurrent code in this PR yet.