diag(gc): attribute a copying minor's root scan per scanner (#7915) - #7931
Merged
Conversation
The aggregate `root_sources.runtime_mutable_scanners` counter sums all 82 registered scanners and every pass into one number, which is enough to see that a cost is per-root and not enough to say WHICH registry holds the roots. #7915 was written on that gap: "82 scanners over 218,455 pointer roots" is arithmetically true, and the attribution says it is one scanner -- `crate::box::scan_box_roots_mut` is 92% of the pointer roots and 89.5% of root-scan time, while the suspected promise registry visits 12 slots. `[gc-scanner-profile]` rides the existing PERRY_GC_DIAG knob (no new knob) and reports, per scanner, wall time and slots/pointer-roots/ rewrites. Registration sites carry their own path as the name via `stringify!` in two `gc_init` macros, so a name cannot drift from the list it describes. Also adds counters for the three `gc_safepoint_moving_minor` entry guards that had none -- a precise safepoint that returned without collecting had one observable explanation and three invisible ones -- and teaches `gc_runtime_root_holders.py` the new registration macros, which its MIN_REGISTERED floor caught immediately. Claude-Session: https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughGC root scanners now retain registration names and expose per-scanner copying-minor timing and root statistics. Moving safepoint diagnostics now count allocation, unsafe-zone, and root-lock blocks. Scanner-holder lint recognizes the new registration macros. ChangesGC diagnostics
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
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 #7915 (as far as it can be closed — see "What this does not do").
What #7915 asked, and what the measurement says
Both halves turn out to be artefacts of which counter was read. Reproduced on
origin/main,asyncpipeat 240 batches,PERRY_GC_TRACE=1 PERRY_GC_DIAG=1:1. The "zero objects" minor handled 200 201 of them.
copied_objectscounts survivor-space copies only. On a promoting cycle it isstructurally zero, which is the same shape as CLAUDE.md's "a gate must assert
its subject was live" — a counter that cannot be non-zero on the path being
measured. 101 ms / 200 201 objects is 505 ns/object, not a fixed per-cycle cost.
2. The root population is one registry, not 82 scanners.
This PR adds the per-scanner attribution that shows it.
crate::box::scan_box_roots_mutis 92 % of the pointer roots and 89.5 % of all root-scan time; the other 81
scanners are 8.8 % between them. The suspected culprit, the promise registry,
visits 12 slots and costs 4–43 µs.
3. …and that 89.5 % is not root-scan overhead, it is the object trace.
A cheap slot visit costs ~4 ns (
intern_table_mutable_root_scanner: 16 384slots, 1 144 pointer roots, 68 µs). The box scanner costs 98.8 ns/slot because
essentially every box slot is a from-space pointer, so visiting it in
CopyingMarkmode evacuates the object it names — 93 380 of minor #1's 156 236evacuations are driven straight out of box roots. 98.8 ns/pointer-root is the
same price as a measured full evacuation (123 ns/object).
So the fixed per-root overhead the issue is aimed at is about
4 ns × 265 000 ≈ 1 ms of a 65–101 ms pause. Generational filtering, dirty
tracking of the registries and cheaper registry representations all attack that
1 ms. I built the third one (slab-allocated box cells, so the scan streams
instead of chasing ~121 500 scattered 8-byte mallocs in hash order, twice per cycle) and it
measured +0.03 % instructions / −1.8 % cycles / no RSS change / pause
indistinguishable over 7 runs, so it is not in this PR.
What is in this PR
Instruments and one lint fix — no behaviour change.
[gc-scanner-profile], per registered scanner: wall time andslots/pointer-roots/rewrites, sorted by time, once per copying minor, under
the existing
PERRY_GC_DIAG(no new knob — CLAUDE.md's GC knobkill-policy). Registration sites carry their own path as the name via
stringify!ingc_init'sreg_scanner!/reg_budgeted_scanner!macros, soa name cannot drift from the list it describes. The aggregate
root_sources.runtime_mutable_scannerscounter sums all 82 scanners and allpasses into one number — enough to see that a cost is per-root, not enough to
say which registry holds the roots, which is exactly how gc: asyncpipe collects at 1200-1650 ns/object, including a 122 ms minor that handled zero objects #7915 came to read as
a property of "the registries".
Counters for the three unobserved
gc_safepoint_moving_minorentryguards. Only
budgetedwas counted, so a precise safepoint that returnedwithout collecting had one observable explanation and three invisible ones.
[gc-incremental]now also printssafepoints_blocked(in_alloc=… unsafe_zone=… root_lock=…). This is what makes"an active
setjmp/tryregion suppresses the copying minor" (perf(runtime): promise resolution with an object pays a 78.5% thenable-probe tax (~9% of asyncpipe) #7910) atestable claim rather than an argument about the guard list.
scripts/gc_runtime_root_holders.pylearns the two registration macros.They expand to
gc_register_*calls but the gate matches the call name, sointroducing them dropped its registered-scanner count from 122 to 24 and every
holder reached only from
gc_initwould have read as uncovered. ItsMIN_REGISTEREDfloor caught it, which is what that floor is for.Where the cost actually is (and it is not the collector)
asyncpipe'syoung_survival_permilleis 695 and 895, against 0–4for every other churn-shaped program in the corpus — and it is a churn program
by construction: each batch builds 200
Reqs / 200Oks / 200 strings, foldsthem into six numbers, and drops the batch.
A one-env-var probe (temporary, not in this PR) makes
scan_box_roots_mutskipmarking while still rewriting:
young_survival_permillecopied_objectsfreed_bytesasyncpipe's real live young set is ~6 600 objects. The other 96 % is aliveonly because a completed async activation's boxed locals are still GC roots —
the transform boxes every body local of every
asyncfunction and nothing everfrees or clears one, so every local of all 48 000 completed activations is a
permanent root. At 6 634 live objects a minor costs ~0.8 ms instead of 62 ms.
The SIGABRT is the control, not a failure: not marking is unsound, which is
precisely why the fix is clearing an activation's boxes at its terminal state
rather than not marking them. That is a codegen change in the async transform
and wants its own issue; this PR contributes the evidence that it is the lever.
Full write-up:
gc-handoff/ROOTS-NOTES.md§7–8.Validation
cargo test --release -p perry-runtime --lib(RUST_TEST_THREADS=1),GC/copying + box suites: 127 passed, 0 failed.
cargo fmt --all -- --check,scripts/check_file_size.sh,scripts/gc_runtime_root_holders.py(+--self-test),scripts/addr_class_inventory.py(+--self-test).Summary by CodeRabbit
New Features
PERRY_GC_DIAG.Documentation