fix(gc): the pacing snapshot reports the boundary the predicate uses; the ZealGuard release becomes an assertion (#7729, #7733, #7735 review follow-ups) - #7739
Conversation
📝 WalkthroughWalkthroughMajor-GC pacing now uses a shared inclusive escalation-threshold calculation. Telemetry and tests cover disabled, overflow, and boundary cases. Zeal tests and documentation describe poll-arm restoration and collection-count behavior. ChangesGC pacing and zeal behavior
Estimated code review effort: 4 (Complex) | ~45 minutes 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 |
![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
🧹 Nitpick comments (1)
crates/perry-runtime/src/gc/policy.rs (1)
2747-2747: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider guarding the shift against out-of-range values.
1usize << shiftpanics on debug builds ifshift >= usize::BITS. Production keeps the backoff shift capped at 2, so this is not reachable today.major_pacing_escalation_threshold_forispub(super)and accepts anyu32, so a future caller or test could pass a larger shift.checked_shlreturnsNone, which matches the documented "no arena reading can reach this boundary" contract.♻️ Optional hardening
- let growth = growth_num.saturating_mul(1usize << shift); + let growth = growth_num.saturating_mul(1usize.checked_shl(shift)?);🤖 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` at line 2747, Guard the shift in the growth calculation within major_pacing_escalation_threshold_for by using checked_shl or equivalent handling for shift values at least usize::BITS. Preserve the documented no-arena-reading contract by returning the existing no-boundary result when the shift cannot be represented, while retaining current behavior for valid shifts.
🤖 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 23: Correct the parenthesis placement in
changelog.d/7729-gc-zeal-allocation-pacing.md lines 23-23 and
crates/perry-runtime/src/gc/zeal.rs lines 215-216: change “at (the outermost
microtask-pump boundary, which ...” to “at the outermost microtask-pump boundary
(which ...” in both documentation copies.
---
Nitpick comments:
In `@crates/perry-runtime/src/gc/policy.rs`:
- Line 2747: Guard the shift in the growth calculation within
major_pacing_escalation_threshold_for by using checked_shl or equivalent
handling for shift values at least usize::BITS. Preserve the documented
no-arena-reading contract by returning the existing no-boundary result when the
shift cannot be represented, while retaining current behavior for valid shifts.
🪄 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: dc7ecd40-03c3-404c-a57f-7a78dd6509bd
📒 Files selected for processing (6)
changelog.d/7729-gc-zeal-allocation-pacing.mdcrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/telemetry.rscrates/perry-runtime/src/gc/tests/triggers.rscrates/perry-runtime/src/gc/zeal.rsdocs/src/internals/memory-model.md
| | 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. | ||
| Row 0 reproduces the pre-fix behaviour on the shipped binary: 283,857 forced collections for 283,852 polls — one per back-edge poll, plus a handful from the other safepoint zeal forces at (the outermost microtask-pump boundary, which the poll counter does not count). Near 1:1, not exactly. 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.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the parenthesis placement in both documentation copies.
The explanatory clause should start after boundary, not before the.
changelog.d/7729-gc-zeal-allocation-pacing.md#L23-L23: changeat (the outermost microtask-pump boundary, which ...toat the outermost microtask-pump boundary (which ....crates/perry-runtime/src/gc/zeal.rs#L215-L216: apply the same wording correction.
📍 Affects 2 files
changelog.d/7729-gc-zeal-allocation-pacing.md#L23-L23(this comment)crates/perry-runtime/src/gc/zeal.rs#L215-L216
🤖 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 23, Correct the
parenthesis placement in changelog.d/7729-gc-zeal-allocation-pacing.md lines
23-23 and crates/perry-runtime/src/gc/zeal.rs lines 215-216: change “at (the
outermost microtask-pump boundary, which ...” to “at the outermost
microtask-pump boundary (which ...” in both documentation copies.
…thout diagnostics Claude-Session: https://claude.ai/code/session_015JgLM9UWGa6WAMix7CvhQJ
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 `@changelog.d/7739-gc-pacing-snapshot-boundary.md`:
- Around line 15-17: Update the changelog text describing the snapshot’s
null/None value so it consistently states that no escalation boundary is
available when either major pacing is disabled or the growth calculation
overflows usize. Replace the conflicting claim that null means only pacing is
disabled, while preserving the existing explanation of the checked arithmetic
and trace-key semantics.
🪄 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: c6394a8a-f424-4e4b-87b6-b12381f29aae
📒 Files selected for processing (2)
changelog.d/7739-gc-pacing-snapshot-boundary.mdcrates/perry-runtime/src/gc/policy.rs
| The fix is structural rather than a second correct formula. There is now **one** definition of the boundary, `major_pacing_escalation_threshold_bytes`: `arena_growth_full_escalation_due_inner` is literally `in_use >= it`, and the snapshot reports it verbatim, floor included. `None` means "no arena reading escalates" — either pacing is disabled (`PERRY_GC_MAJOR_PACING_FLOOR_MB=0`) or the growth term overflowed `usize`, which is the same statement about the world; the helper uses `checked_*` rather than `saturating_*` because saturating would report `usize::MAX` and then claim an arena of `usize::MAX` escalates, which the strict `>` clause never would. | ||
|
|
||
| The trace key follows the semantics: **`escalate_at_or_above_bytes`**, replacing `escalate_above_bytes`. The predicate's floor clause is a `>=`, and the old name was half of why the reported figure and the decision could disagree. `null` now means pacing is off. Nothing in `scripts/`, `.github/` or `docs/` consumed the old key. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Clarify the meaning of null.
Line 15 says None also represents a growth calculation that overflowed usize. Line 17 says null means only that pacing is disabled. These statements conflict. State that null means no escalation boundary is available because pacing is disabled or the calculation overflowed. (raw.githubusercontent.com)
Proposed wording
- The trace key follows the semantics: **`escalate_at_or_above_bytes`**, replacing `escalate_above_bytes`. The predicate's floor clause is a `>=`, and the old name was half of why the reported figure and the decision could disagree. `null` now means pacing is off. Nothing in `scripts/`, `.github/` or `docs/` consumed the old key.
+ The trace key follows the semantics: **`escalate_at_or_above_bytes`**, replacing `escalate_above_bytes`. The predicate's floor clause is a `>=`, and the old name was half of why the reported figure and the decision could disagree. `null` means no escalation boundary is available because pacing is disabled or the growth calculation overflowed `usize`. Nothing in `scripts/`, `.github/` or `docs/` consumed the old key.📝 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.
| The fix is structural rather than a second correct formula. There is now **one** definition of the boundary, `major_pacing_escalation_threshold_bytes`: `arena_growth_full_escalation_due_inner` is literally `in_use >= it`, and the snapshot reports it verbatim, floor included. `None` means "no arena reading escalates" — either pacing is disabled (`PERRY_GC_MAJOR_PACING_FLOOR_MB=0`) or the growth term overflowed `usize`, which is the same statement about the world; the helper uses `checked_*` rather than `saturating_*` because saturating would report `usize::MAX` and then claim an arena of `usize::MAX` escalates, which the strict `>` clause never would. | |
| The trace key follows the semantics: **`escalate_at_or_above_bytes`**, replacing `escalate_above_bytes`. The predicate's floor clause is a `>=`, and the old name was half of why the reported figure and the decision could disagree. `null` now means pacing is off. Nothing in `scripts/`, `.github/` or `docs/` consumed the old key. | |
| The fix is structural rather than a second correct formula. There is now **one** definition of the boundary, `major_pacing_escalation_threshold_bytes`: `arena_growth_full_escalation_due_inner` is literally `in_use >= it`, and the snapshot reports it verbatim, floor included. `None` means "no arena reading escalates" — either pacing is disabled (`PERRY_GC_MAJOR_PACING_FLOOR_MB=0`) or the growth term overflowed `usize`, which is the same statement about the world; the helper uses `checked_*` rather than `saturating_*` because saturating would report `usize::MAX` and then claim an arena of `usize::MAX` escalates, which the strict `>` clause never would. | |
| The trace key follows the semantics: **`escalate_at_or_above_bytes`**, replacing `escalate_above_bytes`. The predicate's floor clause is a `>=`, and the old name was half of why the reported figure and the decision could disagree. `null` means no escalation boundary is available because pacing is disabled or the growth calculation overflowed `usize`. Nothing in `scripts/`, `.github/` or `docs/` consumed the old key. |
🤖 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/7739-gc-pacing-snapshot-boundary.md` around lines 15 - 17, Update
the changelog text describing the snapshot’s null/None value so it consistently
states that no escalation boundary is available when either major pacing is
disabled or the growth calculation overflows usize. Replace the conflicting
claim that null means only pacing is disabled, while preserving the existing
explanation of the checked arithmetic and trace-key semantics.
…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
…thout diagnostics Claude-Session: https://claude.ai/code/session_015JgLM9UWGa6WAMix7CvhQJ
d76d87d to
2d20498
Compare
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 `@Cargo.toml`:
- Line 318: Revert the contributor-owned version metadata changes: restore
[workspace.package].version in Cargo.toml at lines 318-318 to 0.5.1432 and
Current Version in CLAUDE.md at lines 11-11 to 0.5.1432. Use the PR-keyed
changelog fragment for this change and do not update release metadata elsewhere.
🪄 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: f1537a40-7d70-42b3-b963-c64c4139adae
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
CLAUDE.mdCargo.toml
|
|
||
| [workspace.package] | ||
| version = "0.5.1432" | ||
| version = "0.5.1433" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Revert the contributor-owned version metadata changes.
Both files change the release version from 0.5.1432 to 0.5.1433, although this PR must not include a version bump.
Cargo.toml#L318-L318: restore[workspace.package].versionto0.5.1432.CLAUDE.md#L11-L11: restoreCurrent Versionto0.5.1432.
Use the PR-keyed changelog fragment for this change. The maintainer owns release metadata updates.
Based on learnings: contributors must not update release/version metadata themselves.
📍 Affects 2 files
Cargo.toml#L318-L318(this comment)CLAUDE.md#L11-L11
🤖 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 `@Cargo.toml` at line 318, Revert the contributor-owned version metadata
changes: restore [workspace.package].version in Cargo.toml at lines 318-318 to
0.5.1432 and Current Version in CLAUDE.md at lines 11-11 to 0.5.1432. Use the
PR-keyed changelog fragment for this change and do not update release metadata
elsewhere.
Sources: Coding guidelines, Learnings
Merging as v0.5.1433The snapshot bug is the important one, and the framing is rightA diagnostic that misreports the quantity it exists to prove is this repo's most expensive recurring failure —
The structural fix is what makes this landable rather than a second correct formula. There is now one definition — The And the doc comment names why the divergence went untested: every reachable unit test sat below the floor, where two different formulas agree on the I verified the sabotage, and got it wrong firstDropping the floor from the shared helper fails My first attempt appeared not to reproduce it, and that was my error: I filtered on The
|
1 participant
Review follow-ups from #7729 / #7733 / #7735 that were raised and landed anyway. All three PRs stay as merged; nothing here reverts or redoes them.
1.
major_pacing_snapshotunder-reported the escalation boundarycrates/perry-runtime/src/gc/policy.rs. The snapshot recomputed the boundary asbaseline × growthand dropped the floor on the floor of the function:...while the predicate it mirrors,
arena_growth_full_escalation_due, also rejects every reading below that floor. Wherever the floor dominates, the reported threshold named a boundary the collector does not use:baseline == 0)0— "escalates at any size"baseline = 4 MB, growth 2baseline = 64 MB, growth 2This matters more than its "minor" label because of why the snapshot exists: #7733 added it so the pacing subject could be asserted live in the GC trace, rather than a gate merely proving nothing threw. A diagnostic that misreports the quantity it exists to prove is this repo's most expensive recurring failure mode (
PERRY_GC_FORCE_EVACUATEinert for everygc()-driven test, the matrix's--pressureknob disabling the path it measured,moved=summing two collectors).The fix is structural rather than a second correct formula: there is now one definition of the boundary,
major_pacing_escalation_threshold_bytes. The predicate is literallyin_use >= it, and the snapshot reports it verbatim, floor included.Nonemeans "no arena reading escalates" — either pacing is disabled (PERRY_GC_MAJOR_PACING_FLOOR_MB=0) or the growth term overflowedusize, which is the same statement about the world, and is why the helper useschecked_*rather thansaturating_*(saturating would reportusize::MAXand then claim an arena ofusize::MAXescalates, which the>clause never would).The trace key follows the semantics:
escalate_at_or_above_bytes, notescalate_above_bytes— the predicate's floor clause is a>=, and the old name was half of why the figure and the decision could disagree. No consumer of the old key exists inscripts/,.github/ordocs/.Tests.
the_reported_escalation_boundary_is_the_one_the_predicate_decides_onchecks the named floor-dominates and growth-dominates cases plus baseline-zero, shift, and pacing-disabled, then goes exhaustive overfloor × growth × baseline × shift, probing each boundary's own ±1 neighbourhood against an oracle that is a deliberate independent transcription of the four clauses the predicate used to spell out inline (not a call into the code under test).the_shipped_predicate_and_the_shipped_snapshot_read_one_boundarythen drives the real predicate against the real snapshot on the live arena, so a future re-split fails even if the pure helper stays correct.Both are sabotage-checked, and the two sabotages are different on purpose:
major_pacing_snapshotonly (the actual historical bug) →the_shipped_predicate_and_the_shipped_snapshot_read_one_boundaryfails onbaseline = 0:escalate_at_or_above_bytes (Some(0))against a verdict offalseon a 0-byte arena. The matrix test passes, correctly — the pure helper was untouched.left: Some(8388609), right: Some(33554432).Restored after each; the final tree is byte-identical to the pushed commits.
2. The
ZealGuardrelease assertion was missingcrates/perry-runtime/src/gc/tests/triggers.rs.zeal_holds_the_poll_word_armed_with_nothing_pendingassertedPERRY_GC_POLL_ARMED > 0inside the guard scope and then only narrated the release in a comment. IfZealGuard'sDropever stopped giving the arm back, the process-global word would stay non-zero for the life of the test binary, every later test would silently take the poll's slow path, and this test would still pass.It now captures the baseline before the guard and asserts
base + 1inside,baseafter the drop — the same shapea_deferral_arms_the_poll_word_and_draining_disarms_italready uses a few lines up.Sabotage-checked: with
disarm_poll()commented out ofZealGuard::drop, the test fails (left: 2, right: 1) on the new post-drop assertion; restored, it passes.3. Wording
changelog.d/7729-gc-zeal-allocation-pacing.md— thebytes_allocated / stridebound is qualified to a positive stride, sincePERRY_GC_ZEAL_ALLOC_KB=0is a supported every-poll mode and deliberately outside it. And "reproduces the pre-fix 1:1 behaviour exactly" against a table showing 283,857 collections for 283,852 polls becomes the truth: one per back-edge poll, plus a handful from the other safepoint zeal forces at — the outermost microtask-pump boundary, which callsgc_safepoint_moving_minorwithoutnote_loop_poll_reached. Near 1:1, not exactly. The same claim ingc/zeal.rs'sZEAL_DEFAULT_STRIDE_BYTESdoc is corrected identically. (Fragments for merged PRs are folded into the release notes at tag time, so correcting them in place is what actually reaches a reader;changelog.d/README.mdhas no rule against it.)docs/src/internals/memory-model.md— line 135 already said polls became default-on in fix(gc): make the moving-loop poll default ON in the code, not just the doc (#7690, #7682) #7721; the caveat 20 lines below still said "default off since fix(gc): disable evacuating minor by default pending #7154 (use-after-free on dynamically-added fields) #7161". It now states the current default, its kill switch, the two gaps that survive it (alloc-free loop bodies by design, the specializedfor/for-of/for-inlowerings by omission), and the gc: zeal + compiled loop polls performs zero copying minors on compute-only benches — green instrument runs may be vacuous #7604 exit-70 verdict that means you no longer have to remember to check.Validation
cargo test --release -p perry-runtime→1977 passed; 0 failed, cargo's own exit code 0 (captured directly, not through a pipe). All three tests confirmed to have run, by name, not merely to have not failed.cargo fmt --all -- --checkclean;scripts/check_file_size.shOK.The trace, end to end.
gc-handoff/bench/retain.tscompiled withPERRY_NO_AUTO_OPTIMIZE=1against this branch'slibperry_runtime.a(PERRY_RUNTIME_DIRpinned at the freshly built archives), run underPERRY_GC_TRACE=1:The first two rows are the fix:
baseline = 0is the pre-first-full state, where the old snapshot printedescalate_above_bytes: 0. Then the escalated full rebaselines to 66 MB, reclaims little, and backs off to shift 1 — so the reported boundary becomes4xthe baseline, above the floor, and growth takes over. The block is present, non-vacuous, and each row is arithmetically consistent with the predicate.Canary:
gc-handoff/apps/iso_miss.tsprintschecksum 437840 misses 0.Not run locally:
scripts/gc_instrument_smoke.sh. Its arm 6 carries a wall-clock budget and this host is heavily loaded, so a local red would not have been informative; CI's runner is the right place for it.No version bump — maintainer bumps at merge.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests