fix(gc): pace PERRY_GC_ZEAL by allocation so the instrument terminates (#7728) - #7729
Merged
Conversation
proggeramlug
pushed a commit
that referenced
this pull request
Aug 9, 2026
|
Warning Review limit reached
Next review available in: 2 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughGC zeal now uses allocation-paced moving collections with a default 4 KiB stride. The ChangesGC zeal pacing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Workload
participant js_gc_loop_safepoint
participant GCZeal
participant MovingMinor
Workload->>js_gc_loop_safepoint: reach loop poll
js_gc_loop_safepoint->>GCZeal: check allocation stride
GCZeal-->>js_gc_loop_safepoint: due or paced
js_gc_loop_safepoint->>MovingMinor: collect when due
MovingMinor-->>GCZeal: post-collection nursery occupancy
GCZeal-->>Workload: record poll and pacing metrics
Possibly related PRs
🚥 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: 4
🤖 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 `@CLAUDE.md`:
- Around line 143-147: Remove the documentation changes shown in CLAUDE.md and
restore the file to its pre-PR state. Keep the PR-keyed changelog fragment and
other dedicated documentation updates unchanged, since CLAUDE.md must remain
maintainer-owned.
In `@crates/perry-runtime/src/gc/policy.rs`:
- Around line 2304-2309: Ensure the zeal rearm logic near
gc_safepoint_moving_minor() advances ZEAL_NEXT_FORCE_BYTES only when that call
actually completes a forced minor collection, not when an entry guard causes an
early return. Propagate an explicit completion result from
gc_safepoint_moving_minor() or use equivalent state, preserve the due threshold
while blocked, and add a regression test covering a blocked due poll that later
becomes eligible.
In `@docs/src/internals/memory-model.md`:
- Line 135: Update the adjacent loop-poll caveat to remove the stale claim that
PERRY_GC_MOVING_LOOP_POLLS=1 is default-off or requires compile-time enablement.
Direct users to inspect the zeal verdict’s loop_polls= field to confirm loop
coverage, keeping the surrounding GC zeal guidance unchanged.
In `@scripts/gc_instrument_smoke.sh`:
- Around line 374-380: Update the arm6 validation around scale_polls and
scale_forced to fail immediately when scale_polls is zero, before the existing
forced-collections-versus-polls comparison. Preserve the current pacing
assertion for positive poll counts.
🪄 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: a0521ae7-fb91-4385-8fbb-287ffb5b666e
📒 Files selected for processing (10)
.github/workflows/test.ymlCLAUDE.mdchangelog.d/7729-gc-zeal-allocation-pacing.mdcrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/tests/fromspace_protect.rscrates/perry-runtime/src/gc/zeal.rsdocs/src/internals/gc-rooting-invariant.mddocs/src/internals/memory-model.mdscripts/gc_instrument_smoke.sh
Comment on lines
+143
to
+147
| | `PERRY_GC_ZEAL=1` | forces an evacuating minor at **GC safepoints**: `js_gc_loop_safepoint` (loop back-edge) and the outermost microtask-pump safepoint. It bypasses exactly two things — the `GC_SAFEPOINT_PENDING` requirement in `js_gc_loop_safepoint`, and the `gc_budgeted_due_trigger()` "is anything due?" test in `gc_safepoint_moving_minor`. Also makes `gc_force_evacuate_enabled()` true, so survivors actually MOVE. **Allocation-PACED since #7728** — see the row below; it used to collect at every single poll, which cost 24 minutes on a 19 s program once #7721 made polls default-ON. | bypass `gc_safepoint_moving_minor`'s **entry guards**: a safepoint reached mid-allocation (`GC_FLAG_IN_ALLOC`), suppressed (`GC_FLAG_SUPPRESSED`), inside an unsafe FFI zone, under a non-zero `GC_ROOT_LOCK_DEPTH`, or during a budgeted cycle still returns without collecting. Nor does it emit loop polls — those come from the **compile-time** `PERRY_GC_MOVING_LOOP_POLLS` (**default ON since #7721**; it was off from #7161 until then, which is why zeal used to look free — it was collecting nothing), and even then codegen emits **no poll** for a provably alloc-free loop body (by design, `loop_purity::loop_may_allocate`) nor for the specialized `for` / `for-of` / `for-in` lowerings (by omission — see `emit_gc_loop_safepoint`'s COVERAGE note). Zeal on a poll-free binary only fires at event-loop boundaries; a compute-only loop never collects. **You no longer have to remember to check this**: since #7604 a zeal run prints `[gc-zeal] forced_collections=N copying_minors=M moved_objects=K loop_polls=P paced_polls=Q stride_bytes=S` at exit and **exits 70** if N or M is zero, so a run that exercised nothing is a red run rather than a green one. (`process.exit()` and an uncaught throw bypass the exit boundary and get no verdict.) There is deliberately **no level 2**: the alloc-point arm forces a conservative stack scan, which makes the copying minor ineligible, so an "every allocation" zeal would run non-moving minors and move nothing. | | ||
| | `PERRY_GC_ZEAL_ALLOC_KB=N` (default 4) | how much NEW nursery material must accumulate between zeal-forced collections. Zeal's cost is ~511 us of fixed root-scan per collection to relocate a mean of 5.9 objects, so unpaced "every back-edge poll" is one whole collection per loop iteration. The stride is a monotone high-water mark (rearmed to `from_space_after + N`), so total forced collections are bounded by `bytes_allocated / N` even when a collection reclaims nothing. **`=0` restores the literal every-poll mode** — use it for a small fixture, or for a bug window executed exactly once. | change WHICH safepoints are eligible, or weaken evacuation: a paced collection is the same collection, just less often. A recurring window is still caught, after N KB of allocation rather than on the first iteration. | | ||
| | `PERRY_GC_FROMSPACE_SCAN_ABORT=1` | now **implies** `PERRY_GC_FROMSPACE_SCAN=1`. It used to be inert alone (the scan never ran, so nothing aborted, and the run reported success). | — | | ||
|
|
||
| `PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1` together is the pairing that turns a #7154 bug into an immediate precise fault. Compile *and* run with `PERRY_GC_MOVING_LOOP_POLLS=1` for in-loop coverage. | ||
| `PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1` together is the pairing that turns a #7154 bug into an immediate precise fault. Loop polls are default-ON since #7721, so in-loop coverage no longer needs a flag — check `loop_polls=` in the exit verdict rather than assuming. If a hunt needs maximum sensitivity on a small program, add `PERRY_GC_ZEAL_ALLOC_KB=0`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the CLAUDE.md documentation change.
This PR already contains a PR-keyed changelog fragment and dedicated documentation updates. Keep this file unchanged.
Based on learnings, contributors must not edit CLAUDE.md in external PRs; maintainers own release and version metadata updates.
🤖 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 `@CLAUDE.md` around lines 143 - 147, Remove the documentation changes shown in
CLAUDE.md and restore the file to its pre-PR state. Keep the PR-keyed changelog
fragment and other dedicated documentation updates unchanged, since CLAUDE.md
must remain maintainer-owned.
Source: Learnings
Comment on lines
+2304
to
+2309
| gc_safepoint_moving_minor(); | ||
| // Rearm from the level measured AFTER the collection, so the next | ||
| // forced one costs a full stride of new allocation on top of whatever | ||
| // survived — see `gc/zeal.rs` for why this is a high-water mark and not | ||
| // a delta. | ||
| super::zeal::note_zeal_poll_collection(crate::arena::copying_from_space_in_use_bytes()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Rearm only after a zeal collection completes.
gc_safepoint_moving_minor() can return at Line 2199 when an entry guard blocks collection. Lines 2304-2309 then advance ZEAL_NEXT_FORCE_BYTES anyway.
This skips the due collection. The next eligible poll must wait for another full stride of allocation. Return an explicit “collection completed” result from gc_safepoint_moving_minor(), or otherwise rearm only after this thread actually forces the zeal minor. Add a regression test for a due poll that is initially blocked and later becomes eligible.
🤖 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 2304 - 2309, Ensure the
zeal rearm logic near gc_safepoint_moving_minor() advances ZEAL_NEXT_FORCE_BYTES
only when that call actually completes a forced minor collection, not when an
entry guard causes an early return. Propagate an explicit completion result from
gc_safepoint_moving_minor() or use equivalent state, preserve the due threshold
while blocked, and add a regression test covering a blocked due poll that later
becomes eligible.
| | `PERRY_GC_PROTECT_FROMSPACE=poison` | As above without `mprotect`: poison only. Use where a fault is unwanted, or for the sub-page block edges `mprotect` cannot cover (those are always poison-filled and counted separately). | | ||
| | `PERRY_GC_PROTECT_FROMSPACE_DEPTH=N` | How many retired page-sets stay quarantined (default `4`, minimum `1`). Expired sets are restored to read/write and **recycled back into Eden**, never freed, so the quarantine is a ring: steady-state footprint is bounded by `N × from-space bytes` and no `mprotect`'d page is ever handed to the system allocator. | | ||
| | `PERRY_GC_ZEAL=1` | Force an evacuating minor at **every GC safepoint** — loop back-edge polls and the outermost microtask-pump boundary — instead of only when nursery pressure is due. Implies `PERRY_GC_FORCE_EVACUATE`, so survivors actually move. (Until #7611 an ambient `PERRY_GEN_GC_EVACUATE=0` silently vetoed that, leaving zeal moving nothing and therefore surfacing nothing — the knob was deleted for exactly that footgun.) Zeal also does **not** bypass `gc_safepoint_moving_minor`'s entry guards (in-allocation, suppressed, unsafe FFI zone, non-zero root-lock depth, budgeted cycle): a safepoint reached in any of those states still declines to collect. Modelled on V8 `--stress-scavenge` / SpiderMonkey `gcZeal`. Composes with the two above; that pairing is what turns a rooting bug into an immediate precise fault. | | ||
| | `PERRY_GC_ZEAL=1` | Force an evacuating minor at **GC safepoints** — loop back-edge polls and the outermost microtask-pump boundary — instead of only when nursery pressure is due. **Allocation-paced since #7728** (`PERRY_GC_ZEAL_ALLOC_KB`, default 4; `=0` restores the literal every-poll mode): unpaced, one collection per loop iteration cost ~511 µs to relocate a mean of 5.9 objects, which made zeal unusable on real workloads once #7721 turned back-edge polls on by default. Implies `PERRY_GC_FORCE_EVACUATE`, so survivors actually move. (Until #7611 an ambient `PERRY_GEN_GC_EVACUATE=0` silently vetoed that, leaving zeal moving nothing and therefore surfacing nothing — the knob was deleted for exactly that footgun.) Zeal also does **not** bypass `gc_safepoint_moving_minor`'s entry guards (in-allocation, suppressed, unsafe FFI zone, non-zero root-lock depth, budgeted cycle): a safepoint reached in any of those states still declines to collect. Modelled on V8 `--stress-scavenge` / SpiderMonkey `gcZeal`. Composes with the two above; that pairing is what turns a rooting bug into an immediate precise fault. | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the adjacent loop-poll caveat.
Line 135 states that allocation pacing shipped after loop polls became default-on. Lines 153-156 still state that PERRY_GC_MOVING_LOOP_POLLS=1 is default-off and required at compile time.
Replace that stale guidance. Tell users to inspect loop_polls= in the zeal verdict when they need to confirm loop coverage.
🤖 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 `@docs/src/internals/memory-model.md` at line 135, Update the adjacent
loop-poll caveat to remove the stale claim that PERRY_GC_MOVING_LOOP_POLLS=1 is
default-off or requires compile-time enablement. Direct users to inspect the
zeal verdict’s loop_polls= field to confirm loop coverage, keeping the
surrounding GC zeal guidance unchanged.
proggeramlug
pushed a commit
that referenced
this pull request
Aug 9, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@changelog.d/7729-gc-zeal-allocation-pacing.md`:
- Line 11: Update the collection-bound statement in the changelog to qualify it
for a positive stride, and explicitly exclude `PERRY_GC_ZEAL_ALLOC_KB=0`, which
is supported every-poll mode and has no finite `bytes_allocated / stride` bound.
- Around line 17-23: Update the wording in the changelog row-0 explanation to
avoid claiming exact 1:1 behavior, or explicitly account for the five-collection
difference between forced collections and polls. Keep the existing benchmark
results and other correctness claims unchanged.
🪄 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: 33383975-04cd-4979-8cc6-5a43ef69f6de
📒 Files selected for processing (3)
changelog.d/7729-gc-zeal-allocation-pacing.mdcrates/perry-runtime/src/gc/zeal.rsscripts/gc_instrument_smoke.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/gc_instrument_smoke.sh
- crates/perry-runtime/src/gc/zeal.rs
|
|
||
| The earlier build that ran "instantly with the correct answer" was **vacuous**. `PERRY_GC_MOVING_LOOP_POLLS` was default-OFF there (#7161), so a compute-only program reached no loop safepoint and zeal forced nothing; that build also predates the #7604 exit verdict, so it exited 0 in silence. #7721 flipped the poll default ON — correctly, it is a large collector win — and in the same commit turned zeal from free-and-vacuous into correct-but-unusable. Isolated rather than assumed: the *old* compiler with `PERRY_GC_MOVING_LOOP_POLLS=1` forced at compile and run time already costs 35.8 s for one round under zeal against 0.62 s without, so no commit broke zeal — zeal was never paced, and the poll default is what exposed it. #7254 had already logged "a striking concentration of multi-minute-plus runs" under this pairing and left the population untriaged; this is that triage. | ||
|
|
||
| Zeal now forces a collection at the first poll at which `PERRY_GC_ZEAL_ALLOC_KB` (default 4) of new nursery material has accumulated — the model V8 (`--gc-interval`) and SpiderMonkey (`gcZeal(mode, frequency)`) both use, and for the same reason. The stride is a **monotone high-water mark**, not a "bytes since" delta: each forced collection rearms to `from_space_after + stride`, so a collection that reclaims nothing (an escalation to a non-moving full mark-sweep, which #7592 and #7682 both produced in the field) still demands another full stride of genuinely new allocation. Total forced collections are bounded by `bytes_allocated / stride` whatever the collector does with them, which makes this a bound rather than a hope. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Qualify the collection bound for zero stride.
The allocation bound applies only when the stride is positive. PERRY_GC_ZEAL_ALLOC_KB=0 is a supported every-poll mode, so bytes_allocated / stride does not describe its collection count.
Proposed wording
- Total forced collections are bounded by `bytes_allocated / stride` whatever the collector does with them, which makes this a bound rather than a hope.
+ For a positive stride, total forced collections are bounded by `bytes_allocated / stride`, regardless of collector behavior. `PERRY_GC_ZEAL_ALLOC_KB=0` remains the explicit every-poll exception.This finding is based on the supported zero-stride mode documented in this fragment.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Zeal now forces a collection at the first poll at which `PERRY_GC_ZEAL_ALLOC_KB` (default 4) of new nursery material has accumulated — the model V8 (`--gc-interval`) and SpiderMonkey (`gcZeal(mode, frequency)`) both use, and for the same reason. The stride is a **monotone high-water mark**, not a "bytes since" delta: each forced collection rearms to `from_space_after + stride`, so a collection that reclaims nothing (an escalation to a non-moving full mark-sweep, which #7592 and #7682 both produced in the field) still demands another full stride of genuinely new allocation. Total forced collections are bounded by `bytes_allocated / stride` whatever the collector does with them, which makes this a bound rather than a hope. | |
| Zeal now forces a collection at the first poll at which `PERRY_GC_ZEAL_ALLOC_KB` (default 4) of new nursery material has accumulated — the model V8 (`--gc-interval`) and SpiderMonkey (`gcZeal(mode, frequency)`) both use, and for the same reason. The stride is a **monotone high-water mark**, not a "bytes since" delta: each forced collection rearms to `from_space_after + stride`, so a collection that reclaims nothing (an escalation to a non-moving full mark-sweep, which `#7592` and `#7682` both produced in the field) still demands another full stride of genuinely new allocation. For a positive stride, total forced collections are bounded by `bytes_allocated / stride`, regardless of collector behavior. `PERRY_GC_ZEAL_ALLOC_KB=0` remains the explicit every-poll exception. |
🤖 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 `@changelog.d/7729-gc-zeal-allocation-pacing.md` at line 11, Update the
collection-bound statement in the changelog to qualify it for a positive stride,
and explicitly exclude `PERRY_GC_ZEAL_ALLOC_KB=0`, which is supported every-poll
mode and has no finite `bytes_allocated / stride` bound.
Comment on lines
+17
to
+23
| | 0 (pre-fix) | 283,857 | 1,629,647 | 142.6 s | | ||
| | 1 | 70,929 | 815,460 | 36.1 s | | ||
| | **4 (default)** | **19,314** | **325,830** | **10.2 s** | | ||
| | 16 | 5,070 | 129,959 | 3.0 s | | ||
| | 64 | 1,291 | 52,357 | 1.1 s | | ||
|
|
||
| Row 0 reproduces the pre-fix 1:1 behaviour exactly on the shipped binary. Every row keeps `copying_minors == forced_collections` and `moved > 0`, so no stride degrades the instrument into non-moving sweeps. 4 KB rather than the faster 16/64 is deliberate — this is a correctness instrument, so the default errs toward sensitivity, still collecting once per ~15 loop iterations while being 14x cheaper than unpaced. The zeal-OFF path is untouched: the same workload without zeal is 4.49 s before and after. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the “exactly 1:1” claim.
The table reports 283,857 forced collections and 283,852 polls. That is near 1:1, but it is not exact. Change the wording or explain the five additional collections.
Proposed wording
- Row 0 reproduces the pre-fix 1:1 behaviour exactly on the shipped binary.
+ Row 0 reproduces the pre-fix near-1:1 behaviour on the shipped binary: 283,857 forced collections for 283,852 polls.This finding uses the counts stated in this fragment.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | 0 (pre-fix) | 283,857 | 1,629,647 | 142.6 s | | |
| | 1 | 70,929 | 815,460 | 36.1 s | | |
| | **4 (default)** | **19,314** | **325,830** | **10.2 s** | | |
| | 16 | 5,070 | 129,959 | 3.0 s | | |
| | 64 | 1,291 | 52,357 | 1.1 s | | |
| Row 0 reproduces the pre-fix 1:1 behaviour exactly on the shipped binary. Every row keeps `copying_minors == forced_collections` and `moved > 0`, so no stride degrades the instrument into non-moving sweeps. 4 KB rather than the faster 16/64 is deliberate — this is a correctness instrument, so the default errs toward sensitivity, still collecting once per ~15 loop iterations while being 14x cheaper than unpaced. The zeal-OFF path is untouched: the same workload without zeal is 4.49 s before and after. | |
| | 0 (pre-fix) | 283,857 | 1,629,647 | 142.6 s | | |
| | 1 | 70,929 | 815,460 | 36.1 s | | |
| | **4 (default)** | **19,314** | **325,830** | **10.2 s** | | |
| | 16 | 5,070 | 129,959 | 3.0 s | | |
| | 64 | 1,291 | 52,357 | 1.1 s | | |
| Row 0 reproduces the pre-fix near-1:1 behaviour on the shipped binary: 283,857 forced collections for 283,852 polls. Every row keeps `copying_minors == forced_collections` and `moved > 0`, so no stride degrades the instrument into non-moving sweeps. 4 KB rather than the faster 16/64 is deliberate — this is a correctness instrument, so the default errs toward sensitivity, still collecting once per ~15 loop iterations while being 14x cheaper than unpaced. The zeal-OFF path is untouched: the same workload without zeal is 4.49 s before and after. |
🤖 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 `@changelog.d/7729-gc-zeal-allocation-pacing.md` around lines 17 - 23, Update
the wording in the changelog row-0 explanation to avoid claiming exact 1:1
behavior, or explicitly account for the five-collection difference between
forced collections and polls. Keep the existing benchmark results and other
correctness claims unchanged.
added 11 commits
August 9, 2026 23:18
…default poll path is unchanged Claude-Session: https://claude.ai/code/session_015JgLM9UWGa6WAMix7CvhQJ
…ng-invariant pages (#7728) Claude-Session: https://claude.ai/code/session_015JgLM9UWGa6WAMix7CvhQJ
…tions/polls ratio (#7728) Claude-Session: https://claude.ai/code/session_015JgLM9UWGa6WAMix7CvhQJ
proggeramlug
force-pushed
the
gc/zeal-alloc-pacing
branch
from
August 9, 2026 21:18
c0b95a6 to
efc56f0
Compare
Contributor
Author
Merging as v0.5.1428 — the pacing question was tested against real history, not arguedI commissioned an adversarial audit on the one thing that could make this a bad trade: does pacing weaken the instrument in a way that would have hidden a bug this project actually found? The answer is no, and the reasoning is structural rather than lucky. Every #7154-family reproducer was checked against its own fixture:
Two reasons this holds:
And the density check makes it concrete: your own measurement is 19,314 forced collections over 283,852 polls at the 4 KB default — ~273 bytes per iteration, so a 600-iteration constructor loop gets ~40 forced collections, not one. The other four things I wanted verified, all confirmed in code
Scope is right too: the diff only paces Two follow-ups, neither blocking
Heads-up on the merge order#7735 rewrites the same lines of Gates 21/21. |
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.
Closes #7728.
PERRY_GC_ZEAL=1— the primary instrument for moving-GC correctness bugs, and half of the pairing that produced the precise fault behind #7682 — stopped completing on real workloads. Ongc-handoff/apps/iso_miss.ts(19 s without zeal) it now times out at 240 s with no output.It was never a livelock, and the "good" endpoint was never good
Scaling the round count on the pinned quiet host:
Perfectly linear — 40 rounds is ~24 minutes, not a hang. Zeal forced a collection at every back-edge poll: ~511 µs of fixed per-collection cost (root scan over the shadow stack plus ~55 side-table scanners) to relocate a mean of 5.9 objects. Nearly all of it is the collection's fixed overhead, not the relocation zeal exists to stress.
The
f3e14a61eendpoint was fast because it was vacuous.PERRY_GC_MOVING_LOOP_POLLSwas default-OFF there (#7161), so a compute-only program reached no loop safepoint and zeal forced nothing — and that build predates the #7604 exit verdict, so it exited 0 in silence. #7721 flipped the poll default ON (correctly — it is a large collector win) and in the same commit turned zeal from free-and-vacuous into correct-but-unusable.Isolated rather than assumed: the old
f3e14a61ecompiler withPERRY_GC_MOVING_LOOP_POLLS=1at compile and run time already costs 35.8 s for one round under zeal, against 0.62 s without. No commit broke zeal. Zeal was never paced, and the poll default is what exposed it. #7254 had already logged "a striking concentration of multi-minute-plus runs" under this pairing and left it untriaged; this is that triage.The fix
Zeal is now allocation-paced, the model V8 (
--gc-interval) and SpiderMonkey (gcZeal(mode, frequency)) both use: it forces a collection at the first poll at whichPERRY_GC_ZEAL_ALLOC_KB(default 4) of new nursery material has accumulated since the last one.The stride is a monotone high-water mark, not a "bytes since" delta — each forced collection rearms to
from_space_after + stride. A collection that reclaims nothing (an escalation to a non-moving full mark-sweep, which #7592 and #7682 both produced in the field) therefore still demands another full stride of real allocation. Total forced collections are bounded bybytes_allocated / stridewhatever the collector does with them, which makes this a bound rather than a hope.PERRY_GC_ZEAL_ALLOC_KB=0restores the literal every-poll semantics. That is the right setting for a small fixture or a bug window executed exactly once, and it is whatgc_instrument_smoke.shnow pins for its own tiny fixture so this change cannot quietly weaken the existing gate.What pacing gives up, stated rather than buried: a window crossed a single time may now fall between two forced collections. A window that recurs — every shape in the #7154 family, which is why the reproducers are loops — is still caught, after N KB of allocation instead of on the first iteration.
Why this could regress silently, and what now stops it
scripts/gc_instrument_smoke.shruns zeal end-to-end and is green. It could not see this: its fixture is deliberately sized at ~1200 polls "so the zeal arm costs seconds rather than minutes", and at that size every-poll and paced are indistinguishable. The gate was well built and pointed at a workload too small to expose the axis that broke.So arm 6 is new and budgeted: a 400k-iteration workload at the shipped default (with
env -uso the file's every-poll pin does not apply), requiring correct output inside a wall-clock budget that sits between the paced cost and the unpaced ~200 s. It also asserts non-vacuity — forced collections, copying minors and moved objects all non-zero, andforced < loop_polls— because "fast because it collects nothing" would be a worse regression than the slow instrument it replaces.Tests
In the required
cargo-testgate (gc/tests/fromspace_protect.rs):zeal_pacing_bounds_forced_collections_but_still_moves_objects— the regression test. Drives a hot poll loop and asserts forced collections are bounded well below the poll count, paired with two liveness assertions (collections still forced, survivors still moved). Sabotage-checked: pinning the stride to 0 fails it with2000 forced for 2000 polls, exactly the pre-fix ratio.zeal_alloc_stride_zero_restores_every_poll_collection— the OFF state of the new knob, per the binding kill-policy.zeal_pacing_rearms_above_survivors_so_a_useless_collection_cannot_loop— pins the monotone high-water mark against a delta-based rewrite.zeal_alloc_stride_knob_parses_both_states— including that0is meaningful rather than garbage.Docs
CLAUDE.md's instrument table gains the pacing row and loses a stale claim: it still said loop polls were "default off since #7161", which #7721 had made false. That sentence is why zeal's cost was invisible in the first place.
Summary by CodeRabbit
New Features
PERRY_GC_ZEAL_ALLOC_KB; the default is 4 KiB, while0preserves collection at every poll.Documentation
Tests
Measured (pinned quiet host,
ssh perry@perry-macos.local)The full workload under zeal: 240 s timeout → 98.8 s with the correct answer (
checksum 437840 misses 0), forcing 193,087 collections out of 2,838,560 polls, all of them copying minors, relocating 3,115,719 objects. Unpaced, the same run costs ~1,426 s.The stride sweep is one binary and one env var at a quarter scale — the cleanest available A/B — and
loop_pollsis 283,852 in every row, so the knob provably changes only the decision to collect, not the number of safepoints:ALLOC_KBRow 0 reproduces the pre-fix 1:1 ratio exactly on the shipped binary (283,857 collections for 283,852 polls). Every row keeps
copying_minors == forced_collectionsandmoved > 0, so no stride quietly degrades the instrument into non-moving sweeps.4 KB rather than the faster 16/64 is deliberate: this is a correctness instrument, so the default errs toward sensitivity — one collection per ~15 loop iterations, still 14x cheaper than unpaced.
The zeal-OFF path is untouched, which matters because back-edge polls are now on every allocating loop by default: the same workload without zeal is 4.49 s on
mainand 4.49 s here. The pacing work all sits inside the!GC_SAFEPOINT_PENDINGbranch, so a default build reaches the same single cached-bool read and return it did before.Note on a pre-existing failure
gc::tests::runtime_roots::generator_attach_prototypehas 3 tests failing in the debug profile, and they fail identically on pristinec156f8a41(1962 filtered there vs 1966 here — exactly the 4 tests this PR adds). They are alloc-point/trigger-arming tests, untouched by this change. Not introduced here; flagged rather than left unmentioned.Follow-up: arm 6 was weak until it was actually run
Two defects in my own gate, found by running it rather than reasoning about it:
The fixture did not allocate. The obvious version allocates a record per iteration and drops it, which scalar-replaces into nothing: 6 forced collections and 17 moved objects over 400,000 polls. The arm ran the loop and never gave the collector anything to relocate. Pushing into a bounded rolling array (plus a string concat) takes
moved_objectsfrom 17 to 640,364.A wall-clock budget could not be the discriminator. Measured on the quiet host, the fixture is 0.49 s paced vs 11.85 s unpaced — a real 24x, but both fit inside any budget loose enough not to flake on a shared CI runner. So the arm now keys on the collections-to-polls ratio, which is host-independent and exact: the regression's signature is 1:1 (measured 200,069 forced for 200,064 polls), the shipped default is 1-in-40, and the gate fails above 1-in-4. The budget stays as the weaker "terminates at all" guard.
Sabotage-checked, like the unit test: forcing arm 6 back to every-poll fails it with
The full
gc_instrument_smoke.shpasses end to end on the final build (all six arms, exit 0), including the pre-existing arms: 13/13 quarantine probes clean, and #7254's reproducer still pinned.Confirmed on the final HEAD build
The measurements above were taken on the first build of this branch; the later control-flow refactor was re-measured on a fresh
--releasebuild of the final HEAD and produced byte-identical counters (forced_collections=193087 copying_minors=193087 moved_objects=3115719 loop_polls=2838560 paced_polls=2645478), 101.5 s wall, correct answer — with the no-zeal arm at 5.17 s.