gc: the evacuation knobs stop lying about what they gate (#7611, #6946, #7604) - #7684
Conversation
71a4874 to
20d3edb
Compare
|
Warning Review limit reached
Next review available in: 3 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
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 |
#7604) Three knobs believed to exercise something that did not. #7611 -- PERRY_GEN_GC_EVACUATE is DELETED. Measured on the pinned quiet host, identical binaries and protocol, only the knob differing: 0 of 96 gc-ratchet cells moved (12 probes x 8 counters), bit-identical medians, `check` exit 0. PERRY_GEN_GC=0 moved 79 cells by the same procedure, so the harness was sensitive and this knob was not. Mechanism confirmed from the code: it gated the non-copying fallback's policy evacuation, while every counter the ratchet reads comes from the copying minor it never gated. Its one unique live effect was a footgun -- it vetoed `gc_force_evacuate_enabled()`, so an ambient `PERRY_GEN_GC_EVACUATE=0` silently disarmed PERRY_GC_ZEAL. The branch it gated is NOT deleted with it: `evacuation_policy_allowed` is still false on every budgeted low-pause cycle, and `budgeted_low_pause_minor_does_not_evacuate` asserts that arm behaviourally. #6946 -- an explicit `gc()` under PERRY_GC_FORCE_EVACUATE now runs an EVACUATING minor before the full mark-sweep. The knob is read only on the minor path while `gc()` ran a full sweep, which never evacuates, so five suites that drive collection as `gc(); assertFreed()` under it exercised no evacuation at all. #7657 removed the forced conservative scan that made a copying minor ineligible here, so this is now possible; `FullEscalation::Refused` keeps the pacing predicates from turning it back into a non-moving sweep. #7604 -- zeal liveness is now readable and a vacuous zeal run FAILS. `zeal_forced_collections()` was unreachable from a compiled program: no JS API, no diagnostic line, no exit report, while CLAUDE.md told operators to check it. The only alternative, PERRY_GC_DIAG=1, wrote 212 MB of stderr in ten minutes on a 400k-iteration probe. Two process-global counters are added (`copying_minor_cycles`, `moved_objects_total`) and the exit boundary prints `[gc-zeal] forced_collections=N copying_minors=M moved_objects=K`, exiting 70 when N or M is zero. Shown able to fail: removing the forced minor from `manual_gc_collect_now` turns `explicit_gc_under_forced_evacuation_runs_a_moving_minor` red with `before=0 after=0` (0 compile errors, `Running unittests` present); the zeal verdict has three unit tests covering both failure causes and the pass case.
#7611 deleted the knob but memory-model.md still listed it as a live escape hatch and the zeal row still said it could veto forced evacuation. A documented knob that no longer exists is the same defect the deletion was for. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Audit — merging as v0.5.1392Three knobs, three different verdicts, each with the measurement that produced it. That is the right output for this class. #7611 — deleted, and for a better reason than "unexercised"The measurement stood (0 of 96 cells, with The footgun is what justifies deletion over an arm. It vetoed Keeping #6946 — made real; #7604 — the measurement was inert, not the knob
And #7604's three findings are worth more than the issue was:
The verdict exiting 70 rather than printing a line is the correct shape, and your own note is the proof: the compute-only run reads One thing I fixed on the way in
Gates: 24/24 lint, fmt clean, Your correction that #7666's large-Eden escalation was not the mechanism for #7604 on any workload you could reach is noted — I had offered it as a lead and it didn't hold. |
20d3edb to
617fe86
Compare
Closes #7611. Closes #6946. Closes #7604.
Three knobs believed to exercise something that did not. One is deleted, one
is made real, one is made checkable. Each is reported with what was
measured, whether the knob or the measurement was inert, and how the result is
shown able to fail.
#7611 —
PERRY_GEN_GC_EVACUATEis deletedThe knob was inert where anyone was looking; the measurement was sound.
#7611's measurement stands: identical binaries and protocol on the pinned quiet
host, only the knob differing, 0 of 96 gc-ratchet cells moved — bit-identical
medians across 12 probes × 8 counters,
checkexit 0.PERRY_GEN_GC=0moved 79cells by the same procedure, so the harness was sensitive and this knob was not.
The mechanism #7611 flagged as unconfirmed is confirmed, from the code. The
knob gated
evacuation_policy_allowed: the C4b tenured→old-gen policyevacuation in the non-copying fallback minor, plus old-page defrag selection.
Every counter the ratchet reads (
copied_objects,copied_bytes,promoted_*)comes from
gc_collect_minor_copying_fast_path, which the knob never gated andwhich is reached before the fallback path.
Deleted rather than given an arm, per the binding kill-policy, and for a
reason beyond "nobody exercises it": its one unique live effect was a footgun. It
vetoed
gc_force_evacuate_enabled(), so an ambientPERRY_GEN_GC_EVACUATE=0silently disarmed
PERRY_GC_ZEAL— the #7154 instrument.zeal_implies_forced_evacuationused to take that precedence arm and
returnwithout exercising zeal at all,which is the vacuous-green shape the policy exists to catch. It now has one arm
and that arm always runs.
The branch is not deleted with the knob, because the branch has another
controller that is exercised.
evacuation_policy_allowedis still false onevery budgeted low-pause cycle, and
budgeted_low_pause_minor_does_not_evacuateasserts that arm behaviourally — nothing moved, no forwarding stub, old-page
selection skipped,
trace.evacuation_policy.reason == "low_pause_non_moving".What stopped existing is the untested configuration, not the code path.
#6946 —
PERRY_GC_FORCE_EVACUATEon thegc()path is fixedThe knob was genuinely inert here. It is read only on the minor path while
manual_gc_collect_nowran a full mark-sweep, which never evacuates. Five suitesstill drive collection as
gc(); assertFreed()under it and exercised noevacuation at all.
An explicit
gc()under the knob now runs an evacuating minor first, thenthe full sweep. #7657 is what made this possible: the site used to take
ManualGcScanGuard::force_full_scan, and a forced conservative scan makes thecopying minor ineligible outright (
CopiedMinorFallbackReason::ConservativeStack).With precise roots the copying minor here is exactly as sound as the full sweep
that follows.
New
FullEscalation::Refusedkeeps the two throughput-pacing predicates(
copied_minor_promotion_handoff_due,arena_growth_full_escalation_due) fromturning that minor back into a non-moving full sweep — the original bug in a new
place. It is scoped to this one caller; every automatic path keeps
Allowed, sothe array-growth-stub reclamation those predicates exist for is untouched.
Default-off knob, so an ordinary
gc()is unchanged.Shown able to fail
explicit_gc_under_forced_evacuation_runs_a_moving_minorasserts both that acopying minor completed and that the survivor's address changed — the counter
without the address cannot tell a copying minor that moved nothing from one that
moved this object, and the address without the counter cannot tell a copying
minor from any other reason a slot changed.
Deleting the forced minor from
manual_gc_collect_nowand re-running:error[count 0 andRunning unittestspresent, so the red is the assertion andnot a build break.
#7604 — zeal liveness is readable, and a vacuous zeal run now fails
Here the measurement was the inert thing, and the underlying worry was still
right — with a cause the issue did not name.
What does not reproduce on current
main(v0.5.1384). Onbenchmarks/gc_ratchet/probes/01_nursery_churn.ts, compiled withPERRY_GC_MOVING_LOOP_POLLS=1and run underPERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_DIAG=1, the recipeproduced 741,630
[gc-copy-minor] ranlines with realcopied_objects, and[gc-fromspace-protect] … retired_set=#Nthroughout. The knob is not inert.(The issue's own benches,
gc-handoff/bench/*.ts, are not in the repo, so itsexact workload could not be re-run.)
What is real, and is worse than "the recipe sometimes doesn't fire".
"check
crate::gc::zeal_forced_collections()is nonzero". There was no JSAPI, no diagnostic line and no exit report through which to read it from a
compiled program. The only alternative,
PERRY_GC_DIAG=1plus grep, wrote212 MB of stderr in ten minutes on that same 400k-iteration probe — so it
was not a usable check either.
compute-only loop compiled with the flag contains zero
js_gc_loop_safepointcall sites, because codegen deliberately omits the pollfor provably alloc-free bodies (
loop_purity::loop_may_allocate) andincidentally omits it for the specialized
for/for-of/for-inlowerings (
emit_gc_loop_safepoint's own COVERAGE note). Under zeal thatbinary collected 5 times, at event-loop boundaries only — and nothing said so.
nm/objdump -d BIN | grep -c js_gc_loop_safepointreports 0 on a binary whose polls then fire 20,064times. An operator following that advice concludes the polls are absent when
they are not.
The fix. Three process-global counters —
copying_minor_cycles(),moved_objects_total(),loop_polls_reached()— and a verdict at theprocess-exit boundary that exits 70 when the instrument did not fire.
Note what a two-counter verdict would have done with the second run:
forced=5,copying_minors=5,moved_objects=4— every counter says "live".Only
loop_pollssays the loop was never covered. That is why there are three.Stated limitations rather than hidden ones:
process.exit()terminates vialibc::_exitand never reaches the exit boundary, as does an uncaught throw.Shown able to fail
zeal_verdict:forced=0;cycles=0(everyforced collection escalated to a non-moving full sweep, which
forced > 0alone calls live);
loop_polls=0with polls requested, using the measuredcompute-only counters; the passing case with its numbers; and a pin that
cycles>0, moved=0is not a failure andloop_polls=0without the requestis fine — so a future "tighten it" edit has to argue with a test.
it previously exited 0 silently, the allocating probe exits 0 with its numbers,
and an unzealed run of the same binary prints no
[gc-zeal]line at all.Validation
cargo test -p perry-runtime --lib --no-fail-fastcargo test -p perry-codegen --lib --no-fail-fastnative_root_coveragetestscargo check --all-targetslint-job commands, each exit status checkedrustup run stable cargo fmt --all -- --checkerror[counts andRunning unittestspresence were checked on every test run,so a green line is a run that happened rather than a build that did not.
CLAUDE.md
Three corrections, content only — no version bump:
PERRY_GEN_GC_EVACUATEand gains thegc()-pathbehaviour of
PERRY_GC_FORCE_EVACUATE;PERRY_GEN_GC_EVACUATE=0precedence caveat (there is nosuch knob), gains the two ways codegen emits no poll, and replaces "check
zeal_forced_collections()" — which was not performable — with the exitverdict;
PERRY_GC_FORCE_EVACUATEprecedent is marked closed andkept, because it is the argument for the policy.