fix(gc): make old-page defrag opt-in again until a workload can exercise it - #7922
Merged
Conversation
📝 WalkthroughWalkthroughOld-generation defragmentation is now disabled by default. Only ChangesOld-generation defragmentation opt-in
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🤖 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_defrag.rs`:
- Around line 109-130: Update OldDefragTestDisable and OldDefragTestEnable to
save the current OLD_DEFRAG_TEST_OVERRIDE value when constructed, then restore
that saved Option<bool> in Drop instead of clearing it to None. Preserve nested
guard state, including an outer enable or disable override.
🪄 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: da36a3ac-2cd8-4fd9-a22d-149e98b69d92
📒 Files selected for processing (2)
changelog.d/7922-old-defrag-default-off.mdcrates/perry-runtime/src/gc/oldgen_defrag.rs
Comment on lines
+109
to
+130
| /// 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)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore the previous test override on drop.
OldDefragTestDisable::new() overwrites an active outer override. Drop then always sets the override to None. If an OldDefragTestEnable guard encloses this guard, dropping the inner guard loses the enabled state and falls back to the environment value.
Store the prior Option<bool> in the guard and restore it on drop. Apply the same pattern to OldDefragTestEnable.
Proposed fix
-pub(crate) struct OldDefragTestDisable;
+pub(crate) struct OldDefragTestDisable {
+ previous: Option<bool>,
+}
impl OldDefragTestDisable {
pub(crate) fn new() -> Self {
- OLD_DEFRAG_TEST_OVERRIDE.with(|c| c.set(Some(false)));
- OldDefragTestDisable
+ let previous = OLD_DEFRAG_TEST_OVERRIDE.with(|c| c.replace(Some(false)));
+ Self { previous }
}
}
impl Drop for OldDefragTestDisable {
fn drop(&mut self) {
- OLD_DEFRAG_TEST_OVERRIDE.with(|c| c.set(None));
+ OLD_DEFRAG_TEST_OVERRIDE.with(|c| c.set(self.previous));
}
}🤖 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_defrag.rs` around lines 109 - 130, Update
OldDefragTestDisable and OldDefragTestEnable to save the current
OLD_DEFRAG_TEST_OVERRIDE value when constructed, then restore that saved
Option<bool> in Drop instead of clearing it to None. Preserve nested guard
state, including an outer enable or disable override.
proggeramlug
force-pushed
the
fix/7917-old-defrag-default-off
branch
from
August 12, 2026 06:45
cfedea8 to
b150cc6
Compare
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.
Summary
Makes old-page defragmentation opt-in again (
PERRY_GC_OLD_DEFRAG=1), reverting only the default flip from #7913. Every piece of #7913's correctness work stays — this is not a revert of that PR.Closes #7917.
Why only the default
#7913's contract work audits well and is worth keeping regardless of the default:
scripts/gc_root_dominance_allowlist.jsonis still empty, so the ~40 previously-exempted@perry_class_keys_*hits were genuinely fixed rather than re-suppressed.open_gapandunverifiedverdicts now fail outright.PARSE_KEY_RING(GC: enumerate the unrooted runtime-side caches — the population no static checker can see #7231) andDIAG_CHANNEL_BY_KEY, whose safety rested explicitly on "only old-gen defrag can move them", were fixed rather than exempted.becomes_real_whennamed this exact trigger.The default flip is the separable part, and it shipped without the evidence its own issue required. #7876's stated acceptance criteria:
No such corpus exists. Additionally, every GC gate (
gc-ratchet,gc-moving-witnesses,gc-root-dominance,gc-native-roots,gc-ptr-shape-off-witness) was stillqueued/pendingand had not executed when #7913 merged.The structural reason this cannot be validated yet
Selection requires
dead_bytes >= live_byteson an old page — promote-then-die at scale. Measured survival on the benchmark corpus:retainfamily survives at 999–1000‰, so its old pages are ~fully live anddead_bytes ≈ 0churnfamily runs at 0–4‰ survival and promotes almost nothing, so there is barely an old generation to fragmentshapesruns 1 cycle;asyncpiperuns 0No program in the 19-benchmark corpus can produce a candidate page. So shipping it on by default buys no benefit signal and no regression signal, while inheriting the full old-address rewrite surface. It is a footprint feature for long-lived servers with heterogeneous promoted objects — a workload class the suite does not contain.
The intended end state is one mode, not two: when a fragmentation workload exists and shows benefit, flip it on and delete the off branch (or delete the feature). Per CLAUDE.md, a mode that still exists is a decision that hasn't been made.
Changes
old_page_defrag_enabled_from_valuenow matches1/on/trueinstead of negating0/off/false, so an unrecognised value is OFF — a typo cannot silently enable old-generation relocation.OldDefragTestDisable, an RAII guard setting the thread-local override toSome(false). Before this the override was only everSome(true)orNone, so the OFF state had no deterministic hook at all.Test plan
old_page_defrag_is_opt_in_via_perry_gc_old_defrag— both arms plus the unrecognised-value case.disabled_defrag_short_circuits_before_taking_a_page_snapshot— the behavioural OFF assertion, which did not previously exist.select_old_page_defrag_pages_from_snapshotdoes not consult the knob (the gate is only inselect_old_page_defrag_pages), so every pre-existing selection test bypasses the switch entirely.if !old_page_defrag_enabled()early-out turns the new test RED and leaves the value-mapping test green — so it detects the gate's placement, not just its string mapping.cargo test -p perry-runtime --lib gc::oldgen_defrag— 2 passed.On the observable this test uses
It asserts the page-meta snapshot call counter, not the returned selection, and the positive control is load-bearing.
The first version of this test asserted only "disabled returns an empty selection". That is vacuous — a bare test process has no eligible old pages, so it passes identically against a kill switch that does nothing. It failed on its own positive-control assertion, which is how the problem was caught. Asserting the counter also pins that the short-circuit happens before the O(old pages) snapshot, so a disabled collector pays nothing per minor.
The counter is a
thread_local!, as is the override, so this is safe under parallel test execution.Update: the "cannot be validated" claim is now MEASURED, not inferred
The reasoning above argued from survival ratios that no corpus program can produce a candidate page. A separate GC workstream then measured it directly, with defrag at #7913's new default:
So old-page relocation never selects a page anywhere in the corpus. Two consequences:
selected_pages > 0before its verdict counts.Related and now merged: #7919 pins that old-page relocation expands a described-but-unexpanded promoted page run before moving anything (the #7914 interaction). Notably the defence has to sit at the reader, not at selection — a freshly described page has
dead_bytes == 0so it is never selected, but #7913 widens evacuation to whole source blocks, so a described page can still be dragged in by a selected neighbour.Summary by CodeRabbit
PERRY_GC_OLD_DEFRAG=1,on, ortrue.