fix(gc): the seeded schedule arms the poll word, like zeal (#7781) - #7782
Conversation
PERRY_GC_SCHEDULE_RATE=1 saw 6 safepoints against zeal's 9,648 loop polls on the same reproduction: nothing kept the poll word armed for the schedule mode, so its loop-safepoint bypass sat behind a gate that never opened. resolve_poll_seed keeps the seed when the schedule is enabled, and ScheduleGuard mirrors ZealGuard's arm/release pair. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughScheduled GC now keeps the GC poll word armed at startup and during ChangesGC schedule poll arming
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-runtime/src/gc/poll_arm.rs (1)
128-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for startup schedule arming.
The new regression test exercises
ScheduleGuard::set, which callsarm_poll()directly. It does not exerciseresolve_poll_seed()orschedule::gc_schedule_enabled(). Removing this condition would still leave that test passing. Add a test for startup seed resolution with scheduled GC enabled, or factor the predicate into a separately testable helper.🤖 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/poll_arm.rs` at line 128, Add targeted coverage for the startup schedule-arming predicate near ScheduleGuard::set and arm_poll: enable scheduled GC and verify startup seed resolution through resolve_poll_seed or schedule::gc_schedule_enabled reaches the expected arming behavior. Alternatively, extract the combined gc_zeal_enabled/schedule::gc_schedule_enabled predicate into a helper and test both enabled and disabled cases, ensuring the regression test would fail if either condition were removed.
🤖 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/poll_arm.rs`:
- Line 128: Add targeted coverage for the startup schedule-arming predicate near
ScheduleGuard::set and arm_poll: enable scheduled GC and verify startup seed
resolution through resolve_poll_seed or schedule::gc_schedule_enabled reaches
the expected arming behavior. Alternatively, extract the combined
gc_zeal_enabled/schedule::gc_schedule_enabled predicate into a helper and test
both enabled and disabled cases, ensuring the regression test would fail if
either condition were removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bf4fe85a-8d9a-4ae7-aa3a-5b4cc633ae45
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
CLAUDE.mdCargo.tomlchangelog.d/7782-schedule-arms-poll-word.mdcrates/perry-runtime/src/gc/poll_arm.rscrates/perry-runtime/src/gc/schedule.rscrates/perry-runtime/src/gc/tests/triggers.rs
…poll saturates off()-disarm-then-Drop-rearm leaks +1 permanently when the disarm lands on a zero word (saturation loses the decrement, the paired arm does not). The leak pinned the poll armed for the rest of the test binary: the timing test slowed and the generation-gate contract took a safepoint drain mid-stage, 2/2 consistently. Only set() arms; only its own Drop releases. 3/3 full-suite runs clean after. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Merging as v0.5.1451 — and this is the #7741 precondition, demonstratedFound testing the maintainer's stated condition for #7741: that
Both clean under The bug I shipped inside the first version of this fix, kept on the recordThe first The bookkeeping is now deliberately asymmetric — only The saturation that caused this is itself correct (#7735 chose it so an underflow could not pin the word armed via wrap) — the lesson is that saturating operations break pair-wise symmetric protocols, and any future arm/disarm pair around this word needs the same asymmetric shape.
#7741 is now unblocked on its stated condition. |
Fixed
The seeded GC schedule never armed the poll word, so
PERRY_GC_SCHEDULE_RATE=1was an event-loop-boundary instrument only (gc: the seeded schedule never arms the poll word — SCHEDULE_RATE=1 sees 6 safepoints where zeal sees 9,648 (blocks #7741) #7781). On gc: two rooting gap tests crash on macOS under PERRY_NO_AUTO_OPTIMIZE=1 while Linux passes them (untriaged) #7606's reproduction it saw 6 safepoints where zeal's runs cross 9,648 / 19,248 loop polls — the "collect at every opportunity" end of the dial saw six opportunities. perf(gc): the loop back-edge poll's no-work path becomes one global load (#7721) #7735 collapsed the back-edge poll's no-work path to one load ofPERRY_GC_POLL_ARMED;resolve_poll_seedkept the word armed for zeal, and nothing armed it for the schedule mode, so the loop-safepoint bypass Make a rare garbage-collector bug reproducible on demand (PERRY_GC_SCHEDULE_SEED) #7317 added sat behind a gate that never opened.resolve_poll_seednow keeps the startup seed whenschedule::gc_schedule_enabled(), exactly as for zeal, andScheduleGuardmirrorsZealGuard's arm/release pair so test-time schedules reach polls too. Regression testthe_schedule_holds_the_poll_word_armed_like_zealmirrors the zeal test and is sabotage-verified (removing the arm fails it).Re-run with the fix, same binary, same quarantine (
PERRY_GC_PROTECT_FROMSPACE=1, depth 800):rest_argument_rootingsame_module_call_rootingThis was found testing Remove the older collector stress-test setting, now that the newer one covers it #7741's precondition — that
SCHEDULE_RATE=1be demonstrated equivalent to zeal on a real reproduction before the older instrument is deleted. The precondition now holds; without this fix it measurably did not.Summary by CodeRabbit
Bug Fixes
Documentation
Chores