From d056f96f407bfdee292e008f5a3722304476752b Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Wed, 15 Jul 2026 07:46:25 +0000 Subject: [PATCH 1/2] Add an End-phase empty-block return scaffold and fix incremental sweeper 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 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. --- ...eturn-empty-blocks-at-end-of-collection.js | 70 +++++++ Source/JavaScriptCore/heap/BlockDirectory.cpp | 47 +++++ Source/JavaScriptCore/heap/BlockDirectory.h | 7 +- .../heap/CONCURRENT-BLOCK-RETURN-AUDIT.md | 182 ++++++++++++++++++ Source/JavaScriptCore/heap/Heap.cpp | 9 + .../heap/IncrementalSweeper.cpp | 6 +- Source/JavaScriptCore/heap/MarkedSpace.cpp | 9 + Source/JavaScriptCore/heap/MarkedSpace.h | 1 + Source/JavaScriptCore/runtime/OptionsList.h | 2 + 9 files changed, 331 insertions(+), 2 deletions(-) create mode 100644 JSTests/stress/return-empty-blocks-at-end-of-collection.js create mode 100644 Source/JavaScriptCore/heap/CONCURRENT-BLOCK-RETURN-AUDIT.md diff --git a/JSTests/stress/return-empty-blocks-at-end-of-collection.js b/JSTests/stress/return-empty-blocks-at-end-of-collection.js new file mode 100644 index 0000000000000..4ae3ece2ebecf --- /dev/null +++ b/JSTests/stress/return-empty-blocks-at-end-of-collection.js @@ -0,0 +1,70 @@ +//@ runDefault("--returnEmptyBlocksAtEndOfCollection=1", "--retainedEmptyBlocksPerDirectory=0") +//@ runDefault("--returnEmptyBlocksAtEndOfCollection=1", "--retainedEmptyBlocksPerDirectory=1", "--useGenerationalGC=0") +//@ runDefault("--returnEmptyBlocksAtEndOfCollection=1", "--retainedEmptyBlocksPerDirectory=0", "--useConcurrentGC=0") + +// Returning blocks that marking proved empty must never return one that still holds a reachable +// cell. Every survivor here is checked after the collection that could have freed its block. + +function shouldBe(actual, expected) { + if (actual !== expected) throw new Error(`bad value: expected ${expected} but got ${actual}`); +} + +// 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; +} + +// Sustained churn with NO explicit gc() first: an explicit collection sweeps synchronously and +// shrinks, which returns the empty blocks before the End phase ever sees them. Only automatic, +// allocation-triggered collections drive the path under test. +const held = []; +for (let round = 0; round < 30; ++round) { + const survivors = churn(round); + for (let j = 0; j < survivors.length; ++j) shouldBe(survivors[j].i, j * 10); + if (round % 10 === 0) held.push(survivors[0]); +} +for (let k = 0; k < held.length; ++k) shouldBe(held[k].round, k * 10); + +const kept = []; +for (let round = 0; round < 20; ++round) { + const survivors = churn(round); + gc(); + for (let j = 0; j < survivors.length; ++j) { + const o = survivors[j]; + shouldBe(o.round, round); + shouldBe(o.i, j * 10); + shouldBe(o.payload[2], j * 10 + 2); + } + // Hold every other round's survivors across the next round so blocks age past one cycle. + if (round % 2 === 0) kept.push(survivors); +} + +gc(); +shouldBe(kept.length, 10); +for (let k = 0; k < kept.length; ++k) { + const survivors = kept[k]; + shouldBe(survivors.length, 2000); + shouldBe(survivors[0].round, k * 2); + shouldBe(survivors[1999].i, 19990); +} + +// 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); diff --git a/Source/JavaScriptCore/heap/BlockDirectory.cpp b/Source/JavaScriptCore/heap/BlockDirectory.cpp index c069adcb5b565..eea167e2b5137 100644 --- a/Source/JavaScriptCore/heap/BlockDirectory.cpp +++ b/Source/JavaScriptCore/heap/BlockDirectory.cpp @@ -225,6 +225,7 @@ void BlockDirectory::prepareForAllocation() m_emptyCursor = 0; assertSweeperIsSuspended(); + m_allocatedSinceLastCollection = !edenBits().isEmpty(); edenBits().clearAll(); if (Options::useImmortalObjects()) [[unlikely]] { @@ -402,6 +403,52 @@ void BlockDirectory::shrink() } } +void BlockDirectory::returnEmptyBlocks(unsigned retainCount) +{ + assertSweeperIsSuspended(); + + // Only a directory that allocated during the cycle that just ended keeps a cache, so the + // retained set stays proportional to the working set rather than to directory count. Note + // this does give up donors for findEmptyBlockToSteal(): under an allocator with a page cache + // of its own, re-requesting a block is cheaper than holding one here against a maybe. + if (!m_allocatedSinceLastCollection) + retainCount = 0; + + unsigned retained = 0; + Locker locker(bitvectorLock()); + for (size_t index = 0; index < m_blocks.size(); ++index) { + index = (emptyBits() & ~destructibleBits() & ~inUseBits()).findBit(index, true); + if (index >= m_blocks.size()) + break; + + // Every empty block left behind counts against retainCount, whether it was kept as cache + // or skipped below, so the option bounds the footprint a directory can sit on. + if (retained < retainCount) { + retained++; + continue; + } + + // Unlike shrink(), which only ever runs on swept blocks, these are still unswept. + // ~WeakSet destroys weak blocks without running their pending finalizers and without + // rehoming ones a live Weak points into; isOnList() rejects a set that WeakSet::sweep + // emptied but left active. Anything else stays for the sweeper. + MarkedBlock::Handle* block = m_blocks[index]; + if (!block->weakSet().isTriviallyDestructible()) { + retained++; + continue; + } + + ASSERT(!isInUse(index)); + setIsInUse(index, true); + { + DropLockForScope scope(locker); + markedSpace().freeBlock(block); + } + // freeBlock() deleted the handle; removeBlock() already cleared every bit for this index, + // so do not touch it again -- the mutator may own it by now. + } +} + // FIXME: rdar://139998916 MarkedBlock::Handle* BlockDirectory::findMarkedBlockHandleDebug(MarkedBlock* block) { diff --git a/Source/JavaScriptCore/heap/BlockDirectory.h b/Source/JavaScriptCore/heap/BlockDirectory.h index affda31b638b4..562ce72b3c4e5 100644 --- a/Source/JavaScriptCore/heap/BlockDirectory.h +++ b/Source/JavaScriptCore/heap/BlockDirectory.h @@ -72,6 +72,7 @@ class BlockDirectory { void NODELETE snapshotUnsweptForFullCollection(); void sweep(); void shrink(); + void returnEmptyBlocks(unsigned retainCount); void assertNoUnswept(); size_t cellSize() const { return m_cellSize; } CellAttributes attributes() const { return m_attributes; } @@ -185,7 +186,11 @@ class BlockDirectory { // this number is bound by capacity of Vector m_blocks, which must be within unsigned. unsigned m_emptyCursor { 0 }; unsigned m_unsweptCursor { 0 }; // Points to the next block that is a candidate for incremental sweeping. - + + // Snapshot of edenBits() taken in prepareForAllocation() just before it clears them, so that + // returnEmptyBlocks() can tell a directory that is part of the working set from a cold one. + bool m_allocatedSinceLastCollection { false }; + // FIXME: All of these should probably be references. // https://bugs.webkit.org/show_bug.cgi?id=166988 Subspace* m_subspace { nullptr }; diff --git a/Source/JavaScriptCore/heap/CONCURRENT-BLOCK-RETURN-AUDIT.md b/Source/JavaScriptCore/heap/CONCURRENT-BLOCK-RETURN-AUDIT.md new file mode 100644 index 0000000000000..2bc6903b2b86c --- /dev/null +++ b/Source/JavaScriptCore/heap/CONCURRENT-BLOCK-RETURN-AUDIT.md @@ -0,0 +1,182 @@ +# Container audit: freeing empty MarkedBlocks from the collector thread after the world resumes + +Status: **audit only — no concurrent code written yet.** This is the artifact that step 1 of the plan +asks for: the complete list of shared structures the free path touches, their current readers and +writers, and what each would require. Line numbers are against `4895f45dfbd0` plus the End-phase +measurement scaffold in this branch. + +## Summary + +The concurrent design is not blocked by the allocator or by the block memory. It is blocked by four +bookkeeping containers whose current safety rests on a single sentence: + +> `BlockDirectory::removeBlock()` asserts `assertIsMutatorOrMutatorIsStopped()` +> (`BlockDirectory.cpp:182`) — **either the world is stopped, or you are the API-lock-owning mutator +> thread.** + +Every world-running reader of these containers satisfies that same predicate, so today *there is +only ever one thread*. The containers need no locks because they have never had a second writer. +A collector-thread writer after `resumeThePeriphery()` is that second writer, and it invalidates +the invariant for readers in four files at once, with **zero compile-time signal** — the assertion +is `ASSERT_ENABLED`-only (`BlockDirectory.cpp:554-565`), so release builds get no protection at all. + +**The `IncrementalSweeper` precedent does not transfer.** It frees blocks with the world running +(`IncrementalSweeper.cpp:113-118`), which looks like the hard case already solved — but the sweeper +is a `JSRunLoopTimer` callback **on the mutator thread**. It is serialization by thread identity, +which is exactly what this change destroys. + +## The containers + +| # | Container | Owner | Guard today | Verdict | +|---|---|---|---|---| +| 1 | `m_blocks` (`MarkedBlockSet`: `HashSet` + `TinyBloomFilter`) | `MarkedSpace.h:225` | **none** | **Blocker** | +| 2 | `m_blocks` (`Vector`), `m_freeBlockIndices` | `BlockDirectory.h:172-173` | **none** (unannotated) | **Blocker** | +| 3 | `IsoCellSet::m_bits`, `m_blocksWithBits` | `IsoCellSet.h:89-90` | half-locked | **Blocker** | +| 4 | `MarkedSpace::m_capacity` | `MarkedSpace.h:217` | **none** | Blocker (lost update) | +| 5 | `Heap::m_blockBytesAllocated` | `Heap.h:991` | none | Minor (`RESOURCE_USAGE` only) | +| 6 | `AlignedMemoryAllocator::freeAlignedMemory` | 3 subclasses | thread-safe | **Safe** | + +### 1. `MarkedBlockSet` — the widest blast radius + +`MarkedSpace::freeBlock` does `m_blocks.remove(&block->block())` (`MarkedSpace.cpp:403`). `remove()` +can shrink the table and then `recomputeFilter()` walks the whole set (`MarkedBlockSet.h:57-63`). +Concurrent readers exist and are **unlocked**: conservative scanning (`ConservativeRoots.cpp:87,204`), +`HeapUtil.h:68`, `VMInspector.cpp:182`, and `SamplingProfiler.cpp:494`. A rehash under any of them is +a use-after-free on the backing table, not a stale read. + +Two mitigating facts, both verified: + +- **The sampling-profiler thread never touches this container.** `takeSample` carries raw `CalleeBits` + and validates later *on the mutator thread* under `m_lock` (`SamplingProfiler.cpp:489-494`). Its + own thread only consults `CodeBlockSet`, which *has* a lock. `CodeBlockSet` is therefore the + in-tree precedent for exactly this problem. +- **Conservative scanning is world-stopped-only** (`ASSERT(m_heap.objectSpace().isMarking())`, + `ConservativeRoots.cpp:89`). So it can stay lock-free, but only if we make this explicit: + +> **Proposed invariant:** the collector free path never runs while the world is stopped, and +> conservative scanning never runs while the world is running. Disjoint by construction. + +The filter itself is a non-issue: it only ever *gains* bits, so a stale read is a false positive the +subsequent `set.contains()` resolves. + +### 2. `BlockDirectory::m_blocks` / `m_freeBlockIndices` — the asymmetry + +``` +Vector m_blocks; // BlockDirectory.h:172 — NOT guarded +Vector m_freeBlockIndices; // BlockDirectory.h:173 — NOT guarded +BlockDirectoryBits m_bits WTF_GUARDED_BY_LOCK(m_bitvectorLock); // :177 — guarded +``` + +`addBlock` takes `Locker locker { m_bitvectorLock }` (`BlockDirectory.cpp:146`) and does **all** of +its work under it, including the `m_blocks.append()` that can **reallocate the buffer** and the +`m_bits.resize()`. `removeBlock` does **none** of its vector work under the lock: + +``` +187 subspace()->didRemoveBlock(block->index()); // unlocked +189 m_blocks[block->index()] = nullptr; // unlocked +190 m_freeBlockIndices.append(block->index()); // unlocked <- index published as free +193 Locker locker(bitvectorLock()); // lock finally taken +194-197 clear every bit for the index +``` + +Three consequences: + +- **Realloc race.** Mutator `addBlock` reallocs `m_blocks` while the collector writes `nullptr` into + the freed old buffer. +- **Inverted publication order.** The index reaches the free list (`:190`) *before* its bits are + cleared (`:194`). `addBlock` asserts the opposite (`:166-169`). A reader that takes the lock inside + that window sees `canAllocate=true, inUse=false` and loads `m_blocks[i] == nullptr` — no reader + null-checks a bitvector-derived index. +- **Word sharing.** All ten bit kinds for one index live in one `Segment`, one 32-bit word covers 32 + indices (`BlockDirectoryBits.h:73`). Any unlocked bit RMW corrupts 31 neighbours. Note + `LocalAllocator.cpp:250` writes `setIsEden` with **no lock** and an admitting FIXME. + +**Lock-ordering trap:** `removeBlock` calls `subspace()->didRemoveBlock()` at `:187`, and +`IsoCellSet::didRemoveBlock` acquires `m_bitvectorLock` *itself* (`IsoCellSet.cpp:108`). `WTF::Lock` +is not recursive, so naively wrapping `removeBlock`'s body in the lock **self-deadlocks**. This must +be resolved before the obvious fix is even available. (`releaseAssertAcquiredBitVectorLock()` at +`:192` is *not* an unlock — it is an empty static-analysis annotation, `BlockDirectory.h:103`.) + +### 3. `IsoCellSet` — a genuinely lock-free hot reader + +`IsoCellSet::add` (`IsoCellSetInlines.h:43-48`) loads `m_bits[blockIndex].get()` with **no lock** and +dereferences it. The bit-set itself is atomic (`concurrentTestAndSet`); the *pointer load* is not. +`IsoCellSet::didRemoveBlock` nulls that slot **outside** the lock (`IsoCellSet.cpp:111`), freeing the +`BitSet`. That is a UAF. + +`add` really does run off the mutator: `GlobalExecutable::visitChildrenImpl` (`GlobalExecutable.cpp:51-52`) +and `FunctionExecutable::visitChildrenImpl` (`FunctionExecutable.cpp:106`) call it from **parallel +marker threads**. + +Taking the lock in `add` is a hot-path regression. **Recommended instead: don't free the `BitSet` in +`didRemoveBlock` at all** — clear it and let the slot keep the allocation (indices are recycled via +`m_freeBlockIndices`). No reader ever dereferences freed memory, and the lock-free fast path +survives. Cost: one `BitSet` per retired index per set. + +Only `IsoSubspace` overrides `didRemoveBlock`; `CompleteSubspace` inherits the empty base +(`Subspace.cpp:138-140`), so the majority of blocks are unaffected. + +### 4. `MarkedSpace::m_capacity` + +Plain `size_t` (`MarkedSpace.h:217`). `-=` in `freeBlock` (`:403`, would become collector) races `+=` +in `didAddBlock` (`:558`, mutator). Tearing is not the issue; the **lost update** is — the error is +cumulative and never healed, and `m_capacity` feeds GC sizing via `Heap::capacity()`. Make it +`std::atomic`, relaxed. + +### 6. The allocators are fine + +All three `AlignedMemoryAllocator` subclasses (`FastMalloc`, `Gigacage`, `Structure` — +there is no `IsoAlignedMemoryAllocator` in this tree) are thread-safe and none assumes the mutator. +`StructureAlignedMemoryAllocator`'s system-heap fallback keeps its own bitmap but correctly guards it +with `m_lock`. Its `USE(MIMALLOC)` branch shares one process-wide `mi_heap_t`: a cross-thread +`mi_free` is legal (mimalloc routes it to the page's atomic `xthread_free` list), **but the block +does not return to the owning heap's free list immediately** — it is reclaimed at the owner's next +allocation. That defers reclaim relative to today's mutator-side free, which partially undercuts the +motivation and should be measured, not assumed. + +## The `m_lastActiveBlock` claim — REFUTED + +The plan states that `resumeAllocating()` only reinstates `m_lastActiveBlock`, "a block the allocator +claimed and still holds `inUse` on". **That premise is false.** + +`LocalAllocator::stopAllocating` assigns `m_lastActiveBlock = m_currentBlock` (`LocalAllocator.cpp:83`) +*after* `MarkedBlock::Handle::stopAllocating` has already cleared the bit — its last statement is +`directory()->didFinishUsingBlock(this)` (`MarkedBlock.cpp:241`), i.e. `setIsInUse(handle, false)`. +The tree then asserts the whole `inUse` vector is empty right afterwards (`BlockDirectory.cpp:206-213`). +So `m_lastActiveBlock` is a raw pointer to a **not-inUse** block, fully eligible for any freer +selecting on `~inUseBits()`, and `resumeAllocating` dereferences it with no liveness check +(`LocalAllocator.cpp:94`). + +**The conclusion survives, but for a different reason: phase ordering.** `prepareForAllocation()` +→ `LocalAllocator::reset()` nulls `m_lastActiveBlock` unconditionally (`LocalAllocator.cpp:47-53`), +and it runs at `Heap.cpp:1807`, immediately before the End-phase call site. That — not the claim +protocol — is what makes the scaffold safe, and it is why the scaffold **must** stay after +`prepareForAllocation()`. The comment at the call site now says so. + +This matters for the concurrent design: once the world resumes, the next `stopTheWorld` repopulates +`m_lastActiveBlock` across every LocalAllocator with not-inUse blocks while a collector-thread freer +is walking `emptyBits() & ~inUseBits()`. **Fixing this structurally is a prerequisite**, not a +detail. The smaller of the two options: keep `inUse` set for `m_lastActiveBlock` (don't call +`didFinishUsingBlock` in `Handle::stopAllocating`; clear it in `resumeAllocating`/`reset` instead), +which also makes the bit mean what `BlockDirectoryBits.h:45` claims — "acts like a lock bit". + +There is a second, narrower window of the same class: `Handle::resumeAllocating` can return early +without setting `inUse` (`MarkedBlock.cpp:275-281`) while `LocalAllocator::resumeAllocating` +unconditionally assigns `m_currentBlock` (`LocalAllocator.cpp:96`). Unreachable today only because +`endMarking` precedes `prepareForAllocation` inside `endPhase`. Nothing asserts it. + +## What step 2 actually costs + +1. Resolve the `didRemoveBlock` recursion (`WTF::Lock` is not recursive) — a `WTF_REQUIRES_LOCK` + overload, deleting the inner `Locker`. +2. Annotate `m_blocks` / `m_freeBlockIndices` `WTF_GUARDED_BY_LOCK(m_bitvectorLock)` and let the + compiler enumerate the fallout — that list *is* the work. +3. Reorder `removeBlock`: clear bits → null the slot → publish the index last. +4. Fix `m_lastActiveBlock` structurally (above). +5. Lock or defer `MarkedBlockSet::remove`; make `m_capacity` atomic. +6. Leave `IsoCellSet::add` lock-free by not freeing the `BitSet`. +7. Re-justify each of ~12 unlocked bit accessors whose only warrant is + `assertSweeperIsSuspended()` / `assertIsMutatorOrMutatorIsStopped()` — both are claims about the + *incremental sweeper* and the *mutator*, neither of which a collector-thread freer is. + +Every race above is release-build-silent. TSan is the gate, not assertions. diff --git a/Source/JavaScriptCore/heap/Heap.cpp b/Source/JavaScriptCore/heap/Heap.cpp index 10905dfe305c7..2fda250644ba4 100644 --- a/Source/JavaScriptCore/heap/Heap.cpp +++ b/Source/JavaScriptCore/heap/Heap.cpp @@ -1805,6 +1805,15 @@ NEVER_INLINE bool Heap::runEndPhase(GCConductor conn) m_codeBlocks->clearCurrentlyExecutingAndRemoveDeadCodeBlocks(vm()); m_objectSpace.prepareForAllocation(); + + // Marking reached fixpoint, so emptyBits() names blocks with no reachable cells, and + // prepareForAllocation() just reset every LocalAllocator, so none of them holds a block + // pointer. That reset -- not the inUse claim protocol -- is what makes this safe here: + // m_lastActiveBlock is NOT inUse (MarkedBlock::Handle::stopAllocating clears it), so this + // call must stay after prepareForAllocation(). + if (Options::returnEmptyBlocksAtEndOfCollection()) + m_objectSpace.returnEmptyBlocks(Options::retainedEmptyBlocksPerDirectory()); + updateAllocationLimits(); if (m_verifier) [[unlikely]] { diff --git a/Source/JavaScriptCore/heap/IncrementalSweeper.cpp b/Source/JavaScriptCore/heap/IncrementalSweeper.cpp index 5137afd7b95e5..4fb4837bffb96 100644 --- a/Source/JavaScriptCore/heap/IncrementalSweeper.cpp +++ b/Source/JavaScriptCore/heap/IncrementalSweeper.cpp @@ -128,7 +128,11 @@ bool IncrementalSweeper::sweepNextBlock(VM& vm, SweepTrigger trigger) void IncrementalSweeper::startSweeping(JSC::Heap& heap) { - scheduleTimer(); + // Scheduling overwrites any deadline already pending (JSRunLoopTimer::Manager::scheduleTimer), + // and this runs at the end of every collection. A client that collects more often than the + // deadline would push it back forever and the sweeper would never run at all. + if (!timeUntilFire()) + scheduleTimer(); m_currentDirectory = heap.objectSpace().firstDirectory(); } diff --git a/Source/JavaScriptCore/heap/MarkedSpace.cpp b/Source/JavaScriptCore/heap/MarkedSpace.cpp index 369ca66607002..31554475f9fa5 100644 --- a/Source/JavaScriptCore/heap/MarkedSpace.cpp +++ b/Source/JavaScriptCore/heap/MarkedSpace.cpp @@ -415,6 +415,15 @@ void MarkedSpace::shrink() }); } +void MarkedSpace::returnEmptyBlocks(unsigned retainCountPerDirectory) +{ + forEachDirectory( + [&] (BlockDirectory& directory) -> IterationStatus { + directory.returnEmptyBlocks(retainCountPerDirectory); + return IterationStatus::Continue; + }); +} + void MarkedSpace::beginMarking() { switch (heap().collectionScope().value()) { diff --git a/Source/JavaScriptCore/heap/MarkedSpace.h b/Source/JavaScriptCore/heap/MarkedSpace.h index 1c22af51cddc4..d84bf0abd614c 100644 --- a/Source/JavaScriptCore/heap/MarkedSpace.h +++ b/Source/JavaScriptCore/heap/MarkedSpace.h @@ -130,6 +130,7 @@ class MarkedSpace { template void forEachSubspace(const Functor&); void shrink(); + void returnEmptyBlocks(unsigned retainCountPerDirectory); void freeBlock(MarkedBlock::Handle*); void didAddBlock(MarkedBlock::Handle*); diff --git a/Source/JavaScriptCore/runtime/OptionsList.h b/Source/JavaScriptCore/runtime/OptionsList.h index dba9d6e9411ba..0105b1580576c 100644 --- a/Source/JavaScriptCore/runtime/OptionsList.h +++ b/Source/JavaScriptCore/runtime/OptionsList.h @@ -249,6 +249,8 @@ bool hasCapacityToUseLargeGigacage(); v(Unsigned, preciseAllocationCutoff, 100000, Normal, nullptr) \ v(Bool, dumpSizeClasses, false, Normal, nullptr) \ v(Bool, stealEmptyBlocksFromOtherAllocators, true, Normal, nullptr) \ + v(Bool, returnEmptyBlocksAtEndOfCollection, false, Normal, "return blocks that marking proved empty to the block allocator in the End phase instead of waiting for the incremental sweeper's timer. Measurement scaffold: this adds work to the stop-the-world pause, so it is not meant to ship on"_s) \ + v(Unsigned, retainedEmptyBlocksPerDirectory, 1, Normal, "empty blocks a directory allocated out of during the last cycle may keep as an allocation cache when returnEmptyBlocksAtEndOfCollection is set"_s) \ v(Bool, eagerlyUpdateTopCallFrame, false, Normal, nullptr) \ v(Bool, dumpZappedCellCrashData, false, Normal, nullptr) \ \ From 5668b216db467796eced3f422760ae62a56c0334 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Wed, 15 Jul 2026 08:02:29 +0000 Subject: [PATCH 2/2] Release returned blocks' memory after the world resumes, not inside the 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. --- Source/JavaScriptCore/heap/BlockDirectory.cpp | 5 +-- .../heap/CONCURRENT-BLOCK-RETURN-AUDIT.md | 43 ++++++++++++++++--- Source/JavaScriptCore/heap/Heap.cpp | 9 +++- Source/JavaScriptCore/heap/MarkedBlock.cpp | 10 ++++- Source/JavaScriptCore/heap/MarkedBlock.h | 8 +++- Source/JavaScriptCore/heap/MarkedSpace.cpp | 25 +++++++++++ Source/JavaScriptCore/heap/MarkedSpace.h | 10 ++++- Source/JavaScriptCore/runtime/OptionsList.h | 2 +- 8 files changed, 97 insertions(+), 15 deletions(-) diff --git a/Source/JavaScriptCore/heap/BlockDirectory.cpp b/Source/JavaScriptCore/heap/BlockDirectory.cpp index eea167e2b5137..ce55f7d2a14bf 100644 --- a/Source/JavaScriptCore/heap/BlockDirectory.cpp +++ b/Source/JavaScriptCore/heap/BlockDirectory.cpp @@ -442,10 +442,9 @@ void BlockDirectory::returnEmptyBlocks(unsigned retainCount) setIsInUse(index, true); { DropLockForScope scope(locker); - markedSpace().freeBlock(block); + markedSpace().deferBlockDestruction(block); } - // freeBlock() deleted the handle; removeBlock() already cleared every bit for this index, - // so do not touch it again -- the mutator may own it by now. + // removeBlock() already cleared every bit for this index, so do not touch it again. } } diff --git a/Source/JavaScriptCore/heap/CONCURRENT-BLOCK-RETURN-AUDIT.md b/Source/JavaScriptCore/heap/CONCURRENT-BLOCK-RETURN-AUDIT.md index 2bc6903b2b86c..7b383d9ae88b5 100644 --- a/Source/JavaScriptCore/heap/CONCURRENT-BLOCK-RETURN-AUDIT.md +++ b/Source/JavaScriptCore/heap/CONCURRENT-BLOCK-RETURN-AUDIT.md @@ -1,11 +1,34 @@ # Container audit: freeing empty MarkedBlocks from the collector thread after the world resumes -Status: **audit only — no concurrent code written yet.** This is the artifact that step 1 of the plan -asks for: the complete list of shared structures the free path touches, their current readers and -writers, and what each would require. Line numbers are against `4895f45dfbd0` plus the End-phase -measurement scaffold in this branch. +Status: **the audit below is why the shipped design splits the free in half instead of making these +containers concurrent.** Read "Outcome" first; the rest is the evidence. -## Summary +## Outcome + +`~MarkedBlock::Handle` is one unsafe line and three safe ones: + +``` +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 +``` + +Every hazard in this audit lives in `removeBlock`. None of it lives in `freeAlignedMemory`. So the +implementation does the bookkeeping half in the End phase, where the mutator is already quiesced and +the cost is a bit-scan, and defers the memory release to after `resumeThePeriphery()`, where the +expensive part runs concurrently with the mutator. By then the block is unlinked from every +directory, from `MarkedBlockSet`, and from every `IsoCellSet` — **nothing can reach it, so there is +no shared state left to race.** The containers below never need to become concurrent. + +This also dodges the race that kills the "walk the directories after resume" design outright: +`m_requests.removeFirst()` (`Heap.cpp:1843`) runs *before* `resumeThePeriphery()` (`Heap.cpp:1893`), +so a queued collection can start the instant the mutator resumes — running `stopThePeriphery` → +`stopAllocating`, which repopulates every `m_lastActiveBlock` with a **not-inUse** block, while the +freer is walking `emptyBits & ~inUseBits`. No phase-ordering argument saves that; see the +`m_lastActiveBlock` section. + +## Summary of the containers (why the above was necessary) The concurrent design is not blocked by the allocator or by the block memory. It is blocked by four bookkeeping containers whose current safety rests on a single sentence: @@ -165,7 +188,9 @@ without setting `inUse` (`MarkedBlock.cpp:275-281`) while `LocalAllocator::resum unconditionally assigns `m_currentBlock` (`LocalAllocator.cpp:96`). Unreachable today only because `endMarking` precedes `prepareForAllocation` inside `endPhase`. Nothing asserts it. -## What step 2 actually costs +## What making the containers concurrent WOULD have cost (not done, and not needed) + +Kept for the record, because if anyone later wants the full-concurrent walk, this is the bill: 1. Resolve the `didRemoveBlock` recursion (`WTF::Lock` is not recursive) — a `WTF_REQUIRES_LOCK` overload, deleting the inner `Locker`. @@ -179,4 +204,8 @@ unconditionally assigns `m_currentBlock` (`LocalAllocator.cpp:96`). Unreachable `assertSweeperIsSuspended()` / `assertIsMutatorOrMutatorIsStopped()` — both are claims about the *incremental sweeper* and the *mutator*, neither of which a collector-thread freer is. -Every race above is release-build-silent. TSan is the gate, not assertions. +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())`). diff --git a/Source/JavaScriptCore/heap/Heap.cpp b/Source/JavaScriptCore/heap/Heap.cpp index 2fda250644ba4..6d6539d1a97e6 100644 --- a/Source/JavaScriptCore/heap/Heap.cpp +++ b/Source/JavaScriptCore/heap/Heap.cpp @@ -1859,7 +1859,14 @@ NEVER_INLINE bool Heap::runEndPhase(GCConductor conn) m_totalGCTime += m_lastGCEndTime - m_lastGCStartTime; if (endingCollectionScope == CollectionScope::Full) m_lastFullGCEndTime = m_lastGCEndTime; - return changePhase(conn, CollectorPhase::NotRunning); + bool result = changePhase(conn, CollectorPhase::NotRunning); + + // changePhase() ran resumeThePeriphery(), so the world is running again by here. Release the + // memory of the blocks unlinked above now, off the pause. This may race the mutator, and is + // safe only because those blocks are already unreachable -- see destroyDeferredBlocks(). + m_objectSpace.destroyDeferredBlocks(); + + return result; } bool Heap::changePhase(GCConductor conn, CollectorPhase nextPhase) diff --git a/Source/JavaScriptCore/heap/MarkedBlock.cpp b/Source/JavaScriptCore/heap/MarkedBlock.cpp index 4430cbd053e23..fbdc427a998b7 100644 --- a/Source/JavaScriptCore/heap/MarkedBlock.cpp +++ b/Source/JavaScriptCore/heap/MarkedBlock.cpp @@ -159,12 +159,20 @@ MarkedBlock::Handle::~Handle() if (!(balance % 10)) dataLog("MarkedBlock Balance: ", balance, "\n"); } - m_directory->removeBlock(this, BlockDirectory::WillDeleteBlock::Yes); + if (!m_isRemovedFromDirectory) + m_directory->removeBlock(this, BlockDirectory::WillDeleteBlock::Yes); m_block->~MarkedBlock(); m_alignedMemoryAllocator->freeAlignedMemory(m_block); heap.didFreeBlock(blockSize); } +void MarkedBlock::Handle::removeFromDirectoryForDeferredDestruction() +{ + ASSERT(!m_isRemovedFromDirectory); + m_directory->removeBlock(this, BlockDirectory::WillDeleteBlock::Yes); + m_isRemovedFromDirectory = true; +} + MarkedBlock::MarkedBlock(VM& vm, Handle& handle) { new (&header()) Header(vm, handle); diff --git a/Source/JavaScriptCore/heap/MarkedBlock.h b/Source/JavaScriptCore/heap/MarkedBlock.h index 1bf8d98a2bae3..a0d26ca69c207 100644 --- a/Source/JavaScriptCore/heap/MarkedBlock.h +++ b/Source/JavaScriptCore/heap/MarkedBlock.h @@ -123,7 +123,12 @@ class MarkedBlock { public: ~Handle(); - + + // Unlink from the directory now and let ~Handle skip removeBlock(). Splits a free into its + // bookkeeping half, which needs the mutator quiesced, and its release-the-memory half, + // which does not. See MarkedSpace::deferBlockDestruction(). + void removeFromDirectoryForDeferredDestruction(); + MarkedBlock& block(); MarkedBlock::Header& blockHeader(); @@ -241,6 +246,7 @@ class MarkedBlock { CellAttributes m_attributes; bool m_isFreeListed { false }; + bool m_isRemovedFromDirectory { false }; unsigned m_index { std::numeric_limits::max() }; AlignedMemoryAllocator* m_alignedMemoryAllocator { nullptr }; diff --git a/Source/JavaScriptCore/heap/MarkedSpace.cpp b/Source/JavaScriptCore/heap/MarkedSpace.cpp index 31554475f9fa5..a0bbb0fd5e781 100644 --- a/Source/JavaScriptCore/heap/MarkedSpace.cpp +++ b/Source/JavaScriptCore/heap/MarkedSpace.cpp @@ -424,6 +424,31 @@ void MarkedSpace::returnEmptyBlocks(unsigned retainCountPerDirectory) }); } +void MarkedSpace::deferBlockDestruction(MarkedBlock::Handle* block) +{ + // The bookkeeping half of a free: everything that touches state the mutator shares. It must + // happen while the mutator is quiesced, so it happens here, in the End phase. + m_capacity -= MarkedBlock::blockSize; + m_blocks.remove(&block->block()); + block->removeFromDirectoryForDeferredDestruction(); + m_deferredDestroyBlocks.append(block); +} + +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; +} + void MarkedSpace::beginMarking() { switch (heap().collectionScope().value()) { diff --git a/Source/JavaScriptCore/heap/MarkedSpace.h b/Source/JavaScriptCore/heap/MarkedSpace.h index d84bf0abd614c..43c194971a022 100644 --- a/Source/JavaScriptCore/heap/MarkedSpace.h +++ b/Source/JavaScriptCore/heap/MarkedSpace.h @@ -131,6 +131,9 @@ class MarkedSpace { void shrink(); void returnEmptyBlocks(unsigned retainCountPerDirectory); + void deferBlockDestruction(MarkedBlock::Handle*); + void destroyDeferredBlocks(); + bool hasDeferredBlocks() const { return !m_deferredDestroyBlocks.isEmpty(); } void freeBlock(MarkedBlock::Handle*); void didAddBlock(MarkedBlock::Handle*); @@ -223,7 +226,12 @@ class MarkedSpace { bool m_conservativeScanIsPrepared { false }; Lock m_directoryLock; MarkedBlockSet m_blocks; - + + // Blocks unlinked from every directory and from m_blocks, whose memory has not been released + // yet. Only ever touched by the collection that filled it, before the mutator resumes and + // immediately after, so it needs no lock of its own. + Vector m_deferredDestroyBlocks; + SentinelLinkedList> m_activeWeakSets; SentinelLinkedList> m_newActiveWeakSets; diff --git a/Source/JavaScriptCore/runtime/OptionsList.h b/Source/JavaScriptCore/runtime/OptionsList.h index 0105b1580576c..98e37dc98b9ea 100644 --- a/Source/JavaScriptCore/runtime/OptionsList.h +++ b/Source/JavaScriptCore/runtime/OptionsList.h @@ -249,7 +249,7 @@ bool hasCapacityToUseLargeGigacage(); v(Unsigned, preciseAllocationCutoff, 100000, Normal, nullptr) \ v(Bool, dumpSizeClasses, false, Normal, nullptr) \ v(Bool, stealEmptyBlocksFromOtherAllocators, true, Normal, nullptr) \ - v(Bool, returnEmptyBlocksAtEndOfCollection, false, Normal, "return blocks that marking proved empty to the block allocator in the End phase instead of waiting for the incremental sweeper's timer. Measurement scaffold: this adds work to the stop-the-world pause, so it is not meant to ship on"_s) \ + v(Bool, returnEmptyBlocksAtEndOfCollection, false, Normal, "return blocks that marking proved empty to the block allocator at the end of every collection instead of waiting for the incremental sweeper's timer. Unlinking happens in the End phase; the memory is released after the world resumes"_s) \ v(Unsigned, retainedEmptyBlocksPerDirectory, 1, Normal, "empty blocks a directory allocated out of during the last cycle may keep as an allocation cache when returnEmptyBlocksAtEndOfCollection is set"_s) \ v(Bool, eagerlyUpdateTopCallFrame, false, Normal, nullptr) \ v(Bool, dumpZappedCellCrashData, false, Normal, nullptr) \