-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(gc): re-arm the idle reclaimer on elapsed idle — a declined compaction currently parks the heap 221 MB high #9860
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| **A declined idle compaction is no longer a terminal state: the memory reducer | ||
| re-arms on elapsed idle as well as on mutator collections, so a heap that parks | ||
| 1.3 points under the compactor's residue gate gets revisited instead of holding | ||
| 221 MB until the next turn.** | ||
|
|
||
| Measured on the compiled claude-code TUI, one 400-char turn then a 120 s idle | ||
| window, quiet host (load < 0.1), both rounds of each arm: | ||
|
|
||
| | arm | after turn | after 120 s idle | | ||
| |---|---|---| | ||
| | A | 757 / 759 MB | **512 / 527 MB** | | ||
| | R | 738 / 742 MB | **748 / 748 MB** | | ||
|
|
||
| R *ends the turn 19 MB better than A* and finishes 221 MB worse. The reclaimer's | ||
| own diagnostic says why, and it is a closed loop: | ||
|
|
||
| 1. **The compactor's residue gate declines**, reproducibly and narrowly. | ||
| `compaction_owed` gate 1 wants residue ≥ 25 % of old-gen occupancy; A is at | ||
| **25.94 / 25.95 %** and starts two compactions, R is at **23.68 / 23.67 %** | ||
| and starts none. Within-arm spread across rounds is 0.01–0.02 points: a | ||
| stable operating point just under a threshold, not a coin-flip. | ||
| 2. **The decline removes the only event that could revisit it.** The reducer's | ||
| activity gate needs `2^backoff` collections *it did not start*, and | ||
| `external_collections()` subtracts only the reducer's own — so a **compaction | ||
| is what registers as external**. A's trace shows each one contributing | ||
| exactly +1 (`external_collections` 13 → 14 → 15 across three attempts, one | ||
| compaction between each). R stays at 9, `since_attempt` never reaches 1, and | ||
| there is no second attempt in the whole window. | ||
| 3. So the heap parks, and the largest piece of the loss is downstream of that: | ||
| A right-sizes the arena from **182.45 MB of capacity to 81.79 MB** across its | ||
| three observations, while R holds **168.82 MB** on one. Roughly 87 MB of | ||
| capacity + 57 MB of young blocks + 38 MB of old-gen ≈ 182 of the 221 MB. | ||
|
|
||
| **The fix extends an exemption that already exists twelve lines above it**, for | ||
| the identical deadlock: `StartReason::ArenaRightSize` bypasses the same gate | ||
| because arena blocks need a second full observation that an idle mutator will | ||
| never produce (#9709). This adds `StartReason::IdleElapsed` on the same | ||
| reasoning — a requirement denominated in *mutator collections* cannot be met by | ||
| a heap whose mutator is idle, which is precisely when the reducer is wanted. | ||
|
|
||
| **Why the gate constant was not the fix, on measurement rather than principle.** | ||
| Lowering `IDLE_COMPACT_MIN_RESIDUE_PCT` from 25 to 23 would have let R start a | ||
| compaction — and the same R binary in a 5 s window *did* clear the gate, at | ||
| 25.81 %, ran the compaction, and **released 0** (`kept_promise=false`, | ||
| `backoff_shift 0→1`). Nor is that peculiar to R: A's own second compaction | ||
| releases 0 at **54.6 %** residue. Half of A's compactions in this capture | ||
| released nothing, aborting ~4x earlier (`pause_us` 107k/161k against 442k) on | ||
| what looks like a budget. The knob is not merely forbidden; it does not work. | ||
|
|
||
| **Anti-spin needs no new rule.** The elapsed wait is | ||
| `IDLE_RECLAIM_REARM_MS << backoff_shift` — the *same* shift that prices the | ||
| activity arm — so an unproductive full doubles it: 15 s, 30 s, 60 s, 120 s, | ||
| 240 s. And the arm is **disarmed entirely at `IDLE_RECLAIM_MAX_BACKOFF_SHIFT`** | ||
| rather than merely slowed, because five unproductive attempts establish there is | ||
| nothing to give and an idle process must not pay a whole-heap mark forever. | ||
| A productive full resets the shift, so a heap still returning memory keeps being | ||
| asked every 15 s — which is the case this exists for. `IDLE_RECLAIM_REARM_MS` is | ||
| deliberately larger than `IDLE_RECLAIM_MIN_INTERVAL_MS` so the rate floor is | ||
| never the binding constraint and the two gates cannot be confused in a diag. | ||
|
|
||
| Two tests, each sabotage-proved: a parked heap with **no** external collection | ||
| anywhere gets a second attempt at the wait and not before, identified by reason | ||
| rather than by attempt count; and an unproductive streak doubles the wait each | ||
| time and then stops. Removing the arm fails the first, removing the backoff | ||
| scaling fails the second's "must not re-arm before the doubled wait", and | ||
| removing the disarm fails its "at the maximum shift the elapsed arm is | ||
| disarmed". | ||
|
|
||
| The young half of the loss is **not** addressed here and is measured, not | ||
| assumed: after R's single reclaim, `[gc-general-reclaim] examined=66 released=0 | ||
| has_live=39 aging=22` — 39 of 66 arena blocks hold a live object, against 3 of | ||
| 65 in A, and only an evacuation can consolidate those. Whether an idle young | ||
| evacuation is also needed is a separate question and a separate change. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| **The tiny-parse pressure guard now prices the collections it forces by the | ||
| adaptive step's productivity backoff (#9831).** On the compiled claude-code | ||
| TUI a 3300-character streamed reply spent 30–41 s of CPU in the base arm and | ||
| 27.8–29.2 s with the fix (mean −19 %, every interleaved pair a win), with | ||
| post-turn and post-idle RSS flat within the base's own spread and peak RSS | ||
| unchanged. | ||
|
|
||
| #9831 measured the `ArenaBytes` arm firing 51 times in one 66-delta reply, | ||
| each collection freeing a median 131 KB, while the adaptive step sat | ||
| saturated at 1 GiB — and located the discarded backoff in the arm's own | ||
| ceiling clamp. That clamp was not what re-fired the arm: between two firings | ||
| the arena grew a few hundred KB against a trigger armed 16–128 MB above the | ||
| post-collection total. What pulled the trigger down was the tiny-parse | ||
| pressure guard, which after every `JSON.parse` growing the arena by ≤ 1 MB | ||
| tested the absolute `arena_in_use_bytes() >= 48 MB` and, if it held, set the | ||
| trigger to "now". That is a quantity no collection can lower below the live | ||
| set, so on a heap that sits above it permanently every small parse (one per | ||
| SSE delta) forced a minor whose backoff nothing read — #9589's shape one | ||
| trigger over. | ||
|
|
||
| The guard now also requires the arena to have grown, since the last | ||
| collection of any kind ended, by a headroom priced from the step: the step | ||
| rescaled so its power-on value buys the 16 MB headroom floor and each | ||
| doubling the arm's clamp discards buys one more doubling, bounded by the | ||
| trigger ceiling. A productive collection keeps today's cadence; an | ||
| unproductive one earns room. The parse-boundary collector re-prices a | ||
| pending request so a collection that already satisfied it is not followed by | ||
| a second. `PERRY_GC_DIAG=1` gains a `[gc-tiny-parse] forced collection …` | ||
| witness line. The arm's own arithmetic is unchanged and now documents why | ||
| (pricing it directly was measured at −10.8 % CPU for +22 % footprint, the | ||
| issue's refuted branch). | ||
|
|
||
| Validation: `test_memory_json_churn.ts` (the guard's motivating shape) is | ||
| byte-identical in output and RSS in all four GC modes; 48/48 `test_gap_gc_*` | ||
| and 8/8 `test_gap_json_*` pass; nine new `gc::tests::tiny_parse_pressure` | ||
| tests pin the pricing and the predicate, sabotage-proved against both the | ||
| old absolute guard and a raw-step pricing. |
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the unproductive elapsed-retry schedule.
note_cycle_completedraisesbackoff_shiftbefore the elapsed arm evaluatesIDLE_RECLAIM_REARM_MS << backoff_shift. Therefore, the first elapsed retry after an unproductive full waits 30 s, followed by 60 s, 120 s, and 240 s. The 15 s wait applies only at shift 0, such as after a productive full. Update both cited entries; the changelog fragment is folded into GitHub Release notes.crates/perry-runtime/src/gc/idle_reclaim.rs#L143-L149changelog.d/9831-idle-reclaim-elapsed-rearm.md#L50-L55🤖 Prompt for AI Agents