-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(gc-ratchet): 10_store_receiver_across_alloc runs no minor collection — give it margin #9833
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,7 +33,41 @@ | |
| declare function gc(): void; | ||
|
|
||
| const SLOTS = 1024; | ||
| const ITERATIONS = 200000; | ||
|
|
||
| // ITERATIONS EXISTS TO GIVE THIS PROBE MARGIN, NOT TO MAKE IT LONGER. | ||
| // | ||
| // This probe went **inert on main for part of 2026-08-18..09-06** and nobody | ||
| // noticed: `minor_cycles` fell from 1 to 0, so no evacuating minor ran, so the | ||
| // three conditions below could not bite and the probe measured nothing. It | ||
| // still "passed" everything except a gc-ratchet gate that was already red for | ||
| // unrelated reasons (#9829, #9832), which is why it went weeks undetected. | ||
| // | ||
| // The cause was margin, not a bug: at 200,000 iterations the probe allocated | ||
| // just enough to cross the nursery threshold exactly once. #8313 shrank a | ||
| // two-field object from 56 to 40 bytes — a change everyone wants — and that | ||
| // alone dropped the total under the threshold. **A probe that fires exactly one | ||
| // collection is one optimisation away from firing none**, and any future | ||
| // allocation win re-creates this silently. | ||
| // | ||
| // Measured on `main` @ d36a1af0c, 40-byte objects, three repeats each: | ||
| // | ||
| // ITERATIONS minor_cycles wall_ms | ||
| // 200,000 0 26 <- inert, shipped for weeks | ||
| // 600,000 2 47 | ||
| // 1,200,000 4 81 | ||
| // 2,400,000 9 147 | ||
| // | ||
| // 2,400,000 is chosen so the probe still runs several evacuating minors after a | ||
| // further 8x reduction in allocated bytes per object. The cost is ~120 ms on a | ||
| // metric the gate does not band (`wall_ms`), which is the cheapest insurance in | ||
| // the suite. | ||
| // | ||
| // INVARIANT, and please check it if you touch this file: this probe must report | ||
| // `minor_cycles >= 2`. `gc_ratchet.py` refuses to PIN a baseline whose | ||
| // `minor_cycles < 1`, so a fully inert probe cannot be blessed — but it will | ||
| // happily pin `minor_cycles == 1`, which is the marginal state that produced | ||
| // this outage. One is not margin. | ||
|
Comment on lines
+65
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift Enforce the documented
🤖 Prompt for AI Agents |
||
| const ITERATIONS = 2400000; | ||
|
|
||
| // (1) module-level, so the receiver is loaded from a global handle rather than | ||
| // a shadow slot. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| **The `10_store_receiver_across_alloc` GC-ratchet probe was running no | ||
| collection at all, and has been given margin** (#9833, fixes #9832). | ||
|
|
||
| The probe exists to catch a store receiver held in a register across an | ||
| evacuating minor — the stale-root class of #6970 / #9523 — and its own header | ||
| lists three conditions that must all hold for it to bite, the third being an | ||
| allocating right-hand side. On `main` it reported `minor_cycles = 0`: no minor | ||
| ran, so no evacuation happened, so there was no window and the probe measured | ||
| nothing. `freed_bytes = 0` alongside `copied_objects = 0` rules out "a minor ran | ||
| and found nothing live". | ||
|
|
||
| The cause was margin rather than a bug. At 200,000 iterations the probe crossed | ||
| the nursery threshold exactly once, and #8313 — shrinking a two-field object | ||
| from 56 to 40 bytes — put it under. A probe that fires exactly one collection is | ||
| one optimisation away from firing none. It is now 2,400,000 iterations, which | ||
| measured 9 minors and keeps several after a further eightfold reduction in bytes | ||
| per object, for about 120 ms on `wall_ms`, which the gate does not band. | ||
|
|
||
| Verified by sabotage rather than by the counter moving: removing the allocating | ||
| RHS returns `minor_cycles=0 copied_objects=0 freed_bytes=0`, the exact signature | ||
| the probe had while broken. | ||
|
|
||
| `heap_used_bytes` returns from 464,072 to 244,648 against a pinned baseline of | ||
| 220,384 — the +110.57 % that cell showed on `main` was the post-`gc()` residue | ||
| of a run in which nothing was ever collected, not retention. | ||
|
|
||
| Five further probes (`01`, `02`, `03`, `09`, `11`) currently sit at | ||
| `minor_cycles == 1` and are one allocation win away from the same silent state; | ||
| that is recorded in #9832 and not addressed here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 50370
🤖 get_repo_knowledge executed:
get_repo_knowledge PerryTS/perry /tmp/coderabbit-repo-knowledge/perryts-perry-d4a878bc/learningsLength of output: 13201
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 12992
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 6177
Enforce the declared liveness floor and substantiate the eightfold margin.
10_store_receiver_across_alloc.tsdeclaresminor_cycles >= 2, butgc_ratchet.py::inspect_artifactrejects only medians below 1, and the current baseline pins this probe at 1. Add a per-probe minimum of 2 to pin-time validation and repin the baseline.🤖 Prompt for AI Agents