diff --git a/changelog.d/7922-old-defrag-default-off.md b/changelog.d/7922-old-defrag-default-off.md new file mode 100644 index 0000000000..0ca9423af4 --- /dev/null +++ b/changelog.d/7922-old-defrag-default-off.md @@ -0,0 +1,13 @@ +### Fixed + +- **Old-page defragmentation is opt-in again** (`PERRY_GC_OLD_DEFRAG=1`). #7913's rewrite-contract work — the `PARSE_KEY_RING`/diagnostics/perf-hooks rewrite coverage, class-key locals as precise mutable roots, all-or-nothing source-block evacuation, and the tightened runtime-holder policy — **all stays**; only the default flip is reverted. + + #7876's own acceptance criteria required "a dependency-scale stress corpus clean" before re-enabling, and no such corpus exists. The structural reason it cannot exist yet: selection needs `dead_bytes >= live_bytes` on an old page, i.e. promote-then-die at scale, and no program in the 19-benchmark corpus can produce a candidate page — the `retain` family survives at 999–1000‰ (old pages ~fully live) and the `churn` family promotes almost nothing. Default-on therefore bought no benefit signal and no regression signal while inheriting the full old-address rewrite surface. Every GC gate was also still queued and unexecuted when it merged. + + An unrecognised value is now **OFF** rather than ON, so a typo cannot silently enable old-generation relocation. + + When a fragmentation workload exists that can exercise this, the losing arm gets deleted rather than left standing. + +- **The OFF state has behavioural coverage for the first time.** `select_old_page_defrag_pages_from_snapshot` does not consult the knob — the gate lives only in `select_old_page_defrag_pages` — so every pre-existing selection test bypassed the switch entirely, and the thread-local override was only ever set to `Some(true)`. Adds `OldDefragTestDisable` and a test asserting the disabled path short-circuits **before** the O(old pages) page-meta snapshot, pinning the gate's placement and not merely its effect. + + The test asserts the snapshot call counter rather than the returned selection, with a load-bearing positive control: its first version asserted only "disabled returns nothing", which is vacuous in a process with no eligible old pages and passes identically against a kill switch that does nothing. Sabotage-verified — deleting the early-out turns it red while the value-mapping test stays green. diff --git a/crates/perry-runtime/src/gc/oldgen_defrag.rs b/crates/perry-runtime/src/gc/oldgen_defrag.rs index 52efd2f96f..cefcc68164 100644 --- a/crates/perry-runtime/src/gc/oldgen_defrag.rs +++ b/crates/perry-runtime/src/gc/oldgen_defrag.rs @@ -106,8 +106,32 @@ impl Drop for OldDefragTestEnable { } } +/// RAII *disable* for defrag on this thread, so the OFF arm is exercised +/// deterministically rather than depending on the ambient environment. +/// +/// Without this the OFF state has no behavioural coverage at all: the value +/// mapping is unit-tested, but nothing asserts that a disabled collector +/// actually declines to select a page. That gap is what #7917 records. +#[cfg(test)] +pub(crate) struct OldDefragTestDisable; + +#[cfg(test)] +impl OldDefragTestDisable { + pub(crate) fn new() -> Self { + OLD_DEFRAG_TEST_OVERRIDE.with(|c| c.set(Some(false))); + OldDefragTestDisable + } +} + +#[cfg(test)] +impl Drop for OldDefragTestDisable { + fn drop(&mut self) { + OLD_DEFRAG_TEST_OVERRIDE.with(|c| c.set(None)); + } +} + fn old_page_defrag_enabled_from_value(value: Option<&str>) -> bool { - !matches!(value, Some("0") | Some("off") | Some("false")) + matches!(value, Some("1") | Some("on") | Some("true")) } fn old_page_defrag_enabled() -> bool { @@ -123,9 +147,23 @@ fn old_page_defrag_enabled() -> bool { } pub(super) fn select_old_page_defrag_pages(force: bool) -> OldPageDefragSelection { - // #7876 restored the mutable-root contract for old movable addresses and - // made defrag the production default. Keep an explicit kill switch for - // field diagnosis and rollback without shipping a second binary. + // #7876 restored the mutable-root contract for old movable addresses, and + // #7913 shipped that restoration with defrag ON by default. The contract + // work is sound and stays; the DEFAULT is what this reverts (#7917). + // + // #7876's own acceptance criteria said to "re-enable defrag only after the + // reproducer and a dependency-scale stress corpus are clean". No such + // corpus exists yet, and none of the 19 benchmark programs can produce a + // candidate page: selection needs `dead_bytes >= live_bytes` on an old + // page, which needs promote-then-die at scale. The retain family survives + // at 999-1000 permille and the churn family promotes almost nothing, so + // the suite yields neither a benefit signal nor a regression signal while + // still inheriting the full old-address rewrite surface. + // + // So this is opt-in until a fragmentation workload exists that can + // actually exercise it. When that lands, the losing arm gets DELETED + // rather than left standing -- per CLAUDE.md, a mode that still exists is + // a decision that has not been made. if !old_page_defrag_enabled() { return OldPageDefragSelection::default(); } @@ -135,17 +173,100 @@ pub(super) fn select_old_page_defrag_pages(force: bool) -> OldPageDefragSelectio #[cfg(test)] mod tests { - use super::old_page_defrag_enabled_from_value; + use super::{ + old_page_defrag_enabled_from_value, select_old_page_defrag_pages, OldDefragTestDisable, + OldDefragTestEnable, + }; #[test] - fn old_page_defrag_defaults_on_with_an_explicit_kill_switch() { - assert!(old_page_defrag_enabled_from_value(None)); + fn old_page_defrag_is_opt_in_via_perry_gc_old_defrag() { + // Unset means OFF: defrag is opt-in until a fragmentation workload + // exists that can demonstrate it (#7917). + assert!(!old_page_defrag_enabled_from_value(None)); assert!(old_page_defrag_enabled_from_value(Some("1"))); assert!(old_page_defrag_enabled_from_value(Some("on"))); assert!(old_page_defrag_enabled_from_value(Some("true"))); assert!(!old_page_defrag_enabled_from_value(Some("0"))); assert!(!old_page_defrag_enabled_from_value(Some("off"))); assert!(!old_page_defrag_enabled_from_value(Some("false"))); - assert!(old_page_defrag_enabled_from_value(Some("unexpected"))); + // Anything unrecognised is OFF, so a typo cannot silently enable + // old-generation relocation. + assert!(!old_page_defrag_enabled_from_value(Some("unexpected"))); + } + + /// The OFF arm, asserted through the gated entry point rather than through + /// the value mapping. + /// + /// This matters because `select_old_page_defrag_pages_from_snapshot` does + /// NOT consult the knob — the gate lives only in + /// `select_old_page_defrag_pages` — so every pre-existing selection test + /// bypasses the switch entirely. Before this test the OFF state had no + /// behavioural coverage at all (#7917). + /// + /// The observable is the page-meta snapshot counter rather than the + /// returned selection, for two reasons. It needs no old-arena fixture, and + /// more importantly an empty selection is **not** evidence on its own: a + /// bare test process has no eligible old pages, so asserting only + /// "disabled returns nothing" passes just as happily against a kill switch + /// that does nothing at all. That is the gate-that-cannot-fail shape this + /// codebase keeps re-learning, and the first version of this test walked + /// straight into it. + /// + /// So the positive control is load-bearing: it proves the enabled path + /// really does reach the snapshot, which is the thing the disabled path + /// must then be shown to skip. + /// + /// It also pins the *placement* of the gate, not merely its effect: the + /// short-circuit must happen before the O(old pages) snapshot, so a + /// disabled collector pays nothing on every ordinary minor. + #[test] + fn disabled_defrag_short_circuits_before_taking_a_page_snapshot() { + use crate::arena::old_page_meta_snapshot_calls_for_tests as snapshot_calls; + + let before_enabled = snapshot_calls(); + let enabled = { + let _enable = OldDefragTestEnable::new(); + select_old_page_defrag_pages(true) + }; + let enabled_calls = snapshot_calls() - before_enabled; + + let before_disabled = snapshot_calls(); + let disabled_forced = { + let _disable = OldDefragTestDisable::new(); + select_old_page_defrag_pages(true) + }; + let disabled_unforced = { + let _disable = OldDefragTestDisable::new(); + select_old_page_defrag_pages(false) + }; + let disabled_calls = snapshot_calls() - before_disabled; + + assert_eq!( + enabled_calls, 1, + "positive control: enabled defrag must reach the page snapshot. If \ + this is 0 the assertions below prove nothing, because a switch \ + that never runs looks identical to one that correctly declines" + ); + + assert_eq!( + disabled_calls, 0, + "the kill switch must short-circuit BEFORE the O(old pages) \ + snapshot, so a disabled collector pays nothing per minor" + ); + + // `force` bypasses the dead>=live ratio, so this also proves the gate + // beats a forced selection rather than merely losing the ratio test. + assert_eq!(disabled_forced.selected_pages, 0); + assert_eq!(disabled_forced.candidate_pages, 0); + assert!(disabled_forced.pages.is_empty()); + assert!(disabled_forced.page_order.is_empty()); + assert_eq!(disabled_forced.selected_live_bytes, 0); + assert_eq!(disabled_forced.selected_reclaimable_bytes, 0); + assert_eq!(disabled_unforced.selected_pages, 0); + assert!(disabled_unforced.pages.is_empty()); + + // Sanity: the enabled arm returned a real (possibly empty) selection + // rather than the disabled default, i.e. the two paths are distinct. + let _ = enabled; } }