docs(gc): policy gate defaults said OFF while the code defaults ON (#6987) - #6989
Conversation
gc_incremental_enabled's doc header claimed 'EXPERIMENTAL — default OFF' eight lines above a body comment reading 'DEFAULT ON (#6180 flip)'. gc_safepoint_moving_minor described its gate gc_moving_safepoint_enabled as '(default off)' when that accessor is !matches!(env, 0|off|false), i.e. default ON. Both now state the real default and name the kill switch. Also records why the incremental default is load-bearing beyond pause times: with the stepper on, budgeted cycles skip the conservative stack-scan subphase structurally, so a compiled program completes precise-roots-only cycles in its shipped config. Reasoning that assumes the automatic arms force a conservative scan is wrong by default — the mistake #6972 made while this comment still said the gate was off. gc_moving_loop_polls_enabled's 'opt-in, default OFF' is accurate and unchanged. Refs #6987
📝 WalkthroughWalkthroughThe change corrects documentation for two garbage-collection policy gates whose implementations default to ON, adds their kill-switch details, and records how the stale documentation affected reasoning about incremental collection and precise-root-only cycles. ChangesGC policy documentation
Estimated code review effort: 1 (Trivial) | ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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.
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 `@crates/perry-runtime/src/gc/policy.rs`:
- Around line 1405-1406: Update the documentation comment for
gc_moving_safepoint_enabled to list every supported disabling value—0, off, and
false—in the PERRY_GC_MOVING_SAFEPOINT kill-switch description, keeping the
default-enabled behavior documented.
🪄 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: 0fe72633-47c7-4ab6-8d40-314a77ae9df1
📒 Files selected for processing (2)
changelog.d/6989-gc-policy-default-docs.mdcrates/perry-runtime/src/gc/policy.rs
| /// gated by `gc_moving_safepoint_enabled` (**default ON**; the kill switch is | ||
| /// `PERRY_GC_MOVING_SAFEPOINT=0`). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document all accepted disabling values.
The accessor is documented as disabling the gate for 0, off, or false, but this comment lists only PERRY_GC_MOVING_SAFEPOINT=0. Include all supported values so the policy comment matches the actual configuration contract.
🤖 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/policy.rs` around lines 1405 - 1406, Update the
documentation comment for gc_moving_safepoint_enabled to list every supported
disabling value—0, off, and false—in the PERRY_GC_MOVING_SAFEPOINT kill-switch
description, keeping the default-enabled behavior documented.
Fixes #6987.
What
Two GC policy gates documented themselves as default OFF while defaulting ON.
gc_incremental_enabled(policy.rs:291)DEFAULT ON (#6180 flip)— stated in its own body comment eight lines belowgc_safepoint_moving_minor(policy.rs:1396)gc_moving_safepoint_enabled(default off)"!matches!(env, "0"|"off"|"false")— default ONgc_moving_loop_polls_enabled(policy.rs:320) also says "opt-in, default OFF" and that one is accurate (matches!(env, "1"|"on"|"true")) — left unchanged.Why this is not a cosmetic nit
It changed a merge decision. PR #6972 dismissed the production-reachability of #6970 on the grounds that
gc_check_triggerforces the conservative scan on both automatic arms. That reasoning depends on incremental mode being off. It is on, so budgeted cycles skip the conservative subphase structurally (gc/cycle.rs, classifier mode), independent of any guard.Measured with
PERRY_GC_TRACE=1and no environment variables set: 3 completed cycles, 2 withconservative_root_count = 0. A compiled program completes precise-roots-only cycles in its shipped configuration today.The #6972 conclusion was reached honestly from the documented behaviour and was wrong because the documentation was wrong. Two separate investigations tripped over these comments within hours of each other.
So the fix adds more than corrected defaults: it records why the incremental default is load-bearing for rooting arguments, not just pause times, so the next reader doesn't repeat the inference.
Scope
Comment-only — no behaviour change, no code touched.
A test asserting the defaults was considered and deliberately not included here. Both gates cache in a
OnceLockover a process-global env read, so an in-process assertion is order-dependent and would need either a subprocess harness or extracting the decision into a purefn(Option<&str>) -> boolhelper. That refactor touches GC policy while several rooting PRs are in flight against neighbouring code, and it is the kind of change that deserves its own review rather than riding along with a doc correction. #6987 stays open for it.Validation
cargo fmt --all -- --checkclean. No code paths modified, so no behavioural verification is claimed or needed.Summary by CodeRabbit