fix(gc): run old-gen reclaim precisely at safepoints, stop scanning on OS memory pressure, and census every conservative-scan fallback - #7166
Conversation
…s; census every conservative-scan fallback #7148. Six force_full_scan() sites forced the conservative native-stack scan, four of them without any user calling gc(). A scanning cycle is not merely slower: it makes the copying minor ineligible, so it runs no copying minor at all (gc_ratchet README: heap_used_bytes +364%..+5371%, minor_cycles -> 0 on all eight probes). Per-site disposition: - OldReclaim alloc point -> DEFER. gc_safepoint_moving_minor now drains the old-gen reclaim as the same full mark-sweep with precise roots. The alloc-point arm keeps a slack valve bounded in the trigger's OWN unit (old-gen in-use + external side bytes, one 32 MB growth quantum), because an arena-unit slack would never expire for the >16 KB old-arena workload #5476 exists for -- #7024's bug inverted. - Host pressure -> DEFER when a generated frame is live (arm the deferral, return the already-documented code 1); collect PRECISELY, no scan, when the shadow stack is empty -- the run-loop-boundary case the module's own docs call typical, and the case where deferring would shed nothing because an idle process reaches no safepoint. - Nursery-churn valve, emergency reclaim, gc(), perry/gc minor() -> KEEP, now counted. Emergency reclaim provably cannot defer: it runs after an allocation has already failed and the caller's next act is to panic, so there is no next safepoint. Every site is now recorded in gc/scan_fallback.rs with a PERRY_GC_DIAG line, and the two deferral paths record a drain counter -- so a gate can assert its subject was live (CLAUDE.md four-ways-a-gate-cannot-fail #4) instead of only that nothing crashed. Also: PERRY_CONSERVATIVE_STACK_SCAN=full failed 134/1574 runtime tests. In the test build a pinned per-thread override now beats an env request for Full -- the env may make the scan less aggressive than a test declared, never more. Kept rather than deleted because =full is the gc_ratchet's validated sensitivity arm, the only end-to-end proof that gate can fail.
Every deferral test asserts BOTH that the conservative valve did not fire and that the precise safepoint collection which replaced it DID run (a drain counter). Checking only the first half would pass on a tree where the trigger never armed at all -- the largest possible regression reported as a pass (CLAUDE.md, four ways a gate cannot fail, #4). Adds two test-only scoped overrides next to the existing force_legacy_gc_pacing pattern: pinned moving pacing (so a deferral test cannot silently run under legacy pacing and pass for the wrong reason) and a shrinkable old-gen deferral slack (the shipped slack is 32 MB; crossing it for real would mean committing 32 MB of old-gen inside a unit test).
…pt and how it was verified
📝 WalkthroughWalkthroughThe GC runtime classifies conservative scans and safepoint drains, defers eligible reclamation to precise safepoints, retains tagged fallback paths, and adds tests and documentation for pressure handling and scan-mode precedence. ChangesGC fallback and reclaim flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AllocationOrPressure
participant GCPolicy
participant PreciseSafepoint
participant ScanFallbackCounters
AllocationOrPressure->>GCPolicy: request collection
GCPolicy->>GCPolicy: check slack and generated-frame state
GCPolicy->>PreciseSafepoint: defer eligible collection
PreciseSafepoint->>GCPolicy: drain deferred collection
GCPolicy->>ScanFallbackCounters: record safepoint drain or tagged fallback
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
…conservative arm outright Two follow-ups the compiler found. 1. gc/roots.rs crossed the 2000-line file-size gate (1992 -> 2035). The conservative-scan mode block is topically self-contained -- the mode enum, its env/override precedence, ManualGcScanGuard, and the decision the mark phase consults -- so it moves to roots/scan_mode.rs. roots.rs is now 1851. 2. ConservativeScanSite::HostPressure was never constructed, because after the deferral js_gc_memory_pressure has NO conservative arm left: it either collects with precise roots or defers. So the variant is deleted rather than kept 'for completeness'. An enum arm nothing can produce is a claim no test can check -- the host-pressure tests now assert automatic_scan_fallback_total == 0 instead, which catches reintroduction at any automatic site. The counter read APIs are #[cfg(test)]; ops observability is the PERRY_GC_DIAG line, which is unconditional.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-runtime/src/gc/scan_fallback.rs (1)
67-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the
HostPressuredoc comments to reflect the new deferral behavior.
ConservativeScanSite::HostPressure(Line 67-69) is documented as "js_gc_memory_pressurewith a generated frame live above us." That describes the pre-#7148 behavior, when a live frame forced a conservative scan. After this PR's change inpressure.rs, a live frame now defers to a safepoint instead of scanning, and the code never callsforce_full_scan(ConservativeScanSite::HostPressure)anywhere. Sibling variantsOldReclaimSlackValveandNurseryChurnSlackValvewere updated to describe their new "after the safepoint deferral ran out of slack" semantics, butHostPressure's comment was not updated to match.Similarly,
SafepointDrainKind::HostPressure(Line 132-134) is documented as "collected at a safepoint," but it is recorded directly insidejs_gc_memory_pressurewhen no frame is live (a synchronous inline collection), not via thejs_gc_loop_safepoint→gc_safepoint_moving_minordrain path that the other two kinds use.Since this module's stated purpose is to make claims about scan fallback measurable and precise, stale doc comments on the exact site this PR redesigned undercut that goal. Clarify that
HostPressure(theConservativeScanSitevariant) is now structurally unreachable — its associated scan is superseded by deferral and, if the deferral ever fails to drain, gets attributed toOldReclaimSlackValveorNurseryChurnSlackValveinstead.📝 Suggested doc update
- /// `js_gc_memory_pressure` with a generated frame live above us. - /// Host-driven. + /// `js_gc_memory_pressure`. Historically fired when a generated frame was + /// live above us; since `#7148` that case defers to a safepoint instead, so + /// this variant is retained for the census but is never actually recorded + /// (a valve firing in that scenario is attributed to + /// `OldReclaimSlackValve` or `NurseryChurnSlackValve` instead). Host-driven. HostPressure,- /// A host memory-pressure request collected at a safepoint (`#7148`). + /// A host memory-pressure request collected synchronously with precise + /// roots because no generated frame was live (`#7148`) — not a deferred + /// drain through `js_gc_loop_safepoint` like the other two kinds. HostPressure,Also applies to: 132-135
🤖 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 `@crates/perry-runtime/src/gc/scan_fallback.rs` around lines 67 - 69, Update the documentation for both HostPressure variants: in ConservativeScanSite, state that the variant is structurally unreachable because host-pressure handling defers to a safepoint, with any failed deferral attributed to OldReclaimSlackValve or NurseryChurnSlackValve; in SafepointDrainKind, describe that host-pressure collection is recorded synchronously by js_gc_memory_pressure when no frame is live rather than through the safepoint drain path.
🤖 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.
Nitpick comments:
In `@crates/perry-runtime/src/gc/scan_fallback.rs`:
- Around line 67-69: Update the documentation for both HostPressure variants: in
ConservativeScanSite, state that the variant is structurally unreachable because
host-pressure handling defers to a safepoint, with any failed deferral
attributed to OldReclaimSlackValve or NurseryChurnSlackValve; in
SafepointDrainKind, describe that host-pressure collection is recorded
synchronously by js_gc_memory_pressure when no frame is live rather than through
the safepoint drain path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d6919dbf-ba85-4bab-b8ad-9ee9fdcee9dc
📒 Files selected for processing (10)
benchmarks/gc_ratchet/README.mdchangelog.d/7166-gc-safepoint-deferral-scan-census.mdcrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/pressure.rscrates/perry-runtime/src/gc/roots.rscrates/perry-runtime/src/gc/scan_fallback.rscrates/perry-runtime/src/gc/tests/mod.rscrates/perry-runtime/src/gc/tests/roots.rscrates/perry-runtime/src/gc/tests/scan_fallback.rs
…ous inline collection, not a deferred drain (CodeRabbit)
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 `@crates/perry-runtime/src/gc/roots/scan_mode.rs`:
- Around line 98-122: Add #[must_use] to the ManualGcScanGuard struct
declaration so callers receive a compiler warning when force_full_scan returns
an unbound guard that would immediately drop. Leave force_full_scan and Drop
behavior unchanged.
In `@crates/perry-runtime/src/gc/scan_fallback.rs`:
- Around line 224-227: Update the documentation comment above the
conservative-scan fallback counter to say there are three automatic sites,
matching the variants classified as automatic by is_automatic(). Leave the
clarification that explicit gc() requests are excluded unchanged.
🪄 Autofix (Beta)
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: 723efde9-9683-482f-95c4-d630ddaf18d5
📒 Files selected for processing (7)
crates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/roots.rscrates/perry-runtime/src/gc/roots/scan_mode.rscrates/perry-runtime/src/gc/scan_fallback.rscrates/perry-runtime/src/gc/tests/roots.rscrates/perry-runtime/src/gc/tests/scan_fallback.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/perry-runtime/src/gc/tests/roots.rs
- crates/perry-runtime/src/gc/tests/scan_fallback.rs
- crates/perry-runtime/src/gc/mod.rs
- crates/perry-runtime/src/gc/policy.rs
| pub(crate) struct ManualGcScanGuard { | ||
| engaged: bool, | ||
| } | ||
|
|
||
| impl ManualGcScanGuard { | ||
| pub(crate) fn force_full_scan(site: super::ConservativeScanSite) -> Self { | ||
| super::record_scan_fallback(site); | ||
| let engaged = CONSERVATIVE_STACK_SCAN_OVERRIDE.with(|c| { | ||
| if c.get().is_some() { | ||
| return false; | ||
| } | ||
| c.set(Some(ConservativeStackScanMode::Full)); | ||
| true | ||
| }); | ||
| Self { engaged } | ||
| } | ||
| } | ||
|
|
||
| impl Drop for ManualGcScanGuard { | ||
| fn drop(&mut self) { | ||
| if self.engaged { | ||
| CONSERVATIVE_STACK_SCAN_OVERRIDE.with(|c| c.set(None)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add #[must_use] to ManualGcScanGuard.
ManualGcScanGuard relies entirely on Drop to un-pin the conservative scan mode. If a caller writes ManualGcScanGuard::force_full_scan(site); as a bare statement instead of binding it (let _guard = ...), the temporary drops at the end of that statement and the conservative scan never actually covers the intended work. The guard's own docstring says this exact class of bug (a missing precise root rescued only by the conservative scan) is the #4977 issue this guard was built to prevent, so a silent misuse here reintroduces that bug.
Mark the type #[must_use] so misuse produces a compiler warning.
🛡️ Proposed fix
+#[must_use = "binding this guard to `_` or a name keeps the scan mode pinned for its intended scope; a bare statement drops it immediately"]
pub(crate) struct ManualGcScanGuard {
engaged: bool,
}📝 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.
| pub(crate) struct ManualGcScanGuard { | |
| engaged: bool, | |
| } | |
| impl ManualGcScanGuard { | |
| pub(crate) fn force_full_scan(site: super::ConservativeScanSite) -> Self { | |
| super::record_scan_fallback(site); | |
| let engaged = CONSERVATIVE_STACK_SCAN_OVERRIDE.with(|c| { | |
| if c.get().is_some() { | |
| return false; | |
| } | |
| c.set(Some(ConservativeStackScanMode::Full)); | |
| true | |
| }); | |
| Self { engaged } | |
| } | |
| } | |
| impl Drop for ManualGcScanGuard { | |
| fn drop(&mut self) { | |
| if self.engaged { | |
| CONSERVATIVE_STACK_SCAN_OVERRIDE.with(|c| c.set(None)); | |
| } | |
| } | |
| } | |
| #[must_use = "binding this guard to `_` or a name keeps the scan mode pinned for its intended scope; a bare statement drops it immediately"] | |
| pub(crate) struct ManualGcScanGuard { | |
| engaged: bool, | |
| } | |
| impl ManualGcScanGuard { | |
| pub(crate) fn force_full_scan(site: super::ConservativeScanSite) -> Self { | |
| super::record_scan_fallback(site); | |
| let engaged = CONSERVATIVE_STACK_SCAN_OVERRIDE.with(|c| { | |
| if c.get().is_some() { | |
| return false; | |
| } | |
| c.set(Some(ConservativeStackScanMode::Full)); | |
| true | |
| }); | |
| Self { engaged } | |
| } | |
| } | |
| impl Drop for ManualGcScanGuard { | |
| fn drop(&mut self) { | |
| if self.engaged { | |
| CONSERVATIVE_STACK_SCAN_OVERRIDE.with(|c| c.set(None)); | |
| } | |
| } | |
| } |
🤖 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 `@crates/perry-runtime/src/gc/roots/scan_mode.rs` around lines 98 - 122, Add
#[must_use] to the ManualGcScanGuard struct declaration so callers receive a
compiler warning when force_full_scan returns an unbound guard that would
immediately drop. Leave force_full_scan and Drop behavior unchanged.
| /// Total conservative-scan fallbacks across the four **automatic** sites. This | ||
| /// is the number #7148 wants driven to zero: explicit `gc()` is a user request | ||
| /// and is not part of the claim. | ||
| #[cfg(test)] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix "four automatic sites" doc comment — only 3 variants are automatic.
is_automatic() (Lines 107-114) returns true for exactly three variants: OldReclaimSlackValve, NurseryChurnSlackValve, EmergencyReclaim. ManualCollect and ManualMinor are explicit. This doc comment says "the four automatic sites", which is inconsistent with the enum in this same file — likely a leftover from before the HostPressure variant (previously automatic) was removed. Given this PR's stated goal is precise, auditable fallback-site accounting, this comment should say "three".
📝 Proposed fix
-/// Total conservative-scan fallbacks across the four **automatic** sites. This
+/// Total conservative-scan fallbacks across the three **automatic** sites. This
/// is the number `#7148` wants driven to zero: explicit `gc()` is a user request
/// and is not part of the claim.📝 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.
| /// Total conservative-scan fallbacks across the four **automatic** sites. This | |
| /// is the number #7148 wants driven to zero: explicit `gc()` is a user request | |
| /// and is not part of the claim. | |
| #[cfg(test)] | |
| /// Total conservative-scan fallbacks across the three **automatic** sites. This | |
| /// is the number `#7148` wants driven to zero: explicit `gc()` is a user request | |
| /// and is not part of the claim. | |
| #[cfg(test)] |
🤖 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 `@crates/perry-runtime/src/gc/scan_fallback.rs` around lines 224 - 227, Update
the documentation comment above the conservative-scan fallback counter to say
there are three automatic sites, matching the variants classified as automatic
by is_automatic(). Leave the clarification that explicit gc() requests are
excluded unchanged.
The full release suite caught the design error: deferring the allocation-point old-gen reclaim breaks #5476's regression test, which asserts that a SINGLE gc_check_trigger call completes the reclaim. That guarantee exists because the workload it was filed for is a compute-only loop reaching no host step, and the bug title was 'RSS climbs unbounded'. Two reasons the deferral was wrong here: 1. The headline RSS argument does not apply. The +364%..+5371% figure is the cost of the copying minor becoming ineligible; this arm runs a FULL mark-sweep, non-moving either way. The scan costs conservative retention for one cycle, not evacuation -- much smaller, and unmeasured. 2. It trades a measured RSS guarantee for an unmeasured one. So the alloc-point arm is unchanged and counted, and gc_safepoint_moving_minor keeps the precise full mark-sweep it gained -- programs reaching a safepoint get the reclaim precisely and NO LATER than before. The fallback is attacked by adding a competing earlier precise path, not by postponing the collection. Removes the old-gen deferral state and the test-only slack override with it. Default-robustness for #7161 (proposed PERRY_GC_MOVING_LOOP_POLLS flip to OFF), now tested in both directions: - old-gen alloc-point arm: does not read the gate at all -> identical behaviour and identical census under either default. - host pressure precise inline collection: does not read the gate -> unaffected. - host pressure deferral: js_gc_loop_safepoint goes no-op, but the lowered arena trigger still makes the next allocation-point check collect the owed cycle; test asserts that backstop directly. - nursery-churn valve: pre-existing code already skips the deferral when polls are off, so every nursery collection scans conservatively -- inert and sound, but the census now makes that cost visible.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-runtime/src/gc/tests/scan_fallback.rs (1)
351-357: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the deferred old-reclaim request was consumed and attributed.
gc_collection_count()can increase for another collection. Also assert thatGC_OLD_RECLAIM_PENDINGis clear and thatOldReclaimAllocPointhas count1aftergc_check_trigger().Proposed test assertions
assert!( gc_collection_count() > collections_before, "LIVE SUBJECT: the deferred critical-pressure cycle actually ran" ); + assert!( + !GC_OLD_RECLAIM_PENDING.with(std::cell::Cell::get), + "the allocation-point backstop must consume the deferred old reclaim" + ); + assert_eq!( + scan_fallback_count(ConservativeScanSite::OldReclaimAllocPoint), + 1, + "the polls-off backstop must retain its fallback attribution" + );🤖 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 `@crates/perry-runtime/src/gc/tests/scan_fallback.rs` around lines 351 - 357, Extend the assertions after gc_check_trigger() in the fallback test to verify that GC_OLD_RECLAIM_PENDING is cleared and OldReclaimAllocPoint records exactly one event. Keep the existing collection-count assertion, but use the pending flag and attribution counter to confirm the deferred old-reclaim request was consumed by the trigger.
🤖 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.
Nitpick comments:
In `@crates/perry-runtime/src/gc/tests/scan_fallback.rs`:
- Around line 351-357: Extend the assertions after gc_check_trigger() in the
fallback test to verify that GC_OLD_RECLAIM_PENDING is cleared and
OldReclaimAllocPoint records exactly one event. Keep the existing
collection-count assertion, but use the pending flag and attribution counter to
confirm the deferred old-reclaim request was consumed by the trigger.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f23b9f03-2fb5-47a7-b482-cc748452f59d
📒 Files selected for processing (3)
crates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/scan_fallback.rscrates/perry-runtime/src/gc/tests/scan_fallback.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/perry-runtime/src/gc/scan_fallback.rs
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 `@changelog.d/7166-gc-safepoint-deferral-scan-census.md`:
- Line 5: Update the Markdown text in the changelog entry so the issue reference
no longer begins with an unescaped hash; prefix `#7148` with descriptive text such
as “Issue” while preserving the existing meaning.
🪄 Autofix (Beta)
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: 9fde6674-2e51-4ef7-9a9c-772b5a4b82dc
📒 Files selected for processing (1)
changelog.d/7166-gc-safepoint-deferral-scan-census.md
|
|
||
| `main` reached a forced conservative native-stack scan from **six** sites, **four of them without any user calling `gc()`**, and nothing measured whether any of them ever ran. A forced scan is not merely an imprecision: it makes the copying minor ineligible (`CopiedMinorFallbackReason::ConservativeStack`), so the cycle it covers runs **no copying minor at all**. `benchmarks/gc_ratchet/README.md` quantifies the end state — `heap_used_bytes` **+364% to +5371%** and `minor_cycles` → **0 on all eight probes**. | ||
|
|
||
| #7148 asks for the fallback to become *unreachable*, not *imprecise*. Per site: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Markdown issue reference.
Line 5 starts with #7148 without a space, which triggers markdownlint MD018. Prefix the reference with Issue or escape the hash.
Proposed fix
-#7148 asks for the fallback to become **unreachable**, not *imprecise*.
+Issue `#7148` asks for the fallback to become **unreachable**, not *imprecise*.🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 5-5: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 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/7166-gc-safepoint-deferral-scan-census.md` at line 5, Update the
Markdown text in the changelog entry so the issue reference no longer begins
with an unescaped hash; prefix `#7148` with descriptive text such as “Issue” while
preserving the existing meaning.
Source: Linters/SAST tools
proggeramlug
changed the title
Measured resultsHost: MacBook (darwin-arm64), release, Build discipline: one Suite
The second row is the #7148 escape-hatch item: 134 → 0. ★ Census — the automatic fallbacks are already unreachable on this corpusEvery
Two things to read off this. The drain column is the live-subject assertion, and it checks out exactly. The automatic fallbacks were already unreachable here before this PR. The gc-ratchet, 7 repeats (matching the pinned baseline)
★ RSS — flat, and that is the correct answer
No RSS win, and none was available — see the census. A change that moved RSS SabotageFive one-hunk sabotages on the finished tree, each reddening exactly the tests Not measured
|
GC × repsel stress matrix —
|
| arm | requires | collected | moved-objects | copy-minor |
|---|---|---|---|---|
evac_minor |
move | 26/27 | 26/27 | 26/27 |
force_evac |
move | 26/27 | 26/27 | 26/27 |
force_verify |
move | 26/27 | 26/27 | 26/27 |
loop_polls |
move | 26/27 | 26/27 | 26/27 |
rep_*_off (7 arms) |
move | 26/27 | 26/27 | 26/27 |
default |
scavenge | 21/27 | 21/27 | 21/27 |
cons_scan_off |
scavenge | 21/27 | 21/27 | 21/27 |
cons_scan_off_force |
scavenge | 21/27 | 21/27 | 21/27 |
The moved-objects and copy-minor columns are the point: an arm that
"passes" while relocating nothing certifies nothing (#6942/#6946/#6950), and
none of the moving arms is in that state here.
cons_scan_off sitting at exactly the default numbers is the expected
result and worth stating explicitly, because #7148 flags this arm's evidence as
circular: PERRY_CONSERVATIVE_STACK_SCAN=off beats the per-thread override
and so disables all the force_full_scan sites at once, which means the arm
reproduces "no conservative scan" by construction rather than by anything this
PR did. It is a consistency check, not evidence for the change — the evidence
for the change is the census in the previous comment, taken with no GC env
set at all.
Caveats
- Run on the same loaded host (load 40–88). The matrix's verdicts are
byte-exactness and liveness, both load-independent. - UNVER=62 was not A/B'd against
main. UNVER means "output matched but the
arm was inert on this row", which is a property of the corpus and the arm, not
of this diff — but I did not run the matrix onmainto confirm the count is
unchanged, so treat the absolute number as unverified.FAIL=0and the
liveness table are the load-bearing results. repsel_p4a3_numarray_growthis UNVER on 20 of 21 arms; that is the bulk of
the 62 and is pre-existing corpus behaviour, not something this PR touches.
1 participant
Closes #7148 (in part — see "What this does not do").
mainhad sixforce_full_scan()sites, four reached without any usercalling
gc(). A forced conservative native-stack scan is not merelyimprecise, it is ruinous: it makes the copying minor ineligible
(
CopiedMinorFallbackReason::ConservativeStack), so the cycle it covers runsno copying minor at all.
benchmarks/gc_ratchet/README.mdmeasures theend-to-end effect:
heap_used_bytes+364% to +5371%,minor_cycles→ 0on all eight probes.
#7148's end state is that the fallback becomes unreachable (always defer to
a precise safepoint), not imprecise (collect without the scan — a soundness
regression). This PR does that for the two automatic sites where a safepoint is
reachable, and makes all six measurable so "it never fires in practice" stops
being an assumption.
Per-site disposition
PERRY_GC_MOVING_LOOP_POLLS?gc/policy.rsgc/policy.rsGC_MOVING_DEFER_SLACK_BYTESbounds the growthgc/mod.rsgc/pressure.rsblockedpathgc(),gc/policy.rsperry/gcminor(),gc/policy.rsSite 1 — the deferral was wrong, and the full suite is what said so
The first version of this PR deferred the allocation-point old-gen reclaim like
the nursery arm. The release suite failed
check_trigger_drives_old_reclaim_to_completion_without_host_stepping— #5476'sregression test, which asserts that a single
gc_check_triggercall (whatevery allocation does) drives the reclaim to completion, because the workload it
was filed for is a compute-only loop that reaches no host step. Two reasons that
verdict is right and not just an awkward test:
+364%..+5371% figure is the cost of the copying minor becoming ineligible —
minor_cycles→ 0. This arm runs a full mark-sweep, which is non-movingwith or without the scan. What the scan costs here is conservative
retention for one cycle, not the loss of evacuation. Much smaller, and I
did not measure it.
title was RSS climbs unbounded. Deferring turns "collected now" into
"collected within one 32 MB growth quantum" on exactly that workload.
So the arm is unchanged and counted, and
gc_safepoint_moving_minorkeepsthe precise full mark-sweep it gained: it used to bail on
OldReclaim("stayson its existing full mark-sweep path") and now runs the same cycle with
SkipDisabledroots. Every program that reaches a safepoint gets its old-genreclaim precisely and no later than before. The fallback is attacked by
adding a competing earlier precise path, not by postponing the collection —
which is a better reading of #7148's "unreachable, not imprecise" than the
deferral was.
Interaction with #7161 (proposed
PERRY_GC_MOVING_LOOP_POLLS→ OFF)#7161 proposes flipping the polls default OFF as a stopgap for #7154. This PR
takes no position on that. It is written so the flip cannot break it, and every
claim above is tested in both directions (
force_moving_gc_pacing/force_legacy_gc_pacing):under either default. Polls off simply means the precise safepoint path is
reached less often (microtask pump only), so the census count rises. Inert,
not unsound.
scan — never reads the gate either.
js_gc_loop_safepointas a drain, but keeps twogate-independent backstops: the outermost microtask-pump boundary (gated by
PERRY_GC_MOVING_SAFEPOINT, a different knob) and the lowered arena trigger,which makes the ordinary allocation-point check collect the owed cycle. A test
pins the second one directly, because it is the one that holds for a program
that never yields to the event loop.
gc_check_triggerskips the deferral entirely and every nursery collectionruns behind a forced scan. That is pre-existing, sound, and unchanged by this
PR — but the census now makes its cost visible, which is a number fix(gc): disable evacuating minor by default pending #7154 (use-after-free on dynamically-added fields) #7161's
reviewer may want: with polls off,
nursery_churn_slack_valvecounts everynursery collection, and that is the
minor_cycles→ 0 regime the table abovequantifies.
Nothing here becomes unsound under either default; the worst case is inert.
Site 4 — the rationale is now tested rather than assumed
The old comment said "a host may deliver the callback with unspilled locals on
the native frames above us". True — but only when there are generated frames
above us, and the module's own first paragraph says the common case is a
run-loop boundary with the JS stack unwound. So: empty shadow stack ⇒ the
precise root set is complete ⇒ collect here with no scan (the copying minor
stays eligible). Live generated frame ⇒ a safepoint is reachable ⇒ arm the
deferral and return
1, the code this API already documents as "triggerlowered, collection deferred".
Deferring unconditionally would have been wrong in the direction that matters:
memory pressure arrives when a process is idle, and an idle process reaches no
loop back-edge and pumps no microtasks, so a blanket deferral would shed nothing
before the jetsam.
Making "unreachable" measurable — and the cross-link
New
gc/scan_fallback.rscounts every one of the six sites (PERRY_GC_DIAGprints
[gc-scan-fallback] site=… automatic=… count=…) and counts everyprecise-safepoint drain that replaced one (
[gc-safepoint-drain] kind=…).Automatic and explicit sites are separated: all eight ratchet probes end with an
explicit full
gc(), so site 5 fires once per probe by construction and acensus that lumped them together would misread.
The statepoints branch (
exp/stackmap-viability,docs/statepoint-gc-experiment.md) states the invariant this measurescompliance with: a collection that skips the conservative stack scan consumes
only precise roots, so it may only begin at a declared safepoint — enforced
there with a thread-local declared-safepoint flag plus a check at the root-scan
subphase, in two levels (
heal/strictpanic). This PR is the reachabilitycensus for the same contract on
main. Census first, enforcement second: thecounts say whether the contract's "anywhere else" arm is ever taken, and the
enforcement check is what makes zero provable rather than observed. That
branch also states, in "Work deliberately left gated", that the next experiment
after the #7114 prerequisite is exactly this. The two compose.
PERRY_CONSERVATIVE_STACK_SCAN=fullFailed 134 of 1574 runtime tests on
main— a shipped escape hatch nobodyhad verified. Fixed rather than deleted, against the kill-policy's default,
for a specific reason:
=fullisgc-ratchet's validated sensitivity arm — therun that produced the 60 regression rows proving that gate can fail. Deleting it
would delete a gate's only end-to-end proof, which is the failure the
kill-policy exists to prevent, one level up.
The failures were never soundness failures. A collector test's central assertion
is "this object should have been collected", and an ambient conservative scan
retains whatever the native stack looks like a pointer to. Since #7147 the test
build has one declared scan mode and the isolation guards are its authority, so
in the test build only a pinned override now beats an env request for
Full: the env var may make the scan less aggressive than a test declared,never more.
=0still beats a pinnedFull, so the bisection hatch is intact(covered by its own test). Production binaries pin no override and are
unchanged.
Gates
A gate must assert its subject was live. Every deferral test here asserts both
that the conservative valve did not fire and that the precise collection
which replaced it did run (drain counter) — checking only the first half would
pass on a tree where the trigger never armed at all, i.e. the largest possible
regression reported as a pass.
Sabotage, five directions, each reverting one hunk on the finished tree:
host_pressure_…=fullprecedence rule revertedconservative_scan_env_full_…old_reclaim_runs_precisely_at_a_safepoint,host_pressure_defers_…old_reclaim_alloc_point_still_completes_immediately…and #5476's owncheck_trigger_drives_old_reclaim_to_completion_without_host_steppingG is the interesting one: it is the design this PR started with, and it reddens
the pre-existing #5476 test as well as the new one. It also leaves
old_reclaim_is_unchanged_when_moving_loop_polls_are_offgreen, which isexactly the property that test pins — the sabotage gates on polls being on, and
the polls-off arm is supposed to be unaffected.
What this does not do
ConservativeStackScanModeis not deleted — #7148 sequences that behindthis work and records why the matrix
cons_scan_offevidence is circular (theenv value beats the override, so that arm already disables all six sites and a
removal branch reproduces it by construction). Sites 3, 5 and 6 keep the scan;
removing those needs precise roots at an arbitrary PC, which is the statepoint
contract's job, not a deferral's.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation