test(gc-ratchet): pin the large-Eden copying-minor cadence (#7481) and re-derive the #7056 RSS numbers under statepoints - #7666
Conversation
📝 WalkthroughWalkthroughThe GC ratchet now supports per-probe runtime environments and adds a 64 MB Eden survivor probe. The harness applies and records environment arms during measurement and classification, validates baseline consistency, reports non-default configurations, and documents the updated probe suite. ChangesGC ratchet environment arms
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Probe
participant GC_Ratchet
participant Runtime
Probe->>GC_Ratchet: Declare gc-ratchet-env values
GC_Ratchet->>Runtime: Apply environment for measurement
GC_Ratchet->>Runtime: Apply environment for classification
Runtime-->>GC_Ratchet: Return metrics and correctness results
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 `@benchmarks/gc_ratchet/gc_ratchet.py`:
- Around line 1312-1316: Require the artifact entry to contain a run_env key
before validating its value in the recorded_env validation flow. Remove the
default empty mapping fallback from entry.get, reject missing fields with
artifact_defect, preserve the existing string-to-string mapping validation, and
add a regression test covering an absent run_env field.
🪄 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: 513617db-2f43-4828-9290-fdd63bfce464
📒 Files selected for processing (6)
.github/workflows/gc-ratchet.ymlbenchmarks/gc_ratchet/README.mdbenchmarks/gc_ratchet/gc_ratchet.pybenchmarks/gc_ratchet/probes/13_large_eden_survivors.tsdocs/engine-plan.mdtests/test_gc_ratchet.py
| recorded_env = entry.get("run_env", {}) | ||
| if not isinstance(recorded_env, Mapping) or not all( | ||
| isinstance(key, str) and isinstance(value, str) for key, value in recorded_env.items() | ||
| ): | ||
| artifact_defect(f"{name}: run_env is not a string-to-string mapping") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject a missing run_env field.
Line 1312 converts an absent field into {}. A baseline that loses run_env then passes artifact validation as a default-arm probe. Require the key to exist before validating its mapping type. Add a regression test for a missing field.
Proposed fix
- recorded_env = entry.get("run_env", {})
- if not isinstance(recorded_env, Mapping) or not all(
+ if "run_env" not in entry:
+ artifact_defect(f"{name}: run_env is missing")
+ continue
+ recorded_env = entry["run_env"]
+ if not isinstance(recorded_env, Mapping) or not all(
isinstance(key, str) and isinstance(value, str) for key, value in recorded_env.items()
):
artifact_defect(f"{name}: run_env is not a string-to-string mapping")📝 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.
| recorded_env = entry.get("run_env", {}) | |
| if not isinstance(recorded_env, Mapping) or not all( | |
| isinstance(key, str) and isinstance(value, str) for key, value in recorded_env.items() | |
| ): | |
| artifact_defect(f"{name}: run_env is not a string-to-string mapping") | |
| if "run_env" not in entry: | |
| artifact_defect(f"{name}: run_env is missing") | |
| continue | |
| recorded_env = entry["run_env"] | |
| if not isinstance(recorded_env, Mapping) or not all( | |
| isinstance(key, str) and isinstance(value, str) for key, value in recorded_env.items() | |
| ): | |
| artifact_defect(f"{name}: run_env is not a string-to-string mapping") |
🤖 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/gc_ratchet.py` around lines 1312 - 1316, Require the
artifact entry to contain a run_env key before validating its value in the
recorded_env validation flow. Remove the default empty mapping fallback from
entry.get, reject missing fields with artifact_defect, preserve the existing
string-to-string mapping validation, and add a regression test covering an
absent run_env field.
…d re-derive the #7056 RSS numbers under statepoints Two halves of the same job: they share the pinned quiet host and the same harness. #7481 named coverage this matrix did not have — "a live copying-minor correctness signal at exactly the cadence the ratchet probes never exercise". All twelve probes ran the shipped 16 MB nursery cap, so every copying minor they had ever exercised was small and frequent. `13_large_eden_survivors` closes that, via a per-probe `// gc-ratchet-env:` declaration that `check` compares like a metric: delete the directive and every band is still satisfied, so the arm itself has to be gated or it is not an arm. The finding that shaped the probe: a large Eden on a *small* retained set runs ZERO copying minors, because `arena_growth_full_escalation_due` escalates every minor to a full mark-sweep once arena in-use clears 32 MB and exceeds twice the post-full baseline. The first draft did exactly that and would have been pinned on a collector it never reached. #7056's RSS numbers were taken under the shadow stack, which stopped being the default in #7370. Re-derived as a 2x2 (root lowering x nursery cap) over 12 probes, 7 repeats, 3 interleaved rotations: the root lowering is not an RSS lever (peak RSS 1.002x, retention 1.000x, 104 of 108 deterministic cells bit-identical), and the nursery cap still is (1.911x peak RSS at 128 MB). Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
… pinned quiet host Thirteen probes at a8f7312, `perry-macos`, driver-gated (clean tree, AC power, CPU-active <= 25% for 60 s before each phase), node oracle v26.5.1. CONTROL, taken with the same binary before the pin: `--check` against the previous artifact (59d5220, #7657) reported every one of its 144 cells `ok` and failed on exactly one line — "probes present now but absent from the baseline: 13_large_eden_survivors". So nothing here is drift; the only new rows are the new probe's, and `wt-scavtenure`'s re-pin is subsumed. Every deterministic cell in the new artifact has spread 0, including all twelve of the new probe's; peak RSS spread 0.169%, wall 4.69% against a 10% band (the suite's widest, and its median reproduces to +0.5% on an independent session). Also records the measurement-context trap this found: a probe compiled with a `package.json` in scope retains one more 1 MiB arena block at `gc()`, which makes an ad-hoc `measure --probes-dir <copy outside the repo>` report 09_try_catch_roots at -17.98% against the pin. Not a collector change; the driver and CI always compile from the repo. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
…ared arm layers over the shell Cosmetic move of PROBE_ENV_RE/RESERVED_PROBE_ENV below the diag-parsing regexes they were splitting, plus a docstring line stating the layering an ad-hoc knob sweep depends on: os.environ applies to every probe that does not declare the knob, and the declaring probe wins for itself. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
…approximately "~16 MB per minor for every default-cap probe" understated 12_large_live_set, which runs 21.8 MB because its tenured-proportional cap term already raises its Eden without any knob. The real spread is 14.6-16.6 MB on eleven of twelve and 21.8 on the twelfth, against 49.7 MB per minor for the new probe -- and the 12_large_live_set row is worth naming, because it is the shipped path to a larger Eden and shows where that path tops out. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Its inline sizing note carried figures from a pre-final tuning run (3 minors, 36/36/68 MB) and said the survivor reads fold into the checksum when they fold into five separately-diffed lines. A stale number in the comment beside the constant it justifies is exactly the shape this campaign keeps paying for. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Audit — merging as v0.5.1379. Plan items 9 and 10 both close.Item 9's answer is a negative result, and that is the useful kindThe root lowering is not an RSS lever — statepoint vs shadow, peak RSS 1.002, retention 1.000, 104 of 108 deterministic cells bit-identical. #7056 was written under the shadow stack and the fear was that #7370 invalidated its numbers; the measurement says the lowering was never the variable. Proving the arms actually differ per probe before comparing them — The section-by-section disposition is better than a headline: §9's own recommendation already shipped (#7377), §7's 4.6×–54.8× false-retention table is gone rather than shrunk (#7657 removed the cause — excess 0.00% on all twelve), §6 survives with Item 10: the probe nearly measured nothing, and you caught it
Shown able to fail, three ways, and I re-verified the one I could locally: deleting the The detail that justifies the whole gate: with the directive gone, retention/RSS/wall all read as improvements (−80%, −37%, −27%) because those bands are one-sided. A probe that gets faster and smaller by not doing its job is indistinguishable from a win. Keeping the inert perturbation in the writeup is the right call. The
|
46da3c6 to
f58a441
Compare
…w-up) (#7668) * fix(gc-ratchet): record a rebase-stable provenance field The commit a pin records is orphaned by the rebase-at-merge that ships it: #7666's artifact names a8f7312, which is not an ancestor of main. Recurs on every pin. crates/'s tree hash identifies the measured code and survives both the rebase and the version bump. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix * chore: bump version to 0.5.1380 Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Closes plan items 9 and 10 of
docs/engine-plan.md. They share the pinned quiet host and the same harness, so they are one PR.Provenance for every number below:
perry-macos(Mac mini, 8-core M1, 8 GB, macOS 26.5.1). Binary built at7bde3de24withcargo build --release -p perry -p perry-runtime-static -p perry-stdlib-staticinto an isolatedCARGO_TARGET_DIR; the.amtimes were confirmed to move after that build, and this PR changes zero.rsfiles (git diff --name-only 7bde3de24 <head> | grep -c '\.rs$'= 0), so it is the right binary for this tree.PERRY_RUNTIME_DIRpinned,PERRY_NO_AUTO_OPTIMIZE=1, node oraclev26.5.1. Host quiet was re-checked before every arm and every phase: 94.6–95.8% idle, zerorustc, zerocargothroughout. No other agent was building on the box.Item 10 — the large-Eden probe arm
#7481 named the gap itself: "a live copying-minor correctness signal at exactly the cadence the ratchet probes never exercise". All twelve probes ran the shipped 16 MB nursery cap, so every copying minor this matrix had ever exercised was small (~16 MB of Eden) and frequent.
A probe may now declare the collector it is a probe of
// gc-ratchet-env: PERRY_GC_SCAVENGE_NURSERY_MB=64It lives in the probe source because it is not possible to read the workload without reading the arm. It reaches every run of that probe — warmup, the timed repeats, both traced runs, and both of
classify's scan modes — and deliberately notcompile_probe(these areOnceLockruntime knobs, and Perry's object cache keys on every codegen env var, #6394, so a compile-time pass would move the cache key without moving a byte of code). OnlyPERRY_*; neverPERRY_CONSERVATIVE_STACK_SCANorPERRY_GC_DIAG, which the harness owns; never a repeated key.The arm is pinned in the artifact and compared like a metric, before any band arithmetic. That is the check that matters: delete the directive and every band is still satisfied, because the numbers would have been re-pinned by the same act that lost the arm.
The finding that shaped the probe
A large Eden on a small retained set runs zero copying minors.
gc/policy.rs'sarena_growth_full_escalation_dueescalates a minor to a full mark-sweep once arena in-use clears 32 MB and exceeds twice the post-full baseline; a 64 MB Eden over a ~1 MB live set satisfies both, every time. The first draft of this probe did exactly that — 0 copying minors, 4 full sweeps — and would have been pinned on a collector it never reached, the #7024 shape inside the arm added to close it.So
PERRY_GC_SCAVENGE_NURSERY_MBis not on its own a "larger Eden" knob: above ~32 MB it is a "no copying minor" knob unless the workload also holds a live set. Sizing the retained set is the fix:KEEPAt the shipped size the four minors free 37, 36, 68 and 68 MB — 49.7 MB per minor — and the first copies 532,482 objects (32 MB) in one cycle. The rest of the suite runs 14.6–16.6 MB per minor on eleven of twelve, and 21.8 MB on
12_large_live_set, whose tenured-proportional cap term (max(influx x scale, tenured/2)) is the shipped path to a larger Eden and tops out around 22 MB even on the biggest workload the suite has. Traced counters are byte-identical across traced runs, and probe stdout is byte-identical to Node 26.5.1.Shown able to fail
Three perturbations, each measured with the real harness and scored by
checkagainst the freshly pinned artifact:checkverdictgc-ratchet: OKgc-ratchet-envdirective deletedFAILEDminor_cycles4 -> 12,step_cycles5 -> 13,copied_objects532,482 -> 286,246,copied_bytes-45.56%PERRY_GC_SCAVENGE=0FAILEDminor_cycles4 -> 0 andcopied + promoted1,074,380 -> 0 — plus 11 REGRESSION rows including retention +3135%PERRY_GEN_GC_EVACUATE=0OKThe directive-deleted row is the one that justifies gating the arm at all: with the directive gone, retention -80.27%, peak RSS -37.33% and wall -27.07% all read as improvements, because those bands are one-sided. Only the arm check and the two-sided evacuation counters say what actually happened.
The
PERRY_GEN_GC_EVACUATE=0row is kept because it is the lesson rather than a footnote. That knob does not gate the copying minor — 4 copying minors with it set, 4 without; it gates the C4b old-gen policy evacuation andgc_force_evacuate_enabled. A green gate under a knob whose name promises otherwise reads as "this gate cannot fail". The correct reading was "nothing was perturbed", and the discriminator was counting copying minors before trusting the verdict, not re-reading the harness.Plus unit-level, in
tests/test_gc_ratchet.py: the arm reaching the child is asserted by a stub probe that reports whether the variable arrived (arun_envrecorded but never exported would be an arm that is documented, gated and inert); dropping, changing or adding an arm each failscheck; an unreadablerun_envis a fatal artifact defect; the shipped probe is asserted to still carry its directive, so deleting it turns a unit test red rather than silently retargeting the probe.wt-scavtenureSubsumed. Measured, not assumed. #7432 is merged, its worktree exists on neither host, and the baseline has been re-pinned five times since — most recently in full by #7657 at
59d522052on this same host, which also closed #7652's mixed provenance. The quiet-host driver's--checkat this PR's first commit, against that artifact: every one of its 144 pinned cellsok, failing on exactly one line —probes present now but absent from the baseline: 13_large_eden_survivors, which is the friction adding a probe is supposed to cost. There is no pending re-pin.★ One trap, found the hard way, and it is why this section says 144/144 and not something else. An earlier ad-hoc
measure --probes-dir <copy>reported09_try_catch_roots.heap_used_bytesat −17.98%, which reads exactly like drift since59d522052. It is not drift. A probe compiled with apackage.jsonin scope retains one more 1 MiB arena block atgc()than the same source compiled without one, and 09 sits on that boundary. Reproduced three ways, each stable 3/3: repo directory 5,825,256; three separate/tmpdirectories 4,777,624;/tmpplus a copiedpackage.json5,825,256. Ruled out as build non-determinism in the same session — two builds of one probe from one directory differ byte-for-byte and report the identical number. The driver and CI always compile frombenchmarks/gc_ratchet/probes, so the gate is self-consistent, and ratios between arms measured in the same directory are unaffected, which is what leaves item 9's 2x2 standing.README.mdnow carries the warning.Item 9 — the #7056 RSS numbers, re-derived under the statepoint default
#7056's numbers were taken under the shadow-stack root lowering. Statepoints became the default in #7370, so the question is whether that invalidates them. 2x2 over root lowering x nursery cap, 12 probes, 7 repeats, 3 interleaved rotations (rotation i runs every arm before i+1, so drift perturbs all four equally), every probe byte-identical to the pinned Node oracle in all 72 probe-runs.
The root lowering is not an RSS lever
PERRY_RS4GC=0)gc()104 of 108 deterministic cells are bit-identical. All four that move are on
10_store_receiver_across_alloc, all ≤0.31%: the shadow stack keeps 7 more objects live at that collection point than the statepoint map does (copied_objects8,056 → 8,063).Between-rotation spread: retention 0.000%, peak RSS max 0.621% (median 0.000%), wall max 2.779% (median 0.449%). Every claim above clears its spread by an order of magnitude or is explicitly a null result.
The arms were shown to differ before they were compared — per probe, not per suite.
--trace llvmcensus over all 12:statepoint-example+addrspace(1)root allocas present on 12/12 under the default (168–514addrspace(1)sites each) and 0 of both underPERRY_RS4GC=0, wherejs_shadow_frame_enterrises to match. This is #7056's own poll-site-count discipline: differing bytes are not a differing configuration.What actually invalidated parts of #7056, and it is not #7370
gc_moving_loop_polls_enabled(), and the poll has been default-off since fix(gc): disable evacuating minor by default pending #7154 (use-after-free on dynamically-added fields) #7161. The five arms gc(measurement): poll-time moving buys ~nothing the 16 MB nursery cap does not already buy — delete the loop back-edge poll, do not build pin-based poll moving #7056 tabulates (on/off/off_rt/on_cap128/off_scav) no longer name anything that ships.PERRY_GC_SCAVENGE_NURSERY_MB=128: peak RSS 1.911x, retained heap 2.475x, wall 0.947x — up to 5.005x peak RSS on04_dead_after_deep_stackand 6.350x retention on07_array_grow_evacuate. Identical to within 0.6% under both root lowerings, so it is a pacing result and not a rooting one.classifyat7bde3de24reports excess 0.00% and spread 0 on all twelve, with nomanual_collectscan site anywhere — fix(gc): explicit gc() runs on precise roots — remove the forced conservative stack scan (#7558) #7657 removed the forced scan atgc()rather than the reading.PERRY_CONSERVATIVE_STACK_SCAN=fullunder statepoints takesminor_cyclesto 0 on all twelve probes (copied + promoted= 0 on all twelve), for 1.98x retained heap (0.70x–6.35x) and 1.46x peak RSS (0.93x–3.47x). Smaller than gc(measurement): poll-time moving buys ~nothing the 16 MB nursery cap does not already buy — delete the loop back-edge poll, do not build pin-based poll moving #7056's +364%..+5371%, same sign, same mechanism.benchmarks/json_polyglot/bench_field_access.tsunderPERRY_GC_SCAVENGE_NURSERY_MB=64) exits 0 with an identical checksum on 5/5 runs at7bde3de24, and does run 6 copying minors there, so the arm is not vacuous. The probe added here is what keeps that cadence covered rather than the issue's specific fault.Not re-derived, stated rather than hidden
#7056's largest RSS numbers (§3–§5, 272 MB → 421 MB on
w1_srv_pump/w2_srv_pump_while/w3_mono_sync) came from three server-shaped workloads it wrote and never landed. There is nothing in the tree to re-run and a fresh reimplementation would not be comparable to them. The 12 probes are what replaced them.Also not measured: probe 13's counters cross-host. They are deterministic across repeats, traced runs and rotations on this host, and nothing they depend on is host-derived above 2 GB of RAM (
gc_heap_budget_bytesreturnsNonefor any budget ≥ 1 GB, so both this host and a GitHub runner take the historical constant path) — but theshared_ciprofile gates them and that has not been demonstrated on a second machine.Verification
All green on this branch.
lint-job commands, enumerated from.github/workflows/test.ymlrather than by hand (fewer than 5 would mean the extraction failed): 22 ran, 0 failed, each command's own$?read directly and the runner exiting non-zero if fewer than 22 executed.rustup run stable cargo fmt --all -- --check— clean (it is the first of the 22).python3 -m unittest tests.test_gc_ratchet— 89 tests, OK (17 new).python3 benchmarks/gc_ratchet/gc_ratchet.py validate --scope all— structurally valid.python3 scripts/gc_gate_wiring_check.py(+--self-test) — clean.cargo check --all-targets— exit 0, 0errorlines.cargo test -p perry-runtime --lib --no-fail-fast— 1,917 passed, 0 failed, exit 0. (Both run on the pinned host against this branch; this PR changes no Rust, so they are a no-regression check rather than a subject.)--checkbefore the pin (control),--pinafter,validateafter that.Host contention: none.
rustcandcargocounts were checked at 0 before every arm and every phase, CPU idle stayed in 94.6–95.8% for the whole session, and no other agent was building on the box. One correction to the brief's quiet gate: 1-minute load average on this mini idles at 1.3–1.8, above the "< 1.5" bar, because aWallpaperAerialsExtensionscreensaver holds ~14% of one core continuously.rustc == 0 && cargo == 0 && CPU idle >= 90%is the predicate that actually discriminates here, and it is what the repo's own driver already gates on (cpu_active <= 25%).