perf(gc): stop re-recording the page the write barrier just recorded (#7187 Phase B) - #7298
Merged
Conversation
added 2 commits
August 3, 2026 10:49
…7187 Phase B) `mark_dirty_old_page` fires 1,774,374 times on `batch.ts` with the barrier armed and produces 517 distinct pages: 99.97% of its two thread-local accesses and two hash operations re-insert 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 answers those repeats. The shape is picked from the page sequence rather than from intuition: simulated over the exact sequence the barrier produces, one entry hits 99.7817%, and every larger shape (2/4-entry LRU, 8..2048-entry direct-mapped) buys at most 0.17 further percentage points for more state and more work per call. 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. Invariant: if the cache holds page P then, on this thread, P is in DIRTY_OLD_PAGES *and* P's `OldPageMeta.dirty` is already true — exactly what `mark_dirty_old_page(P)` establishes, so the call is a pure no-op. The cache can only suppress a repeat of a recording that already happened, never a first one, so the remembered set stays complete. Maintained by: populate only after both halves have just been established (`old_page_mark_dirty` now reports whether a metadata entry existed, and a half-recorded page is not cached); invalidate 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 keep it thread-local, like both structures it summarises. Measured before/after on `batch.ts`: every pre-existing barrier counter is identical across the arms (calls 2,265,777; old_to_young_slow_hits and dirty_page_mark_attempts 1,773,744; new_dirty_pages 517), the new `dirty_page_cache_hits` goes 0 -> 1,770,501, and calls reaching the modbuf drop 1,773,744 -> 3,243. The page set is proven unchanged in-process (the set of pages the barrier asked to mark equals the set that reached the recording body: 517 in both arms).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a thread-local one-entry cache for recorded dirty old-generation pages. The write barrier uses cache hits to skip repeated remembered-set insertion. Page metadata removal and clearing invalidate the cache. Telemetry and four GC tests validate cache behavior. ChangesDirty old-page cache
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WriteBarrier
participant DirtyPageCache
participant RememberedSet
participant PageMetadata
WriteBarrier->>DirtyPageCache: Check page
alt Cache hit
DirtyPageCache-->>WriteBarrier: Page already recorded
else Cache miss
WriteBarrier->>RememberedSet: Record dirty page
WriteBarrier->>PageMetadata: Mark page dirty
PageMetadata-->>WriteBarrier: Metadata updated
WriteBarrier->>DirtyPageCache: Cache page
end
PageMetadata->>DirtyPageCache: Invalidate on clear or unregister
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-runtime/src/gc/barrier.rs (1)
1818-1833: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant cache invalidation.
old_page_clear_dirty(page)already invalidates the Phase B cache internally (seepage_meta.rsL904-908), as the comment on Line 1827 acknowledges. The explicitsuper::dirty_page_cache::invalidate()on Line 1830 repeats that invalidation on the same call path.This is harmless because
invalidate()is idempotent, but it contradicts the module's stated design of one invalidation point per falsifying operation. Remove the explicit call, or update the comment to explain why the extra call is intentional.♻️ Proposed fix to remove the redundant invalidate call
fn clear_one_dirty_old_page() -> bool { DIRTY_OLD_PAGES.with(|s| { let mut pages = s.borrow_mut(); let Some(page) = pages.iter().next().copied() else { return false; }; - // Also invalidates the Phase B cache, via `old_page_clear_dirty`. + // Invalidates the Phase B cache, via `old_page_clear_dirty`. crate::arena::old_page_clear_dirty(page); pages.remove(&page); - super::dirty_page_cache::invalidate(); true }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/gc/barrier.rs` around lines 1818 - 1833, Remove the redundant super::dirty_page_cache::invalidate() call from clear_one_dirty_old_page, since crate::arena::old_page_clear_dirty(page) already performs the cache invalidation. Update the nearby comment if needed so it attributes invalidation solely to old_page_clear_dirty while preserving the page-removal behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/perry-runtime/src/gc/barrier.rs`:
- Around line 1818-1833: Remove the redundant
super::dirty_page_cache::invalidate() call from clear_one_dirty_old_page, since
crate::arena::old_page_clear_dirty(page) already performs the cache
invalidation. Update the nearby comment if needed so it attributes invalidation
solely to old_page_clear_dirty while preserving the page-removal behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: da35951a-8fcf-4a08-bac0-5ad3ce44568d
📒 Files selected for processing (8)
changelog.d/7298-dirty-page-mark-cache.mdcrates/perry-runtime/src/arena/page_meta.rscrates/perry-runtime/src/gc/barrier.rscrates/perry-runtime/src/gc/dirty_page_cache.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/telemetry.rscrates/perry-runtime/src/gc/tests/dirty_page_cache.rscrates/perry-runtime/src/gc/tests/mod.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase B of #7187. Phase A (lazy barrier arming) landed as #7250; this is the
other half of the same finding — the redundancy that remains even when the
barrier is armed.
What the measurement said
mark_dirty_old_pageis the tail of the remembered-set half of the writebarrier: it inserts the written slot's 4 KiB page into the thread's
DIRTY_OLD_PAGESmodbuf and mirrors the fact into the arena's per-pagemetadata. Two thread-local accesses and two hash operations, on every
old→young store.
On
benchmarks/app-patterns/kernels/batch.tswith the barrier armed it fires1,774,374 times and produces 517 distinct pages — 99.97% of the work
re-inserts a page that is already in the set. #7170's ranked profile puts the
symbol at 6.73% of that whole program.
The mechanism, picked from the data
The obvious shapes were all simulated over the exact page sequence the
barrier produces, before anything was implemented:
The stores arrive in long same-page runs — 3,872 runs over 1,774,374 calls,
mean 458, longest 13,803 — so the redundancy is consecutive repetition and
one entry captures it. Every larger shape buys ≤0.17 further 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
usizeand one compare is what the data supports.The invariant
Those are exactly what
mark_dirty_old_page(P)establishes, so under theinvariant 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 entire correctness bar here: a page holding an
old→young edge that is not recorded is a live object freed by the next minor,
i.e. heap corruption rather than a slow program.
Maintained by three rules, and the narrowness of the first is the argument:
arena::old_page_mark_dirtynow reports whether a metadata entry existed tostamp; a page recorded in the modbuf but not in the metadata is deliberately
not cached, so the metadata can never drift behind. Half a recording is
not a recording.
that is
clear_one_dirty_old_page— the sole removal; every other touchof
DIRTY_OLD_PAGESis an insert, a read, or the snapshot. For the metadatait is
arena::old_page_clear_dirtyandarena::unregister_old_block_pages,the only two places a
dirtybit goes false or a page's metadata vanishes.would let thread A's mark suppress thread B's, dropping the page from B's
modbuf entirely.
Interaction with Phase A: an unarmed barrier never reaches
mark_dirty_old_page, so while unarmed this cache is never consulted and neverpopulated. The reconstruct that arms the barrier rebuilds the log through
StickyRememberedSet::restore→mark_dirty_old_page, i.e. through the samefunction, and only ever inserts — so it populates the cache exactly as the
barrier would and cannot falsify the invariant.
Before / after
batch.tsarmed with a leadingperry/gccollect()(Phase A otherwiseleaves it unarmed, and an unarmed barrier has no Phase B to measure). macOS
arm64,
perry-devprofile, one target dir per arm, base pinned to thisbranch's merge-base
cdc7dee87, all three artifacts (perry,libperry_runtime.a,libperry_stdlib.a) hash-different between arms.write_barriercountercallsnon_pointer_child_skipsparent_not_old_skipsold_to_young_slow_hitsdirty_page_mark_attemptsdirty_page_cache_hits(new)new_dirty_pagesnew_insertsEvery pre-existing counter is identical; the only difference is the new one.
The page set is proven unchanged in-process, not by diffing two ASLR'd
runs. A temporary instrumented build (not in this diff) recorded both the set
of pages the barrier asked to mark and the set that actually reached the
recording body, and asserted them equal at exit:
batch.tsoutput stays byte-identical to the pinned Node 26.5.1 oracle(
checksum: 1601043.5000) in both arms.No codegen change was needed; this is entirely runtime-side.
Tests
Four new
--libtests ingc/tests/dirty_page_cache.rs(per CLAUDE.md,crates/*/tests/*.rsintegration suites do not run per-PR). Each asserts itsown subject was live rather than that nothing broke — counters are read under
TestGcTraceCaptureGuard::force_enabled(), not behind theif tracing { … }that skips the assertion entirely in a default
cargo testrun:dirty_page_cache_hits == 63/64) and a genuinelynew page is never swallowed;
recorded again,
new_dirty_pagesand all;verify_old_to_young_edges_covered().missing_edges == 0and the young childmarked through the dirty page;
dirtystamp never drifts behind the modbuf.Sabotage-checked, both directions:
if false &&) → 3 of the 4 fail, including"a zero here is an inert fast path, and every other assertion in this file
would still pass";
"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".
What is NOT verified
building). Everything above is counts, which are load-independent; the 6.73%
instruction share is repsel: rule-1 provenance for unbound allocations — the 506 are 96% record literals, and Perry's own CJS IIFE is the wall (#7152 follow-up) #7170's Pi 5 profile, not reproduced here.
batch.ts, macOS arm64, single-threaded).The multi-thread argument — the cache is thread-local like the two structures
it summarises — is reasoned and structural, not exercised by a test.
classify_heap_generation(slot_addr)immediately above the mark wasdeliberately left alone. Hoisting the guard above it would remove a further
1.77M calls to the rank-1 symbol, but only under the extra premise "page P is
in the cache ⇒ a slot in P still classifies
Old", which a block freedmid-cycle can falsify. That is a missed-edge risk for a perf win, so it is
not in this PR; noted on gc/perf: classify_heap_generation is 19% of batch.ts (57.4% total GC bookkeeping) with ZERO collections running — write-barrier tower needs its own lever (#5094 evidence) #7187 as the obvious follow-up.
gc::tests::teardown::map_set_*are red on this branch — and verified redat the merge-base
cdc7dee87with the identical filter, so pre-existing.Summary by CodeRabbit
Performance
Diagnostics
Bug Fixes
Tests