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>
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