test(gc): measure the grow-then-churn pacing transition and pin it as a ratchet probe (#7737 item 3) - #7783
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis change adds the fourteenth GC ratchet probe. The probe grows a retained object cache, performs array-allocation churn, verifies retained data, forces GC, and emits metrics. Documentation, preflight counts, changelog content, configuration tests, and version metadata now include the change. ChangesGC ratchet probe
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Probe as 14_grow_then_churn.ts
participant Cache as Retained cache
participant Ring as Churn ring
participant GC as Garbage collector
Probe->>Cache: Build and verify retained Row graph
Probe->>Ring: Allocate and recycle short-lived arrays
Probe->>Cache: Traverse cache and compute liveSum
Probe->>GC: Force collection
Probe->>Probe: Emit checksums and heap/RSS metrics
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
proggeramlug
force-pushed
the
gc/7737-grow-then-churn-probe
branch
from
August 10, 2026 15:51
8817352 to
a1fd5a7
Compare
proggeramlug
marked this pull request as ready for review
August 10, 2026 16:11
![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 `@benchmarks/gc_ratchet/README.md`:
- Line 84: Update the cadence statement in the README to scope the approximately
16 MB copying-minor claim only to the default configuration arm, rather than all
probes in the matrix. Keep the surrounding probe-count description intact and
ensure the wording does not include the documented 64 MB and 1 MB nursery arms.
🪄 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: d7cb3151-2422-4cd3-935a-2a9088de6f63
📒 Files selected for processing (6)
.github/workflows/gc-ratchet.ymlbenchmarks/gc_ratchet/README.mdbenchmarks/gc_ratchet/baseline/gc-ratchet-v1.jsonbenchmarks/gc_ratchet/probes/14_grow_then_churn.tschangelog.d/7783-grow-then-churn-probe.mdtests/test_gc_ratchet.py
| ## A probe may declare the collector it is a probe *of* (the large-Eden arm) | ||
|
|
||
| Twelve of the thirteen probes run the shipped configuration, so every copying | ||
| Twelve of the fourteen probes run the shipped configuration, so every copying |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Limit the cadence statement to the default arm.
The text says that every copying minor exercised by the matrix was approximately 16 MB. Lines 99-100 document 13_large_eden_survivors.ts with a 64 MB nursery, and 14_grow_then_churn.ts uses a 1 MB nursery. The current sentence contradicts the documented arms.
Proposed wording
-Twelve of the fourteen probes run the shipped configuration, so every copying
-minor this matrix had ever exercised was small (~16 MB) and frequent.
+Twelve of the fourteen probes run the shipped configuration (~16 MB). The
+explicit 64 MB and 1 MB arms exercise separate copying-minor cadences.🤖 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 `@benchmarks/gc_ratchet/README.md` at line 84, Update the cadence statement in
the README to scope the approximately 16 MB copying-minor claim only to the
default configuration arm, rather than all probes in the matrix. Keep the
surrounding probe-count description intact and ensure the wording does not
include the documented 64 MB and 1 MB nursery arms.
added 3 commits
August 10, 2026 19:52
… a ratchet probe (#7737 item 3) The major-pacing backoff (#7726) had two well-covered ends and an untested middle. retain.ts-shaped growth is pure, so the shift climbs to its cap and stays; tree.ts-shaped churn is always high-yield, so the shift never leaves 0. Nothing exercised a workload that is the first and then becomes the second — the shape a warm-cache phase followed by steady churn actually has. 14_grow_then_churn.ts is that workload: an all-live 300k-row cache that walks the shift 0 -> 1 -> 2 on growth alone, then push-grown array churn with the cache still live so the pacing baseline stays up. #7737 reasoned the delayed reclaim would cost "up to 4x more transient RSS". A same-binary A/B — one runtime at MAJOR_PACING_BACKOFF_SHIFT_MAX = 2, one at 0, archives rebuilt and mtime-verified, stdout byte-identical — measured the boundary moving 56.3 MB vs 14.1 MB (the predicted 4x) while peak RSS went the other way: 70.5/70.2/70.5 MB shipped against 72.6/72.6/72.6 MB without the backoff, for two fewer full mark-sweeps and identical collector work (24 copying minors, 306,715 objects moved in both). Footprint on this shape is set by the steady state the minors hold, not by cumulative stub debt, so the escalation boundary is never the binding constraint. So the probe pins the measured state rather than asserting the predicted bound. The two gc-ratchet-env directives that set its absolute scale are asserted by a new test, the same discipline 13_large_eden_survivors gets.
…ix cells that moved Control run twice on the pinned host before pinning: the same six cells regressed in both (04_dead_after_deep_stack.copied_objects, and 11_collect_at_depth heap_used/copied/promoted, whose survivors stopped being promoted on first copy). A release binary built from a8f7312 - the commit the previous artifact was pinned at - reproduces that artifact with ZERO regression cells, and both binaries are deterministic 6-of-6 within themselves, so the shift is code and not drift. 78 commits separate the pins, twelve in the collector; gc-ratchet is advisory-only (#7737 item 4), so none of them had to re-pin. Named in the artifact notes rather than absorbed.
proggeramlug
force-pushed
the
gc/7737-grow-then-churn-probe
branch
from
August 10, 2026 18:03
8fb4944 to
a88e0d7
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 both release metadata edits: set [workspace.package].version
in Cargo.toml (318-318) back to 0.5.1456 and set Current Version in CLAUDE.md
(11-11) back to 0.5.1456; do not modify other files.
🪄 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: 8fec8838-2756-4ca7-b4fa-d977eb52fce2
⛔ 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.1456" | ||
| version = "0.5.1457" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Revert both release metadata edits.
This contributor PR must not update release metadata. The maintainer owns the version bump and must update both files together during merge or release.
Cargo.toml#L318-L318: revert[workspace.package].versionto0.5.1456.CLAUDE.md#L11-L11: revertCurrent Versionto0.5.1456.
📍 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 both release metadata edits: set
[workspace.package].version in Cargo.toml (318-318) back to 0.5.1456 and set
Current Version in CLAUDE.md (11-11) back to 0.5.1456; do not modify other
files.
Source: Learnings
Contributor
Author
|
Audit complete — merging. Verified:
This closes #7737 item 3 — the last engineering item. Item 4 (promoting gc-ratchet/gc-stress to required contexts) remains the maintainer/admin action already tracked there. |
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 the last engineering item of #7737. Item 3 is what this PR does; items 1 and 2 are already fixed on
main(theOBJECT_PROTOTYPES_NONEMPTYlatch releases when a prune drains the registry;gc/tests/triggers.rsasserts the positive direction of the pacing recording). Item 4 — promotinggc-ratchet/gc-stressto required contexts — is a maintainer action and is untouched here.The untested middle
The major-pacing backoff (#7726) has two well-covered ends.
retain.ts-shaped growth is pure, so every escalated full reclaims almost nothing and the shift climbs to its cap of 2 and stays.tree.ts-shaped churn is always high-yield, so the shift never leaves 0. Nothing exercised a workload that is the first and then becomes the second — which is the shape a warm-cache phase followed by steady churn actually has.14_grow_then_churn.tsis that workload: an all-live 300k-row cache that walks the shift 0 → 1 → 2 on growth alone, then push-grown array churn with the cache still live so the pacing baseline stays up.What it measured — not what the issue predicted
#7737 reasoned the delayed reclaim would cost "up to 4× more transient RSS", since array-growth forwarding stubs are reclaimable only by a full mark-sweep. A same-binary A/B (one runtime at
MAJOR_PACING_BACKOFF_SHIFT_MAX = 2, one at0, archives rebuilt and mtime-verified between arms, stdout byte-identical):The boundary moves 4×, exactly as reasoned. Peak RSS does not follow it, and does not move in the predicted direction — the shipped backoff is 2.2 MB (3%) lower, reproducibly, while running two fewer fulls for identical collector work. Footprint on this shape is set by the steady state the minors hold, not by cumulative stub debt, so the escalation boundary is never the binding constraint.
So the probe pins the measured state rather than asserting the predicted bound. A future change that makes the delayed reclaim start to bind moves
rss_bytesoff the pin and the ratchet reports it.Provenance: M1 Max laptop,
perry-dev, peak RSS from plain runs with no GC tracing (tracing perturbs RSS, so counters and footprint come from separate runs). Not the pinned mini that owns the artifact — the comparison transfers, the absolute bytes belong to that context.The probe reaches its subject
The failure this suite has paid for most often is a knob that moves the workload off the path it was chosen to exercise, so this was checked rather than assumed:
PERRY_GC_TRACE=1reportsbackoff_shiftreaching 2 with the boundary at 56.3 MB, and the harness recordsminor_cycles=24with 306,715 objects moved, satisfyingcheck's liveness rule with a collector that demonstrably ran.The two
gc-ratchet-envdirectives (PERRY_GC_SCAVENGE_NURSERY_MB=1,PERRY_GC_MAJOR_PACING_FLOOR_MB=1) set the absolute scale of a mechanism that is otherwise a ratio — at the shipped 16 MB / 32 MB the same three escalations need a live set in the hundreds of MB. A new test asserts both, the same discipline13_large_eden_survivorsalready gets: losing either line leaves a probe that still passes, still collects, and no longer reaches the cap it exists to hold.It is also the most stable probe in the suite — 0% spread on both retention metrics, 0.022% on RSS, against 4.7% and 5.4% for
12_large_live_setand13_large_eden_survivors.The re-pin is not a clean carry-over, and says so
Adding a probe changes the probe set, so the artifact must be re-pinned. The control was not green, and rather than absorbing that, the artifact notes name every cell:
04_dead_after_deep_stack11_collect_at_depth11_collect_at_depth11_collect_at_depthProbe 11's survivors stopped being promoted on first copy and are now copied into survivor space — the #7558 shape, running the other way.
Attribution, so it is recorded as code and not drift. The control ran twice and reported the same six cells both times. A release binary was built from
a8f73122d, the commit the previous artifact was pinned at, and--checkfrom that tree against that artifact reported zero regression cells. Running one probe directly under each binary is deterministic 6-of-6 within each arm and differs between them. Two further cells (13_large_eden_survivorsRSS, +7.2%) fired in the first control and not the second, so those are host variability on a sampled metric and are not claimed as a shift.#7742 is excluded as the cause, by its own PR's evidence. #7744 measured
gc_ratchetagainst this same artifact before it merged and reported: "the six cells it reports as REGRESSION are reported identically byorigin/mainitself — pre-existing drift against the pinned artifact." The same six. So the flip predates the whole-block promotion change and comes from one of the earlier collector commits — #7733 yield-adaptive major pacing, #7690 / #7682 evacuation at precise safepoints, #7721 / #7735 the loop back-edge poll, #7317 the seeded schedule, or the #7469 thread-local work.Why it went unrecorded: 78 commits separate the two pins, twelve in the collector, and
gc-ratchetis not a required context — #7737 item 4 — so none of them ever had to re-pin. It has now been observed by at least two people without being recorded, because an advisory gate cannot force the question.It is pinned so the next change has a truthful baseline. If the promote-to-copy flip on
11_collect_at_depthwas not intended by whichever change caused it, then this artifact is the wrong fix and the code is the right one — that call is the maintainer's, and the notes say so in the artifact itself.Validation
run_gc_ratchet_baseline.sh --checkagainst the new artifact on the pinned host, clean tree, all 14 probes: 0 regression cells,gc-ratchet: OK, exit 0.correctness=pass).tests/test_gc_ratchet.py: 92 passed, including the new directive assertion.scripts/check_file_size.sh: exit 0.PERRY_NO_AUTO_OPTIMIZE=1, runtime dir pinned. This branch changes no Rust at all — a probe.ts, the README, a workflow comment, a Python test and the artifact — so the release binary is the right one for this tree.Summary by CodeRabbit
New Features
Documentation
Tests
Chores