perf(gc): defer old-object page registration off the promote path (from #7623) - #7624
Merged
Conversation
📝 WalkthroughWalkthroughOld-generation allocation now defers page registration into a bounded buffer. Flushes update page indexes before GC operations, metadata reads, traversal, and removals. Tests cover capacity limits, lifecycle boundaries, cleanup ordering, deduplication, and eager-registration equivalence. ChangesOld-page registration lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OldAllocator
participant PageMetadata
participant OldPageIndex
participant GCOrReader
OldAllocator->>PageMetadata: defer old-object page registration
PageMetadata->>PageMetadata: buffer registrations
GCOrReader->>PageMetadata: request GC operation or index data
PageMetadata->>PageMetadata: flush deferred registrations
PageMetadata->>OldPageIndex: update page objects and metadata
PageMetadata-->>GCOrReader: continue with complete index state
Possibly related issues
Possibly related PRs
🚥 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/perry-runtime/src/arena/tests.rs (2)
1386-1401: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the single-page assumption explicit.
The test walks only
page = generation_page_for_addr(headers[0])and then assertsvisitedequals all 64 headers. That holds only while 64 headers at a 64-byte stride fit in one page, which requiresGENERATION_PAGE_SIZE >= 4096. IfGENERATION_PAGE_SIZEis ever reduced, this test fails with a confusing element-mismatch diff rather than a clear cause.Add an assertion that states the requirement.
🧪 Proposed assertion
let headers = synthetic_old_headers(64); let page = generation_page_for_addr(headers[0]); + assert_eq!( + generation_page_for_addr(headers[63]), + page, + "this test assumes all 64 headers land on one page" + );🤖 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/arena/tests.rs` around lines 1386 - 1401, Add an explicit assertion in the test around the generation page setup, using the existing GENERATION_PAGE_SIZE constant, to require that 64 headers at a 64-byte stride fit within one page (at least 4096 bytes). Keep the existing single-page walk and visited-header assertions unchanged.
1245-1261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe source-text scan proves textual presence, not that the constructor calls the flush point.
The test reads three files and checks for the substring
old_pages_begin_gc_cycle(). A commented-out call, a call in an unrelated function in the same file, or a call behind a disabledcfgall satisfy the assertion. A rename or a move of the call into a shared helper produces a false failure.A behavioral test is stronger: construct each cycle through its real constructor with a pending registration in the buffer, then assert
deferred_old_page_registrations_len()is 0.cycle_start_flushes_deferred_registrationsalready shows this pattern forold_pages_begin_gc_cycledirectly.If the three constructors cannot be driven from a unit test, keep this scan and narrow it to the enclosing function using an AST match rather than a whole-file substring.
🤖 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/arena/tests.rs` around lines 1245 - 1261, Replace the whole-file substring assertions in every_cycle_constructor_routes_through_the_flush_point with behavioral coverage that invokes each real cycle constructor from mod.rs, cycle.rs, and policy.rs using a pending deferred old-page registration, then asserts deferred_old_page_registrations_len() is zero; reuse the setup pattern from cycle_start_flushes_deferred_registrations. If direct construction is not possible, retain the scan but restrict it to the relevant enclosing constructor via an AST-based match.crates/perry-runtime/src/arena/page_meta.rs (1)
757-779: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the page-overlap iteration shared with
old_object_page_overlaps.Lines 758-768 duplicate the overlap computation in
old_object_page_overlaps(lines 596-617) exactly. The duplication is deliberate; the inline form avoids aVecallocation per entry, which is the point of this change. The risk is divergence: a future fix to one copy silently changes page accounting in the other.An allocation-free iterator helper, used by both, keeps the performance property and removes the second copy.
🤖 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/arena/page_meta.rs` around lines 757 - 779, Extract the duplicated page-overlap calculation into an allocation-free iterator helper, then reuse it in both old_object_page_overlaps and the pending-object loop near the visible overlap logic. Preserve the current overlap boundaries, page traversal, and per-entry accounting without introducing a Vec allocation.
🤖 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.
Inline comments:
In `@crates/perry-runtime/src/arena/page_meta.rs`:
- Around line 753-756: Update the comment above run_page and run_base_len to
avoid claiming entries are strictly allocation-ordered or always share a page
consecutively. Document that entries may revisit an earlier page, and that
recapturing run_base_len from the current headers.len() only widens the
deduplication window safely because in-batch addresses are pairwise distinct as
established above.
---
Nitpick comments:
In `@crates/perry-runtime/src/arena/page_meta.rs`:
- Around line 757-779: Extract the duplicated page-overlap calculation into an
allocation-free iterator helper, then reuse it in both old_object_page_overlaps
and the pending-object loop near the visible overlap logic. Preserve the current
overlap boundaries, page traversal, and per-entry accounting without introducing
a Vec allocation.
In `@crates/perry-runtime/src/arena/tests.rs`:
- Around line 1386-1401: Add an explicit assertion in the test around the
generation page setup, using the existing GENERATION_PAGE_SIZE constant, to
require that 64 headers at a 64-byte stride fit within one page (at least 4096
bytes). Keep the existing single-page walk and visited-header assertions
unchanged.
- Around line 1245-1261: Replace the whole-file substring assertions in
every_cycle_constructor_routes_through_the_flush_point with behavioral coverage
that invokes each real cycle constructor from mod.rs, cycle.rs, and policy.rs
using a pending deferred old-page registration, then asserts
deferred_old_page_registrations_len() is zero; reuse the setup pattern from
cycle_start_flushes_deferred_registrations. If direct construction is not
possible, retain the scan but restrict it to the relevant enclosing constructor
via an AST-based match.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a82fb53e-3feb-4058-a549-317a24a6d52a
📒 Files selected for processing (4)
crates/perry-runtime/src/arena/allocators.rscrates/perry-runtime/src/arena/mod.rscrates/perry-runtime/src/arena/page_meta.rscrates/perry-runtime/src/arena/tests.rs
Comment on lines
+753
to
+756
| // Entries arrive in allocation order, so consecutive ones share a page; | ||
| // cache that page's pre-batch length across the run. | ||
| let mut run_page: Option<usize> = None; | ||
| let mut run_base_len: usize = 0; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Tighten the comment: entries are not strictly ordered by address.
The comment states entries arrive in allocation order so consecutive entries share a page. The hole-reuse path in arena_alloc_gc_old can return an address below a previously deferred bump allocation, so pending can revisit an earlier page.
The cache stays correct in that case. On a revisit, run_base_len is recaptured at the current headers.len(), which is greater than or equal to the pre-batch length, so the dedup window only widens and never misses a pre-batch entry. Widening is safe only because in-batch addresses are pairwise distinct, which lines 738-742 already establish.
State that reasoning here. The current wording presents strict ordering as a property, and a future change could rely on it.
📝 Proposed comment change
- // Entries arrive in allocation order, so consecutive ones share a page;
- // cache that page's pre-batch length across the run.
+ // Consecutive entries usually share a page, so cache that page's
+ // pre-batch list length across the run. Hole reuse can hand back a
+ // lower address, so a page CAN be revisited within one batch; the
+ // recaptured length is then >= the pre-batch length, which only widens
+ // the dedup window. That is safe because in-batch addresses are
+ // pairwise distinct (see the doc comment above).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Entries arrive in allocation order, so consecutive ones share a page; | |
| // cache that page's pre-batch length across the run. | |
| let mut run_page: Option<usize> = None; | |
| let mut run_base_len: usize = 0; | |
| // Consecutive entries usually share a page, so cache that page's | |
| // pre-batch list length across the run. Hole reuse can hand back a | |
| // lower address, so a page CAN be revisited within one batch; the | |
| // recaptured length is then >= the pre-batch length, which only widens | |
| // the dedup window. That is safe because in-batch addresses are | |
| // pairwise distinct (see the doc comment above). |
🤖 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/arena/page_meta.rs` around lines 753 - 756, Update
the comment above run_page and run_base_len to avoid claiming entries are
strictly allocation-ordered or always share a page consecutively. Document that
entries may revisit an earlier page, and that recapturing run_base_len from the
current headers.len() only widens the deduplication window safely because
in-batch addresses are pairwise distinct as established above.
Contributor
Author
Local verification (interim — pinned-host A/B still queued behind a public-baseline run)The pinned mini is busy with a The subject is live, and sized
1,657,956 objects — 113,226,480 bytes — promoted in a single copying minor, The collector's behaviour is unchangedThe GC census is byte-identical between arms after normalising only the Output hashes identical at 200k and 500k, and byte-identical stdout on all 11 Sabotage: 9 cases, 9 caughtEach flush site removed one at a time; the test that is supposed to catch it
The last row is the #7024/#6942 guard: if a later refactor makes this PR inert, GC zeal + from-space protection, both armsWorkloads recompiled with The instrument was live (110 protected retirements per arm) and the counts match. Gates
Loaded-host pre-check (NOT a citable number)Dev Mac at load ~29–90, so this is a directional signal only, best-of-N user Pinned-host wall/RSS/pause plus the official |
proggeramlug
pushed a commit
that referenced
this pull request
Aug 8, 2026
proggeramlug
pushed a commit
that referenced
this pull request
Aug 8, 2026
proggeramlug
pushed a commit
that referenced
this pull request
Aug 8, 2026
Contributor
Author
Pinned-host A/B — raw outputRun on json_pipeline + gc bench set + GC censusgc-ratchet (#7609 baseline), both arms — verdicts and every rss/wall row |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@changelog.d/7624-defer-old-page-registration.md`:
- Around line 149-160: Update the “+31 MB peak RSS at 500k” measurement in the
changelog to explicitly label it as an intermediate or historical result,
distinguishing it from the final 500k peak RSS value of 1,114.7 MB while
preserving the existing explanation of the allocation-heavy flush.
- Around line 125-130: Update the 200k GC census values in the changelog to
match the PR summary, or explicitly identify the separate runs that produced
each set of values; keep the reported object and byte counts internally
consistent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 034930e9-a560-4dce-9f32-de588f2ce42a
📒 Files selected for processing (2)
changelog.d/7624-defer-old-page-registration.mdcrates/perry-runtime/src/arena/page_meta.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/perry-runtime/src/arena/page_meta.rs
Comment on lines
+125
to
+130
| Output hashes identical at both sizes, and the **GC census is identical** at | ||
| both sizes — same cycles, same `promoted_objects`/`promoted_bytes`, same sweep | ||
| and reclaim. That is the check that this is bookkeeping and not a behaviour | ||
| change: 200k promotes 1,657,962 objects / 113,226,896 bytes and 500k promotes | ||
| 4,117,011 / 280,996,760, all through the path this PR touches, and none of it | ||
| moves. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files 'changelog.d/*.md' | rg '(^|/)7624-defer-old-page-registration\.md$|changelog\.d' | sed -n '1,80p'
printf '\nTarget changelog lines 100-140:\n'
cat -n changelog.d/7624-defer-old-page-registration.md | sed -n '100,145p'
printf '\nPR/summary mentions of these census values:\n'
rg -n '1,657,9(56|62)|113,226,(480|896)|promoted_objects|promoted_bytes|200k|500k' . -S | sed -n '1,220p'Repository: PerryTS/perry
Length of output: 50369
Keep the 200k GC census in the changelog consistent.
changelog.d/7624-defer-old-page-registration.md reports 1,657,962 objects / 113,226,896 bytes, while the PR summary used 1,657,956 objects / 113,226,480 bytes for the same 200k workload. Use one run or state which runs produced each value.
🤖 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 `@changelog.d/7624-defer-old-page-registration.md` around lines 125 - 130,
Update the 200k GC census values in the changelog to match the PR summary, or
explicitly identify the separate runs that produced each set of values; keep the
reported object and byte counts internally consistent.
Comment on lines
+149
to
+160
| ### What the measurement changed in the patch | ||
|
|
||
| Both are recorded because they are the reason the final numbers look the way | ||
| they do — and because one of them is a correction to a claim I made first. | ||
|
|
||
| 1. **+31 MB peak RSS at 500k** (1,110 → 1,142 MB, reproducibly, all 5 rounds). | ||
| Not the deferral — the *flush*: ~63 flushes per run, each `mem::take`ing the | ||
| pending buffer so the next burst re-grew a ~1 MB `Vec` from empty, plus a | ||
| second ~1 MB staging `Vec` for the page-meta updates, allocated and freed per | ||
| batch. The flush now holds both table borrows at once and applies the meta | ||
| update inline, and hands the pending buffer back to its thread-local. **A | ||
| flush allocates nothing.** |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching target:"
git ls-files 'changelog.d/7624-defer-old-page-registration.md' || true
echo
echo "Relevant sections:"
if [ -f changelog.d/7624-defer-old-page-registration.md ]; then
wc -l changelog.d/7624-defer-old-page-registration.md
sed -n '1,220p' changelog.d/7624-defer-old-page-registration.md | cat -n
fi
echo
echo "Search for RSS/value mentions:"
rg -n "RSS|peak RSS|1,110|1,114|1,142|flush|allocation|allocates" changelog.d/7624-defer-old-page-registration.md || trueRepository: PerryTS/perry
Length of output: 15061
Label the RSS measurement as historical.
1,114.7 MB is the final 500k peak RSS in the measured table, while 1,110 → 1,142 MB describes the allocation-heavy flush that was changed away from. Mark the +31 MB line as an intermediate/historical result so it does not conflict with the final numbers.
🤖 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 `@changelog.d/7624-defer-old-page-registration.md` around lines 149 - 160,
Update the “+31 MB peak RSS at 500k” measurement in the changelog to explicitly
label it as an intermediate or historical result, distinguishing it from the
final 500k peak RSS value of 1,114.7 MB while preserving the existing
explanation of the allocation-heavy flush.
Contributor
Author
Clean-host re-run — raw output (supersedes the earlier A/B)The earlier A/B ran while a second gate + post-run idleness checkA/B: json_pipeline + gc bench + census (5 interleaved rounds)gc-ratchet, both arms, clean host |
added 11 commits
August 8, 2026 11:53
`register_old_object_pages` pays two RefCell borrows, two Vec allocations, a hash lookup and a linear dedup scan of the page's object list per object — and the scan grows as the page fills, so a burst of old-gen births into one 4 KiB page is quadratic in the objects it lands there. Since #7613's promote-on-first-copy that path is hot on ordinary workloads: a copying minor promotes straight into old-gen via `arena_alloc_gc_old`, so json_pipeline pushes ~113 MB of promotions per run through it. `arena_alloc_gc_old` now records `(header_addr, total_size)` in a thread-local buffer and one batched flush folds the whole burst in, holding a single borrow of each table and scanning only the entries that predate the batch — zero dedup comparisons for the fresh pages a bump-allocated promotion burst actually fills. Allocation policy is unchanged: the `old_free_take_exact` hole probe stays, so this is bookkeeping only. Soundness rests on one rule: every reader AND every remover of OLD_GEN_PAGE_OBJECTS / OLD_GEN_PAGE_META flushes first. Removers matter as much as readers — a removal that runs while an entry is still deferred is a no-op, and the later flush would then resurrect a dead object. `arena_alloc_gc_old_excluding_pages` (old-page defrag relocation) stays eager: it is rare, its per-object cost is dominated by the memcpy beside it, and keeping it eager narrows the proof obligation. Origin: extracted from #7623, whose pretenure half was a measurement confound and is not merging. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
…h claim Three follow-ups to the deferral, all soundness/documentation: - `old_pages_reset_sweep_accounting` flushes. The per-object OLD_GEN_PAGE_META writers that follow it call `refresh_policy_bits`, which reads `allocated_bytes`; flushing at sweep entry means a page's policy bits are never recomputed from a count missing this cycle's promotions. - `OldArenaPageObjectCursor::next` debug-asserts the buffer is empty. `new` flushes, and the budgeted stepping window marks without allocating into old-gen — this pins that claim instead of paying a thread-local read per object to re-establish it. - Fixed a comment that named a test which does not exist. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
The per-obligation tests pin the flush sites that exist today; they are blind to one added later. `deferred_registration_flush_sites` closes that: both page tables are thread-locals private to `page_meta.rs`, so the toucher set is enumerable from source, and every toucher must either flush or carry a written argument for why the deferral cannot be observed there. A stale exemption fails too — a name that no longer touches either table must be deleted — so the list cannot rot into blanket suppression, and the gate asserts it found at least ten touchers so a parser regression cannot make it vacuously green. It is not a hypothetical gate: on its first run it caught `OldArenaPageObjectCursor::next`, which is deliberately flush-free (its `new` flushes and the stepping window cannot re-fill the buffer, which `next` debug-asserts). That is now an exemption with the argument attached rather than an undocumented gap. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Measured on the pinned mini, the first version of the batched flush cost **+31 MB peak RSS** on json_pipeline 500k — reproducibly, in all five interleaved rounds (1,110 MB → 1,142 MB). The cause was the flush itself, not the deferral: 4.1M promotions at a 64k cap is ~63 flushes, and each one `mem::take`d the pending buffer (so the next burst re-grew a ~1 MB `Vec` from empty) and staged its page-meta updates in a second ~1 MB `Vec` that was allocated and freed per batch. Both are now gone. The batch holds the `OLD_GEN_PAGE_OBJECTS` and `OLD_GEN_PAGE_META` borrows at once — distinct thread-local cells, so no aliasing — and applies each page's `allocated_bytes`/`object_count`/policy-bit update inline, which is exactly what `update_old_page_meta_for_object` did with the staging Vec. The pending buffer is cleared and handed back to its thread-local so the next burst refills something already 64k entries wide. A flush now allocates nothing. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
…4k -> 8k) The gc-ratchet's `pinned_host` profile priced the inherited 64k-entry cap: `11_collect_at_depth.rss_bytes` 34,652,160 -> 35,733,504, **+1,081,344 B — the 1 MB buffer, essentially exactly**. It was the ONLY regression row across all twelve probes on an arm whose GC counters were otherwise byte-identical to the baseline, and the base arm produced zero regression rows on the same host, so the attribution is unambiguous. Nothing wanted 64k. The cap exists to amortise the per-batch loop, and 8k does that ~8,000x; now that the flush is allocation-free the extra batches cost only the loop entry. 8k entries is 128 KB. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
…al buffer I cut the cap 64k -> 8k on the theory that `11_collect_at_depth.rss_bytes` (+3.1% on the `pinned_host` profile) WAS the 1 MB buffer. The re-measure disproves it: shrinking the buffer 8x moved the cell +16 KB in the WRONG direction (+1,081,344 B -> +1,097,728 B) when it should have shed ~0.9 MB. Two more facts point away from the deferral: that probe promotes ZERO objects, so this PR's path is inert on it, and the base arm produced zero regression rows on the same host in the same session, so it is not host drift. The remaining hypothesis, untested, is allocator segment granularity under a runtime ~10 KB larger. The constant's doc comment and the changelog fragment now say this outright rather than carrying the tidier claim I made first. The 8k cap stays because 128 KB beats 1 MB on its own terms, not because it fixed anything. `shared_ci` — the profile CI gates on — is OK on both arms. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
…ve the RSS row MEASUREMENT CORRECTION. The previous wall/user/RSS table was taken while a second `run_public_baseline` was executing on the mini. I had checked once, seen the first baseline's `SCRIPT REAL EXIT=1`, and treated "idle" as a durable property; a new run started 16 minutes later and overlapped the measurement. The re-run gates on all three of: no baseline process, a `SCRIPT REAL EXIT=` marker, and 1-min load < 2.0 — and re-checks all three afterwards (post-run: OK, load 1.97, no baseline started mid-run). Clean numbers are SMALLER than the contaminated ones, and much tighter: json 200k wall -3.9% (was -4.9%), 500k -3.7% (was -4.1%), all 20 paired deltas negative. Base 200k spread went from 7% to 0.7%. The contaminated table's 200k RSS "win" of -3.7% was noise; it is -0.5%. Deltas are now medians of PAIRED per-round deltas. `cycles` is bimodal in both arms, so median-of-medians reported +18.5% where the paired statistic is +0.0% — the interleaving exists precisely to support the paired read. RSS ROW RESOLVED. `11_collect_at_depth.rss_bytes` was flagged as an unexplained ~+1.07 MB with an untested allocator-granularity hypothesis. Measuring `origin/main` on the same idle host answers it: base reads 35,651,584 there (+2.88% over the pinned artifact, just under the band) vs fix's 35,749,888, so fix is +98 KB over base, not +1.07 MB. Base independently fails ten other `pinned_host` RSS cells, because the artifact is pinned at 0.5.1346 and we are at 0.5.1355. Both arms pass `shared_ci`, which is what CI gates. fix vs base across all 144 ratchet cells: 107 of 108 GC-semantic cells byte-identical (the exception is the de-gated, sample-dependent `12_large_live_set.heap_used_bytes`, differing by less than a quarter of its documented spread); memory median +0.23%; wall median +0.0%. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
proggeramlug
force-pushed
the
perf/old-page-registration-deferral
branch
from
August 8, 2026 09:58
f807ecb to
73e7726
Compare
Contributor
Author
Audit before merge — verified, merged as v0.5.1360Census reproduced to the byte on my own build: 3 cycles, 1,657,966 Sabotage: removing all nine flush sites at once turns three tests red — On the corrected numbers: −3.9% / −3.7% is a smaller win than the What makes this PR unusually good, for the record
One data point for #7630/#7633My census here reads 113,227,216 bytes / 1,657,966 objects promoted at 200k |
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.
Extracted from #7623 per its audit. That PR's static-pretenure half was a
measurement confound and is not merging; this is the finding buried inside it
that stands on its own — and unlike the pretenure half, it pays on current
main, with no new codegen and no new allocator policy.
The cost
register_old_object_pagesis written for the occasional old-gen birth it wasintroduced for. Per object it pays:
RefCellborrows (OLD_GEN_PAGE_OBJECTS, thenOLD_GEN_PAGE_META),Vecallocations (old_object_page_overlaps,added_pages),containsscan of that page's object list, which grows as thepage fills — quadratic in the objects a burst lands on one 4 KiB page.
Since #7613's promote-on-first-copy that is no longer an occasional path.
A copying minor promotes straight into old-gen (
gc/copying.rs'smove_young→
arena_alloc_gc_old), andPERRY_GC_DIAGputs a number on it — json_pipeline200k promotes 1,657,956 objects / 113,226,480 bytes in a single minor, every
one through that function. That is the same ~113 MB the #7623 audit measured as
the moved cohort, now attributed to the code path that bills for it.
The change
arena_alloc_gc_oldrecords(header_addr, total_size)in a thread-localbuffer; one batched flush folds the whole burst in. The flush holds a single
borrow of each table, allocates no per-object
Vec, and — the part thatremoves the quadratic term — scans only the portion of a page's object list
that predates the batch. A bump-allocated promotion burst fills fresh
pages, where that prefix is empty and the dedup scan disappears.
Skipping the in-batch entries is sound because they are pairwise distinct: each
comes from a live allocation, and an address cannot be handed out twice without
an intervening free, which cannot happen without a flush. Hole reuse — the
reason the dedup exists at all — hands back an address registered before the
batch, so it is still covered (
batched_flush_matches_eager_registrationpinsexactly that case).
Allocation policy is deliberately unchanged. #7623 also dropped the
old_free_take_exacthole probe on its pretenure allocator; that is a policychange with its own RSS consequences and it is not here. This PR is bookkeeping
only — which is why the GC census can be, and is, asserted byte-identical.
Soundness: one rule, and removers matter as much as readers
Readers alone would not be enough. A removal that runs while an entry is still
deferred is a no-op, and the later flush then puts the dead object back —
a resurrected index entry pointing into swept or recycled memory. That is the
failure mode
removing_a_deferred_object_does_not_resurrect_itexists for.Both tables are thread-locals private to
arena/page_meta.rs, so the toucherset is closed and the rule is checkable rather than remembered:
deferred_registration_flush_sitesenumerates every function in that file thattouches either table and requires it to flush or to carry a written argument
for why the deferral cannot be observed there. Stale exemptions fail too. It is
not hypothetical — on its first run it caught
OldArenaPageObjectCursor::next.Full per-site and per-caller tables are in the changelog fragment. Two points
worth stating here:
classify_heap_generation— every barrier remember-decision — reads theblock-level
PAGE_GENERATIONSmap, whichregister_old_block_pagespopulates when a block is created. It never consults the object index, so it
is unaffected. (Same conclusion the gc: pretenure-accumulator admission infrastructure (#7598, scope-reduced per audit) #7623 audit reached for its shape;
re-verified for this caller set.)
arena_alloc_gc_old_excluding_pages(old-page defrag relocation) stayseager. Deferring would be sound, but it is rare, its per-object cost is
dominated by the
copy_nonoverlappingbeside it, and keeping it eagernarrows the proof obligation to the one path that measurably needs it.
Evidence
Eight unit tests, one per obligation plus the source-level gate. All
sabotage-verified: a harness removes one flush site at a time and requires
the matching test to go red — 9 cases, 9 caught, including "revert the
promote path to eager registration", so a later refactor cannot silently make
this PR inert (the #7024/#6942 failure mode).
every_cycle_constructor_routes_through_the_flush_pointis the second half ofthe cycle-start claim: one test proves
old_pages_begin_gc_cycleflushes, thatone proves all three constructors still call it.
every_old_gen_birth_path_sets_tenuredstays green.The GC census is byte-identical between arms (same cycles, same
promoted_objects/promoted_bytes, same sweep and reclaim), output hashesmatch at 200k and 500k, and all 11 gc-bench workloads produce byte-identical
stdout. GC zeal + from-space protection pass on both arms with the instrument
provably live.
Measured — pinned quiet host (
perry-macos, M1 mini)Both arms
perry-dev, identical package set, one target dir each. Workloadscompiled on the dev Mac with
PERRY_NO_AUTO_OPTIMIZE=1and the prebuiltexecutables shipped to the mini, so nothing was rebuilt on the measurement
host. 5 rounds, base/fix interleaved within each round, every row hash-verified.
Idleness was gated, not assumed. The run waits for all three of: no
run_public_baselineprocess, aSCRIPT REAL EXIT=marker in/tmp/baseline_mini.log, and 1-min load < 2.0 — then settles 60 s, measures,and re-checks all three afterwards. Load recorded by the A/B itself at its
own start:
1.62 2.04 2.45.Deltas are medians of per-round paired deltas, which is the statistic the
interleaving exists to support; see the
cyclesnote below for whymedian-of-medians is not safe here.
json_pipeline
All 20 paired json deltas are negative — 200k wall −4.5/−3.9/−4.6/−3.9/−3.9%,
500k wall −3.9/−3.7/−3.4/−3.7/−3.9%. Output hashes identical at both sizes.
The clean host both shrank the effect and shrank the noise. Base 200k wall
now spans 1.53–1.54 s (0.7%) where under the concurrent baseline it spanned
1.63–1.75 s (7%). The honest win is smaller than the superseded table
claimed (−3.9%/−3.7% vs −4.9%/−4.1%), and that table's 200k RSS "win" (−3.7%)
was noise — it is −0.5% here.
gc bench set (
gc-handoff/bench)All eleven produce byte-identical stdout. The wins land where the mechanism
predicts —
retain/retain1/deeplistare the promote-heavy ones.GC census — identical, and load-independent
CENSUS 200k IDENTICAL,CENSUS 500k IDENTICAL: same cycle sequence, samepromoted_objects/promoted_bytes, same sweep and reclaim. 200k promotes1,657,962 objects / 113,226,896 bytes; 500k promotes 4,117,011 /
280,996,760 — all through the path this PR touches, none of it moving.
gc-ratchet (the #7609 baseline), both arms, clean host
Both arms measured back-to-back in the same session on the clean host,
measure --repeats 7, thencheckon both profiles. 144 cells per arm.origin/main)shared_ci(what CI gates on)pinned_hostRead the base column first. Pure
origin/mainfailspinned_hoston thishost with ten RSS rows of its own (
03_cross_gen_writes+3.83%,08_map_set_sidetables+4.20%,04_dead_after_deep_stack+3.72%, …). Thepinned artifact was captured at
main 26b9c9d59(0.5.1346) and we are at0.5.1355, so the profile's RSS bands no longer describe this host/version.
"fix fails
pinned_host" is therefore not a statement about this PR — theonly sound comparison is base vs fix in the same session, which is what follows.
fix vs base, all 144 cells:
12_large_live_set.heap_used_bytes(59,946,104 → 59,944,160, −1,944 B) — theone cell the harness explicitly de-gates by probe override because it is
conservative-stack-scan sample-dependent, with a documented spread of 9,072 B
over 36 runs. The difference is under a quarter of that spread. Every
copied_*,promoted_*,freed_bytes,minor_cycles,step_cyclesandheap_total_bytescell is identical.(largest:
07_array_grow_evacuate.peak_rss_bytes+1.46%).where the deferral has almost nothing to do; the promote-heavy work is
json_pipeline's.
And this retires the open question from the earlier revision. I had flagged
11_collect_at_depth.rss_bytesas an unexplained ~+1.07 MB, with "allocatorsegment granularity" as an untested hypothesis. Measuring base on the same
clean host answers it:
11_collect_at_depth.rss_bytesorigin/mainfix is +98,304 B (+0.28%) above base, not +1.07 MB. Base already sat at 96%
of the allowance, so the cell tips over on a rounding-scale difference. The row
is ~91% pre-existing drift in
origin/mainand ~9% this PR. No allocator-granularitystory is needed, and the one I floated should be disregarded.
Not in scope
gc-root-dominanceis N/A.Summary by CodeRabbit