diff --git a/changelog.d/7298-dirty-page-mark-cache.md b/changelog.d/7298-dirty-page-mark-cache.md new file mode 100644 index 0000000000..5daf50b715 --- /dev/null +++ b/changelog.d/7298-dirty-page-mark-cache.md @@ -0,0 +1,75 @@ +perf(gc): stop re-recording the page the write barrier just recorded (#7187 Phase B). + +`mark_dirty_old_page` is the tail of the remembered-set half of the write +barrier: it inserts the written slot's 4 KiB page into the thread's +`DIRTY_OLD_PAGES` modbuf and mirrors the fact into the arena's per-page +metadata — two thread-local accesses and two hash operations, on every +old→young store. Measured on `benchmarks/app-patterns/kernels/batch.ts` with +the barrier armed, it fires **1 774 374 times and produces 517 distinct +pages**: 99.97% of the work re-inserts a page that is already there. #7170's +ranked profile puts the symbol at 6.73% of that whole program. + +A **one-entry, thread-local last-page cache** now answers those repeats. The +shape was picked from the page sequence, not from intuition — simulating cache +designs over the exact sequence the barrier produces: + +| shape | hit rate | calls left | +|---|---:|---:| +| **1-entry (chosen)** | **99.7817%** | **3 873** | +| 2-entry LRU | 99.8730% | 2 253 | +| 4-entry LRU | 99.8730% | 2 253 | +| 512-entry direct-mapped | 99.9423% | 1 023 | +| 2048-entry direct-mapped | 99.9531% | 832 | + +The stores arrive in long same-page runs (3 872 runs, mean 458, longest +13 803), so the redundancy is *consecutive* repetition and one entry captures +it. Every larger shape buys ≤0.17 percentage points for more state, an index +computation and — for the direct-mapped variants — kilobytes of thread-local +storage on a path that runs on every heap store. + +**The invariant.** If the cache holds page `P`, then on this thread +`P ∈ DIRTY_OLD_PAGES` *and* `P`'s `OldPageMeta.dirty` is already `true`. Those +are exactly what `mark_dirty_old_page(P)` establishes, so under the invariant +the call is a pure no-op. The cache can only suppress a *repeat* of a recording +that already happened, never a first one — the remembered set stays complete, +which is the whole correctness bar (a page holding an old→young edge that is +not recorded is a live object freed by the next minor). It is maintained by +three rules: the cache is populated only after **both** halves have just been +established (`arena::old_page_mark_dirty` now reports whether a metadata entry +existed, and a page recorded in only one place is deliberately not cached); it +is invalidated at every point that can falsify either half — `clear_one_dirty_ +old_page`, the sole `DIRTY_OLD_PAGES` removal, plus `arena::old_page_clear_ +dirty` and `arena::unregister_old_block_pages`; and it is thread-local, like +both structures it summarises, so one thread's mark can never suppress +another's. + +Interaction with Phase A (#7250): an unarmed barrier never reaches +`mark_dirty_old_page`, so this cache is simply never consulted while unarmed. +The reconstruct that arms the barrier rebuilds the log through the same +function and only ever inserts, so it populates the cache exactly as the +barrier would and cannot falsify the invariant. + +Measured before/after on `batch.ts` (armed via a leading `perry/gc` `collect()`; +macOS arm64, `perry-dev` profile, separate target dir per arm, artifact hashes +asserted different). Every pre-existing barrier counter is **identical** across +the two arms — `calls` 2 265 777, `non_pointer_child_skips` 280 536, +`parent_not_old_skips` 211 497, `old_to_young_slow_hits` 1 773 744, +`dirty_page_mark_attempts` 1 773 744, `new_dirty_pages` 517, `new_inserts` 517 +— and the only difference is the new `dirty_page_cache_hits`, 0 → **1 770 501**. +Calls reaching the modbuf and the arena metadata: **1 773 744 → 3 243**. The +page set is unchanged and proven so in-process rather than by comparing two +ASLR'd runs: an instrumented build recorded both the set of pages the barrier +*asked* to mark and the set that actually reached the recording body, and they +are equal (517) in both arms. `batch.ts` output stays byte-identical to the +pinned Node oracle. + +New `--lib` tests in `gc/tests/dirty_page_cache.rs` (four), each asserting its +own subject was live rather than that nothing broke: the fast path really +fires (`dirty_page_cache_hits > 0` under a forced trace guard), a genuinely new +page is never swallowed, a clear really invalidates — so a store to the same +page afterwards is recorded again — and a minor whose stores were 255/256 cache +hits still has `missing_edges == 0` with the young child marked through the +dirty page. Sabotage-checked both ways: deleting the fast path fails three of +the four, deleting the clear-side invalidation fails the completeness test with +"the next store to that page would be dropped and the old→young edge lost for +good". diff --git a/crates/perry-runtime/src/arena/page_meta.rs b/crates/perry-runtime/src/arena/page_meta.rs index 7767ed2598..0368bced4c 100644 --- a/crates/perry-runtime/src/arena/page_meta.rs +++ b/crates/perry-runtime/src/arena/page_meta.rs @@ -319,6 +319,10 @@ pub(crate) fn unregister_old_block_pages(pages: &[usize]) { index.remove(&page); } }); + // #7187 Phase B: the other place a page's dirty stamp stops existing — the + // metadata entry itself is gone. A cached page whose metadata was dropped + // is no longer a complete recording, so drop the cache. + crate::gc::dirty_page_cache_invalidate(); } #[inline] @@ -877,12 +881,18 @@ pub(crate) fn old_arena_page_index_remove_object(header_addr: usize, total_size: }); } -pub(crate) fn old_page_mark_dirty(page: usize) { +/// Stamp `page`'s metadata dirty. Returns whether a metadata entry existed to +/// stamp: #7187 Phase B's "already dirty" cache may only remember a page whose +/// recording is complete in BOTH the modbuf and here, so it has to know. +pub(crate) fn old_page_mark_dirty(page: usize) -> bool { OLD_GEN_PAGE_META.with(|meta| { if let Some(page_meta) = meta.borrow_mut().get_mut(&page) { page_meta.dirty = true; + true + } else { + false } - }); + }) } pub(crate) fn old_page_clear_dirty(page: usize) { @@ -891,6 +901,11 @@ pub(crate) fn old_page_clear_dirty(page: usize) { page_meta.dirty = false; } }); + // #7187 Phase B: one of the two places a page's `dirty` stamp can go false, + // so one of the places the barrier's cached page can stop being a complete + // recording. Invalidating here rather than at the callers covers the GC's + // own clear loop and the tests that reach for this directly. + crate::gc::dirty_page_cache_invalidate(); } #[cfg(test)] diff --git a/crates/perry-runtime/src/gc/barrier.rs b/crates/perry-runtime/src/gc/barrier.rs index 9f561fa6e6..4edfcfd808 100644 --- a/crates/perry-runtime/src/gc/barrier.rs +++ b/crates/perry-runtime/src/gc/barrier.rs @@ -911,6 +911,10 @@ pub(super) fn bump_write_barrier_trace_counter(counter: BarrierTraceCounter) { } BarrierTraceCounter::NewInserts => counters.new_inserts += 1, BarrierTraceCounter::DirtyPageMarkAttempts => counters.dirty_page_mark_attempts += 1, + BarrierTraceCounter::DirtyPageCacheHits => { + counters.dirty_page_mark_attempts += 1; + counters.dirty_page_cache_hits += 1; + } BarrierTraceCounter::NewDirtyPages => counters.new_dirty_pages += 1, BarrierTraceCounter::ConservativeParentSpanMarks => { counters.conservative_parent_span_marks += 1; @@ -1308,17 +1312,46 @@ pub(super) fn remember_old_to_young_external_slot(parent_addr: usize, slot_addr: ) } +/// #7187 Phase B: record `page` in this thread's modbuf, unless it is already +/// there. Returns whether the page was NEWLY inserted. +/// +/// The guard is the whole of Phase B — see [`super::dirty_page_cache`] for the +/// invariant it rests on and the measurement that picked a one-entry cache. +/// Armed on `batch.ts` this call fires 1 774 374 times for 517 distinct pages; +/// the guard turns 99.78% of those into a thread-local load and a compare. +#[inline] pub(super) fn mark_dirty_old_page(page: usize) -> bool { + if super::dirty_page_cache::dirty_old_page_already_marked(page) { + // Bumps `dirty_page_mark_attempts` too, so that counter keeps meaning + // "calls", comparable across the change, and + // `attempts - dirty_page_cache_hits` is what still reaches the modbuf. + bump_write_barrier_trace_counter(BarrierTraceCounter::DirtyPageCacheHits); + return false; + } + mark_dirty_old_page_uncached(page) +} + +/// Out of line: the hot path is the guard above, and this body's two +/// thread-local accesses plus two hash operations are the 6.73% #7170 measured. +#[inline(never)] +fn mark_dirty_old_page_uncached(page: usize) -> bool { bump_write_barrier_trace_counter(BarrierTraceCounter::DirtyPageMarkAttempts); ever_dirty_note(page); - DIRTY_OLD_PAGES.with(|s| { + let inserted = DIRTY_OLD_PAGES.with(|s| { let inserted = s.borrow_mut().insert(page); - crate::arena::old_page_mark_dirty(page); if inserted { bump_write_barrier_trace_counter(BarrierTraceCounter::NewDirtyPages); } inserted - }) + }); + // Cache ONLY when the arena stamp landed as well. `old_page_mark_dirty` + // does nothing for a page with no metadata entry, and caching such a page + // would let a later `old_page_summary()` under-report `dirty_pages` if the + // metadata appeared afterwards. Half a recording is not a recording. + if crate::arena::old_page_mark_dirty(page) { + super::dirty_page_cache::note_dirty_old_page_marked(page); + } + inserted } thread_local! { @@ -1782,6 +1815,9 @@ fn dirty_old_pages_empty() -> bool { DIRTY_OLD_PAGES.with(|s| s.borrow().is_empty()) } +/// The **sole** path that removes a page from `DIRTY_OLD_PAGES`. Every other +/// touch of that set is an insert, a read, or the snapshot — which is why +/// #7187 Phase B's cache needs exactly one invalidation point on this side. fn clear_one_dirty_old_page() -> bool { DIRTY_OLD_PAGES.with(|s| { let mut pages = s.borrow_mut(); @@ -1790,6 +1826,14 @@ fn clear_one_dirty_old_page() -> bool { }; crate::arena::old_page_clear_dirty(page); pages.remove(&page); + // DELIBERATELY redundant with `old_page_clear_dirty`, which invalidates + // too (#7187 Phase B rule 2). The cache's invariant has two halves and + // this line owns the modbuf one: an edit that stops the arena side from + // invalidating — or a page whose metadata entry no longer exists, so + // `old_page_clear_dirty` finds nothing to clear — must not silently + // leave the cache asserting a page this function just removed. The cost + // is one thread-local store on the cold clear path. + super::dirty_page_cache::invalidate(); true }) } diff --git a/crates/perry-runtime/src/gc/dirty_page_cache.rs b/crates/perry-runtime/src/gc/dirty_page_cache.rs new file mode 100644 index 0000000000..74bd44b8f0 --- /dev/null +++ b/crates/perry-runtime/src/gc/dirty_page_cache.rs @@ -0,0 +1,119 @@ +//! #7187 Phase B — the write barrier's "this page is already dirty" cache. +//! +//! Split out of `barrier.rs`, which is at the 2 000-line cap +//! `scripts/check_file_size.sh` enforces. +//! +//! # What this removes +//! +//! [`super::barrier::mark_dirty_old_page`] is the tail of the remembered-set +//! half of the write barrier: it inserts the written slot's 4 KiB page number +//! into the thread's `DIRTY_OLD_PAGES` modbuf and mirrors the fact into the +//! arena's per-page metadata. Two thread-local accesses and two hash +//! operations, on every old→young store. +//! +//! Measured on `benchmarks/app-patterns/kernels/batch.ts` with the barrier +//! armed: **1 774 374 calls producing 517 distinct pages** — 99.971% of the +//! work is re-inserting a page that is already in the set. #7170's ranked +//! profile puts `mark_dirty_old_page` at 6.73% of that whole program. +//! +//! # Why a one-entry cache, and not something cleverer +//! +//! Because that is what the page sequence says. Simulating cache shapes over +//! the exact sequence `mark_dirty_old_page` sees on `batch.ts` (armed): +//! +//! | shape | hit rate | calls left | +//! |---|---:|---:| +//! | **1-entry (this)** | **99.7817%** | **3 873** | +//! | 2-entry LRU | 99.8730% | 2 253 | +//! | 4-entry LRU | 99.8730% | 2 253 | +//! | 512-entry direct-mapped | 99.9423% | 1 023 | +//! | 2048-entry direct-mapped | 99.9531% | 832 | +//! +//! The stores arrive in long same-page runs (3 872 runs over 1 774 374 calls; +//! mean run 458, longest 13 803), so the whole redundancy is *consecutive* +//! repetition and one entry captures it. Every larger shape buys ≤0.17 +//! percentage points for more state, an index computation, and — for the +//! direct-mapped variants — kilobytes of thread-local storage on a path that +//! runs on every heap store. One `usize` and one compare is the mechanism the +//! data supports. +//! +//! # The invariant +//! +//! > **If `LAST_DIRTY_OLD_PAGE` holds page `P` (i.e. is not [`NO_PAGE`]) then, +//! > on this thread, `P ∈ DIRTY_OLD_PAGES` *and* `P`'s `OldPageMeta.dirty` is +//! > already `true`.** +//! +//! Both halves are exactly what `mark_dirty_old_page(P)` establishes, so under +//! the invariant that call is a pure no-op and skipping it cannot lose an +//! old→young edge. The remembered set stays **complete**: the cache can only +//! suppress a *repeat* of a recording that already happened, never a first one. +//! +//! It is maintained by three rules, and the deliberate narrowness of the first +//! is the whole soundness argument: +//! +//! 1. **Only [`note_dirty_old_page_marked`] populates it, and only after both +//! halves have just been established** — including the arena stamp, which is +//! conditional (`old_page_mark_dirty` silently does nothing for a page with +//! no metadata entry). A page recorded in the modbuf but not in the metadata +//! is deliberately *not* cached, so the metadata can never drift behind. +//! 2. **[`invalidate`] runs on every path that can falsify either half.** For +//! `DIRTY_OLD_PAGES` that is `clear_one_dirty_old_page` — the sole removal +//! (every other touch is an insert, a read, or the snapshot). For the +//! metadata it is `arena::old_page_clear_dirty` and +//! `arena::unregister_old_block_pages`, the only two places a `dirty` bit +//! goes false or a page's metadata disappears. +//! 3. **It is thread-local, like both things it summarises.** `DIRTY_OLD_PAGES` +//! and `OLD_GEN_PAGE_META` are per-thread; a process-global cache would let +//! thread A's mark suppress thread B's, dropping the page from B's modbuf +//! entirely — a missed edge, i.e. heap corruption, not a slow program. +//! +//! # Interaction with Phase A (#7250) +//! +//! Phase A leaves the remembered-set half of the barrier **unarmed** until the +//! first read of the log, and an unarmed barrier never reaches +//! `mark_dirty_old_page` at all — so in the unarmed state this cache is simply +//! never consulted and never populated. The reconstruct that arms the barrier +//! (`arm_and_reconstruct_remembered_set_if_unarmed`) rebuilds the log by +//! calling `StickyRememberedSet::restore`, which goes through +//! `mark_dirty_old_page` like everything else: the cache is populated by the +//! reconstruct exactly as it would be by the barrier, and, since the +//! reconstruct only ever *inserts*, it cannot falsify the invariant. + +use std::cell::Cell; + +/// "Nothing cached". Not a reachable page number: pages are `addr >> 12`, so +/// `usize::MAX` would need a 76-bit address. +const NO_PAGE: usize = usize::MAX; + +thread_local! { + static LAST_DIRTY_OLD_PAGE: Cell = const { Cell::new(NO_PAGE) }; +} + +/// Is `page` known to be recorded already? See the module invariant. +#[inline] +pub(super) fn dirty_old_page_already_marked(page: usize) -> bool { + debug_assert_ne!(page, NO_PAGE, "page number collides with the empty marker"); + LAST_DIRTY_OLD_PAGE.with(Cell::get) == page +} + +/// Record that `page` is now in `DIRTY_OLD_PAGES` **and** stamped dirty in the +/// arena page metadata. Callers must have established both immediately before. +#[inline] +pub(super) fn note_dirty_old_page_marked(page: usize) { + LAST_DIRTY_OLD_PAGE.with(|cell| cell.set(page)); +} + +/// Drop the cached page. Called from every path that can remove a page from +/// `DIRTY_OLD_PAGES` or un-stamp / discard its arena metadata — see rule 2 in +/// the module doc. Cheap enough (one thread-local store) that these callers do +/// not check whether the page they touched is the cached one. +pub(crate) fn invalidate() { + LAST_DIRTY_OLD_PAGE.with(|cell| cell.set(NO_PAGE)); +} + +/// Test-only: is the cache currently empty? Lets the #7187 Phase B tests assert +/// that an invalidation really happened rather than that nothing broke. +#[cfg(test)] +pub(super) fn is_empty_for_tests() -> bool { + LAST_DIRTY_OLD_PAGE.with(Cell::get) == NO_PAGE +} diff --git a/crates/perry-runtime/src/gc/mod.rs b/crates/perry-runtime/src/gc/mod.rs index 8bc22cba0f..5c79ac8864 100644 --- a/crates/perry-runtime/src/gc/mod.rs +++ b/crates/perry-runtime/src/gc/mod.rs @@ -66,6 +66,11 @@ mod trace; pub(crate) use trace::*; mod barrier; pub use barrier::*; +mod dirty_page_cache; +// #7187 Phase B: `crate::arena`'s page-metadata module invalidates the +// barrier's "already dirty" page cache when it un-stamps or discards a page. +// Re-exported under an unambiguous name — `arena` cannot see `gc`'s privates. +pub(crate) use dirty_page_cache::invalidate as dirty_page_cache_invalidate; mod barrier_arming; // #7277: every item in `barrier_arming` is `pub(super)` (i.e. `pub(in gc)`), // which is narrower than `pub(crate)` — so the glob re-exported nothing and diff --git a/crates/perry-runtime/src/gc/telemetry.rs b/crates/perry-runtime/src/gc/telemetry.rs index c8fe66bb5c..abae571489 100644 --- a/crates/perry-runtime/src/gc/telemetry.rs +++ b/crates/perry-runtime/src/gc/telemetry.rs @@ -480,6 +480,10 @@ pub(super) struct BarrierTraceCounters { pub(super) remembered_set_insert_attempts: u64, pub(super) new_inserts: u64, pub(super) dirty_page_mark_attempts: u64, + /// #7187 Phase B: dirty-page mark attempts short-circuited by the + /// "already dirty" page cache. Counted INSIDE `dirty_page_mark_attempts`, + /// so `attempts - cache_hits` is what still reaches the modbuf. + pub(super) dirty_page_cache_hits: u64, pub(super) new_dirty_pages: u64, pub(super) conservative_parent_span_marks: u64, pub(super) unarmed_skips: u64, @@ -497,6 +501,7 @@ impl BarrierTraceCounters { remembered_set_insert_attempts: 0, new_inserts: 0, dirty_page_mark_attempts: 0, + dirty_page_cache_hits: 0, new_dirty_pages: 0, conservative_parent_span_marks: 0, unarmed_skips: 0, @@ -515,6 +520,11 @@ pub(super) enum BarrierTraceCounter { RememberedSetInsertAttempts, NewInserts, DirtyPageMarkAttempts, + /// #7187 Phase B: a `mark_dirty_old_page` call the "already dirty" cache + /// answered without touching the modbuf or the arena page metadata. Bumps + /// `dirty_page_mark_attempts` as well, so that counter keeps meaning + /// "calls" and stays comparable with pre-Phase-B measurements. + DirtyPageCacheHits, NewDirtyPages, ConservativeParentSpanMarks, /// #7187: a barrier call whose child WAS a heap pointer but which exited @@ -1006,6 +1016,7 @@ impl GcCycleTrace { "remembered_set_insert_attempts": self.write_barrier.remembered_set_insert_attempts, "new_inserts": self.write_barrier.new_inserts, "dirty_page_mark_attempts": self.write_barrier.dirty_page_mark_attempts, + "dirty_page_cache_hits": self.write_barrier.dirty_page_cache_hits, "new_dirty_pages": self.write_barrier.new_dirty_pages, "conservative_parent_span_marks": self.write_barrier.conservative_parent_span_marks, "unarmed_skips": self.write_barrier.unarmed_skips, diff --git a/crates/perry-runtime/src/gc/tests/dirty_page_cache.rs b/crates/perry-runtime/src/gc/tests/dirty_page_cache.rs new file mode 100644 index 0000000000..55de1ac853 --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/dirty_page_cache.rs @@ -0,0 +1,276 @@ +//! #7187 Phase B — the write barrier's "this page is already dirty" cache. +//! +//! Kept in its own file rather than appended to `barrier.rs`, which is already +//! at the 2 000-line cap `scripts/check_file_size.sh` enforces. +//! +//! Four tests, shaped so that each one **asserts its own subject was live** +//! (CLAUDE.md failure mode #4). A cache that never populated would make every +//! "nothing broke" assertion here pass while proving nothing, which is exactly +//! how `PERRY_GC_FORCE_EVACUATE` stayed inert for months (#6942/#6946). So the +//! counters are read under a forced trace guard — not `if tracing { … }`, which +//! skips the assertion entirely in the default `cargo test` run — and every +//! test states a non-zero `dirty_page_cache_hits`. +//! +//! What is under test, in one line each: +//! +//! 1. the fast path fires, and a genuinely NEW page is never swallowed by it, +//! 2. the clear invalidates it — the sabotage-sensitive one: without this the +//! page is lost from the modbuf forever, which is a missed old→young edge, +//! 3. a real minor collection whose stores were overwhelmingly cache hits +//! still has complete old→young coverage and does not free the child, +//! 4. the arena's per-page `dirty` stamp never drifts behind the modbuf. + +use super::super::*; +use super::support::*; + +/// An old parent whose field array spans at least two 4 KiB generation pages, +/// plus two field indices that land on different pages. The cache is a +/// *page* cache, so a single-page fixture could not distinguish "the fast path +/// works" from "the fast path swallows everything". +unsafe fn old_parent_spanning_two_pages() -> (usize, *mut u64, usize, usize) { + const FIELDS: u32 = 2048; // 16 KiB of slots: ≥ 4 generation pages. + let (old_obj, fields) = alloc_old_test_object(FIELDS); + let first_page = crate::arena::generation_page_for_addr(fields as usize); + let mut other = None; + for i in 0..FIELDS as usize { + if crate::arena::generation_page_for_addr(fields.add(i) as usize) != first_page { + other = Some(i); + break; + } + } + let other = other.expect("test object did not span two generation pages"); + (old_obj as usize, fields, 0, other) +} + +/// Store a fresh nursery pointer into `fields[index]` through the real barrier +/// entry point, and return the page that store should have dirtied. +unsafe fn barriered_young_store(parent: usize, fields: *mut u64, index: usize) -> usize { + let young = crate::arena::arena_alloc_gc(40, 8, GC_TYPE_OBJECT) as usize; + let slot = fields.add(index); + *slot = ptr_bits(young); + js_write_barrier_slot(ptr_bits(parent), slot as u64, ptr_bits(young)); + crate::arena::generation_page_for_addr(slot as usize) +} + +#[test] +fn test_7187b_repeat_marks_hit_the_cache_and_a_new_page_still_gets_recorded() { + let _guard = GcTestIsolationGuard::new(); + reset_remembered_set(); + clear_marks(); + let _trace = TestGcTraceCaptureGuard::force_enabled(); + let _ = take_write_barrier_trace_counters(); + + // The barrier must be ARMED, or this test measures Phase A's unarmed skip + // and says nothing about Phase B. + assert!( + barrier_remembering_armed(), + "Phase B only exists in the armed state — an unarmed barrier never \ + reaches mark_dirty_old_page at all" + ); + + let (parent, fields, first_index, other_index) = unsafe { old_parent_spanning_two_pages() }; + + const REPEATS: usize = 64; + let mut page_a = 0usize; + for _ in 0..REPEATS { + page_a = unsafe { barriered_young_store(parent, fields, first_index) }; + } + + let counters = take_write_barrier_trace_counters(); + assert_eq!( + counters.dirty_page_mark_attempts, REPEATS as u64, + "every store must still ATTEMPT a page mark — the counter keeps meaning \ + 'calls', so it stays comparable with the pre-Phase-B measurement" + ); + assert_eq!( + counters.dirty_page_cache_hits, + REPEATS as u64 - 1, + "all but the first repeat must be answered by the cache — a zero here \ + is an inert fast path, and every other assertion in this file would \ + still pass" + ); + assert_eq!( + counters.new_dirty_pages, 1, + "the page is recorded exactly once, which is the point" + ); + + // …and it really is recorded, in BOTH places a recording lives. + assert_eq!(remembered_dirty_page_count(), 1); + assert!( + old_page_dirty_for(page_a), + "the arena page metadata must mirror the modbuf entry" + ); + + // A genuinely NEW page must not be swallowed. This is the completeness + // half: the cache may only suppress a repeat, never a first recording. + let page_b = unsafe { barriered_young_store(parent, fields, other_index) }; + assert_ne!(page_a, page_b, "fixture must produce two distinct pages"); + let counters = take_write_barrier_trace_counters(); + assert_eq!( + counters.dirty_page_cache_hits, 0, + "a different page must MISS the cache" + ); + assert_eq!(counters.new_dirty_pages, 1); + assert_eq!(remembered_dirty_page_count(), 2); + assert!(old_page_dirty_for(page_a) && old_page_dirty_for(page_b)); + + // Returning to the first page misses (one entry), re-marks, and — this is + // the property that matters — does not lose or duplicate anything. + let back = unsafe { barriered_young_store(parent, fields, first_index) }; + assert_eq!(back, page_a); + let counters = take_write_barrier_trace_counters(); + assert_eq!(counters.dirty_page_cache_hits, 0); + assert_eq!(counters.new_dirty_pages, 0, "page A was already recorded"); + assert_eq!(remembered_dirty_page_count(), 2); + + reset_remembered_set(); + clear_marks(); +} + +#[test] +fn test_7187b_clearing_the_remembered_set_invalidates_the_cache() { + let _guard = GcTestIsolationGuard::new(); + reset_remembered_set(); + clear_marks(); + let _trace = TestGcTraceCaptureGuard::force_enabled(); + let _ = take_write_barrier_trace_counters(); + + let (parent, fields, first_index, _other) = unsafe { old_parent_spanning_two_pages() }; + let page = unsafe { barriered_young_store(parent, fields, first_index) }; + // Subject live: the cache is populated, so there is something to invalidate. + let _ = unsafe { barriered_young_store(parent, fields, first_index) }; + assert!( + take_write_barrier_trace_counters().dirty_page_cache_hits > 0, + "the cache never populated — this test's subject does not exist" + ); + assert!(!crate::gc::dirty_page_cache::is_empty_for_tests()); + assert_eq!(remembered_dirty_page_count(), 1); + + remembered_set_clear(); + + assert_eq!(remembered_dirty_page_count(), 0); + assert!( + !old_page_dirty_for(page), + "the clear must un-stamp the arena page metadata too" + ); + assert!( + crate::gc::dirty_page_cache::is_empty_for_tests(), + "the clear removed the page from the modbuf but left the cache claiming \ + it is recorded — the next store to that page would be dropped and the \ + old→young edge lost for good" + ); + + // The consequence, stated as behaviour rather than as internal state: after + // a clear, a store to the SAME page must be recorded again. + let _ = take_write_barrier_trace_counters(); + let again = unsafe { barriered_young_store(parent, fields, first_index) }; + assert_eq!(again, page); + let counters = take_write_barrier_trace_counters(); + assert_eq!( + counters.dirty_page_cache_hits, 0, + "the first store after a clear must not be answered from the cache" + ); + assert_eq!(counters.new_dirty_pages, 1); + assert_eq!(remembered_dirty_page_count(), 1); + assert!(old_page_dirty_for(page)); + + reset_remembered_set(); + clear_marks(); +} + +#[test] +fn test_7187b_minor_after_cache_heavy_stores_keeps_the_young_child_alive() { + let _guard = GcTestIsolationGuard::new(); + reset_remembered_set(); + clear_marks(); + let _trace = TestGcTraceCaptureGuard::force_enabled(); + let _ = take_write_barrier_trace_counters(); + + let (parent, fields, first_index, _other) = unsafe { old_parent_spanning_two_pages() }; + let slot = unsafe { fields.add(first_index) }; + let page = crate::arena::generation_page_for_addr(slot as usize); + + // Hammer the same slot so that the LAST surviving child's edge is recorded + // through a run that is overwhelmingly cache hits. If the cache could lose + // the page, this is the shape that loses it. + let mut young = 0usize; + for _ in 0..256 { + young = crate::arena::arena_alloc_gc(40, 8, GC_TYPE_OBJECT) as usize; + unsafe { + *slot = ptr_bits(young); + } + js_write_barrier_slot(ptr_bits(parent), slot as u64, ptr_bits(young)); + } + let counters = take_write_barrier_trace_counters(); + assert!( + counters.dirty_page_cache_hits >= 255, + "the run must have been served by the cache (hits={}) or it is not \ + exercising Phase B", + counters.dirty_page_cache_hits + ); + assert!(old_page_dirty_for(page)); + + // The old parent is a live root for the purposes of this check. + let old_header = unsafe { header_from_user_ptr(parent as *const u8) }; + unsafe { + (*old_header).gc_flags |= GC_FLAG_MARKED; + } + let stats = verify_old_to_young_edges_covered(); + assert_eq!( + stats.missing_edges, 0, + "a cache-served store run left an old→young edge uncovered — that is a \ + swept-live-object bug, not a slow program" + ); + assert!(stats.checked_old_to_young_edges >= 1); + + // And the collector really finds the child through the remembered set. + let valid_ptrs = build_valid_pointer_set(); + let marked = mark_remembered_set_roots(&valid_ptrs); + assert!( + marked.newly_marked >= 1, + "remembered-set root marking found no young child" + ); + unsafe { + let child_header = header_from_user_ptr(young as *const u8); + assert_ne!( + (*child_header).gc_flags & GC_FLAG_MARKED, + 0, + "the surviving young child must be marked through the dirty page" + ); + (*old_header).gc_flags &= !GC_FLAG_MARKED; + } + + reset_remembered_set(); + clear_marks(); +} + +#[test] +fn test_7187b_cache_never_outruns_the_arena_dirty_stamp() { + // The cache remembers "recorded", and a recording lives in TWO places: the + // modbuf and `OldPageMeta.dirty`. `old_page_mark_dirty` silently does + // nothing for a page with no metadata entry, so caching on the strength of + // the modbuf alone would let the metadata drift behind. Assert the pairing + // directly: every page the cache is willing to answer for is stamped. + let _guard = GcTestIsolationGuard::new(); + reset_remembered_set(); + clear_marks(); + let _trace = TestGcTraceCaptureGuard::force_enabled(); + let _ = take_write_barrier_trace_counters(); + + let (parent, fields, first_index, other_index) = unsafe { old_parent_spanning_two_pages() }; + for index in [first_index, other_index, first_index, other_index] { + let page = unsafe { barriered_young_store(parent, fields, index) }; + assert!( + old_page_dirty_for(page), + "page {page} answered by the barrier is not stamped dirty" + ); + assert!( + !crate::gc::dirty_page_cache::is_empty_for_tests(), + "a completed recording must populate the cache" + ); + } + assert_eq!(remembered_dirty_page_count(), 2); + + reset_remembered_set(); + clear_marks(); +} diff --git a/crates/perry-runtime/src/gc/tests/mod.rs b/crates/perry-runtime/src/gc/tests/mod.rs index 013a5a3b4b..dbc1502221 100644 --- a/crates/perry-runtime/src/gc/tests/mod.rs +++ b/crates/perry-runtime/src/gc/tests/mod.rs @@ -10,6 +10,7 @@ mod copying_side_tables; mod cycle_state; mod dead_owner_side_tables; mod debt_pacer; +mod dirty_page_cache; mod error_side_tables; mod evacuation; mod fromspace_protect;