perf(gc): direct-mapped dirty-page cache — sixteen ways instead of one - #9030
Conversation
The write barrier's dirty-page cache held ONE page, on the strength of a batch.ts simulation whose store pattern was long same-page runs. The ECS component-update rows falsified that shape: each entity's sweep stores into every component column in turn, so the store pages ALTERNATE and the single entry misses almost every time. The uncached path then pays a thread-local resolution (`_tlv_get_addr`) plus a hash-set insert per store — `mark_dirty_old_page_uncached` and that resolution together measured 35-40% of BOTH update rows' frames, the dominant remaining cost on each. The cache becomes sixteen direct-mapped ways in the same hot-TLS home, indexed by the page number's low bits — page numbers are `addr >> 12`, so neighbouring columns' pages land in distinct ways (the one access pattern low-bit indexing is exactly right for). Hits bypass the whole uncached path, so the thread-local and the insert stop executing rather than getting cheaper. The invariant is unchanged and per-way: a cached page is recorded in `DIRTY_OLD_PAGES` and stamped in the arena metadata; `invalidate` clears every way on the same removal paths as before. The PerryTS#8949 process-global mirror is retired rather than widened: it existed to shave the single cell's dependent-load chain, bought +0.17% then, and the multi-way map supersedes both its mechanism and its rationale. The Phase B test that pinned the one-entry eviction ("returning to the first page misses") now pins the stronger contract: both alternating pages stay cached, and the completeness property — the cache only ever suppresses a repeat recording, never a first one — is asserted unchanged. Claude-Session: https://claude.ai/code/session_01FUvFrRNZyc5qknBiJbYbby
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe dirty-page cache changes from one entry to a 16-way direct-mapped cache. ChangesDirty-page cache widening
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR expands the internal dirty-page cache so alternating component pages can avoid repeated remembered-set work while preserving existing GC recording and invalidation behavior. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides detailed context, rationale, implementation details, and test results, but it does not follow the required template. It omits the required section headings and does not provide a Related issue entry or checklist status.
✨ Finishing Touches🧪 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 |
|
Measured on the quiet host — 9 alternating pairs per row, control = merge base
The two-component row — where alternating column pages made the strongest mechanistic case — moved 0.3%. That is a null against a 35-40% attribution, and this PR set its own bar: closing rather than merging. What this establishesThis is now the fifth instance in this campaign of a profile self-time attribution on a runtime/GC leaf failing to convert to frame time (barrier-entry Two possible readings of this null, both actionable: either the one-entry cache was already hitting (the alternating-pages theory is wrong about the actual store order after command-buffer grouping — plausible, since component values here are numbers, whose stores skip the remembered set entirely at the child prologue), or the sampled time is attribution skid across the barrier's call boundary. Distinguishing them needs the diagnostics-feature rebuild for real hit/miss counters — which is what should have preceded this build, per the counters-first lesson already in this campaign's notes. The change itself is behaviorally sound (serial 2784/0, the strengthened Phase B contract passes), so if counters later show genuine thrash on some other workload, this branch is recoverable from the PR. Not merged on a null. |
Retiring the PerryTS#8949 process-global mirror left `darwin_tsd::base()` with no callers, so `-D warnings` failed the build on dead_code. Removed it, along with a doc paragraph that described the mirror and is now false. Its doc block had also absorbed `get()`'s: the two `///` runs were contiguous, so "Read thread-specific-data slot `slot`" plus its `# Safety` clause were attached to `base()` while `get()` -- the `unsafe fn` that clause is ABOUT -- carried none. Deleting `base()` would have taken that safety documentation with it, so it is moved back onto `get()`. Also adds the missing changelog.d fragment.
|
Merged. First, an apology and a correction: I briefly closed this PR. My initial push reported On the change itself — the property that matters is that the cache can only ever suppress a repeat recording, never a first one, and it survives the widening:
I sabotage-checked the second one specifically, since it is the whole safety argument: making The Also good that the Phase B test was strengthened rather than relaxed: Fixed on the branch
While doing that I found a pre-existing doc bug worth mentioning: Added the missing Validation: |
Inserting `pub(crate) use match_all::dispatch_regexp_string_iterator_method_builtin`
between the existing `#[cfg(feature = "regex-engine")]` and the `pub use` below
it moved the attribute onto the NEW line, leaving the original export ungated.
With the feature off, `perry-runtime` then names a module that does not exist:
error[E0432]: unresolved import `match_all`
It passes `cargo test -p perry-runtime --lib` (default features on) and fails
`cargo check -p perry`, which is why it was invisible to the crate-level run.
Same attribute-stealing shape as the doc comments repaired in #9013 and #9030 —
an inserted line silently inherits the attribute or doc block above it.
…top corrupting state (#9019) (#9066) * fix(runtime): reserve iterator raw-field floor so own next patches stop corrupting state (#9019) A by-name property write on a builtin collection iterator object derived its field index from the (empty) keys array, so the first user property landed at field 0 and overwrote the backing-collection pointer. it.foo = 1 made iteration report done immediately; it.next = fn made the next builtin advance dereference the closure as a SetHeader and SIGSEGV under for...of. Storage: the first by-name append to a reserved-layout receiver (array/ map/set/string/buffer/regexp iterators, iterator helpers) now seeds the keys array with floor leading tombstones (the #9038 hole marker every lookup/enumeration/delete path already skips), so user keys append past the raw internal fields; the hole-squeeze compaction preserves the reserved prefix. Dispatch: the class-id iterator dispatchers honor an own next before the builtin advance (non-callable own values throw per IteratorNext), while the canonical prototype thunks keep running the builtin algorithm so a patch delegating to its bound original cannot re-enter itself. The fused for...of arms validate the iterator result, and the stored-closure drain paths bind this to the iterator per Call(next, iterator). * docs: changelog fragment for #9066 * refactor(runtime): keep the reserved-floor seed out of the raw-handle ledger NaN-boxed handles in ensure_reserved_floor_keys and the existing refresh_roots_after_alloc macro (moved above the seed hook) in the by-name tail, so scripts/raw_handle_debt.py stays within its ceilings. * fix(runtime): close the defineProperty and entry-lane append surfaces for reserved floors (#9019) ensure_key_in_keys_array (the accessor-define keys claim) seeds the reserved floor before its keys-null create arm, and the entry-lane transition cache declines reserved-layout class ids so an unseeded iterator can never receive a foreign sub-floor slot from an edge minted by another keyless family sharing its birth ShapeId. * fix(runtime): restore the regex-engine cfg the new export took Inserting `pub(crate) use match_all::dispatch_regexp_string_iterator_method_builtin` between the existing `#[cfg(feature = "regex-engine")]` and the `pub use` below it moved the attribute onto the NEW line, leaving the original export ungated. With the feature off, `perry-runtime` then names a module that does not exist: error[E0432]: unresolved import `match_all` It passes `cargo test -p perry-runtime --lib` (default features on) and fails `cargo check -p perry`, which is why it was invisible to the crate-level run. Same attribute-stealing shape as the doc comments repaired in #9013 and #9030 — an inserted line silently inherits the attribute or doc block above it. --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
First profiling of the two ECS component-update rows (10k single: 2.35x node; 10k two-component: 3.47x node) found their dominant cost in one place: the write barrier's remembered-set miss path at 35-40% of both frames —
mark_dirty_old_page_uncached18-19% self plus_tlv_get_addr16-18% self (theDIRTY_OLD_PAGESthread-local resolution), plus the hash insert.Why the one-entry cache misses here
The cache held ONE page, justified by a
batch.tssimulation whose stores arrive in long same-page runs (the module doc's table). These rows falsify that shape: each entity's update sweep stores into every component column in turn, so the store pages alternate — the exact pattern a single entry can never hold. Every store then takes the uncached path.(The distinction from the round-4 'barrier family closed' result: that was the hit path on the command-buffer row, measured memory-bound three ways. This is the miss path on different rows, where the work is avoidable rather than shaveable.)
The change
Sixteen direct-mapped ways in the same hot-TLS home, indexed by the page number's low bits — page numbers are
addr >> 12, so neighbouring columns' pages land in distinct ways. Hits bypass the whole uncached path, so the thread-local resolution and the insert stop executing rather than getting cheaper.DIRTY_OLD_PAGESand stamped in the arena metadata;invalidateclears every way on the same removal paths as before, so the cache can still only suppress a repeat recording, never a first one.way_testscase pins the alternating-pages regression directly.Suites
Runtime serial 2784/0; dirty-page suite 22/22, barrier 74/74, tls_hot 10/10. Gate + paired measurements on both update rows (plus a migration-row guard against hit-path regressions) follow in comments; per the usual bar, if the rows do not move this gets closed rather than merged.
Claude-Session: https://claude.ai/code/session_01FUvFrRNZyc5qknBiJbYbby
Summary by CodeRabbit
Performance Improvements
Bug Fixes
Tests