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..ce55f7d2a14bf 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,51 @@ 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().deferBlockDestruction(block); + } + // removeBlock() already cleared every bit for this index, so do not touch it again. + } +} + // 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..7b383d9ae88b5 --- /dev/null +++ b/Source/JavaScriptCore/heap/CONCURRENT-BLOCK-RETURN-AUDIT.md @@ -0,0 +1,211 @@ +# Container audit: freeing empty MarkedBlocks from the collector thread after the world resumes + +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. + +## 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: + +> `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 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`. +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 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 10905dfe305c7..6d6539d1a97e6 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]] { @@ -1850,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/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/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 369ca66607002..a0bbb0fd5e781 100644 --- a/Source/JavaScriptCore/heap/MarkedSpace.cpp +++ b/Source/JavaScriptCore/heap/MarkedSpace.cpp @@ -415,6 +415,40 @@ void MarkedSpace::shrink() }); } +void MarkedSpace::returnEmptyBlocks(unsigned retainCountPerDirectory) +{ + forEachDirectory( + [&] (BlockDirectory& directory) -> IterationStatus { + directory.returnEmptyBlocks(retainCountPerDirectory); + return IterationStatus::Continue; + }); +} + +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 1c22af51cddc4..43c194971a022 100644 --- a/Source/JavaScriptCore/heap/MarkedSpace.h +++ b/Source/JavaScriptCore/heap/MarkedSpace.h @@ -130,6 +130,10 @@ class MarkedSpace { template void forEachSubspace(const Functor&); 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*); @@ -222,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 dba9d6e9411ba..98e37dc98b9ea 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 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) \ \