diff --git a/changelog.d/7968-old-reclaim-baseline-credit.md b/changelog.d/7968-old-reclaim-baseline-credit.md new file mode 100644 index 0000000000..b31fbe64b1 --- /dev/null +++ b/changelog.d/7968-old-reclaim-baseline-credit.md @@ -0,0 +1,53 @@ +### Fixed + +- **GC: credit every promotion to the old-reclaim baseline, untraced or not (#7965).** + #7902 made a copying minor skip `credit_promoted_bytes_to_old_baseline` for an + untraced whole-block promotion, on the grounds that "live by construction" is a + marked-liveness claim `PromotionLiveness::AssumeAllLive` does not make. The + premise is right and the conclusion does not follow, because + `GC_LAST_OLD_RECLAIM_IN_USE_BYTES` 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 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. `old_in_use - baseline` then collapses into absolute + occupancy, and `gc_old_reclaim_growth_band_bytes`'s proportional half + (`baseline / OLD_RECLAIM_GROWTH_DIVISOR`) collapses with it, leaving the + constant floor — **a constant band pacing a collector whose per-cycle cost is + O(live)**, which is the quadratic shape #7592 removed at this trigger and #7594 + removed one generation down. The pin re-created it. + + Bisected on the **full count** over `8260a9e50..d78efca41`: `0809ada92 → + e886b56dd` (#7900 + #7901) is flat at ≤0.6% and both are exonerated; + `e886b56dd → 1bd5eeb6b` (#7902) carries the whole regression. The + granularity hypothesis originally attached to #7965 — that the absolute arm is + decided by promotion *step size* — is refuted here by `deeplist`'s + untraced-promotion trace being byte-identical across the boundary. + + Measured on the `gc-handoff` corpus, 19/19 byte-exact against the expected + output and exit 0 in both arms: `retain` **1 full → 0** and 8 237 M → 2 176 M + instructions retired (−73.6%) with peak RSS 311 → 249 MB; `retain_wide` + **1 full → 0** and 5 741 M → 2 859 M (−50.2%). No program gains a cycle or a + full, and the other 14 move by at most 0.15%. Against the pre-regression base + the arm lands *below* it (`retain` −23.4%, `retain1` −31.5%, `deeplist` + −17.5%), which is #7960's independently-reported figure and the arithmetic + check that the regression is gone rather than masked. + + #7902's three other changes are kept — the bounded, heap-scaled untraced + budget, the clamped `implied_dead_bytes` charge, and + `request_old_reclaim_for_untraced_promotions`. They close a real defect and + none of them paces on this quantity: each acts on *evidence about the + promoted cohort*, whereas a pinned pacing base acts on every program that + retains. + + Gated by `an_untraced_promotion_credits_the_old_reclaim_baseline`, which + drives a real untraced-promoting copying minor through the production entry + point and asserts the baseline advanced by exactly the bytes it relocated + (with the untraced counters asserted non-zero, so an evacuating or traced + cycle cannot satisfy it), then replays `retain`'s measured promotion series + and asserts both that the credited baseline keeps `old_reclaim_pressure_due` + false **and** that the same series against an uncredited baseline fires. That + second assertion caught its own first draft, which did not cross the band + while the `RETAINING` latch was armed. diff --git a/crates/perry-runtime/src/gc/copying.rs b/crates/perry-runtime/src/gc/copying.rs index ee5d4f3379..b9a708ac52 100644 --- a/crates/perry-runtime/src/gc/copying.rs +++ b/crates/perry-runtime/src/gc/copying.rs @@ -1854,20 +1854,17 @@ pub(super) fn run_copied_minor_attempt( collector.stats.copied_objects, collector.stats.promoted_objects, ); - // #7592: promoted bytes are live by construction — credit them to the - // old-reclaim baseline BEFORE the pressure check below, or the check reads - // the stale baseline and schedules a full that is guaranteed to free - // nothing (see `credit_promoted_bytes_to_old_baseline`). + // #7592: credit the bytes this minor moved into old-gen to the old-reclaim + // baseline BEFORE the pressure check below, or the check reads a stale + // baseline and schedules a full that is guaranteed to free nothing (see + // `credit_promoted_bytes_to_old_baseline`). // - // #7902: "live by construction" is a MARKED-liveness claim, and an untraced - // promotion makes none — it uses `PromotionLiveness::AssumeAllLive`. Those - // bytes are the uncertain class the untraced budget bounds, so crediting - // them here would tell old-reclaim pacing that a cohort nobody has looked - // at is clean, and defer the very collection that could decide it. Charge - // only what a traced cycle actually marked. - if !untraced { - credit_promoted_bytes_to_old_baseline(collector.stats.promoted_bytes); - } + // #7965: UNCONDITIONAL, including for an untraced promotion — see + // `credit_promoted_bytes_to_old_baseline`, which carries the argument. In + // one line: the baseline is the base of a GROWTH measurement, not a + // liveness claim, and withholding it pins that base at 0 on exactly the + // workloads that reach this path. + credit_promoted_bytes_to_old_baseline(collector.stats.promoted_bytes); // Everything outside from-space retains its pre-minor accounting. Remove // the from-space share of that accounting, then add back exactly the // objects that survived by copy or promotion. This also preserves objects diff --git a/crates/perry-runtime/src/gc/policy.rs b/crates/perry-runtime/src/gc/policy.rs index 92cd2888a1..fe2287d5c8 100644 --- a/crates/perry-runtime/src/gc/policy.rs +++ b/crates/perry-runtime/src/gc/policy.rs @@ -1577,6 +1577,39 @@ pub(super) fn copied_minor_promotion_handoff_due(trigger_kind: GcTriggerKind) -> /// quickly now wait for the growth band instead of the next threshold /// crossing. That is deliberate — a promoted-then-dead cohort big enough to /// matter moves the band by its own size. +/// +/// # ★ Every promotion is credited, including an UNTRACED one (#7965) +/// +/// #7902 made the call site skip this for a `PromotionLiveness::AssumeAllLive` +/// promotion, reasoning that "live by construction" is a marked-liveness claim +/// an untraced cycle does not make. The premise is right and the conclusion +/// does not follow, because **this 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 already seen. +/// +/// Withholding it does not defer a little reclamation, it degenerates the +/// predicate. A fully-live young generation promotes untraced on *every* +/// cycle, so on exactly the workloads that reach the untraced path nothing +/// else credits the baseline and it stays pinned at 0. Then +/// `old_in_use - baseline` collapses into `old_in_use` — absolute occupancy, +/// not growth — and [`gc_old_reclaim_growth_band_bytes`]'s proportional half +/// (`baseline / OLD_RECLAIM_GROWTH_DIVISOR`) collapses with it, leaving the +/// constant floor. **A constant band pacing a collector whose per-cycle cost is +/// O(live)** is the quadratic shape #7592 removed here and #7594 removed one +/// generation down. Measured on `retain` (#7965): 0 fulls → 1–2 fulls, +/// 2 841 M → 8 237 M instructions retired, +25% peak RSS, and the same on +/// `retain1` / `retain_wide` / `retain_wide1` / `deeplist`. +/// +/// The uncertain cohort #7902 is right to worry about — assumed-live bytes +/// parked in old-gen by a predictor that has since been contradicted — is +/// bounded by the three instruments #7902 itself added, none of which paces on +/// this quantity: `untraced_promotion_budget_bytes` forces a measuring cycle, +/// `implied_dead_bytes` charges that run against `PROMOTED_DEAD_BUDGET_BYTES`, +/// and [`request_old_reclaim_for_untraced_promotions`] schedules the reclaim +/// outright when the measurement contradicts the predictor. 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. pub(super) fn credit_promoted_bytes_to_old_baseline(promoted_bytes: usize) { if promoted_bytes == 0 { return; diff --git a/crates/perry-runtime/src/gc/tests/promote_in_place.rs b/crates/perry-runtime/src/gc/tests/promote_in_place.rs index 5deea2c82e..8aa1c81ba0 100644 --- a/crates/perry-runtime/src/gc/tests/promote_in_place.rs +++ b/crates/perry-runtime/src/gc/tests/promote_in_place.rs @@ -502,6 +502,112 @@ fn an_untraced_promotion_indexes_the_objects_it_could_not_prove_live() { } } +/// #7965: an UNTRACED promotion must credit the old-reclaim baseline. +/// +/// The baseline is the base of a GROWTH measurement, not a liveness claim, so +/// "an untraced cycle proved nothing" is not a reason to withhold it — see +/// `credit_promoted_bytes_to_old_baseline`. #7902 withheld it and pinned the +/// baseline at 0 on every fully-live workload, which cost `retain` two full +/// mark-sweeps and 2 841 M → 8 237 M instructions retired. +/// +/// Two halves, because either alone is a presence check: +/// +/// 1. the real collector, driven through the same entry point as production, +/// must move the baseline by exactly the bytes it moved into old-gen — and +/// the counters must show the UNTRACED path is what ran; +/// 2. the CONSEQUENCE, replayed on `retain`'s measured promotion schedule: the +/// credited baseline keeps `old_reclaim_pressure_due` false at every step, +/// and the same schedule against an uncredited baseline fires. Without that +/// second half a green run would not distinguish "the credit works" from +/// "this schedule never approached a trigger". +#[test] +fn an_untraced_promotion_credits_the_old_reclaim_baseline() { + { + let _guard = CopyingNurseryTestGuard::new(4); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + let _promote = InPlacePromotionTestGuard::untraced(); + + let child = young_leaf(); + js_shadow_slot_set(0, ptr_bits(child)); + + let before = GC_LAST_OLD_RECLAIM_IN_USE_BYTES.with(|b| b.get()); + let trace = collect_minor_trace(GcTriggerKind::Direct); + assert_copied_minor_trace(&trace, true, CopiedMinorFallbackReason::None, false); + + // Live subject: the UNTRACED promotion path ran and moved something. + // A cycle that evacuated, or promoted with a trace, would exercise the + // arm this test is not about. + assert!( + trace.copying_nursery.in_place_promotion + && untraced_promotion_cycles() > 0 + && trace.copying_nursery.promoted_bytes > 0, + "the untraced promotion must have run and promoted something \ + (in_place={}, untraced_cycles={}, promoted_bytes={}); declined because: {}", + trace.copying_nursery.in_place_promotion, + untraced_promotion_cycles(), + trace.copying_nursery.promoted_bytes, + crate::gc::copying::last_untraced_decline_reason() + ); + + let after = GC_LAST_OLD_RECLAIM_IN_USE_BYTES.with(|b| b.get()); + assert_eq!( + after.saturating_sub(before), + trace.copying_nursery.promoted_bytes, + "#7965: the baseline must advance by exactly the bytes this cycle \ + relocated into old-gen, whether or not it traced them" + ); + } + + let _iso = GcTestIsolationGuard::new(); + + // The mechanism as arithmetic, independent of which arm happens to fire + // and of the `GC_MAJOR_PACING_RETAINING` latch: a baseline pinned at zero + // collapses the band's proportional half, so an adaptive band degenerates + // into the constant floor however large the live old generation grows. + let floor_band = gc_old_reclaim_growth_band_bytes(0); + assert!( + gc_old_reclaim_growth_band_bytes(512 * 1024 * 1024) > floor_band, + "the proportional half is what a zero baseline collapses; without it \ + this test is not about the same quantity" + ); + + // `retain`'s measured promotion steps (#7965 trace, `PERRY_GC_DIAG=1`). + // Cycle 0 evacuates and every following cycle promotes the whole young + // generation untraced, so on a workload like this NOTHING else credits the + // baseline — which is why withholding the credit pins it at zero forever. + const RETAIN_UNTRACED_PROMOTION_BYTES: [usize; 4] = + [18_742_816, 26_213_656, 35_650_552, 37_747_640]; + + let previous = GC_LAST_OLD_RECLAIM_IN_USE_BYTES.with(|b| b.get()); + GC_LAST_OLD_RECLAIM_IN_USE_BYTES.with(|b| b.set(0)); + let mut old_in_use = 0usize; + let mut uncredited_fired = false; + for step in RETAIN_UNTRACED_PROMOTION_BYTES.iter().cycle().take(16) { + old_in_use += step; + credit_promoted_bytes_to_old_baseline(*step); + let baseline = GC_LAST_OLD_RECLAIM_IN_USE_BYTES.with(|b| b.get()); + assert!( + !old_reclaim_pressure_due(old_in_use, baseline), + "a full scheduled purely because promotion moved bytes into old-gen \ + is guaranteed to free nothing (old_in_use={old_in_use}, baseline={baseline})" + ); + // The same occupancy read against a baseline nobody credits — the + // #7902 state. + uncredited_fired |= old_reclaim_pressure_due(old_in_use, 0); + } + GC_LAST_OLD_RECLAIM_IN_USE_BYTES.with(|b| b.set(previous)); + + // The subject was genuinely at risk: a heap made ENTIRELY of objects a + // minor just relocated there does schedule a full once the baseline stops + // tracking it. Without this the loop above would not distinguish "the + // credit works" from "this schedule never approached a trigger". + assert!( + uncredited_fired, + "#7965: an uncredited baseline must make this schedule due — if it does \ + not, the assertions above prove nothing about the credit" + ); +} + #[test] fn a_low_survival_cycle_still_evacuates_and_moves_the_object() { // NOTE: `CopyingNurseryTestGuard::new` takes the copying-nursery isolation