docs(gc-rooting): #7211 is fixed, and add the runtime-cache class - #7281
Conversation
… class Three corrections to the GC rooting invariant documentation: 1. Remove the "And the one that is still open (PerryTS#7211)" section. Issue PerryTS#7211 (ClassExprFresh rooting predicate) was fixed in PerryTS#7226, and the allowlist is now empty, verified by scripts/gc_root_dominance_allowlist.json. 2. Add the runtime-cache class as class 5 to the taxonomy. This class describes thread-local or static cells holding GC pointers with no registered scanner, distinguished by failing 10/10 rather than intermittently. Real instances: PerryTS#7226 (js_value_typeof strings, rawJSON key) and PerryTS#7239 (ten runtime caches). 3. Update the hazard-2 paragraph to reference PerryTS#7226 as the PR that fixed PerryTS#7211, and clarify that the PerryTS#7211 entries were deleted when the predicate was fixed. 4. Update the "Root before you call" rule to reference PerryTS#7211 specifically rather than "bug PerryTS#5" since the numbering has changed. Verified against: - Issue PerryTS#7211 is closed - scripts/gc_root_dominance_allowlist.json is empty - PerryTS#7226 commit message documents the runtime-cache class - CLAUDE.md:253 consistent with these changes
📝 WalkthroughWalkthroughThe GC-rooting invariant documentation now describes five failure modes, including unscanned runtime caches. It also updates gate-promotion status and explains the ChangesGC-rooting invariant documentation
Estimated code review effort: 1 (Trivial) | ~4 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/src/internals/gc-rooting-invariant.md`:
- Around line 115-119: Revise the next section’s description of
scripts/gc_root_dominance_check.py to state that it detects only emitted-LLVM
rooting hazards, not runtime-table cache issues. Keep PERRY_GC_ZEAL,
PERRY_GC_PROTECT_FROMSPACE, and PERRY_GC_PROTECT_FROMSPACE_DEPTH as the checks
for runtime caches, and preserve the existing guidance to register mutable
roots.
- Around line 98-103: Update the “Runtime-cache class” description to avoid
claiming failure always occurs at collection `#0`. Explain that the pointer
becomes stale at the first moving collection after the cache is populated and
remains stale until code rewrites the cache, while preserving the distinction
from register-class bugs.
- Line 53: Run perry-doc-tests against docs/src and update all 13 untagged
TypeScript code fences in the documentation: mark illustrative fences with the
typescript,no-test tags, or replace them with {%#include ...%} directives where
the content should be sourced from a file.
🪄 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: ece7df8c-adfc-4980-8643-99ab98cd6f80
📒 Files selected for processing (1)
docs/src/internals/gc-rooting-invariant.md
| cost every time. | ||
|
|
||
| ## The four ways it has actually broken | ||
| ## The five ways it has actually broken |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cargo run -p perry-doc-tests -- --lint docs/srcRepository: PerryTS/perry
Length of output: 143
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching linter/doc-test names:\n'
git ls-files | rg -i 'perry-doc-tests|doc.*(test|lint)|lint' | head -200 || true
printf '\nCargo manifests mentioning perry-doc-tests:\n'
rg -n 'perry-doc-tests|doc-tests|lint' Cargo.toml Cargo.lock -S || true
printf '\nChanged/added file context:\n'
rg -n 'typescript|ts|js|markdown-magic|remark' docs/src/internals/gc-rooting-invariant.md -S -C 2 || true
printf '\nRepository commands/scripts:\n'
git ls-files | rg '(^|/)(package\.json|README\.md|CONTRIBUTING\.md|Makefile|justfile|taskfile\.ya?ml|scripts/.*\.sh|nixpacks|deno\.json|bun\.lockb)' | xargs -r grep -n 'cargo run -p perry-doc-tests\|--lint\|perry-doc-tests\|rustfmt\|clippy' 2>/dev/null || trueRepository: PerryTS/perry
Length of output: 143
🏁 Script executed:
cargo run -p perry-doc-tests -- --lint docs/srcRepository: PerryTS/perry
Length of output: 143
🏁 Script executed:
cargo run -p perry-doc-tests -- --lint docs/srcRepository: PerryTS/perry
Length of output: 143
🏁 Script executed:
cargo run -p perry-doc-tests -- --lint docs/srcRepository: PerryTS/perry
Length of output: 143
🏁 Script executed:
cargo run -p perry-doc-tests -- --lint docs/srcRepository: PerryTS/perry
Length of output: 398
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cargo run -p perry-doc-tests -- --lint docs/srcRepository: PerryTS/perry
Length of output: 398
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rustup default stable || true
cargo run -p perry-doc-tests -- --lint docs/srcRepository: PerryTS/perry
Length of output: 143
🏁 Script executed:
cargo run -p perry-doc-tests -- --lint docs/srcRepository: PerryTS/perry
Length of output: 5134
Run perry-doc-tests before merge.
cargo run -p perry-doc-tests -- --lint docs/src reports 13 untagged ts/typescript fences in the docs src tree. Tag those fences as typescript,no-test or make them {%#include ...%} directives.
🤖 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 `@docs/src/internals/gc-rooting-invariant.md` at line 53, Run perry-doc-tests
against docs/src and update all 13 untagged TypeScript code fences in the
documentation: mark illustrative fences with the typescript,no-test tags, or
replace them with {%#include ...%} directives where the content should be
sourced from a file.
Source: Learnings
| ### 5. Runtime-cache class (#7226, #7239) | ||
|
|
||
| `Expr::ClassExprFresh` roots its class object only when it believes the static | ||
| *initializers* can collect: | ||
| A thread-local or static cell holding a GC pointer that no registered scanner | ||
| rewrites. Unlike the register-class bugs above (which go bad intermittently when a | ||
| collection lands in a narrow window), a runtime cache goes bad at collection #0 | ||
| and stays bad. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe the first moving collection, not collection #0.
A lazy cache can be populated after earlier collections. Code can also rewrite the cache later. State that the pointer becomes stale at the first moving collection after population and remains stale until rewritten.
Proposed wording
- a runtime cache goes bad at collection `#0`
- and stays bad.
+ an unregistered runtime cache becomes stale at the first moving
+ collection after it is populated and stays stale until rewritten.📝 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.
| ### 5. Runtime-cache class (#7226, #7239) | |
| `Expr::ClassExprFresh` roots its class object only when it believes the static | |
| *initializers* can collect: | |
| A thread-local or static cell holding a GC pointer that no registered scanner | |
| rewrites. Unlike the register-class bugs above (which go bad intermittently when a | |
| collection lands in a narrow window), a runtime cache goes bad at collection #0 | |
| and stays bad. | |
| ### 5. Runtime-cache class (`#7226`, `#7239`) | |
| A thread-local or static cell holding a GC pointer that no registered scanner | |
| rewrites. Unlike the register-class bugs above (which go bad intermittently when a | |
| collection lands in a narrow window), an unregistered runtime cache becomes stale | |
| at the first moving collection after it is populated and stays stale until rewritten. |
🤖 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 `@docs/src/internals/gc-rooting-invariant.md` around lines 98 - 103, Update the
“Runtime-cache class” description to avoid claiming failure always occurs at
collection `#0`. Explain that the pointer becomes stale at the first moving
collection after the cache is populated and remains stale until code rewrites
the cache, while preserving the distinction from register-class bugs.
| **`scripts/gc_root_dominance_check.py` is structurally blind to this class** — it | ||
| reads emitted LLVM IR and cannot see a runtime table. The instruments that catch it | ||
| are `PERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800` | ||
| on a real workload. When adding a cache of a heap pointer, register it in | ||
| `gc_register_mutable_root_scanner` in `gc/mod.rs` in the same commit. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clarify the static-checker scope.
This paragraph says that scripts/gc_root_dominance_check.py cannot detect runtime tables. The next section at Line 125 says that the static checker is the only instrument that sees “this class” before a crash. Revise that statement to limit the checker to emitted-LLVM rooting hazards. Keep GC zeal and from-space protection as the runtime-cache checks.
🤖 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 `@docs/src/internals/gc-rooting-invariant.md` around lines 115 - 119, Revise
the next section’s description of scripts/gc_root_dominance_check.py to state
that it detects only emitted-LLVM rooting hazards, not runtime-table cache
issues. Keep PERRY_GC_ZEAL, PERRY_GC_PROTECT_FROMSPACE, and
PERRY_GC_PROTECT_FROMSPACE_DEPTH as the checks for runtime caches, and preserve
the existing guidance to register mutable roots.
2 participants
docs/src/internals/gc-rooting-invariant.mdteaches contributors the rooting invariant using real bugs as case studies, and it had drifted from reality in two ways that matter.#7211 is closed, not open. The page carried a section titled "And the one that is still open (#7211)". It was fixed by #7226, and the checker's allowlist is now empty (
scripts/gc_root_dominance_allowlist.jsonreads"entries": [], annotated "EMPTY IS THE GOAL AND IT IS CURRENTLY MET"). The stale section is removed and the hazard-2 paragraph now cites #7226.The runtime-cache class was missing from the taxonomy. It is added as a named class with its tell — a thread-local or static cell holding a GC pointer that no registered scanner rewrites — citing #7226 (the interned
typeofresult strings and the rawJSON key) and #7239 (ten runtime caches: CACHED_ENV, CACHED_PERMISSION, CACHED_REPORT, ERROR_CONSTRUCTOR_PTR, INPUT_HANDLER, RESIZE_CALLBACK, FRAME_CALLBACKS, CURRENT_NEW_TARGET, ACCESSOR_RECEIVER_OVERRIDE, PENDING_FETCH_SIGNAL).This one is worth naming explicitly because it is invisible to the static checker by construction: the checker reads emitted IR, and a runtime-side cache never appears there. It was also the hardest instance of the class to find.
The "rules of thumb" cross-reference was renumbered to match, and
CLAUDE.md:253was checked for consistency (it already describes the runtime cache as a sibling class and points here for the full writeup).Docs only — one file, 25 insertions, 31 deletions.
Summary by CodeRabbit
ClassExprFreshallowlist guidance following predicate fixes.