Remove the older collector stress-test setting, now that the newer one covers it - #7741
Open
jdalton wants to merge 3 commits into
Open
Remove the older collector stress-test setting, now that the newer one covers it#7741jdalton wants to merge 3 commits into
jdalton wants to merge 3 commits into
Conversation
A PerryTS#7154-class bug is caught or missed by the GC schedule, not the bug, so re-running one binary re-runs one schedule and explores almost nothing. PERRY_GC_SCHEDULE_SEED=<u64> makes 'should this safepoint collect?' a deterministic pseudo-random function of the seed and a per-thread safepoint ordinal, at a density set by PERRY_GC_SCHEDULE_RATE (default 0.05), and a failing seed is a reproducer. scripts/gc_schedule_fuzz.sh sweeps seeds and prints a reproduce command per failure. Coexists with PERRY_GC_ZEAL (allocation-paced deterministic stress, PerryTS#7728): when both are set, zeal's pacing owns the loop-poll arm, the seed ticks only at safepoints zeal hands down, and a collection both would force is counted as zeal's, never twice. The poll arming word (PerryTS#7735) keeps its startup seed for either mode, so a schedule-only run cannot be silently disarmed. The forced-evacuation implication is unconditional for both modes, per PerryTS#7611. The seed is printed at startup, at exit (with the PerryTS#7604 liveness counters), on panic, and from a chaining async-signal-safe fatal-signal reporter that the from-space quarantine re-layers, so SEED + PROTECT_FROMSPACE reports both the seed and the precise fault site.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThe runtime replaces GC zeal with seeded safepoint scheduling, allocation pacing, forced evacuation, instrumentation, liveness reporting, signal diagnostics, tests, fuzzing tools, smoke validation, and updated documentation. ChangesSeeded GC schedule
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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 |
PERRY_GC_SCHEDULE_RATE=1 selects every candidate safepoint, and PERRY_GC_SCHEDULE_ALLOC_KB carries the allocation pacing (PerryTS#7728) with the same 4 KB default and the same 0 escape hatch, so the removed knob has no behaviour the schedule does not reach. Two knobs differing only in how they pick safepoints are two configurations to keep exercised, and keeping both had already forced a precedence rule to stop their counters double-counting one minor. The PerryTS#7604 liveness counters move to gc/instruments.rs -- they count what the collector did, not what forced it -- and feed both the schedule's exit summary and schedule_liveness_report, which keeps the verdict's three causes and its exit 70 at the rate-1 endpoint. Below the endpoint a sampling run that forces nothing is legitimate (RATE=0 is the control arm), so no verdict is issued there. Every removed test has a schedule counterpart; three were dropped as exact duplicates. The smoke script's two stress arms run against the schedule with their non-vacuity assertions intact.
jdalton
force-pushed
the
feat/gc-zeal-removal
branch
from
August 10, 2026 02:33
096c2a1 to
1c82a54
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-runtime/src/gc/mod.rs (1)
992-1037: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winGate the liveness verdict on the main thread, as the exit summary already is.
js_gc_release_current_thread_collection_side_allocationsruns on every thread teardown.schedule::report_exit_summary()guards against non-final counters withis_main_thread_or_unrecorded()andSUMMARY_EMITTED.emit_schedule_liveness_verdict()has no such guard.Under
PERRY_GC_SCHEDULE_RATE=1, a worker thread that tears down before the main thread reads the process-global counters early. If the schedule has not yet forced a collection on any thread,schedule_verdictreturnsErrandstd::process::exit(70)terminates the whole process. The run then reports a vacuous-instrument failure that did not occur.Calling
std::process::exitfrom a worker thread also runsatexithandlers while other threads still execute.Apply the same main-thread and once-only gate.
🐛 Proposed fix
fn emit_schedule_liveness_verdict() { + // Same ownership rule as `report_exit_summary`: the counters are + // process-global, so only the thread that tears down last may judge them. + if !crate::native_handle::is_main_thread_or_unrecorded() { + return; + } match schedule_liveness_report() { None => {} Some(Ok(summary)) => eprintln!("{summary}"), Some(Err(complaint)) => { eprintln!("{complaint}"); std::process::exit(70); } } }Based on learnings: record the thread that installs the failure reporter as the runtime main thread and ensure it owns the once-only exit summary; keep the unrecorded-main-thread fallback limited to paths where the GC schedule never activates.
🤖 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/mod.rs` around lines 992 - 1037, Gate emit_schedule_liveness_verdict() behind the same main-thread and once-only ownership rules as schedule::report_exit_summary(), so worker-thread teardown cannot evaluate the verdict or call process exit. Record the thread installing the failure reporter as the runtime main thread, and keep the unrecorded-main-thread fallback only when the GC schedule never activates. Ensure the main thread owns the single exit-summary/verdict emission.Source: Learnings
🤖 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-codegen/src/lower_call/extern_timers.rs`:
- Line 35: Add PERRY_GC_SCHEDULE_SEED with a valid deterministic value to the
reproduction command in the documentation comment near the extern-timers
context, alongside PERRY_GC_SCHEDULE_RATE and PERRY_GC_PROTECT_FROMSPACE, so
scheduled collections are actually enabled and the moving-GC case remains
reproducible.
In `@crates/perry-runtime/src/gc/policy.rs`:
- Around line 2504-2517: Update gc_safepoint_moving_minor to return whether it
actually handled the safepoint, returning false on every blocked or early-exit
condition and true after successful handling. In the polling flow around
schedule_poll_collection_due, only call note_schedule_poll_collection with the
post-safepoint allocation level when the returned handled state is true;
preserve the existing paced path without consuming a schedule slot when handling
is blocked.
In `@crates/perry-runtime/src/gc/schedule.rs`:
- Around line 425-429: Update parse_schedule_alloc_kb so parsed allocation
values that would overflow or exceed a sane configured maximum are clamped or
rejected rather than converted to usize::MAX; preserve the default stride for
invalid or rejected input and ensure schedule_poll_collection_due continues
producing valid poll decisions.
In `@crates/perry-runtime/src/gc/tests/schedule.rs`:
- Around line 192-230: Protect schedule counter baselines, updates, and delta
assertions in the affected test with the shared lock held by every test that
uses these counters, rather than relying only on CopyingNurseryTestGuard. Update
the test setup around gc_schedule_safepoints() and
gc_schedule_forced_collections() to acquire the common lock, or reset both
counters to zero under that lock, so assertions such as safepoints_before + 1
cannot observe parallel-test increments.
In `@crates/perry-runtime/src/gc/tests/triggers.rs`:
- Around line 867-885: Update
a_resolved_seed_holds_the_poll_word_armed_with_nothing_pending to invoke
poll_arm::resolve_poll_seed (or js_gc_loop_safepoint) while ScheduleGuard::set
is active, then assert the poll remains armed after that call rather than
relying on startup state. Add a complementary test or branch using
ScheduleGuard::off() that invokes the same resolution path and verifies the seed
is released, covering both schedule-enabled and disabled call_once behavior.
In `@docs/src/internals/memory-model.md`:
- Around line 147-153: Correct the 600-poll example in the memory-model
documentation: do not imply that PERRY_GC_SCHEDULE_RATE=1 alone triggers
collection on every loop back-edge poll. Add PERRY_GC_SCHEDULE_ALLOC_KB=0 to the
example to explicitly enable every-poll collection, or revise the wording to say
collections occur per eligible candidate while retaining the documented default
allocation pacing.
In `@scripts/gc_schedule_fuzz.sh`:
- Around line 80-85: Update RATE handling in the script so unsupported values
are rejected or normalized before execution, and use that effective value
consistently in the run summary and reproduce command. Ensure the seeded arm
does not inherit PERRY_GC_SCHEDULE_ALLOC_KB in default mode; otherwise include
its effective value in both the summary and reproduce command. Apply the same
consistency to the schedule reporting and reproduction logic around the
referenced sections.
---
Outside diff comments:
In `@crates/perry-runtime/src/gc/mod.rs`:
- Around line 992-1037: Gate emit_schedule_liveness_verdict() behind the same
main-thread and once-only ownership rules as schedule::report_exit_summary(), so
worker-thread teardown cannot evaluate the verdict or call process exit. Record
the thread installing the failure reporter as the runtime main thread, and keep
the unrecorded-main-thread fallback only when the GC schedule never activates.
Ensure the main thread owns the single exit-summary/verdict emission.
🪄 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: 9434c9d6-2c46-4259-a4c8-65a18584f025
📒 Files selected for processing (43)
.github/workflows/test.ymlCLAUDE.mdchangelog.d/7317-seeded-gc-schedule-fuzzing.mdchangelog.d/7741-retire-gc-zeal-for-the-seeded-schedule.mdcrates/perry-codegen/src/lower_call/extern_timers.rscrates/perry-codegen/src/stmt/loops.rscrates/perry-runtime/src/arena/quarantine.rscrates/perry-runtime/src/gc/copying.rscrates/perry-runtime/src/gc/instruments.rscrates/perry-runtime/src/gc/layout.rscrates/perry-runtime/src/gc/mod.rscrates/perry-runtime/src/gc/policy.rscrates/perry-runtime/src/gc/poll_arm.rscrates/perry-runtime/src/gc/schedule.rscrates/perry-runtime/src/gc/tests/copying/deferred_finalize_7635.rscrates/perry-runtime/src/gc/tests/evacuation.rscrates/perry-runtime/src/gc/tests/fromspace_protect.rscrates/perry-runtime/src/gc/tests/mod.rscrates/perry-runtime/src/gc/tests/runtime_roots/fs_options_object.rscrates/perry-runtime/src/gc/tests/runtime_roots/generator_attach_prototype.rscrates/perry-runtime/src/gc/tests/runtime_roots/json_shape_template.rscrates/perry-runtime/src/gc/tests/runtime_roots/symbol_description.rscrates/perry-runtime/src/gc/tests/schedule.rscrates/perry-runtime/src/gc/tests/triggers.rscrates/perry-runtime/src/gc/zeal.rscrates/perry-runtime/src/iterator_helpers.rscrates/perry-runtime/src/native_handle.rscrates/perry-runtime/src/object/class_registry/construct.rscrates/perry-runtime/src/path/value_args.rsdocs/engine-plan.mddocs/src/internals/gc-rooting-invariant.mddocs/src/internals/memory-model.mddocs/src/internals/rfc-rooting-by-construction.mddocs/statepoint-gc-experiment.mdrun_parity_tests.shscripts/gc_instrument_smoke.shscripts/gc_schedule_fuzz.shtest-files/test_gap_7564_iter_result_rooting.tstest-files/test_gap_gc_call_argument_rooting.tstest-files/test_gap_gc_regexp_receiver_rooting.tstest-files/test_gap_gc_rest_argument_rooting.tstest-files/test_gap_gc_same_module_call_argument_rooting.tstest-parity/gc_repsel_corpus.txt
💤 Files with no reviewable changes (1)
- crates/perry-runtime/src/gc/zeal.rs
jdalton
changed the title
PERRY_GC_ZEAL for the seeded scheduleA blocked safepoint no longer charges the pacing stride. gc_safepoint_moving_minor reports whether it handled the safepoint and the poll arm rearms only when it did; a safepoint blocked by an entry guard consumes no schedule slot, so charging it a full stride dropped the realised density below the requested rate with nothing in the exit summary to say so. PERRY_GC_SCHEDULE_ALLOC_KB clamps at 1 GiB rather than saturating to usize::MAX. A stride nothing can ever reach leaves the poll path permanently unselected, so the run reports a clean sweep having tested nothing -- an off switch wearing an on label. The poll-word arming test was vacuous: it read the startup value, which is 1 whatever the mode is, and would have failed outright if an earlier test in the binary had already resolved the seed. The resolution is now a resettable flag instead of a std::sync::Once, and the test drives it in both directions. gc_schedule_fuzz.sh rejects an out-of-range rate instead of letting the runtime clamp it and then printing a reproduce command for a density it did not run at, and pins allocation pacing into both the runs and the printed command. Two doc lines named a rate without a seed, which is inert.
✅ Action performedComments resolved. Approval is disabled; enable |
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.
Removing the older collector stress-test setting
Perry ended up with two environment variables that do nearly the same job: force the garbage collector to run far more often than normal, so that memory bugs which normally hide behind lucky timing get shaken out. This pull request keeps the newer one and deletes the older one.
Stacked on #7317, which adds the newer one. Until that merges this branch's diff will include its commit too; I will rebase once it lands so this collapses to just the removal.
What goes away
PERRY_GC_ZEALand its companionPERRY_GC_ZEAL_ALLOC_KBare removed. Everything they did is available throughPERRY_GC_SCHEDULE_SEED:PERRY_GC_SCHEDULE_RATE=1collects at every opportunity — the same moments the old setting collected at.PERRY_GC_SCHEDULE_ALLOC_KBcarries over the old pacing behaviour unchanged, with the same 4 KB default and the same0value meaning "collect at literally every opportunity".So there is no configuration of the old setting that the new one cannot express.
Why remove it instead of keeping both
Two settings that differ only in how they choose moments to collect are two configurations somebody has to keep testing. The project's own policy in
CLAUDE.mdis blunt about this: a mode that still exists is a decision nobody has made. The repository has repeatedly paid for options that quietly stopped being exercised and turned out to have been broken for months.Keeping both had already started to cost something concrete. Because either setting could force the same collection, they needed a rule about which one got to claim it, so their counters would not both count it and make the totals meaningless. That rule exists purely to reconcile a redundancy — it buys nothing except the ability to keep two overlapping settings.
And the newer one is strictly more capable. It covers normal collection frequency at one end, the old setting's maximum frequency at the other, and everything in between — and unlike the old one, when it finds a bug it hands you a number that replays the failure.
The safety checks from the old setting all survive the move
The old setting had picked up a layer of self-checking, and none of it is lost.
There were three counters that record what a run actually did: how many collections moved objects, how many objects moved, and how many collection opportunities the program reached. These now live in
gc/instruments.rs. That is a deliberate relocation rather than a rename — they measure what the collector did, not what asked it to, so putting them inside any one setting's file is how a counter ends up being deleted along with the next setting that happens to carry it. They now also appear in the exit summary, so even a run at a low collection rate reports whether it exercised anything.There was also a verdict at exit that fails the process outright when the run turns out to have done nothing — the point being that "no failures" from a run that never actually collected is not evidence of anything. That verdict survives with the same three failure reasons and the same exit code.
One change worth flagging: the verdict now only applies at the maximum rate. That is deliberate. At the maximum rate, a run that collected nothing genuinely is broken. But at a sampling rate, a run that happens to collect nothing is a perfectly legitimate outcome — a rate of 0 is the documented "on but selects nothing" control, and a sweep seed that fires late is not a malfunction. Applying the strict verdict there would turn every quiet seed into a false alarm.
Test coverage moved rather than shrank
Every test the old setting had now has an equivalent: the pacing limit and its two checks that pacing did not simply switch collection off, the escape hatch that restores collecting at every opportunity, the rule that pacing measures from after each collection so a collection that frees nothing cannot loop forever, the guarantee that the fast-path check stays enabled so the mode cannot silently become a no-op, the guarantee that a collection moves objects, and the check that the collector's stress mode and the memory-protection tooling work together.
Three tests were deleted outright because they were exact duplicates of tests that already existed for the newer setting.
The two end-to-end checks in
scripts/gc_instrument_smoke.sh— one pairing stress with the evacuation verifier, one enforcing that the whole thing still finishes in reasonable time on a realistic workload — now run against the newer setting with their non-vacuity assertions intact. Neither can report success from a run that collected or moved nothing.How this was verified
cargo check -p perry-runtime -p perry-codegen --all-targetsreports no errors. The warnings that remain are pre-existing onmainand are byte-for-byte identical there.scripts/check_file_size.shpasses, andbash -nreports the smoke script is syntactically clean.changelog.d/, where past entries are a historical record and are left untouched.No version bump — the maintainer bumps that at merge time, per the external-contributor flow.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests