perf(gc): stop re-recording the page the write barrier just recorded (#7187 Phase 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".

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)]

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
     })
 }

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<usize> = 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
+}

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

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,