gc: price the idle reclaim by swept bytes, not old-gen occupancy (#9589) - #9643
gc: price the idle reclaim by swept bytes, not old-gen occupancy (#9589)#9643proggeramlug wants to merge 1 commit into
Conversation
…ryTS#9589) The reducer scored a full productive only when `old_gen_in_use_bytes` dropped. That number is the sum of the live old blocks' bump offsets, and a non-moving sweep cannot lower it: dead objects go back to the old-gen free list and the block keeps its offset, so only a whole-block release moves it. The unit tests passed because their litter fills blocks that end up entirely dead; every real workload does not. Measured on the compiled claude-code TUI, in a session with 279 collections behind it: two reducer fulls freed 2.37 MB and 0.51 MB while occupancy stood at 93.6 MB with 50 MB already on the old-gen free list, both scored unproductive, and the activity requirement had doubled twice inside a minute of idle. The reducer was switching itself off on exactly the workload it was built for. Price the full by the freed-bytes count the cycle already reports. The occupancy delta stays in the counters and on the diag line as the whole-block-release signal it actually is, next to a new `reusable=` field: the free-list residue that only compaction can return to the OS, which a budgeted (non-moving) cycle cannot do. Claude-Session: https://claude.ai/code/session_011qE5TRRJFzqxN44K34AnG2
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughIdle reclaim now prices full GC cycles by bytes freed during sweeping instead of old-generation occupancy changes. Diagnostics report reusable free-list bytes. Tests cover productive and below-threshold cycles. ChangesIdle reclaim productivity
Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to Idle GC now recognizes productive non-moving sweeps based on bytes freed, preventing unnecessary reclaim backoff while retaining occupancy diagnostics. Threshold behavior is covered, with no current merge-readiness risk identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description provides a detailed summary, motivation, production evidence, implementation details, related issue reference, test results, and scope limitations. It does not use all template headings or include the checklist, but the required change information is substantially complete. Full details: Docstring CoverageExplanation Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (1 skipped: 1 unsupported.) ✨ 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 |
|
Landed via merge train #9650 (rebase-merge, authorship preserved). |
1 participant
The idle-time reclaim that landed for #9589 switches itself off on the workload it was built for. This is the pricing bug that does it, found by measuring the reducer on the compiled claude-code TUI.
What the subject showed
A real session, 279 collections behind it, both attempts landing in the idle stretch after a paste:
Two fulls, both scored unproductive, activity requirement doubled twice inside a minute of idle. An earlier 300 s run scored four fulls unproductive that had freed 8.7, 5.7, 8.7 and 11.7 MB.
Why
arena::old_gen_in_use_bytesis the sum of the live old blocks' bump offsets. A non-moving sweep hands dead objects to the old-gen free list; the block keeps its offset. Only a whole-block release moves that number, and 130 blocks stayed live through both cycles. So the reducer was pricing a mark-sweep by a meter a mark-sweep cannot move.The unit tests passed because their litter (
litter_old_gen_with_dead_promises) fills blocks that end up entirely dead, so offsets do drop there. The test could only pass — a shape the production workload never has.The change
Price the full by the freed-bytes count the cycle already reports to
note_cycle_completed. The occupancy delta stays in the counters and on the diag line as the whole-block-release signal it actually is, next to a newreusable=field — the old-gen free-list residue, 50 MB on that session, which only compaction can return to the OS.Two new tests price a completion against live occupancy, so no delta is available: one asserts a sweep that freed the bar is productive (this fails on the old pricing,
left: 0 right: 1), one asserts a byte under the bar still backs off.perry-runtime3053 passed / 0 failed with--test-threads=1;gc::tests::idle_reclaim11/11.What this does not fix
The 50 MB on that free list stays there. Returning it needs compaction, and the reducer's cycle cannot compact by construction:
evacuation_policy_allowed = !low_pause_non_moving,low_pause_non_moving = progress_kind.is_budgeted(), and old-page defrag only ever runs as part of a moving minor —GcCycleState::new_fulltakes no page selection at all. Idle-time compaction is a separate change that re-opens #7917 (defrag went opt-in pending a fragmentation stress corpus that still does not exist) and carries a pause at a moment the user can end with a keystroke. Filing it separately with this evidence.Fixes the reducer's half of #9589.
https://claude.ai/code/session_011qE5TRRJFzqxN44K34AnG2
Summary by CodeRabbit
Bug Fixes
Tests