perf(gc): seed promote-on-first-copy from a completed mark-sweep (#7598) - #7613
Conversation
The survival-rate lock in gc/tenuring.rs is one cycle late by construction: it keys on prev_copied, so a previous copying minor must already have filled the survivor space. On a one-burst workload the first copying minor therefore always pays the wasted Eden->survivor copy (json_pipeline 500k: 268 MB copied on cycle 3, the same 268 MB promoted on cycle 4). Every collection that reaches the mark-sweep path -- a full, or a non-copying minor fallback, the two blind spots of retune_after_scavenge -- already walks every Eden header and classifies it live or dead. That census answers the same question one collection earlier. When the surviving cohort alone exceeds the desired survivor occupancy AND >=90% of the classified Eden bytes were live, the existing PROMOTE_LOCK is engaged so the NEXT copying minor enters at S=1. Exit stays the existing influx signal, so no new oscillation path. Budgeted cycles (allocate-black marks every mid-cycle birth) and conservative-scan cycles (unsound, run-varying liveness) are excluded at the callsite. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
…refusals The promote-on-first-copy seed is a policy decision made from the sweep's Eden census, so a plausible-but-wrong census would fire the policy on the wrong workloads without ever crashing. One assertion per way it can be wrong: live counted, dead counted separately, and old-gen live counted in NEITHER -- the last is the sabotage target for the block_idx < resettable_general_n gate. The PERRY_GC_DIAG line prints the census AND the verdict on every mark-sweep, including refusals: a policy that silently declines is indistinguishable from one that never ran (#7024/#7025), and it is how the ratchet probes were shown to evaluate the rule and decline it rather than never reaching it. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe GC now records Eden live and dead bytes during sweeping and uses eligible completed-sweep results to seed the promote-on-first-copy lock. Tests cover census accounting, policy boundaries, exclusions, threshold timing, and unlock behavior. ChangesEden census seeding
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OldgenSweep
participant Cycle
participant Tenuring
OldgenSweep->>Cycle: Record Eden live/dead byte totals
Cycle->>Tenuring: Call seed_promote_lock_from_sweep
Tenuring->>Tenuring: Apply full_seed_promotes_on_first_copy
Tenuring-->>Cycle: Seed threshold 1 or retain current lock
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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
🤖 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/gc/oldgen.rs`:
- Around line 1463-1465: Update the Eden live-byte accounting in the relevant
keep_live_object call sites, including the block around the shown condition and
the corresponding site around process_forwarded_object, to pass true for pinned
and ordinary objects while excluding forwarding stubs from the census. Add a
regression test covering a retained general-block forwarding stub during a
non-copying minor collection and verify it does not inflate eden_live_bytes.
🪄 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: d3641b0b-091f-422e-a9bc-8b664529b1fa
📒 Files selected for processing (5)
crates/perry-runtime/src/gc/copying.rscrates/perry-runtime/src/gc/cycle.rscrates/perry-runtime/src/gc/oldgen.rscrates/perry-runtime/src/gc/tenuring.rscrates/perry-runtime/src/gc/tests/incremental_sweep_reclaim.rs
| if block_idx < self.resettable_general_n { | ||
| self.eden_live_bytes = self.eden_live_bytes.saturating_add((*header).size as u64); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exclude forwarding stubs from the Eden census.
process_forwarded_object calls keep_live_object for retained general-block stubs. A non-copying minor retains every forwarding stub. These bytes therefore increase eden_live_bytes.
An unretained forwarding stub bypasses reclaim_dead_object. Its bytes do not increase eden_dead_bytes.
This asymmetric census can report a high survival rate and seed PROMOTE_LOCK for stale forwarding metadata. Count only non-forwarded Eden objects in this policy census. Add a regression test for a retained general-block forwarding stub during a non-copying minor.
Proposed fix
unsafe fn keep_live_object(
&mut self,
header: *mut GcHeader,
block_idx: usize,
flags: u8,
age_bump_this: bool,
pinned: bool,
+ count_eden_live: bool,
) {
- if block_idx < self.resettable_general_n {
+ if count_eden_live && block_idx < self.resettable_general_n {
self.eden_live_bytes = self.eden_live_bytes.saturating_add((*header).size as u64);
}- self.keep_live_object(header, block_idx, flags, false, false);
+ self.keep_live_object(header, block_idx, flags, false, false, false);Pass true for the pinned and ordinary-object call sites.
Also applies to: 1529-1531
🤖 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/oldgen.rs` around lines 1463 - 1465, Update the
Eden live-byte accounting in the relevant keep_live_object call sites, including
the block around the shown condition and the corresponding site around
process_forwarded_object, to pass true for pinned and ordinary objects while
excluding forwarding stubs from the census. Add a regression test covering a
retained general-block forwarding stub during a non-copying minor collection and
verify it does not inflate eden_live_bytes.
Audit before merge — verified to the byte, merged as v0.5.1349The copy census reproduces exactly on my own build and my own earlier The count-drop deviation from the anti-vacuity signature is correctly The RSS surprise is the best part: −20/−21% where the design note expected Sabotage re-verified: seed made a no-op → exactly the 2 claimed tests red; This is also the first GC-pacing change properly gated since 2026-08-01 — Also noted for the record: the agent's process catch — a #7592 remains open for the |
The two PR audits established that the pretenure mechanism was correct but the target was wrong: json_pipeline's minor-moved cohort (~113 MB) is the runtime-allocated parse tree, not codegen-visible literals (~12 MB total, ~1 MB live at minor time), and the measured 108 MB -> 0 was a confound -- the base arm predated #7613's promote-on-first-copy seed, which on current main fires in both arms. Removed: the born-tenured allocator entry points and their keepalive anchors (an unused #[no_mangle] + #[used] pair is unused configuration per the kill-policy, and un-strippable bytes per the hello-size anchor class), the codegen consumers, and the deferred-page-registration fix (extracted separately on perf/old-page-registration-deferral, crediting this PR's finding). Kept: collect_pretenure_accumulator_locals with its refusal tests, and the explicit region_runs_once parameter on both fact-graph builders (module main/init true, function/method/closure false) with a graph-level test pinning both polarities -- the admission half a future dynamic-feedback pretenurer needs.
1 participant
Closes #7598. The promote-on-first-copy remainder of #7592.
The waste
On promote-heavy workloads every long-lived object is copied twice — Eden →
survivor by one copying minor, survivor → old by the next.
json_pipelineat500k, on
main:The same 268 MB, moved twice.
gc/tenuring.rsalready computes the rightcondition — the survival-rate lock — but it keys on
prev_copied, so it needs aprevious copying minor to have filled the survivor space. It engaged for cycle
4, which is exactly why the waste is confined to cycle 3.
Route chosen: P1 from the design note — seed the decision from a completed
non-copying collection
The other two were considered and rejected on this workload:
objects, so any
PretenureSizeThresholdlow enough to catch them catchesalmost everything, and its stated failure mode — a parse-and-discard loop over
large buffers promoting pure garbage — is real and not bounded by anything the
collector measures. It is a good idea whose threshold must come from a suite
sweep, not from this workload.
alternative: it buys precision, and the problem here is latency.
The obstacle is that
retune_after_scavengeis fed only by copying minors,so full mark-sweeps and non-copying minor fallbacks feed the loop nothing — and
once a workload escalates to those, the loop goes blind exactly when it is
needed. But every collection on that path already walks every Eden header and
classifies it live or dead. That census answers the same question one collection
earlier.
So: at the end of any such sweep, when
(
compute_target_survivals(...) == 1— the module's existing rule, onlyread from a different place), and
would filter nothing" proof, measured instead of inferred),
engage the existing
PROMOTE_LOCKso the next copying minor enters at S=1.Exit is deliberately not this signal — it stays the existing influx-based
unlock, which is already threshold-invariant and already tested, so the seed
cannot introduce an enter/exit oscillation of its own.
Why this signal is not a fixed point of the policy reading it
This issue has produced three self-referential signals already, so here is the
argument rather than the assumption.
The failures were all the same shape: a signal suppressed by the state it is
supposed to leave.
promoted_bytesis zero by construction at S=4, so "wasthe promotion rate high?" can never answer yes while S=4 holds. #7596's first
nursery cap gated from-space occupancy on a total that included from-space, so
it was a bound from-space could never cross. #7594's handoff scheduled a
non-moving full to relieve pressure only a moving cycle can relieve.
The Eden live/dead split has none of that structure, for a structural reason:
it is produced by the mark-sweep's own arena walk. Marks come from
reachability from roots; neither the mark phase nor the sweep walk reads
tenuring_survivals(). The threshold is consulted in exactly one place —copying.rs's per-object move — and this path does not run it. So the signal isnot merely observed to survive at S=4; it cannot be a function of S at all.
Measurably, at both endpoints:
Measured on this workload:
eden_live_bytes=279,964,968 eden_dead_bytes=896 live_pct=99 desired=1,048,576 seeds=true— taken while S was still 4.Determinism (#7432)
#7432 forbids re-deciding S while objects are being moved, because the
copied/promoted split would then depend on root traversal order. The seed is
written at the END of a completed sweep and read at the ENTRY of a later copying
minor (
CopyingNurseryCollector::newsnapshots it once), so every object in acycle still sees exactly one threshold.
Two exclusions at the callsite keep the input deterministic too, and both are
refusals rather than tuning:
and this walk reads MARKED as live, so a churn workload's births would read as
a ~100% live Eden. Same reason the age-bump is suppressed there.
whatever the stack happens to look like a pointer to, by an amount that varies
run to run (
benchmarks/gc_ratchet/README.mdmeasures 8.28 MB / 16% of oneprobe's reported retention). A liveness measurement taken under it is not
sound, and feeding it to a policy would make the gated copy/promote counters
non-deterministic.
No new env knob. The
PERRY_GC_DIAGline prints the census and the verdict onevery sweep, including refusals — a policy that silently declines is
indistinguishable from one that never ran.
Measurements
Copy census — the halving signature, per cycle
Semantic counters,
PERRY_GC_TRACE=1, output hash identical on every row.500k:
This is promotion, not cadence. Cycles 1–3 are unchanged in kind, trigger,
old_beforeandeden_live— cycle 3 receives the same 280,997,080 bytes tothe byte and merely sends them somewhere else. The cycle that disappears is
baseline cycle 4, whose entire content was the second copy: its own Eden influx
was 1.0 MB and it promoted the 268 MB cycle 3 had just parked in the survivor
space. Removing it is removing the waste, not lengthening the collection
interval.
Totals:
Bytes moved: 0.498× at 200k, 0.499× at 500k. Halved, as the design note's
signature requires.
On the anti-vacuity gate. The design note asks for
copied_objects > 0 && promoted_bytes > 0on any cited run. Themainarm satisfies both literally.The changed arm cannot satisfy the first clause because eliminating that copy is
the change; what the clause exists to catch — the #7024/#7025 shape where the
fast arm ran zero collections — is refuted directly: it ran 3 collections,
one of them a copying minor that moved 4,117,015 objects / 280,997,080 bytes
(
[gc-copy-minor] ran … eligible=true fallback=none). Reported asmoved_objects > 0rather thancopied_objects > 0.Wall and RSS — pinned quiet host
perry-macos(M1, 8 GB), load ~1.6, 5 interleaved reps per size, warmupdiscarded,
PERRY_NO_AUTO_OPTIMIZE=1, prebuilt static archives, outputbyte-identical (
cmp) on every row.Spread within each arm was ≤0.03 s on wall and ≤0.25 MB on RSS across the 5
reps.
RSS goes DOWN, which is the opposite of what the design note expected. The
note assumed promoting earlier raises the old-gen high-water mark. It does — but
it removes a larger term: with S=4 the 268 MB cohort exists twice at once at
the peak, as Eden from-space plus survivor to-space. Promoting on first copy
means the peak holds one copy. This is the first change in this campaign whose
wall-time win does not have to be traded against RSS.
gc-ratchet — the official
check, on the #7609 baselineThis is the first GC-pacing change since 2026-08-01 that could be gated properly,
so it was run as a two-arm check on the pinned host: once with a
main(
d4342fff3) build, once with this branch, back to back in the same session, bothbuilt with the same
-p perry -p perry-runtime-static -p perry-stdlib-staticset.The
mainarm establishes what the two commits of drift since the pin(
26b9c9d59) cost on their own, so anything the second arm moves is attributableto this change.
Diffing the two arms' 144-cell tables: every semantic cell is bit-identical.
heap_used_bytes,heap_total_bytes,minor_cycles,step_cycles,copied_objects,copied_bytes,promoted_objects,promoted_bytes,freed_bytes— all twelve probes, no difference at all. The only rows thatdiffer are
rss_bytes,peak_rss_bytesandwall_ms, which move in botharms and stay inside band. The largest is
12_large_live_set.peak_rss_bytes:+0.36% on the main arm, +0.68% on this one (190,955,520 → 191,561,728, a 606 KB
difference on a 190 MB probe, band 3%). It is not a promotion effect — that
probe's
promoted_bytesis identical to the byte in both arms.12_large_live_set.wall_ms— #7610's unexplained +13.58% flag, now gated:maind4342fff3This change does not move that cell (1 ms apart). Separately, and worth
recording on #7610: the +13.58% did not reproduce in this session at all — a
mainbuild measures 3,016 ms against the 3,471 ms the artifact pinned threecommits earlier on the same host, i.e. the regression reads as reversed. That
is a data point for #7610, not a claim by this PR.
The subject was live — and where
The ratchet result above proves no collateral; it does not prove the policy
ran, because on these probes it correctly does nothing. That distinction is the
#7024/#7025 shape, so it is settled by the diagnostic rather than assumed. The
seed prints its census and its verdict on every mark-sweep, including refusals:
So on the largest-live-set probe the rule is evaluated and declined — by the
survival-rate half, at 36% — and on the target workload it is evaluated and
accepted. A policy that silently declines is indistinguishable from one that
never ran; this is why the refusal branch prints.
Sabotage verification
Each mutation was applied to the shipped code and the named tests re-run. All
four turn red, and the tree is green again after restoring:
block_idx < resettable_general_ninkeep_live_object(old-gen bytes leak into the Eden census)full_sweep_eden_census_counts_only_nursery_blocksFAILEDseed_promote_lock_from_sweepa no-opsweep_seed_decides_before_the_first_copying_minor_snapshots_the_threshold,sweep_seed_hands_over_to_the_existing_unlock_pathFAILEDsweep_seed_refuses_a_churn_eden,sweep_seed_rule_is_a_pure_function_of_the_censusFAILEDsweep_seed_refuses_a_small_fully_live_eden,sweep_seed_rule_is_a_pure_function_of_the_censusFAILEDGates (local — CI backlog is deep, so this is the evidence)
cargo test -p perry-runtime --no-fail-fast1885 passed, 0 failed ·cargo fmt --all -- --checkclean ·check_file_size.shOK ·raw_handle_debt.py998 (baseline 998) ·addr_class_inventory.pypassed ·class_id_collisions.pypassed. No codegen change (the diff is four files, allunder
crates/perry-runtime/src/gc/, 314 insertions and no deletions), so theroot-dominance corpus is not implicated.
What this does not do
PERRY_GC_DIAGline uses the existing one, so thekill-policy has nothing new to carry (and gc: PERRY_GEN_GC_EVACUATE=0 moves zero cells on all 12 gc-ratchet probes — the knob's off-state is unexercised #7611 has nothing to add to).
retune_nursery_cap_scale; collection pacing stays exactly where perf(gc): break the survivor-promotion handoff livelock (#7592) #7594 andperf(gc): live-proportional collection budgets at both generations (#7592) #7596 left it.
"allocated in a loop and stored into an accumulator that outlives it" proof.
This removes the double copy for cohorts a mark-sweep can see; a workload whose
first collection is already a copying minor still pays one wasted copy, and
that is what the static route would cover.
Collateral check on GC-shaped workloads outside the ratchet
Six micro-workloads the ratchet does not contain, both arms, semantic counters
and stdout compared.
treeis the workload the survival-rate lock was writtenfor (medium-lived cohorts that do die in the survivor space — the failure mode
this rule must not trip on);
retainis the accumulator shape;churnandpush_clsare pure churn.treeretainchurncyclespush_clsdeeplistdeeplist's 528-byte / 3-object delta is stable across 3 runs per arm, so it isdeterministic rather than noise — and it is not a policy effect. It lands
entirely in cycle 1, a copying minor entered at S=4, while this workload's
first mark-sweep is cycle 3 — so no seed had been evaluated yet and the tenuring
state at cycle 1 is identical in both arms. It is the collector's documented
address-sensitive component in
copied_objects(gc_ratchet/README.md: "asub-0.1% host-dependent component") showing up between two differently-laid-out
binaries. For the record, the seed is reached later on this workload and
reports
live_pct=100 seeds=true already_locked=true— the copying path hadalready locked, so it changes nothing.
Reproducibility of the cited numbers
The change arm was rebuilt from the committed tree (
-p perry -p perry-runtime-static -p perry-stdlib-static, pinnedPERRY_RUNTIME_DIR,PERRY_NO_AUTO_OPTIMIZE=1) andjson_pipeline500k re-run against it: 3collections,
copied_bytes0,promoted_bytes280,997,080, outputbyte-identical to the
mainarm. The committed source reproduces the censusabove.
Summary by CodeRabbit
Performance
Bug Fixes
Documentation