fix(gc): credit every promotion to the old-reclaim baseline, untraced or not (#7965) - #7968
Merged
Merged
Conversation
… or not (#7965) #7902 made a copying minor skip `credit_promoted_bytes_to_old_baseline` for an untraced whole-block promotion, reasoning that "live by construction" is a marked-liveness claim `PromotionLiveness::AssumeAllLive` does not make. The premise is right; the conclusion does not follow, because the baseline is not a liveness claim. It is the base of a growth measurement: `old_in_use - baseline` is meant to read "how much has old-gen grown since the last reclaim decision", and bytes a minor has just relocated there are growth that decision has seen. A fully-live young generation promotes untraced on every cycle, so on exactly the workloads that reach this path nothing else credits the baseline and it stays pinned at 0. Then `old_in_use - baseline` collapses into absolute occupancy and `gc_old_reclaim_growth_band_bytes`'s proportional half (`baseline / 2`) collapses with it, leaving the constant floor — a constant band pacing a collector whose per-cycle cost is O(live), the quadratic shape #7592 removed here and #7594 removed one generation down. Measured on the gc-handoff corpus, 19/19 byte-exact + exit 0 in both arms: `retain` 1 full -> 0 and 8 237 M -> 2 176 M instructions retired, `retain_wide` 1 -> 0 and 5 741 M -> 2 859 M, peak RSS -62 MB and -7 MB. No program gains a cycle or a full; the other 14 move by at most 0.15%. #7902's other three changes are kept: they close a real defect and none of them paces on this quantity.
|
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 (4)
📝 WalkthroughWalkthroughChangesGC baseline credit
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: ✨ 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 |
This was referenced Aug 12, 2026
proggeramlug
marked this pull request as ready for review
August 12, 2026 14:26
proggeramlug
pushed a commit
that referenced
this pull request
Aug 12, 2026
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
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.
Closes #7965.
Bisect result — one commit, one line
A sibling agent bisected
8260a9e50..d78efca41on the full count and Iconfirmed the endpoint independently:
0809ada92 → e886b56dd(#7900 + #7901)e886b56dd → 1bd5eeb6b(#7902)retain+379%,deeplist+253%,retain1+188%,retain_wide+190%, RSS +34–39%cdb67021e(#7900 in, #7901/#7902 out) reproduces the pre-regression censusexactly —
retain5 cycles / 0 fulls / 2 841 M instr / 250.1 MB, matching8260a9e50's recorded figures byte for byte.★ The mechanism in the issue is refuted
#7965 proposed that
old_reclaim_pressure_due's absolute arm is decided bypromotion step size. That is a real property of that arm — it is why #7960
exempted it — but it is not what happened here, and the disproof is clean:
deeplist's untraced-promotion trace is byte-identical across the boundary.Step size did not change.
The actual cause
GC_LAST_OLD_RECLAIM_IN_USE_BYTESis not a liveness claim, it is the baseof a growth measurement:
old_in_use - baselineis meant to read "how much hasold-gen grown since the last reclaim decision", and bytes a minor has just
relocated there are growth that decision has already seen.
A fully-live young generation promotes untraced on every cycle, so on exactly
the workloads that reach this path nothing else credits the baseline and it
stays pinned at 0. Then
old_in_use - baselinecollapses intoold_in_use— absolute occupancy, notgrowth; and
gc_old_reclaim_growth_band_bytes's proportional half (baseline / 2)collapses with it, leaving the constant floor.
So this is the fifth instance of CLAUDE.md's rule, reached from underneath: not
"paced on a quantity the collection does not move" but its corollary — a
constant band pacing a collector whose per-cycle cost is O(live), the
quadratic shape #7592 removed here and #7594 removed one generation down. The
pin is what re-created it.
#7902's other three changes are kept. They close a real defect and none of
them paces on this quantity:
untraced_promotion_budget_bytesforces ameasuring cycle,
implied_dead_bytescharges that run againstPROMOTED_DEAD_BUDGET_BYTES, andrequest_old_reclaim_for_untraced_promotionsschedules the reclaim outright when the measurement contradicts the predictor
that admitted the run. Those act on evidence about the cohort; a pinned
pacing base acts on every program that retains, whether or not anything about it
is uncertain.
Census —
main@54412cdc1vs this branch19/19 byte-exact against the expected output + exit 0 in both arms.
Cycle counts, full counts, instructions retired and peak RSS are all
load-independent; wall time is deliberately not quoted (dev box under load).
No program gains a cycle or a full. Against the pre-regression base the fix
arm lands below it —
retain2 841 → 2 176 M (−23.4%),retain11 396 → 956(−31.5%),
retain_wide3 305 → 2 859 (−13.5%),retain_wide11 204 → 911(−24.3%),
deeplist1 165 → 961 (−17.5%) — which is exactly #7960's honest−14%…−31%. That arithmetic is the check that the regression is gone rather
than masked by something else.
Correctness
PERRY_GC_VERIFY_EVACUATION=1.PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800,with the instrument confirmed to have a live subject where it still has one
(
churn88 retired page-sets,cycles22).retainretires no from-space atall since perf(gc): let the first copying minor decide its promotion from its own trace (#7937) #7960, so that arm is vacuous there by construction.
cargo test -p perry-runtime --lib(RUST_TEST_THREADS=1): 2228 passed, 0failed.
cargo fmt --all -- --check,check_file_size.sh,gc_runtime_root_holders.pygreen.addr_class_inventory.pyreports onePRE-EXISTING hit in
object/tests.rs:646, not on this branch.The gate — on the full count's cause, not on wall time
an_untraced_promotion_credits_the_old_reclaim_baseline. Two halves, becauseeither alone is a presence check:
entry point and asserts the baseline advanced by exactly
trace.copying_nursery.promoted_bytes, within_place_promotionanduntraced_promotion_cycles()asserted non-zero so an evacuating or a tracedcycle cannot satisfy it;
retain's measured promotion series and assertsold_reclaim_pressure_duestays false at every step and that the sameseries against an uncredited baseline does fire.
Half 2's second assertion is the discriminating one, and it earned its place:
it failed on its own first draft, because a 4-step replay does not cross the
band while the
RETAININGlatch is armed. It is now latch-independent, and italso asserts that the band's proportional half is what a zero baseline
collapses — so a green run says the credit is load-bearing, not that nothing was
tried.
★ #7961 / #7929 re-measured on top of this fix
#7961 ("denominate the nursery constant band in objects") merged on top of
this regression, so it was measured against a baseline carrying the spurious
fulls. Re-measured with this fix in both arms (
nursery_cap_object_scale_permilleforced to 1000 in the off arm), same corpus, 19/19 byte-exact + exit 0 in both:
Two findings, and both are narrower than the warning that prompted the check:
representation shrink was measured — the denomination is worth ±0.2% and
zero cycle-count change once perf(gc): main regressed the retain cluster 2.2-4.8x — retain now runs 2 full collections where it ran none (suspect #7901/#7902) #7965 is fixed. fix(gc): denominate the nursery constant band in objects (#7929) #7961's reported
retain−63.1% /
retain1−10.9% were the spurious fulls moving, not the band.It is not the large regression it was predicted to become either.
cycles12 → 22 collections (+83%),iso_miss43 → 61 (+42%),interp31 → 44 (+42%). Those are cheap minors over a dying nursery so instructions
move only +0.8…+1.5%, but the collection count is a load-independent 40–80%
increase.
Deliberately not folded into this PR: #7965's fix does not depend on it, the
two are separable at the census level (every retain row is identical across the
#7929 arm), and re-tuning or reverting a merged band belongs to #7929's owner
with this table in hand. Posted to #7929/#7961.
Full working:
gc-handoff/REGRESS-NOTES.md.Summary by CodeRabbit
Bug Fixes
Documentation