fix(gc-matrix): the collect arms require a productive cycle, and the numarray probe reaches the collector (#7016, #7017) - #7676
Conversation
📝 WalkthroughWalkthroughThe PR adds GC churn coverage for numeric-array growth. It changes ChangesGC liveness validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
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: 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 `@scripts/gc_matrix_liveness_check.py`:
- Around line 81-84: Update the collect requirement in REQUIREMENTS and the
arm_liveness() logic so collect is considered live only when both cycles and
reclaimed are positive, while preserving existing behavior for other
requirements. Add a self-test covering cycles=0 with reclaimed=1 and assert that
it reports a violation.
In `@scripts/gc_repsel_matrix.sh`:
- Line 193: Update the arm descriptions in scripts/gc_repsel_matrix.sh:193-193
and scripts/gc_repsel_matrix.sh:204-204 to remove stale known-inert claims. At
line 193, remove wording that a registry entry still names the blocker or that
the liveness gate will fail when default scavenges again; at line 204, remove or
explicitly mark as historical the claim that PERRY_GC_INCREMENTAL=0 is required
for cons_scan_off to be live.
🪄 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: fd8db6bc-66b6-428e-ac9e-fe0c9e7d35ac
📒 Files selected for processing (5)
changelog.d/7676-gc-matrix-collect-liveness.mdscripts/gc_matrix_liveness_check.pyscripts/gc_repsel_matrix.shtest-files/test_gap_repsel_p4a3_numarray_growth.tstest-parity/gc_matrix_inert_arms.txt
| REQUIREMENTS = { | ||
| "scavenge": ("copying young-gen minor", ("scavenged",)), | ||
| "move": ("any relocation", ("evacuated", "scavenged")), | ||
| "collect": ("any GC cycle", ("cycles",)), | ||
| "collect": ("a productive GC cycle", ("reclaimed",)), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require a cycle in the liveness checker.
arm_liveness() marks collect live when reclaimed > 0, even if cycles == 0. The matrix producer requires both values to be positive at scripts/gc_repsel_matrix.sh Lines 519-523.
Require cycles > 0 for collect in this checker. Add a self-test with cycles=0, reclaimed=1 that expects a violation.
Proposed fix
- if value > 0:
+ if value > 0 and (
+ requires != "collect" or int(cell.get("cycles", 0) or 0) > 0
+ ):
live += 1🤖 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 `@scripts/gc_matrix_liveness_check.py` around lines 81 - 84, Update the collect
requirement in REQUIREMENTS and the arm_liveness() logic so collect is
considered live only when both cycles and reclaimed are positive, while
preserving existing behavior for other requirements. Add a self-test covering
cycles=0 with reclaimed=1 and assert that it reports a violation.
| # --------------------------------------------------------------------------- | ||
| ARMS=( | ||
| "default||%P%|scavenge|as-shipped GC configuration under allocation pressure. ***INERT AT THE MOMENT, AND REGISTERED AS SUCH*** in test-parity/gc_matrix_inert_arms.txt. #7024 made this a relocating arm (the alloc-point trigger defers to js_gc_loop_safepoint -> gc_safepoint_moving_minor, which runs the copying minor on precise rewritable roots); #7161 then flipped PERRY_GC_MOVING_LOOP_POLLS default-OFF pending #7154, and that one env gates BOTH halves of the route -- perry-codegen's moving_safepoint_polls_enabled decides whether the back-edge polls are emitted at all, and perry-runtime's gc_moving_loop_polls_enabled decides whether the trigger defers to them. A default binary has neither, so the minor runs behind ManualGcScanGuard::force_full_scan and the copying minor is ineligible by construction. requires=scavenge STAYS: it is what the shipped default is FOR, the registry entry names what blocks it, and the liveness gate fails the day it scavenges again so the entry cannot outlive its cause. safepoint_minor carries the relocating claim meanwhile." | ||
| "default||%P%|scavenge|as-shipped GC configuration under allocation pressure. ***LIVE AGAIN AS OF 2026-08-09*** -- its known-inert entry was deleted in test-parity/gc_matrix_inert_arms.txt. #7024 made this a relocating arm (the alloc-point trigger defers to js_gc_loop_safepoint -> gc_safepoint_moving_minor, which runs the copying minor on precise rewritable roots); #7161 then flipped PERRY_GC_MOVING_LOOP_POLLS default-OFF pending #7154, and that one env gates BOTH halves of the route -- perry-codegen's moving_safepoint_polls_enabled decides whether the back-edge polls are emitted at all, and perry-runtime's gc_moving_loop_polls_enabled decides whether the trigger defers to them. A default binary has neither, so the minor runs behind ManualGcScanGuard::force_full_scan and the copying minor is ineligible by construction. requires=scavenge STAYS: it is what the shipped default is FOR, the registry entry names what blocks it, and the liveness gate fails the day it scavenges again so the entry cannot outlive its cause. safepoint_minor carries the relocating claim meanwhile." |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove stale known-inert claims from arm descriptions.
The records now state that these arms are live and their registry entries were deleted. The retained wording states the opposite.
scripts/gc_repsel_matrix.sh#L193-L193: Remove the claim that a registry entry still names the blocker and that the gate will fail whendefaultscavenges again.scripts/gc_repsel_matrix.sh#L204-L204: Remove or mark as historical the claim thatPERRY_GC_INCREMENTAL=0is required to makecons_scan_offlive.
📍 Affects 1 file
scripts/gc_repsel_matrix.sh#L193-L193(this comment)scripts/gc_repsel_matrix.sh#L204-L204
🤖 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 `@scripts/gc_repsel_matrix.sh` at line 193, Update the arm descriptions in
scripts/gc_repsel_matrix.sh:193-193 and scripts/gc_repsel_matrix.sh:204-204 to
remove stale known-inert claims. At line 193, remove wording that a registry
entry still names the blocker or that the liveness gate will fail when default
scavenges again; at line 204, remove or explicitly mark as historical the claim
that PERRY_GC_INCREMENTAL=0 is required for cons_scan_off to be live.
9cbc556 to
d7b2486
Compare
… inert-arm entries retire (#7017) Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Audit — merging as v0.5.1388#7016 verified, after I measured it wrong first. My bare invocation reported 0 cycles and I nearly filed that against you. The matrix runs its arms under
Control: my build emits trace fine (a churn kernel shows 2 without pressure, 10 with). So the probe genuinely crosses collections now, and my first reading measured a configuration the gate never runs — the same shape as everything else in this cluster, applied to my own check. #7017 is the right fix for the right reason. And the design note in the source is the part I most want kept: "one collector change cannot pin it permanently false." That is #7657's lesson — re-enabling the adaptive-tenuring seed drove 14 of 58 files moving PASS → honest UNVER is a gain, not a regression: those cells were reporting on a collector that never ran. #7018 closed by refutation, and that is worth as much as a fix: 20 runs without reproduction, plus the structural argument that The unasked-for part is the most valuable. All four entries in Gates: 24/24 lint, matrix self-test and Worth watching when the queue drains: |
d7b2486 to
7ae4ea7
Compare
1 participant
Closes #7016. Closes #7017. Closes #7018 (does not reproduce — evidence below).
Three issues, one disease: a check that could not fail. For each one the first job was to decide whether the probe or the predicate was at fault, and the answer differs.
#7016 — the PROBE. 19 cells measured a collector that never ran.
Reproduced first, before changing anything:
Zero cycles, and
PERRY_GC_DIAG=1printed nothing at all — the file allocates inside one 1 MB arena block and makes nogc_malloccalls, so the arena trigger never arms, andPERRY_GC_HEAP_LIMITcannot reach it (gc_trigger_absolute_ceiling_bytesis budget/4 with a floor). TheUNVERverdict was the gate working.Sections 1–5 are untouched, so their "fully contained, therefore promoted" shape is byte-for-byte what it was; a new section 6 adds
test_gap_repsel_gc_stress's escaping module-level churn sink, with the numeric-array local initialized before the churn, grown bypushpast several capacity doublings while it runs, and read after it in the same iteration.Measured on all 13 arms — PASS on every one, output byte-identical to the pinned Node 26.5.1 oracle in each:
defaultsafepoint_minorevac_minor/force_evac/force_verifygen_gc_off/wb_off/gen_off_verify/all_fourshipped_defaultPer the #7666 lesson, the check is on the copying minor, not merely on a cycle: a probe that allocates a lot can still run zero copying minors when
arena_growth_full_escalation_dueescalates every minor to a full sweep.#7017 — the PREDICATE.
cycles > 0counted a teardown cycle.A
collectcell wasPASSon any cycle. On a small file that cycle lands at the event-loop boundary after the program's last output and reclaims nothing, because everything allocated after it armed was born black — indistinguishable from a run that collected mid-program while the test's representation-selected locals were live.collectnow requires a productive cycle. Following #7657's widening of the gc-ratchet rule tocopied + promoted > 0, the counter names a destination rather than a single number:reclaimedsumssweep_freed,block_reclaim,eden_dead_bytes,freed_bytesanddead_bytes, in both thek=Nand JSON"k": Nspellings.Reading only
k=Nwas itself a vacuous check, and it was caught by measurement.test_gap_gc_symbol_local_rootingruns 86 malloc-count-triggered cycles that free 31.9 MB of symbols; every arena-side counter reads 0 because the malloc sweep's bytes appear only in the JSON trace'ssweep.freed_bytes. A predicate over thek=Nshape alone would have scored it inert.The script says out loud that this is a conservative proxy: a mid-program cycle over a heap that is entirely live reclaims nothing and reads
UNVER. It can under-claim, never over-claim, which is the safe direction for a liveness gate;cyclesover-claimed.Both halves move together.
reclaimedis threaded into the per-cell JSON andgc_matrix_liveness_check.py'sREQUIREMENTS["collect"]reads it. Shown able to fail: reverting the counter to("cycles",)fails exactly the two new self-test cases that pin the change —— and restoring it returns
24 checks, 0 failures. A third case pins that a productive cycle still passes.The per-run liveness table now prints
reclaimedbesidecollected, so the gap stays on screen instead of being folded away:Corpus-wide impact, measured under
gen_gc_offover all 58 files: 12 never collect (alreadyUNVER), 14 collect but reclaim nothing —PASSbefore, honestlyUNVERnow — and 32 are productive, which keeps everycollectarm live and the arm-level gate green.The four known-inert arms are live, and CI has been saying so
Not in the issues, found while validating, and this PR cannot be green without it.
test-parity/gc_matrix_inert_arms.txtregistereddefault,verify_evac,cons_scan_offandcons_scan_off_forceas inert becausePERRY_GC_MOVING_LOOP_POLLSis default-off, making the copying minor "ineligible by construction". The poll flag has not changed and all four scavenge anyway.gc-stressonmain, run 31240304595 (2026-08-08): each satisfiedrequires=scavengeon 41 of 58 cells,defaultatcounter=1384046, and the job was red with fourSTALE-REGISTRYlines. This is not a condition this PR introduces — it is onemainhas been red for.--arms default,verify_evac,cons_scan_off,cons_scan_off_force --filter test_gap_repsel_gc_stress— a file this PR does not touch, under thescavengepredicate this PR does not change — 1/1 live on every arm,defaultcopying 228,181 objects.shipped_default(requires=none, no pressure knob, no GC env at all) copies 340,956 objects on the same file. The shipped configuration relocates today.All four entries are deleted with the measurement recorded in their place, and the four arm notes in
gc_repsel_matrix.shthat repeated the claim in prose are corrected — otherwise they become exactly the stale prose that file's own header warns about. What changed is the collector around the flag (#7370's statepoint default, #7432, #7657, #7666), not the flag. The registry is now empty; if the copying minor ever stops running in the shipped configuration, the gate fails in the other direction on the same line.#7018 — does not reproduce, and the hypothesis is structurally refuted
PERRY_GC_TRACE=1was reported to SIGSEGVtest_gap_repsel_scalar_replaced_localsunder the evacuating arms. 20 runs across both link modes: 0 crashes, stdout byte-identical with and without the flag, on that file and four others.The first link mode was vacuous and is reported as such: auto-optimize relinks the runtime
--no-default-features, sodiagnosticsis off andGcCycleTrace::emitfalls to its stub — all 113[gc] cyclelines read "diagnostics feature disabled", and the real tracer never ran. Re-run against the diagnostics archive: 110 real"event":"gc_cycle"objects, 466,424 objects copied. The arm is demonstrably live and still does not crash. 13.ipscrash reports were produced during the investigation and every one isSIGABRTfrom an unrelated instrument; zero SIGSEGV.Structurally it cannot hold.
PERRY_GC_TRACEis read in exactly one place (gc_trace_enabled(),gc/policy.rs:571) with two call sites: a scalar counter snapshot (policy.rs:760) and a thread-localu64bump (barrier.rs:931). Emission serialises already-accumulated scalars. Nothing on that path dereferences a heap object, walks the object graph, or reads aGcHeader— so it cannot dereference a forwarded pointer, and it shares nothing with #6998/#6995. The issue's description ("the tracer runs inside a collection, walking structures the collector is mid-way through mutating") describesgc/trace.rs, the marking tracer, which runs on every collection regardless of the flag. A name collision, not a defect.Validation
--arms prmatrix over all 58 corpus files, exit 0 on the liveness gate.--arms pr --filter test_gap_repsel_p4a3: 21/21 cells byte-exact,PASS=21 UNVER=0 FAIL=0, all seven armslive.--arms gen_gc_off,wb_off,evac_minor,shipped_default --filter test_gap_repsel_p4a3: the two probes that only reach a teardown cycle now readUNVERon thecollectarms andPASSonevac_minor, which is the intended split.lintgate commands green;gc_matrix_liveness_check.py --self-test24 checks / 0 failures;bash -nclean.One unrelated red seen while validating, reported rather than swallowed
The 58-file
--arms prrun above endedPASS=341 UNVER=64 XFAIL=0 **FAIL=1**:test_gap_repsel_gc_stress×safepoint_minor, output-mismatch,cycles=52 scavenged=38184. It is not from this PR — different file, andsafepoint_minor'srequires=scavengepredicate is untouched here.Re-run in isolation, same binary, same env: 10/10 clean, so it is intermittent, not deterministic.
safepoint_minoris the arm that compiles and runs withPERRY_GC_MOVING_LOOP_POLLS=1— the configuration #7161 turned off by default because of #7154's use-after-free — so an intermittent mismatch there is consistent with that open class. It is untriaged intest-parity/gc_repsel_triage.txtand CI's lastmainrun reportedFAIL=0, so it is a low-rate sampling difference rather than a new deterministic break. Filed separately; deliberately not added to the triage list here, because triaging a red I have not root-caused is how a real defect becomes permanent furniture.Summary by CodeRabbit
Bug Fixes
Tests