perf(gc): debt-proportional assist pacing + budgeted-cycle soundness (#6224)#6226
Merged
Conversation
A fixed 256-unit assist budget lets a tight allocation loop outrun the incremental collector: measured on a 10M-allocation ring benchmark, the budgeted cycle never completed (0 collections vs the synchronous default's 7) and RSS grew 6-22x unbounded. GcDebtSnapshot already measured the shortfall but fed telemetry only. Scale each assist's work budget linearly with the measured debt (1 unit per KB of arena debt + 1 unit per outstanding malloc object). Work-per-assist grows with debt while allocation between assists stays ~constant (one arena block), so the controller self-stabilizes at a bounded equilibrium. No cap: the budget is a ceiling, not a pause floor - a step stops the moment the cycle completes, so the worst case degrades to the synchronous collector's pause, never to unbounded RSS. make_arena_trigger_due now arms just-due (debt ~ 1 byte) instead of trigger=0 (debt = whole arena) so existing bounded-assist tests keep their 'collector keeping up' semantics under debt scaling.
|
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 selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughIncremental GC now scales mutator assistance from allocation debt, drains active budgeted cycles before synchronous collections, and applies allocate-black and minor-only barrier behavior across arena and malloc allocations. Cycle finalization drains late trace work and clears barrier state. ChangesIncremental GC lifecycle
Estimated code review effort: 4 (Complex) | ~50 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Mutator
participant GCPolicy
participant GcCycleState
participant AllocationBarrier
participant SynchronousCollection
Mutator->>GCPolicy: trigger allocation-side assist
GCPolicy->>GcCycleState: step with debt-scaled work units
GcCycleState->>AllocationBarrier: mark allocations born during cycle
SynchronousCollection->>GCPolicy: drain active budgeted cycle
GCPolicy->>GcCycleState: finish remaining phases
GcCycleState-->>SynchronousCollection: cycle complete
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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/policy.rs`:
- Around line 1826-1837: Ensure the fallback does not proceed while a parked
budgeted cycle remains active: after the drain loop around
gc_budgeted_step_work_units_inner, add
debug_assert!(!gc_budgeted_cycle_active()) or return early when the condition is
still true, before synchronous collectors can start a new GcCycleState.
🪄 Autofix (Beta)
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: d2387dcb-db2c-495a-b8b9-03da0224fae5
📒 Files selected for processing (8)
crates/perry-runtime/src/arena/allocators.rscrates/perry-runtime/src/gc/barrier.rscrates/perry-runtime/src/gc/cycle.rscrates/perry-runtime/src/gc/malloc.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/tests/debt_pacer.rscrates/perry-runtime/src/gc/tests/support.rs
Comment on lines
+1826
to
+1837
| for _ in 0..64 { | ||
| let result = gc_budgeted_step_work_units_inner(usize::MAX); | ||
| if !gc_budgeted_cycle_active() { | ||
| return; | ||
| } | ||
| if result.status == JS_GC_STEP_STATUS_SKIPPED { | ||
| break; | ||
| } | ||
| } | ||
| if std::env::var_os("PERRY_GC_DIAG").is_some() { | ||
| eprintln!("[gc-drain] WARNING: parked budgeted cycle could not be drained before synchronous collection"); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the blocking conditions of the budgeted stepper and all callers of the drain.
rg -nP --type=rust -C4 '\bgc_drain_active_budgeted_cycle\b'
echo '--- budgeted stepper: when does it return SKIPPED / refuse progress? ---'
ast-grep run --pattern 'fn gc_budgeted_step_work_units_inner($$$) { $$$ }' --lang rust crates/perry-runtime/src/gc/policy.rs
rg -nP --type=rust -C3 'JS_GC_STEP_STATUS_SKIPPED'Repository: PerryTS/perry
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- locate GC drain / active cycle symbols ---'
rg -n --type=rust 'gc_drain_active_budgeted_cycle|gc_budgeted_cycle_active|gc_budgeted_step_work_units_inner|JS_GC_STEP_STATUS_SKIPPED|parked budgeted cycle|synchronous collection|GC_FLAG_MARKED|mark-seed-queue|barrier' crates/perry-runtime/src
echo '--- file outline: policy.rs ---'
ast-grep outline crates/perry-runtime/src/gc/policy.rs --view expanded | sed -n '1,240p'
echo '--- file outline: mod.rs ---'
ast-grep outline crates/perry-runtime/src/gc/mod.rs --view expanded | sed -n '1,240p'Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- nearby lines in policy.rs around the reported snippet ---'
sed -n '1780,1865p' crates/perry-runtime/src/gc/policy.rs | cat -n
echo '--- nearby lines in mod.rs that mention synchronous collection or drains ---'
rg -n -C6 --type=rust 'synchronous collection|drain|budgeted cycle|GcCycleState|gc_budgeted_cycle_active|gc_drain' crates/perry-runtime/src/gc/mod.rs crates/perry-runtime/src/gc/policy.rs
echo '--- list all gc/ files for context ---'
git ls-files crates/perry-runtime/src/gcRepository: PerryTS/perry
Length of output: 37990
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- find GC policy / mod files ---'
fd -a 'policy.rs|mod.rs' crates/perry-runtime/src/gc
echo '--- search for cycle state / drain names across runtime ---'
rg -n -C4 --type=rust 'GcCycleState|gc_budgeted_cycle_active|gc_drain_active_budgeted_cycle|budgeted_cycle|drain_active|SKIPPED|PERRY_GC_DIAG' crates/perry-runtime/src
echo '--- inspect the gc directory tree ---'
find crates/perry-runtime/src/gc -maxdepth 2 -type f | sortRepository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- policy.rs: budgeted step guard + blocked checks ---'
sed -n '1420,1475p' crates/perry-runtime/src/gc/policy.rs | cat -n
echo
sed -n '1840,1888p' crates/perry-runtime/src/gc/policy.rs | cat -n
echo '--- mod.rs: collection entry points around the drain ---'
sed -n '70,95p' crates/perry-runtime/src/gc/mod.rs | cat -n
echo
sed -n '240,295p' crates/perry-runtime/src/gc/mod.rs | cat -n
echo '--- debt_pacer.rs: manual drain test ---'
sed -n '490,535p' crates/perry-runtime/src/gc/tests/debt_pacer.rs | cat -nRepository: PerryTS/perry
Length of output: 11134
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- public/manual collection entry points ---'
rg -n -C6 --type=rust 'pub extern "C" fn js_gc_collect|fn gc_collect_minor\(|fn gc_collect_inner_with_trigger\(|ManualGcScanGuard|force_full_scan|defer_gc_request|gc_try_emergency_reclaim' crates/perry-runtime/src/gc/mod.rs crates/perry-runtime/src/gc/policy.rs crates/perry-runtime/src/gc/roots.rs
echo '--- policy.rs: BudgetedGcStepGuard definition / enter path ---'
rg -n -C8 --type=rust 'struct BudgetedGcStepGuard|impl BudgetedGcStepGuard|fn enter\(' crates/perry-runtime/src/gc/policy.rsRepository: PerryTS/perry
Length of output: 21716
Guard the fallback before starting a new synchronous cycle. If this loop bails out with gc_budgeted_cycle_active() still true, the sync collectors still continue and can start a fresh GcCycleState on top of the parked one. Add a debug_assert!(!gc_budgeted_cycle_active()) here (or return) so the second-cycle corruption turns into a loud failure.
🤖 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 1826 - 1837, Ensure the
fallback does not proceed while a parked budgeted cycle remains active: after
the drain loop around gc_budgeted_step_work_units_inner, add
debug_assert!(!gc_budgeted_cycle_active()) or return early when the condition is
still true, before synchronous collectors can start a new GcCycleState.
proggeramlug
force-pushed
the
perf/gc-assist-debt-pacing
branch
from
223fab1 to
5a7951f
Compare
Four coupled mechanisms, all latent while budgeted cycles never completed (fixed-256 assists never reached sweep): 1. Incremental mark barrier for budgeted MINOR cycles. It was gated to Full - but the default budgeted path for allocation pressure is Minor, which marked across mutator turns with zero store protection. Minor barriers shade NURSERY children only: a stray old-gen mark would survive the minor's sweep (minors don't walk old gen) and the next full cycle would misread it as already-traced and skip its children. Minors also run BarrierSeedDrain now (before WeakProcessing, so weak decisions read final marks) with minor-scoped tracing. 2. Final seed drain at both barrier-off points (minor jump-to-Sweep, Full DisableBarrier): a store after BarrierSeedDrain completed marked the child but never traced its children. The Full half of this hole predates this branch. 3. Allocate-black for the WHOLE cycle (GC_BIRTH_EXTRA_FLAGS, set at cycle construction, cleared at outcome/Drop): objects born mid-cycle and installed via runtime-internal RAW stores (grown array elements buffers, map entry nodes) bypass the nanboxed value-barrier path, sat unmarked, and were swept live - measured as a deterministic 2,890/32,000 silent graph-node loss. Barrier-window-only birth flags were NOT enough: the loss reproduced identically because the victims were born during BuildValidPointerSet slices, before the barrier enables. Codegen inline-alloc objects don't need this: their installs all cross codegen store barriers. 4. Synchronous collections drain a parked budgeted cycle first (gc_drain_active_budgeted_cycle at both direct-collect entries). Two cycles share GC_FLAG_MARKED, the mark-seed queue, the barrier TLS, and the birth-flag lifecycle; a manual gc() landing mid-cycle erased the parked cycle's marks and its eventual sweep freed live objects (SIGSEGV: swept-live property key hashed in compiled code). The drain loops step() to completion - one call advances only the current phase - and bails if the stepper reports blocked. Verified: 15k-round differential stress (old->young stores + manual gc() + force-evacuate), 7/7 exact-checksum clean, plus a GEN_GC=0 control; was 7/7 SIGSEGV / silent loss before. Regression tests: budgeted_cycle_allocations_are_born_marked_for_the_whole_cycle, manual_gc_drains_parked_budgeted_cycle_first.
proggeramlug
force-pushed
the
perf/gc-assist-debt-pacing
branch
from
5a7951f to
aad8813
Compare
This was referenced Jul 10, 2026
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.
Fixes #6224.
The measured incremental-GC pacing failure, plus the four latent budgeted-cycle soundness holes that working pacing immediately exposed (each found by re-running a differential stress after the previous layer, each verified fail-before/pass-after). Neither half is shippable alone — pacing without the soundness fixes crashes or silently drops live objects, and the soundness fixes without pacing are untestable dead weight (cycles never complete to sweep at all).
1. Debt-proportional mutator-assist pacing (#6224)
Bug (measured): allocation-side assists did a fixed
GC_MUTATOR_ASSIST_WORK_UNITS = 256units of GC work per trigger regardless of how far behind the collector had fallen. A tight allocation loop outruns that: on a 10M-allocation ring benchmark the budgeted cycle never completed (0 collections vs the synchronous default's 7) and RSS grew 6–22× unbounded (3.1 GB at 10M, 11 GB at 40M, vs STW's 501 MB).GcDebtSnapshotalready measured exactly this shortfall — allocation past the armed triggers — but fed telemetry only.Fix: scale each assist's budget linearly with measured debt (
gc_mutator_assist_scaled_work_units): base 256 + 1 unit perGC_ASSIST_DEBT_BYTES_PER_WORK_UNIT = 64bytes of arena debt + 1 unit per outstanding malloc-registry object. Between two block-alloc assists the mutator allocates ~one arena block while the budget grows with total debt, so the controller self-stabilizes at a bounded equilibrium. The gain was chosen empirically: at 1024 bytes/unit, cycles still spanned ~300 MB of allocation (pct_freed156–190% in the re-arm DIAG) and RSS sat at 3.5× STW; at 64 bytes/unit cycles complete within ~their trigger step.No cap, by design: the budget is a ceiling on work, not a pause floor —
GcCycleState::stepstops the moment the cycle completes, and remaining cycle work is bounded by the heap. Worst case under extreme allocation pressure is finishing the cycle in one assist, i.e. exactly the pause the synchronous collector takes on every collection today: incremental degrades gracefully toward STW behavior rather than toward unbounded memory.make_arena_trigger_due(test helper) now arms just-due (debt ≈ 1 byte) instead oftrigger = 0(debt = entire arena), preserving the existing bounded-assist tests' "collector keeping up" semantics under debt scaling.2. Incremental mark barrier for budgeted MINOR cycles
Bug (exposed by fix 1, previously masked by it):
incremental_mark_barrier_enablewas gated toGcCollectionKind::Full— but the default budgeted path for ordinary allocation pressure is Minor. A minor cycle sliced across mutator turns therefore marked with no store protection: a store into an already-scanned object left the stored child unmarked, and the sweep freed it live. Unreachable while pacing was broken (cycles never completed → never swept → the hole never fired; the earlier differential stress "passed" for exactly this reason). With pacing fixed it reproduced 7/7 as a SIGSEGV — a swept-live property-key string hashed through a wild pointer (key_content_hash_impl,object/mod.rs:771).Fix:
AtomicFinalizeSubphase::BarrierSeedDrain(first, so weak/finalization processing reads final marks), with kind-correct trace scoping (minor_only).DisableBarrier) now do a final synchronous seed drain before disabling, closing the late-store window where a child was marked but its own children never traced. This half of the hole existed for Full cycles already.GcCycleStateDrop safety net (disable + clear seeds on an incomplete cycle) now covers both kinds, so the barrier's rawvalid_ptrspointer can never dangle.3. Allocate-black for the whole cycle
Fixes 1–2 still left a deterministic 2,890/32,000 silent live-node loss (no crash — a checksum-mismatch data corruption), reproducing identically under
PERRY_GEN_GC=0(so not minor/RS-specific). Mechanism: objects born mid-cycle and installed via runtime-internal raw stores (a grown array's elements buffer, map entry nodes) never cross the nanboxed value-barrier path; and the victims were born duringBuildValidPointerSetslices — before the barrier even enables (which is why barrier-window-only birth flags reproduced the loss bit-for-bit). Fix:GC_BIRTH_EXTRA_FLAGS— runtime-path allocations are bornMARKEDfrom cycle construction to outcome/Drop. Bounded floating garbage, priced by the pacer. Codegen inline-alloc objects don't need it: their installs all cross codegen store barriers.4. Synchronous collections drain a parked budgeted cycle first
Manual
gc()(and any direct/emergency collection) landing while a budgeted cycle was parked mid-phase constructed a secondGcCycleStatesharingGC_FLAG_MARKED, the mark-seed queue, the barrier TLS, and the birth-flag lifecycle — the interloper's sweep erased the parked cycle's marks, and the parked cycle's eventual sweep freed live objects (the SIGSEGV escalation: a swept-live property-key string hashed through a wild pointer).gc_drain_active_budgeted_cycleat both direct-collect entries finishes the parked cycle through its own machinery first — loopingstep()(one call advances only the current phase; instrumentation caught the single-step version leaving the cyclestill_active=true) and bailing if the stepper reports blocked.Known remaining gap (documented, out of scope): a pointer held only in a stack local with no heap store during the cycle window still needs a final root re-scan (remark phase) to be strictly sound — tracked as the remaining #6180 Stage-2 blocker. Every store-visible pattern is covered by the barrier + drains above.
Measured (N=3 medians unless noted,
/usr/bin/time -l)* the "before" latency/wall numbers were artifacts of under-collection — the collector wasn't doing its job (RSS column).
Full
perry-runtimelib suite: 1225/1226 (the 1 failure is the known CWD-race flakeurl::node_compat::path_to_file_url_posix, failing identically on pristine main). New regression tests:mutator_assist_work_units_scale_with_debt,debt_scaled_assists_cannot_be_outrun_by_allocation(a heap needing ~hundreds of thousands of work units must complete within 300 assist calls — fixed-256 could supply at most 76.8k units),budgeted_cycle_allocations_are_born_marked_for_the_whole_cycle,manual_gc_drains_parked_budgeted_cycle_first.PERRY_GC_INCREMENTALremains default-off; this PR makes the mode viable (bounded RSS + no store-visible UAF) so the Stage-2 default-on decision can rest on the remaining remark-phase work plus these measurements.Final trade-off (all protections active)
Worst observed pause 5.4× better (636 → 118 ms; p999 488 → 111 ms), throughput at parity (B2 32.3 → 32.0 s), RSS 1.8–2× STW (down from 6–22× unbounded) — the remaining gap is the exact valid-pointer BTreeSet held across the sliced cycle plus allocate-black floating garbage, both of which #6179 (page-metadata classification) targets. p99 rises 7 → 53 ms: that is the debt pacer honestly doing collection work the broken version skipped. The remaining known soundness gap for default-on (#6180 Stage 2) is the stack-local remark phase; every store-visible pattern is covered here.
Summary by CodeRabbit