[JSC] Sweeping a MarkedBlock does not read cells in its decommitted pages (+ a testing mode for the page decommit)
of #628: it follows up the MarkedBlock page decommit (e8bd7f041187, Options::decommitUnusedMarkedBlockPages, on by
default), which is on main. Each commit builds on its own.
Why
MarkedBlock::Handle::decommitUnusedPages() gives the interior OS pages of a swept block that hold no live cell back to
the OS after a full collection. Every later sweep of a block whose cells have destructors then calls isZapped() on every
dead cell, including the ones in those pages, and each such read faults the page back in. What comes back is the shared
zero page, so no memory is lost, but:
- every full collection of an idle process pays a few hundred read faults for nothing;
mincore()and/proc/<pid>/smapsreport the pages as mapped again although they cost nothing, which hides real
regressions in page censuses (that is how this was found);decommitUnusedPages()skips them from then on, becausem_decommittedPagessays they are done.
Measured on a large bundled CLI application, 120 s / 230 s after an interactive session ends: pages of MarkedBlocks that
are mapped but hold no live cell 288 / 217 -> 174 / 23. What is left belongs to blocks that were swept to a free list
since the last full collection (their pages are recommitted on purpose).
What
1. Sweeping a MarkedBlock does not read cells in its decommitted pages (20 lines)
A cell that starts in a decommitted page is dead (a page is only decommitted if no live cell overlaps it) and reads as all
zero, i.e. zapped: its destructor ran in the sweep that preceded the decommit. Handle::sweep() copies
m_decommittedPages into m_zeroPagesDuringSweep when it begins (before a sweep to a free list clears it in
recommitPages()), and the destroy lambda of specializedSweep() returns early for a cell that starts in one of those
pages. Which sweep does the skipping is decided once per block: finishSweepKnowingHeapCellType() sends a block whose
mask is non-zero to the unspecialized instantiation of specializedSweep(), the only one that tests the mask per cell
(if constexpr (!specialize)); the specialized instantiations, which sweep every other block of destructible cells, compile
to the same code as before. The free-list construction still writes the first cell of each free run, as before.
Cost on the common path: none. weak-map-constructor-empty / weak-set-constructor-empty (two million destructible cells
swept), --useConcurrentJIT=0, instructions:u, median of 5-7 interleaved runs: main 140.18 M / 140.12 M; commit 1 alone
140.18 M / 140.18 M (+0.00 % / +0.04 %); both commits 140.20-140.23 M / 140.19-140.22 M (+0.02..0.07 %). (The first version
of this commit tested the mask in every destroy call and cost +1.9 % on these two files, 142.77 M.) sizeof(MarkedBlock::Handle) is
unchanged (the new uint16_t sits next to m_decommittedPages in existing padding).
2. Options::poisonDecommittedMarkedBlockPages, $vm.markedBlockStatistics(), $vm.decommittedMarkedBlockPagePoison() (testing only)
The decommit relies on nothing reading a dead cell of a swept block before the block is swept to a free list again: a
decommitted page reads as zero instead of the cell's old contents. Liveness queries (isLive, conservative roots,
forEachLiveCell/forEachDeadCell, IsoCellSet) consult the mark and newlyAllocated bits; the weak-set finalizers of a
block run in that block's sweep before its pages go; the free-list construction writes before it reads; and with commit 1
sweeps skip cells in decommitted pages. What that leaves is a destructor or finalizer that reads another dead cell (the
reason structure blocks are excluded from the decommit), and any future reader. The option turns such a read into a
failure:
- the pages are poisoned when they are decommitted and unpoisoned when the block is swept to a free list or freed;
- under ASan with
__asan_poison_memory_region, so a read is reported where it happens; - without ASan by filling them with
0xbd(and clearing them again at unpoison time, since the sweep treats them as
zapped), so that a reader sees neither a zapped nor a valid cell.
$vm.markedBlockStatistics() returns { blocks, blocksWithDecommittedPages, decommittedPages, pagesPerBlock };
$vm.decommittedMarkedBlockPagePoison() returns "asan", "pattern" or "none" without reading a poisoned byte.
Safety
Commit 1 removes reads, it adds none. The only way it could change behaviour is a destructor that is skipped although it
should run: the skipped cell would have to be not zapped, i.e. written after the decommit, which only an allocation does,
and a block is recommitted (mask cleared, copy taken before that) before it is handed to an allocator. The poison mode of
commit 2 checks the same invariant from the other side: with the 0xbd fill a cell in a decommitted page is not
zapped, so a sweep that did not skip it would call a destructor on garbage and crash. The new test asserts that pages were
decommitted and that the poison is in place, so it cannot pass vacuously.
Tests
jsc shells (release, and debug + ASan with ASAN_OPTIONS=detect_stack_use_after_return=0) of this branch and of main
(cf1b36ec8703), base lists retaken in the same session on the same machine.
New test JSTests/stress/marked-block-decommitted-pages-are-not-read.js (three run lines: non-concurrent GC,
collectContinuously, scribbleFreeCells, all with the poison): 3/3 on release, 3/3 on ASan. It churns cells with and without
destructors, ropes, butterflies and Maps across repeated full and eden collections and heap snapshots, and asserts that
pages were decommitted and that the poison is in place. With commit 1 gutted (if (false && zeroPages)) it crashes
(SIGSEGV) in all three run lines; with only the per-block routing taken out (blocks with decommitted pages go to the
specialized sweeps again) it crashes in the two run lines that do not already force the unspecialized sweep
(scribbleFreeCells does); restored, it passes and the rebuilt binary is byte-identical to the tested one.
On main:
| JSTests/stress | this branch, fail / pass | main, fail / pass | only failing here |
|---|---|---|---|
release, all 5,832 files, --poisonDecommittedMarkedBlockPages=1 | 35 / 5600 | 37 / 5597 (default) | recursive-try-catch.js (intermittent "Used too much heap": fails on main too in 2 of its 3 other runs) |
release, poison + --scribbleFreeCells=1 --verifyGC=1 | 34 / 5601 | 37 / 5597 (default) | none |
| debug + ASan, every fourth file (1,458), poison (real ASan poisoning), run on the previous version of commit 1 | 12 / 1393 | 13 / 1392 (12 + one timeout under load) | none |
main's extra release failures are three timing-dependent tests (int8-repeat-in-then-out-of-bounds.js,
intl-datetimeformat-language-change-mid-construction.js: 3/200 vs 4/200 when repeated on both shells,
class-fields-to-property-key-const-string-ftl.js).
The same two commits were first tested on top of #628's branch (209144af8f6c), where more modes were run; failures / passes
of 5,849 files, base = that branch without the two commits:
| mode | with the commits | base, same mode without the poison | only failing with the commits |
|---|---|---|---|
| default | 35 / 5617 | 36 / 5615 | none |
--collectContinuously=1 | 35 / 5617 | 37 / 5614 | none |
| poison | 36 / 5616 | 36 / 5615 (default) | intl-datetimeformat-language-change-mid-construction.js (the flaky one above) |
poison + --collectContinuously=1 | 35 / 5617 | 37 / 5614 (collectContinuously) | none |
poison + --scribbleFreeCells=1 --verifyGC=1 | 35 / 5617 | 36 / 5615 (scribble + verifyGC) | none |
| ASan every fourth (1,462): poison | 15 / 1406 | 15 / 1406 | none (same 15) |
| ASan every fourth: poison + scribble + verifyGC | 15 / 1406 | 15 / 1406 | none (same 15) |
sizeof(MarkedBlock::Handle) is 104 before and after (static compile). Earlier still (on an older base) the two commits
were run over the full release suite with verifyHeap and useZombieMode and over a 74-file subset of Bun's test suite with
the poison on: no new failure; not repeated here.
Not done
- Structure blocks stay excluded from the decommit (Structure destructors read other dead cells).
- Blocks swept to a free list between full collections keep their pages until the next full collection's sweep; that is
the remaining 174 / 23. - The non-ASan poison costs a
memsetper decommitted run; it is a testing option and stays off.