test(gc-ratchet): classify a retention breach instead of guessing at it (#7559) - #7571
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe GC ratchet adds a ChangesGC ratchet classification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant cmd_classify
participant classify
participant PerryProbe
participant parse_scan_fallbacks
cmd_classify->>classify: request probe classification
classify->>PerryProbe: run with conservative scanning
PerryProbe-->>classify: return metrics, output, and stderr
classify->>PerryProbe: run with scanning disabled
PerryProbe-->>classify: return precise metrics and output
classify->>parse_scan_fallbacks: parse scan diagnostics
parse_scan_fallbacks-->>classify: return scan-site activity
classify-->>cmd_classify: render classification report
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
proggeramlug
pushed a commit
that referenced
this pull request
Aug 7, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 719-720: Remove the unnecessary f-string prefix from the string
literal on the line describing the conservative native-stack scan, leaving its
text and adjacent formatted string unchanged.
- Around line 645-648: Update the benchmark loop around run_once so the
conservative arm explicitly removes SCAN_MODE_ENV from the inherited environment
instead of passing None, while retaining the "off" override for the precise arm.
Apply the same cleanup to diagnostic runs, and add coverage that sets the parent
SCAN_MODE_ENV to "off" and verifies conservative execution still uses the
default scan mode.
- Around line 677-679: Update the diagnostic probe in the surrounding ratchet
flow to retain the result from run_once, check its returncode before calling
parse_scan_fallbacks, and raise RatchetError when the PERRY_GC_DIAG run fails;
add a fixture that exits nonzero only when PERRY_GC_DIAG is set and cover this
failure path.
In `@benchmarks/gc_ratchet/README.md`:
- Around line 365-372: Update the execution-count description in the README to
state that each probe runs in both conservative-scan modes using the configured
repeat count, and mention the additional diagnostic run. Remove the claim that
every probe runs exactly twice while preserving the existing output and
reporting behavior.
In `@changelog.d/7571-gc-ratchet-classify-retention.md`:
- Around line 67-71: Update the changelog note in the _readme section to correct
the test-count claim: the change adds nine related test methods, not six, so
replace the inaccurate count with the correct total or remove the count
entirely. Keep the surrounding description about the end-to-end classify run and
the PERRY_CONSERVATIVE_STACK_SCAN contract test 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: ccdc6806-f35e-41e4-bb85-18f9cd6c09fe
📒 Files selected for processing (6)
benchmarks/gc_ratchet/README.mdbenchmarks/gc_ratchet/baseline/gc-ratchet-v1.jsonbenchmarks/gc_ratchet/gc_ratchet.pybenchmarks/gc_ratchet/tolerances.jsonchangelog.d/7571-gc-ratchet-classify-retention.mdtests/test_gc_ratchet.py
Comment on lines
+645
to
+648
| for label, env in (("conservative", None), ("precise", {SCAN_MODE_ENV: "off"})): | ||
| seen: list[dict[str, int]] = [] | ||
| for _ in range(repeats): | ||
| run = run_once([str(binary)], extra_env=env) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clear the inherited scan-mode override for the conservative arm.
Line 645 passes no environment override for conservative. run_once copies os.environ, so a caller with PERRY_CONSERVATIVE_STACK_SCAN=off makes both arms precise. The report then shows zero residue and does not perform the required comparison.
Explicitly remove SCAN_MODE_ENV for conservative and diagnostic runs. Add a test that sets the parent environment variable to off.
🤖 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 645 - 648, Update the
benchmark loop around run_once so the conservative arm explicitly removes
SCAN_MODE_ENV from the inherited environment instead of passing None, while
retaining the "off" override for the precise arm. Apply the same cleanup to
diagnostic runs, and add coverage that sets the parent SCAN_MODE_ENV to "off"
and verifies conservative execution still uses the default scan mode.
Comment on lines
+677
to
+679
| sites = parse_scan_fallbacks( | ||
| run_once([str(binary)], extra_env={"PERRY_GC_DIAG": "1"})["stderr"] | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject a failed diagnostic run.
The PERRY_GC_DIAG=1 run is used as the source of scan_fallback_sites, but its exit status is ignored. If diagnostics cause the probe to fail, parse_scan_fallbacks can produce an empty census and the classification still succeeds.
Check returncode before parsing stderr and raise RatchetError on failure. Add a fixture that exits nonzero only when PERRY_GC_DIAG is set.
🤖 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 677 - 679, Update the
diagnostic probe in the surrounding ratchet flow to retain the result from
run_once, check its returncode before calling parse_scan_fallbacks, and raise
RatchetError when the PERRY_GC_DIAG run fails; add a fixture that exits nonzero
only when PERRY_GC_DIAG is set and cover this failure path.
Comment on lines
+719
to
+720
| f"own `gc()`, which forces the conservative native-stack scan. `precise` is the", | ||
| f"same reading with `{SCAN_MODE_ENV}=off`, i.e. the retention the", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unused f-string prefix.
Line 719 has no replacement field. Ruff reports F541 for this changed line.
🧰 Tools
🪛 Ruff (0.16.1)
[error] 719-719: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 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 719 - 720, Remove the
unnecessary f-string prefix from the string literal on the line describing the
conservative native-stack scan, leaving its text and adjacent formatted string
unchanged.
Source: Linters/SAST tools
Comment on lines
+365
to
+372
| It runs every probe twice — once as the gate does, once with | ||
| `PERRY_CONSERVATIVE_STACK_SCAN=off` — and prints the split, plus the census of | ||
| which conservative-scan sites actually fired. It refuses to tabulate a probe | ||
| whose *output* changes when the scan is disabled (the scan was load-bearing for | ||
| that probe's correctness, so its precise number is not evidence), and it refuses | ||
| to report a precise reading that is not bit-identical across repeats. The | ||
| conservative reading is allowed to vary and its spread is reported instead — | ||
| that spread on `12_large_live_set` is why #7554 had to stop gating the cell. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the execution-count description.
classify defaults to three repeats in each scan mode and also performs a diagnostic run. “Runs every probe twice” is incorrect. State that it runs probes in both scan modes, with the configured repeat count.
🤖 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` around lines 365 - 372, Update the
execution-count description in the README to state that each probe runs in both
conservative-scan modes using the configured repeat count, and mention the
additional diagnostic run. Remove the claim that every probe runs exactly twice
while preserving the existing output and reporting behavior.
Comment on lines
+67
to
+71
| and in `tolerances.json`'s `_readme`. Six new tests cover it, including an | ||
| end-to-end `classify` run against a stub compiler that plays the #7559 shape and | ||
| one that fails if the runtime renames `PERRY_CONSERVATIVE_STACK_SCAN` — a rename | ||
| would otherwise make every `precise` column silently equal its `conservative` | ||
| one, i.e. a classifier that classifies nothing. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the test-count claim.
The changed test file adds nine related test methods: two parser tests and seven classification or runtime-contract tests. “Six new tests” is inaccurate, including because the next sentence counts the runtime-knob test as part of that set.
State the correct count or remove the count.
🤖 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/7571-gc-ratchet-classify-retention.md` around lines 67 - 71,
Update the changelog note in the _readme section to correct the test-count
claim: the change adds nine related test methods, not six, so replace the
inaccurate count with the correct total or remove the count entirely. Keep the
surrounding description about the end-to-end classify run and the
PERRY_CONSERVATIVE_STACK_SCAN contract test unchanged.
added 3 commits
August 7, 2026 06:35
…it (#7559) `05_closure_capture` reported `heap_used_bytes` +16.44% with every collector counter at +0.00%. Nothing was kept alive that had not been kept alive before: its retention measured with the conservative native-stack scan disabled is **5,329,880 bytes at both endpoints, to the byte**. What moved was false-root residue, one 1 MiB nursery block. Two properties of the measurement point produce that: 1. Every probe reads `process.memoryUsage()` right after an explicit `gc()`, and an explicit `gc()` is the ONE site in Perry that forces the conservative native-stack scan (`ManualGcScanGuard`, #4977; production resolves to `SkipDisabled`). The reading is taken under a root set nothing else in the language uses. 2. `js_arena_stats` sums each arena block's bump-pointer OFFSET, and a block holding one marked object cannot be reset — so one stale stack word costs a whole 1 MiB block. `PERRY_GC_DIAG` counts it directly at the measurement collection: general blocks marked live are 5 with the scan off at BOTH endpoints, 6 with it on at the pinned commit, 7 at v0.5.1321. Across the 74 commits from the 2026-08-05 pin (5e236e6) to v0.5.1321, both arms built with the same package set and both reproducing the pinned artifact byte-for-byte on all twelve probes: `heap_used_bytes` moved on five probes, always by whole blocks, while scan-off retention was byte-identical on ten of twelve and FELL on the other two. The metric is deterministic without being semantic, and the probe's live set is not what it reports: sweeping `05_closure_capture`'s BATCHES 690..710 — live set ~0 throughout — walks it between 6,501,264 and 7,426,960 in a 1 MiB sawtooth. So: `gc_ratchet.py classify` runs every probe under both scan modes and prints the split plus the census of which conservative-scan sites actually fired. A row whose `excess` moved and whose `precise` did not is a false-root artifact; a row whose `precise` moved is a real retention change. It refuses to tabulate a probe whose OUTPUT changes when the scan is disabled (the scan was load-bearing for that probe's correctness, so its precise number is not evidence about the collector), and refuses to report a precise reading that is not bit-identical across repeats. The conservative reading is allowed to vary and its spread is reported instead — that spread on `12_large_live_set` is why #7554 had to stop gating the cell, and raising on it would delete the evidence. No band is changed and nothing is re-pinned: `05_closure_capture` and `02_survivor_promotion` stay red until someone decides, which is what the ratchet is for. The artifact's embedded tolerances copy is synced to the file (prose only — every pct/abs/direction/gating tuple is identical, asserted before the rewrite) because `evaluate` reads the artifact's copy and a drifted file would be a gate whose configuration disagrees with itself. Also: `run_once` now tells `Popen` the exit status `os.wait4` collected behind its back, so a harness run stops emitting a ResourceWarning per probe. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
proggeramlug
force-pushed
the
diag/7559-closure-capture-retention
branch
from
August 7, 2026 04:35
3228e80 to
a21ebae
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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 investigation half of #7559.
The answer
05_closure_capturedid not retain anything extra. Its retention measuredwith the conservative native-stack scan disabled is 5,329,880 bytes at both
endpoints, to the byte. The
+16.44%is false-root residue moving from one1 MiB nursery block to two.
The issue's lead was right that "identical work, more retained" rules out a
change in collector behaviour — but the thing being kept alive is not the
program's data. It is one extra stale word on the native Rust stack at the
instant the probe calls
gc(), andheap_used_bytesamplifies it by ~26,000x.Why the measurement does that
process.memoryUsage()immediately after an explicitgc(), and an explicitgc()is the one site in Perry that forces the conservative native-stackscan (
ManualGcScanGuard, GC: explicit gc() (full collect, default auto stack-scan) reclaims live top-level locals — string fields read back as garbage #4977 — production resolves toSkipDisabled).PERRY_GC_DIAGconfirms the suite's whole conservative-scan census is theprobes' own
gc():site=manual_collect automatic=falseon all twelve, plusa single automatic
old_reclaim_alloc_pointon12_large_live_set.js_arena_statssums block offsets, not live bytes. A bump pointernever moves backwards and a block holding one marked object cannot be reset,
so a single stale stack word costs a whole 1 MiB block. (This is the
nursery's version of the old-generation accounting problem GC: old-gen fragmentation — scattered survivors pin 105 MB of blocks for a ~1 MB live set #7437/gc: old-generation hole free list — swept holes become reusable capacity (#7437) #7443 fixed
for
OLD_ARENAby subtracting swept holes.)PERRY_GC_DIAGcounts it directly at the measurement collection on05_closure_capture:heap_used_bytes5e236e6e2, default scan5e236e6e2, scan offb5a2954ec(v0.5.1321), default scanb5a2954ec(v0.5.1321), scan offBoth minors are bit-identical between the arms (
copied_objects1422 + 1614 =3036, exactly the pinned value), and with the scan off both arms free exactly
8,124,192 bytes at the final mark-sweep and retain exactly 425 forwarded stubs.
The window, all twelve probes
Both arms built
--release -p perry -p perry-runtime-static -p perry-stdlib-staticwith a cold object cache. The baseline arm reproduces the pinned artifact
byte-for-byte on all twelve probes, which is what makes the head arm's
numbers comparable.
heap_used_bytesbase → head01_nursery_churn02_survivor_promotion03_cross_gen_writes04_dead_after_deep_stack05_closure_capture06_string_retention07_array_grow_evacuate08_map_set_sidetables09_try_catch_roots10_store_receiver_across_alloc11_collect_at_depth12_large_live_setheap_used_bytesmoved on five probes, always by whole blocks, three downand two up. Precise retention moved on two, and both went down.
02_survivor_promotion's+2.77%is the same shape one survivor block down(262,160 B ≈ 256 KiB), and its precise retention fell by 1,600 bytes.
The metric is not reporting the live set
05_closure_capturedrops everything before it measures, so its live set is~zero for every workload size. Sweeping
BATCHES690 → 710 with one compiler:A 1 MiB sawtooth over a constant (zero) live set: what is left over is the
un-reset tail blocks' bump pointers, and
arena_reset_empty_blocksnever resetsthe current block or the four before it. Two batches of workload — 123 KB — move
the gated number by 925,696 bytes.
What this PR changes
gc_ratchet.py classifyruns every probe under both scan modes and prints thesplit plus the scan-site census:
A row whose
excessmoved and whoseprecisedid not is a false-root artifact;a row whose
precisemoved is a real retention change.It refuses to tabulate a probe whose stdout changes when the scan is
disabled — then the scan was load-bearing for that probe's correctness and its
precise number is not evidence about the collector — and refuses to report a
precise reading that is not bit-identical across repeats. The conservative
reading is allowed to vary and its spread is reported instead: that spread on
12_large_live_setis exactly why #7554 had to stop gating the cell, andraising on it would delete the evidence. (All twelve probes are verified to
produce byte-identical stdout under both modes.)
Also here: the finding written into
gc_ratchet.py's module docstring, theREADME (a new section on what
heap_used_bytesactually contains, plus amandatory classify step in "When the gate goes red"), and
tolerances.json's_readme; six new tests, including an end-to-endclassifyrun against a stubcompiler that plays the #7559 shape, and one that fails if the runtime renames
PERRY_CONSERVATIVE_STACK_SCAN(a rename would otherwise make everyprecisecolumn silently equal its
conservativeone).run_oncenow handsPopenthe exit statusos.wait4collected behind itsback, so a harness run stops emitting a
ResourceWarningper probe.What this PR deliberately does NOT change
No band is widened and nothing is re-pinned.
05_closure_captureand02_survivor_promotionstay red. The artifact's embedded tolerances copy issynced to the file (prose only — every
pct/abs/direction/gatingtuple isasserted identical before the rewrite) because
evaluatereads the artifact'scopy, and #7557 set the precedent for a deliberate sync without a re-pin.
The remaining decision is a maintainer one and needs the pinned host; it is
written up on #7559. In short, the honest options are (a) re-pin those two cells
with the rationale above recorded in
--notes, or (b) change what the retentionfamily measures. Widening the band is not one of them:
heap_used_bytes'schurn quantum is one whole block, which is also the unit
test_one_falsely_retained_nursery_block_failsasserts must go red — you cannotgate a quantity whose noise quantum equals its signal quantum, so the fix is to
measure a different quantity, not to loosen the band on this one.
Validation
python3 -m unittest discover -s tests -p 'test_gc_ratchet.py'— 63 tests, OKpython3 benchmarks/gc_ratchet/gc_ratchet.py validate— OKpython3 benchmarks/gc_ratchet/gc_ratchet.py measure+check --profile shared_ciat
b5a2954ecreproduces the issue's numbers exactly (05+16.44%, 02+2.77%, every 05 counter+0.00%)bash scripts/check_file_size.sh— OKpython3 scripts/raw_handle_debt.py— 998 (baseline 998)python3 scripts/gc_store_site_inventory.py— passedcargo fmt --all -- --check— cleanNo Rust changed, so the runtime gates are unaffected by construction.
Refs #7559, #7554, #7558
https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
Summary by CodeRabbit
New Features
gc_ratchet.py classifyworkflow to compare conservative and scan-disabled retention results.Documentation
Bug Fixes