perf(gc): stop re-marking a heap where nothing dies — yield-adaptive major pacing + the evacuation move-hook mutex - #7733
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds yield-adaptive backoff for unproductive escalated full collections, exposes major-pacing values in GC telemetry, adds pacing tests, skips prototype-registry work when the registry is empty, and updates the package version. ChangesMajor-GC pacing
Prototype registry guard
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GCTrigger
participant GCPacingPolicy
participant FullCollector
participant GCTelemetry
GCTrigger->>GCPacingPolicy: evaluate escalation threshold
GCPacingPolicy->>GCPacingPolicy: record pre-full arena usage
GCPacingPolicy->>FullCollector: start escalated full collection
FullCollector->>GCPacingPolicy: report reclaimed bytes
GCPacingPolicy->>GCTelemetry: provide major_pacing values
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
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/policy.rs`:
- Around line 1572-1577: The major_pacing_snapshot boundary omits the configured
floor, causing escalate_above_bytes to report zero or too-low values when
escalation is still floor-gated. Update major_pacing_snapshot to include
floor_bytes in the effective threshold, or expose floor_bytes separately and
adjust the telemetry contract; add coverage for a nonzero floor with zero and
low baselines, preserving existing behavior for baselines above the floor.
🪄 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: 1156f50d-d65c-4001-aa4a-c7f0041a3e9c
📒 Files selected for processing (5)
changelog.d/7733-retain-live-set-major-pacing.mdcrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/telemetry.rscrates/perry-runtime/src/gc/tests/triggers.rscrates/perry-runtime/src/object/prototype_chain.rs
| pub(super) fn major_pacing_snapshot() -> (usize, u32, usize) { | ||
| let (_floor, growth_num) = major_pacing_config(); | ||
| let baseline = GC_LAST_FULL_ARENA_IN_USE_BYTES.with(|bytes| bytes.get()); | ||
| let shift = major_pacing_backoff_shift(); | ||
| let threshold = baseline.saturating_mul(growth_num.saturating_mul(1usize << shift)); | ||
| (baseline, shift, threshold) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include the floor in the reported escalation boundary.
major_pacing_snapshot reports only baseline × growth. The predicate also rejects values below floor_bytes at Lines 2582-2587. When baseline == 0, this snapshot reports 0, although escalation cannot occur until the arena reaches the configured floor.
telemetry.rs exports this value as major_pacing.escalate_above_bytes. Return the effective boundary, or emit floor_bytes separately and update the field contract. Add coverage for a nonzero floor with both a zero and a low baseline.
🤖 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/policy.rs` around lines 1572 - 1577, The
major_pacing_snapshot boundary omits the configured floor, causing
escalate_above_bytes to report zero or too-low values when escalation is still
floor-gated. Update major_pacing_snapshot to include floor_bytes in the
effective threshold, or expose floor_bytes separately and adjust the telemetry
contract; add coverage for a nonzero floor with zero and low baselines,
preserving existing behavior for baselines above the floor.
08c7f07 to
89675c0
Compare
Merging as v0.5.1427Two independent changes, and I checked the one that could hurt in production rather than in a benchmark. The unbounded-growth question, verified in codeDelaying a full GC is how RSS runs away, so the backoff needed three properties and has all three:
Scoping it to escalated fulls is also right: an explicit The diagnosis corrects a doc that was confidently wrong
Pricing a full by what it reclaims, measured on the same metric the escalation gate reads, is what stops the two from disagreeing about whether a full helped. That detail is easy to get wrong and would produce oscillation. The move-hook mutex
Worth watching, given this repo's history: #7510 found that one immortal side-table entry nullified every Gates 21/21. |
…7737) (#7740) * fix(gc): release the prototype-registry latch when a prune drains it (#7737) The latch was one-way, so a single Object.setPrototypeOf anywhere in a process permanently disabled #7733's per-evacuated-object fast path. The set moves under the mutex so the clear cannot race an in-flight insert. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix * chore: bump version to 0.5.1432 Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix * style: cargo fmt Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
…7733 follow-up) `major_pacing_snapshot` recomputed the escalation boundary as `baseline x growth` and discarded the floor (`let (_floor, growth_num) = ...`), while `arena_growth_full_escalation_due` also rejects every reading below that floor. Wherever the floor dominated the two disagreed -- most starkly before the first full, where the trace reported `0` ("escalates at any size") for a collector that escalates at 32 MB. That snapshot exists precisely so the pacing subject can be asserted live in the GC trace, so a probe that misreports its own subject is worse than none. There is now one definition of the boundary (`major_pacing_escalation_threshold_bytes`): the predicate is literally `in_use >= it`, and the snapshot reports it verbatim, floor included. The trace key follows the semantics -- `escalate_at_or_above_bytes`, `null` when `PERRY_GC_MAJOR_PACING_FLOOR_MB=0` disables pacing outright. Also: the ZealGuard test asserted the arm was taken and only narrated that it was released, so a Drop that stopped releasing it would have left every later test in the binary on the poll's slow path with the test still green. Claude-Session: https://claude.ai/code/session_015JgLM9UWGa6WAMix7CvhQJ
… the ZealGuard release becomes an assertion (#7729, #7733, #7735 review follow-ups) (#7739) * fix(gc): the pacing snapshot reports the boundary the predicate uses (#7733 follow-up) `major_pacing_snapshot` recomputed the escalation boundary as `baseline x growth` and discarded the floor (`let (_floor, growth_num) = ...`), while `arena_growth_full_escalation_due` also rejects every reading below that floor. Wherever the floor dominated the two disagreed -- most starkly before the first full, where the trace reported `0` ("escalates at any size") for a collector that escalates at 32 MB. That snapshot exists precisely so the pacing subject can be asserted live in the GC trace, so a probe that misreports its own subject is worse than none. There is now one definition of the boundary (`major_pacing_escalation_threshold_bytes`): the predicate is literally `in_use >= it`, and the snapshot reports it verbatim, floor included. The trace key follows the semantics -- `escalate_at_or_above_bytes`, `null` when `PERRY_GC_MAJOR_PACING_FLOOR_MB=0` disables pacing outright. Also: the ZealGuard test asserted the arm was taken and only narrated that it was released, so a Drop that stopped releasing it would have left every later test in the binary on the poll's slow path with the test still green. Claude-Session: https://claude.ai/code/session_015JgLM9UWGa6WAMix7CvhQJ * docs(gc): changelog fragment for #7739; keep the snapshot testable without diagnostics Claude-Session: https://claude.ai/code/session_015JgLM9UWGa6WAMix7CvhQJ * chore: bump version to 0.5.1433 Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
…terwards (retain 3.99x -> 2.54x node, retain_wide 7.01x -> 2.89x) (#7799) * perf(gc): a heap whose young generation is not dying no longer schedules futile full mark-sweeps retain.ts 0.542 -> 0.345 s, retain_wide.ts 1.099 -> 0.454 s, deeplist.ts 0.245 -> 0.123 s, with peak RSS DOWN on all three. Quiet M1 mini, best-of-5. These programs retain every record they allocate, so nothing is ever garbage, yet 79% of retain and 88% of retain_wide was GC pause -- dominated by full mark-sweeps that found the heap fully live (retain 161 ms for 11.9%, retain_wide 98 + 512 ms for 6.8% and 9.6%, deeplist 127 ms for 0.0%; against tree/tree_wide's 40 fulls each at 87.8%/92.3%, which this leaves untouched). The escalation rule was "run a full once the arena grows past 2x the last full's live set". That is right for a heap accumulating garbage and wrong for one that is not: when everything allocated stays alive, doubling is the program working. #7726/#7733's retrospective backoff cannot repair it -- it prices a full after paying for it, and on a monotonically growing live heap deferring a full only makes the next one bigger. The futile full has to be predicted. The prediction is a measurement the collector already takes. young_survival_permille separates the populations by two orders of magnitude with nothing between: churn/churn_alloc/push_cls 0-4, cycles 0, shapes 713-920, retain/retain_wide/deeplist 999-1000. A copying minor at or above 900 permille marks the heap RETAINING, which widens the escalation growth band 4x and re-baselines arena-growth pacing on the occupancy that survived -- the latter is what makes the former reachable, since before the first full the baseline is 0 and the boundary degenerates to the absolute floor, so ANY program retaining more than 32 MB paid a whole-heap mark-sweep for doing so. The same signal and multiplier apply to old_reclaim_pressure_due's growth band. credit_promoted_bytes_to_old_baseline (#7592) already exempts old-gen growth a minor proved live, but a large object is allocated straight into old-gen and never passes through promotion, so its bytes are uncredited growth even when they are the program's live data -- on retain.ts, the element array itself. With the arena-growth escalation correctly declining, that band became the binding constraint and fired a 452 ms full reclaiming 7.6%. Two bounding properties, both asserted by tests: the baseline only ratchets up and the multiplier is >= 1, so the boundary is never lower than before and this can only make fulls rarer, never more frequent; and one non-retaining minor disarms the band with no decay window. `retaining` is emitted in the major_pacing GC trace so a run that never armed it is distinguishable from one that did and had nothing to skip. Two independent wins found while profiling: * An all-pointer array's dirty-card scan was O(live array), not O(dirty pages). scan_dirty_object_slots's Slot arm answers "is this slot dirty?" with a hash-set probe per slot; its Range arm intersects with the dirty-page set directly. LayoutSlotMask::AllPointers reported itself as Masked and so emitted one Slot per element -- 3M probes per minor to find a few hundred known-dirty pages. dirty_slot_ranges_scanned == 0 in every retain.ts trace was recording exactly that. Worth 9% on retain before any pacing change. dirty_slot_ranges_for now also walks whichever of the two sets is smaller. * classify_heap_space_in_range is split into an inline(always) cache-hit arm and an inline(never) miss arm, as #7469 did for classify_heap_generation and for the same reason; and classify_arena no longer reads both survivor-space thread-locals (two _tlv_get_addr calls on Darwin) before a match whose common arms cannot use them. Refuted and not shipped: batching the per-slot old_page_account_dirty_slot map probe into one update per 4 KB page measured as exactly zero (0.344 vs 0.345 s). * docs(changelog): add the 7799 fragment * fix(gc): scope the retaining band to the full-collection decision, not survivor placement gc-ratchet 11_collect_at_depth turned 6,150 promoted objects into 6,139 copied ones: copied_minor_promotion_handoff_pressure_due shares old_reclaim_pressure_due with the OldReclaim escalation, so widening the shared band also stopped the survivor-promotion handoff from firing on a retaining heap. Placement and collection are different questions and only the second one was paying for a futile full, so the multiplier moves to old_reclaim_full_due and the shared band goes back to what it was. Pinned by a test that asserts both directions from one reading. * Revert "fix(gc): scope the retaining band to the full-collection decision, not survivor placement" This reverts 882be57. Both of its justifications were refuted by measurement. The premise was that widening the shared band flipped gc-ratchet's `11_collect_at_depth` from 6,150 promoted objects to 6,139 copied ones. It did not: a gc_ratchet run of the `origin/main` @ 0a2bf15 reference build on the same host produces that flip too, along with `04_dead_after_deep_stack`'s copied_objects row -- six identical gating rows, byte for byte. Comparing the two artifacts cell by cell, EVERY gating metric across all 13 probes is identical between main and this branch; only wall_ms/rss_bytes/peak_rss_bytes differ, and those are the three the shared_ci profile deliberately does not gate. Those rows are red on main on this host, not something this PR did. The principle behind it was wrong too. It carved `copied_minor_promotion_handoff_pressure_due` out as a survivor-PLACEMENT decision that should not read a band derived from full-GC-yield evidence. Its own doc says otherwise -- "whether an imminent promotion justifies a full old reclaim FIRST" -- and `gc::mod.rs` responds to it with `note_survivor_promotion_handoff_full`. It is a full-collection decision like the other two, so one signal and one multiplier across all three callers of `old_reclaim_pressure_due` is the coherent shape, not a carve-out. * docs(changelog): record the two refuted hypotheses and the pre-existing gc-ratchet rows * docs(changelog): record the gap-suite A/B against the main reference build * docs(changelog): the tenth non-snapshot gap failure is pre-existing too * docs(changelog): complete the gap-suite divergence accounting --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
gc-handoff/bench/retain.tsbuilds a 3 M-element array of{a, b}records,keeps every one of them live, then sums a field. Nothing is ever garbage,
and Perry spent 1.26 s of a 1.31 s run inside the collector — 96%. Node
does it in 0.14 s.
Two independent causes, both measured on the pinned quiet M1 mini against
origin/main@c156f8a41.1. Half the pause was two full mark-sweeps that reclaimed 4 MB between them
PERRY_GC_TRACE=1 ./n_retainonmain:arena_growth_full_escalation_dueescalates a minor to a full once arenalive-bytes pass K× (K = 2) the live set measured after the last full. Its own
doc-comment claimed a "legitimately large stable live set (retain-style) does
not over-escalate — its arena hovers near its own baseline".
retain's liveset is not stable, it grows: every doubling crosses the threshold, and each
resulting full marks a heap where nothing has died. 644 ms of pause for 4 MB,
and the second one moved arena in-use by exactly zero.
So price a full by what it reclaims. A full that shrinks arena in-use by less
than 20% shifts the next escalation threshold left by one — capped at 2, so the
multiplier tops out at 8× — and a productive full resets the shift to 0 in one
step. The yield is measured on the same metric the escalation gate reads,
so the two cannot disagree about whether a full helped, and it is scoped to
escalated fulls: an explicit
gc()never moves the backoff, or agc()-in-a-loop test would drive it to the cap. Old-gen garbage is unaffected—
old_reclaim_pressure_duestill forces its own full.2. Every evacuated object took a process-global mutex to hash an empty map
object_static_prototype_owner_moved— theObjectOverflowFieldsmove hook,run once per moved object — took a
Mutex<HashMap<usize, u64>>and ran aSipHash
removeagainst the residualObject.setPrototypeOfregistry.That registry is empty in any program that never re-prototypes a
non-meta-capable owner, and a latch already says so:
OBJECT_PROTOTYPES_NONEMPTY,stored
Releasebefore the insert, and read by both siblings(
object_static_prototype,prune_dead_object_prototype_owners). The movehook was the one reader that skipped it, so a 3 M-record promotion paid 2.5 M
lock/unlock pairs and 2.5 M hash probes against an empty map. It is why a
single-threaded benchmark profiled with
pthread_mutex_lockandstd::hash::random::RandomStatein its top ten.Isolated without a second build by
retain_latched.ts, a twin that latches theregistry through a
RegExpowner: on this binaryretainis 0.81 s andretain_latched0.84 s, and onmainthe two are 1.31/1.32 s — the latch fixis worth 0.03 s, and the control shows the A/B is measuring the right thing.
The bug this nearly shipped as
The first cut recorded the pre-full arena reading at the two
gc_start_budgeted_cycle_for_pressurecall sites. Both correct, both the wrongsites: the escalation the shipped safepoint path actually takes lives in
gc::gc_collect_minor_with_trigger_inner, so the reading was never recorded,update_major_pacing_backoffearly-returned on every cycle, and the changemeasured 1.31 → 1.28 s — the mutex fix alone — with every test green.
The recording now happens inside
arena_growth_full_escalation_dueon thetrueverdict, so a call site added later is priced by construction. The GCtrace gained a
major_pacingblock (baseline_bytes,backoff_shift,escalate_above_bytes) so the subject can be asserted live rather thaninferred from a green run — that block is what showed the backoff sitting at 0
through all seven cycles.
One full is the right answer, not zero
Turning major pacing off entirely (
PERRY_GC_MAJOR_PACING_FLOOR_MB=0, soarena_growth_full_escalation_dueis a constantfalse) makesretainslower, not faster: 1.14 s and 390 MB peak RSS, against 0.81 s / 342 MB
with the backoff and 1.31 s / 373 MB on
main. The array's abandoned backingbuffers are the one thing on this workload that only a full reclaims, so the
surviving full earns its 161 ms — the two on
maindid not. The backoff landsbetween the extremes rather than at one of them, which is the point.
The discriminator, checked on the workload it must NOT touch
tree.tsruns 40 escalated fulls (allarena_bytes) in a 1.64 s run,562 ms of pause. Every one of them takes arena in-use from ~41 MB to ~5 MB —
an 88% yield. The backoff reads 0 through all forty and
treeisbit-for-bit unchanged.
retain's two fulls yield 5.9% and 0.0% and the backoffmoves on the first one. That is the whole heuristic, observed on both sides.
Measurements — quiet M1 mini, best of 5, absolute seconds
mainPeak RSS goes down, not up:
retain373 → 342 MB,retain_wide538 → 470 MB,retain_prealloc416 → 408 MB. Every stdout is byte-identical tonode --experimental-strip-types.GC traces, before → after:
retainretain_widegc_ratchet(shared_ci), measured on both arms on the same host:maincurrently fails four
12_large_live_setcells against the pinned baseline(
heap_total_bytes+19.05%,promoted_objects+10.79%,promoted_bytes+11.17%,
freed_bytes+10.86%); this PR returns all four tookand dropsthat probe's peak RSS 191 → 170 MB. Its post-
gc()heap_used_bytesgoes33.6 → 50.4 MB, still under the 51.7 MB baseline and still reported as an
improvement. Every other cell's verdict is identical on both arms, including
the
04_dead_after_deep_stack/11_collect_at_depthregressions that arealready red on
mainand are byte-identical between arms.gc-handoff/apps/iso_miss.tsprintschecksum 437840 misses 0.Test status
cargo test --release -p perry-runtime --no-fail-fast: 1961 pass, 3 fail.The three are
gc::tests::runtime_roots::generator_attach_prototype::*, andthey are pre-existing on
main— a test binary built atc156f8a41failsthe same three, and they also fail on this branch with major pacing switched
off entirely (
PERRY_GC_MAJOR_PACING_FLOOR_MB=0, which makesarena_growth_full_escalation_duea constantfalse). All three arelive-subject assertions about the arena trigger arming / safepoint deferral,
not about which collection kind runs.
Not fixed here, measured and named
retainis still ~5.8× node. The residue is one number: ~220 ns ofper-object bookkeeping for every promoted object, 2.5 M of them on this
workload —
arena_alloc_gc_old,layout_transfer,old_page_account_promoted_object, the move hooks, and two page-generationclassifications per visited slot. The structural answer is V8-style whole-page
promotion (relabel a nearly-all-live Eden block as old-gen instead of
evacuating it object by object), which needs none of that per-object work.
Second on the list:
gc::verify::restore_surviving_dirty_coveragewalksevery slot of a parent on a pre-cycle dirty page, where the scan it repairs
(
scan_dirty_object_slots) walks only the slots on dirty pages — 8.8% of apure-retain profile, re-walking the whole 3 M-element backing store on every
minor.
Summary by CodeRabbit
Performance
Diagnostics
major_pacingdata to garbage-collection diagnostic output, including thresholds and current pacing state.Documentation