fix(gc): run the globalThis bootstrap in a no-move window (#7217) - #7249
Conversation
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (6)
📝 WalkthroughWalkthroughThe change adds a ChangesGlobal bootstrap GC safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BootstrapTest
participant populate_global_this_builtins
participant GcSuppressScope
participant GCAllocator
BootstrapTest->>GCAllocator: arm pending collection
BootstrapTest->>populate_global_this_builtins: initialize globalThis
populate_global_this_builtins->>GcSuppressScope: enter no-move scope
populate_global_this_builtins->>GCAllocator: allocate built-in objects
GCAllocator-->>GcSuppressScope: defer collection
populate_global_this_builtins-->>BootstrapTest: complete bootstrap
BootstrapTest->>GCAllocator: perform ordinary allocation
GCAllocator-->>BootstrapTest: service deferred collection
Possibly related issues
Possibly related PRs
🚥 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 |
f7f84fb to
4870d65
Compare
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/tests/global_bootstrap.rs`:
- Around line 111-117: Remove the 1-MB arena-growth assertion from
global_this_bootstrap_runs_in_a_no_move_window and apply the pre-arm filler
allocation pattern used by generator_intrinsic_tower_runs_in_a_no_move_window,
ensuring the filler positions the arena so the bootstrap subject allocation
crosses gc_check_trigger() without depending on bootstrap size.
🪄 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: c947c769-41fe-4a6c-b669-48d697a67bad
📒 Files selected for processing (8)
changelog.d/7249-realm-bootstrap-no-move-window.mdcrates/perry-runtime/src/gc/tests/global_bootstrap.rscrates/perry-runtime/src/gc/tests/mod.rscrates/perry-runtime/src/object/global_this/generator.rscrates/perry-runtime/src/object/global_this/populate.rscrates/perry-runtime/src/object/global_this/typed_array.rstest-parity/gc_repsel_corpus.txttest-parity/gc_repsel_triage.txt
| // LIVE SUBJECT, half 1: the window really did span a block boundary, so | ||
| // `arena_alloc_gc` really did reach `gc_check_trigger()` inside it. | ||
| assert!( | ||
| arena_after >= arena_before + (1 << 20), | ||
| "the bootstrap must consume at least one arena block for this test \ | ||
| to say anything (before={arena_before} after={arena_after})" | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List the feature-gated installers inside the bootstrap window.
rg -nP --type=rust -B 2 -A 2 '#\[cfg\(feature' crates/perry-runtime/src/object/global_this/
# Check whether these features are default-on for the runtime crate.
fd -t f 'Cargo.toml' crates/perry-runtime --exec sed -n '/\[features\]/,/^\[/p'Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate files =="
fd -t f 'global_bootstrap.rs|populate.rs|policy.rs|arena.rs' crates/perry-runtime
echo "== global_bootstrap outline =="
ast-grep outline crates/perry-runtime/src/gc/tests/global_bootstrap.rs --view expanded | sed -n '1,220p' || true
echo "== Relevant global_bootstrap region =="
cat -n crates/perry-runtime/src/gc/tests/global_bootstrap.rs | sed -n '80,140p'
echo "== Relevant generator_intrinsic_tower_runs_in_a_no_move_window snippet =="
rg -n "generator_intrinsic_tower_runs_in_a_no_move_window|arena_total_bytes|young_leaf|gc_check_trigger|force_legacy_gc_pacing" crates/perry-runtime/src/gc/tests -A 25 -B 10
echo "== Populate relevant region =="
cat -n crates/perry-runtime/src/object/global_this/populate.rs | sed -n '560,650p'
echo "== Feature snippets for global-* and temporal =="
rg -n 'global-math|global-json|global-reflect|global-atomics|temporal|global-url|global-text|global-webcrypto|global-webfetch|global-websocket' crates/perry-runtime/Cargo.toml crates/perry-runtime/src/object/global_this/populate.rs crates/perry-runtime/src/object/global_this/install_static.rs -A 3 -B 3
echo "== Arena API size constants =="
rg -n 'fn arena_total_bytes|pub const.*BLOCK|BLOCK_SIZE|BLOCK_SIZE_BYTES|allocate|arena_alloc' crates/perry-runtime/src/arena.rs crates/perry-runtime/src/gc crates/perry-runtime/src -g '*.rs' | sed -n '1,220p'Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Candidate file sizes =="
wc -l crates/perry-runtime/src/gc/tests/global_bootstrap.rs crates/perry-runtime/src/object/global_this/populate.rs crates/perry-runtime/src/arena.rs crates/perry-runtime/Cargo.toml
echo "== global_bootstrap region =="
sed -n '80,140p' crates/perry-runtime/src/gc/tests/global_bootstrap.rs
echo "== Robust pattern references =="
rg -n "generator_intrinsic_tower_runs_in_a_no_move_window|arena_total_bytes|young_leaf|arm_one_pending_collection|gc_check_trigger|force_legacy_gc_pacing" crates/perry-runtime/src/gc/tests crates/perry-runtime/src/gc -g '*.rs' | sed -n '1,220p'
echo "== populate namespace installer region =="
sed -n '570,645p' crates/perry-runtime/src/object/global_this/populate.rs
echo "== Feature slices in Cargo.toml =="
sed -n '/^\[features\]/,/^$/p' crates/perry-runtime/Cargo.toml | sed -n '1,80p'
echo "== Targeted feature-gated global namespace installers in populate/install_static =="
rg -n --type=rust 'install_math_namespace|install_json_namespace_members|install_reflect_namespace_members|install_atomics_namespace_members|install_temporal_namespace|global-math|global-json|global-reflect|global-atomics|temporal' crates/perry-runtime/src/object/global_this/populate.rs crates/perry-runtime/src/object/global_this/install_static.rs crates/perry-runtime/Cargo.toml | sed -n '1,220p'
echo "== Arena total API =="
rg -n --type=rust 'pub .*fn .*arena_total_bytes|fn (?:(pub.*?) )?arena_total_bytes|BLOCK_SIZE|bytes_for|general_block' crates/perry-runtime/src/arena.rs crates/perry-runtime/src -g '*.rs' | sed -n '1,220p'Repository: PerryTS/perry
Length of output: 415
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Arena files =="
fd -t f '^arena\.rs$' crates/perry-runtime src crates 2>/dev/null || true
fd -t f 'arena' crates/perry-runtime 2>/dev/null | sed -n '1,100p'
echo "== Arena total API in all tracked Rust files =="
rg -n --type=rust 'arena_total_bytes|arena_alloc_gc\(|general_block|BLOCK_SIZE|BYTES|bytes_for' crates/perry-runtime/src crates/perry-runtime -g '*.rs' | sed -n '1,240p'
echo "== global_bootstrap region =="
sed -n '80,140p' crates/perry-runtime/src/gc/tests/global_bootstrap.rs
echo "== Robust pattern references =="
rg -n --type=rust 'generator_intrinsic_tower_runs_in_a_no_move_window|arena_total_bytes|young_leaf|arm_one_pending_collection|gc_check_trigger|force_legacy_gc_pacing' crates/perry-runtime/src/gc crates/perry-runtime/src/gc/tests -g '*.rs' | sed -n '1,240p'
echo "== populate namespace installer region =="
sed -n '570,645p' crates/perry-runtime/src/object/global_this/populate.rs
echo "== Feature slices in Cargo.toml =="
sed -n '/^\[features\]/,/^$/p' crates/perry-runtime/Cargo.toml | sed -n '1,90p'
echo "== Targeted feature-gated namespace installers =="
rg -n --type=rust 'install_math_namespace|install_json_namespace_members|install_reflect_namespace_members|install_atomics_namespace_members|install_temporal_namespace|global-math|global-json|global-reflect|global-atomics|temporal' crates/perry-runtime/src/object/global_this/populate.rs crates/perry-runtime/Cargo.toml | sed -n '1,200p'Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Feature-only tests and minimal feature names =="
rg -n --type=rust '#\[cfg\(.*test.*features|cfg(test)|cfg\(.*feature=.*full|features.*minimal|no-default-features|default.*global-json|default.*global-math|temporal' crates/perry-runtime tests -g '*.rs' -g 'tests/**/*.rs' 2>/dev/null | sed -n '1,240p'
echo "== targeted features in tests =="
fd -t f '.*\.rs$' crates/perry-runtime/src/gc/tests crates/perry-runtime/tests crates -x sh -c 'rg -n --type=rust "global-j|i|i|global-math|global-reflect|global-atomics|temporal|no-default-features|cfg\\(test" "$1" | sed -n "1,80p"' sh {}
echo "== global_bootstrap full helper and tests =="
sed -n '1,190p' crates/perry-runtime/src/gc/tests/global_bootstrap.rs
echo =="native_arena block size definitions =="
rg -n --type=rust 'GENERAL_.*BLOCK|NURSERY|BLOCK_SIZE|BLOCK_SIZE_BYTES|bytes_for|alloc_gen|arena_total_bytes' crates/perry-runtime/src/native_arena.rs | sed -n '1,220p'
echo "== native_arena relevant size implementation =="
sed -n '300,460p' crates/perry-runtime/src/native_arena.rsRepository: PerryTS/perry
Length of output: 50370
Remove the 1-MB bootstrap footprint precondition.
global_this_bootstrap_runs_in_a_no_move_window requires the single one-shot bootstrap to exceed arena_before + 1 MB, but the feature-gated global namespace installers can reduce that allocation. Apply the pre-arm filler pattern from generator_intrinsic_tower_runs_in_a_no_move_window, so the subject’s own allocation can cross gc_check_trigger() even when the bootstrap is small.
🤖 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/tests/global_bootstrap.rs` around lines 111 -
117, Remove the 1-MB arena-growth assertion from
global_this_bootstrap_runs_in_a_no_move_window and apply the pre-arm filler
allocation pattern used by generator_intrinsic_tower_runs_in_a_no_move_window,
ensuring the filler positions the arena so the bootstrap subject allocation
crosses gc_check_trigger() without depending on bootstrap size.
4870d65 to
6683473
Compare
6683473 to
4609344
Compare
`test_gap_gc_spread_accessor_rooting` SIGSEGV'd 10/10 on the allocation-point route long after three rooting fixes had been verified green at safepoints. The failing collection was in none of the code they touched: `js_get_global_this()` builds the whole realm lazily — here reached from an ordinary property write several hundred loop iterations in — allocates ~1.15 MB doing it, and under an 8 MB heap limit minor #0 lands in the middle of it. #6982 rooted the singleton, which is one pointer. The bootstrap builds a graph, threading `ctor`, `proto` and `ns_obj` as bare `*mut ObjectHeader` locals across dozens of allocating installs in a dozen installer modules. `PERRY_GC_PROTECT_FROMSPACE=1` names it: `set_builtin_property_attrs` <- `intl::install_function` <- `install_constructor` <- `install_intl_namespace` <- `populate_global_this_builtins`, `retired_by_minor=#0`. Confirmed before any code changed: warming `globalThis` at the top of the unmodified reproducer makes it clean 5/5 with six copying minors. A back-edge poll fires only while user JS runs and the bootstrap runs none, so the safepoint route can never expose those locals; the allocation-point route makes the bootstrap's own allocations the collection points and exposes all of them at once. INVARIANT: a bootstrap that builds an IMMORTAL object graph through raw pointers held across its own allocations must run in a NO-MOVE WINDOW. Rooting each holder is unbounded and ungateable (the dominance checker reads LLVM IR and is blind to all of them); the window is one line and costs nothing a collection would have recovered, since everything born there lives for the life of the thread. Allocation-point arm, 10 runs each, base rebuilt bit-identically for the A/B: spread_accessor_rooting exit=139 10/10 -> clean 10/10, quarantine silent static_block_this_rooting `bad 1` 10/10 -> `bad 0` 10/10 loop_polls: all five witnesses green 5/5, all still relocating. RSS on a globalThis-touching hello drops ~230 KB and one GC cycle — the window defers a collection, it does not add one. Gated by a `--lib` unit test that arms one pending collection, shows the bootstrap does not service it, and then shows the same armed request IS serviced by ordinary allocation once the window closes. Sabotage-checked. Windows for the two sibling `ensure_*` intrinsic-tower builders were written and then deliberately dropped: a tower fits inside one arena block's tail and may reach no `gc_check_trigger` at all, so three successive gate designs PASSED with the window deleted. Tracked as #7251 rather than shipped ungated. Two other witnesses stay red on that route for unrelated, now-localized reasons (#7247 `js_regexp_new` `&str` borrows, #7248 stale `js_eq` left operand); their triage entries are retargeted rather than deleted.
4609344 to
e09de5b
Compare
Closes #7217.
The answer to the question #7217 actually asked
It does, and it is not what the issue guessed. The collection that was killing
test_gap_gc_spread_accessor_rootingwas not injs_object_assign_one, was not in interning, and was not in keys-array growth. It was minor #0 landing inside the lazyglobalThisbootstrap.js_get_global_this()builds the entire realm on first use. It is reached lazily — here fromjs_object_set_field_by_name→object_prototype_addr_matches→js_get_global_this_builtin_value, i.e. from an ordinary property write several hundred iterations into the loop, after ~8 MB of churn. The bootstrap then allocates ~1.15 MB of its own in one go, so underPERRY_GC_HEAP_LIMIT=8the very first collection of the program lands in the middle of it.That matters because the bootstrap builds a graph through raw pointers.
intl::install_constructorholdsctor,protoandns_objas bare*mut ObjectHeaderlocals across dozens of allocating installs; so do the error, typed-array, generator, Reflect, Atomics and WebAssembly installers, across a dozen files. #6982 rooted the singleton — one pointer — and that is all it rooted.PERRY_GC_PROTECT_FROMSPACE=1(#7196) names the site without inference:retired_by_minor=#0, deterministic 10/10 — the signature CLAUDE.md records for a table rather than a register, and here it is a whole subsystem's worth of them.Confirmed independently before writing any code: adding one line,
const __warm = typeof (globalThis as any).Intl;, at the top of the unmodified reproducer — so the bootstrap runs while the arena is nearly empty — makes it clean 5/5 on the allocation-point arm, with 6 copying minors and 4 613–5 797 objects copied each. The spread path was never the bug.Why the safepoint route could not see it
A back-edge poll fires only while user JS is running, and the bootstrap runs no user JS. So on the
loop_pollsroute not one of those installer locals is ever live across a collection. On the allocation-point route the bootstrap's own allocations are the collection points, so the whole graph is exposed at once.That is the general statement #7217's second comment asked for: the two routes are not two chances to catch the same bug.
loop_pollscannot expose an unrooted local in any runtime code that does not re-enter user JS — which is most of the runtime. Three fixes verified green there and red here were not three flawed fixes; two of the three were failing on a collection in code they had never touched.The invariant
Rooting each holder individually is unbounded (hundreds of sites across a dozen installer modules) and ungateable — no checker can prove the set complete, and
gc_root_dominance_check.pyreads emitted LLVM IR so it is structurally blind to all of them. The window is one line and is provably enough. It costs nothing a collection would have recovered: every object born in the window is reachable fromglobalThisfor the life of the thread, so a collection inside it frees nothing.The change
One line:
crate::gc::GcSuppressScope(the existing RAII no-move window, nesting-safe, already used bydescriptor_state.rs) at the top ofpopulate_global_this_builtins.GC_FLAG_SUPPRESSEDgatesgc_check_trigger, the budgeted stepper andgc_safepoint_moving_minor, so the window is comprehensive rather than alloc-point-only. No collector behaviour changes anywhere else, and no env knob is added.No installer's rooting was touched. Adding a
RuntimeHandleScopeto one of fifty installers would imply the other forty-nine are fine.Two sibling windows were written and then deliberately dropped — see #7251.
ensure_generator_intrinsicsandensure_typed_array_intrinsicbuild the same shape of immortal tower through the same kind of raw locals and are also reachable lazily ahead of the bootstrap. But a tower is three orders of magnitude smaller than the bootstrap, fits inside one arena block's tail, and may reach nogc_check_triggerat all — three successive gate designs for them PASSED with the window deleted (an armed-collection gate; the same with a block-boundary pre-fill and a re-exec into a virgin process, because the intrinsic slots are process-global and libtest's ordering meantarray::testsbuilt the towers first; and a#[cfg(test)]observer ofgc_is_suppressed()inside the builder, which reported suppressed even with the scope removed, unexplained). Shipping a GC-trigger change with no test that can fail without it is the thing CLAUDE.md's knob-kill policy exists to stop, so the exposure is tracked with both candidate gate designs written up rather than shipped ungated.Verification
Same host, idle,
--profile perry-dev, one target dir, 10 runs per cell, atPERRY_GC_HEAP_LIMIT=8 PERRY_GC_INCREMENTAL=0 PERRY_CONSERVATIVE_STACK_SCAN=offwith no compile-time GC env — the allocation-point arm #7217 names.
The base arm was produced by reverting the three source files, rebuilding, and it came back bit-identical to the pre-change build (
perrymd56142b49fcf7f29f0f7ee9ab09ccc2f2dboth times), while the fixed build is068af60471ba94006e4fe6ac828e326d. The two arms are demonstrably different binaries.8b024958ftest_gap_gc_spread_accessor_rooting(#7217)bad plain 0 hot 0 tail 010/10test_gap_gc_static_block_this_rootingbad 110/10bad 010/10test_gap_gc_inline_ctor_this_rootingtest_gap_gc_assign_string_source_rootingbad char 3 count 310/10bad char 1 count 110/10 — not green, see belowtest_gap_gc_regexp_receiver_rootingOther arms, this PR:
loop_polls(compiled and run withPERRY_GC_MOVING_LOOP_POLLS=1,+FORCE_EVACUATE=1)bad plain 0 hot 0 tail 0, 3/3node --experimental-strip-typesoraclebad plain 0 hot 0 tail 0— byte-exactPERRY_GC_PROTECT_FROMSPACE=1on the reproducerintl::install_function,retired_by_minor=#0)gc_root_dominance_check.py(default +--unrooted-allocas)Cost — the window defers one collection, it does not add one:
console.log("hi", typeof globalThis.Intl)HEAP_LIMIT=8RSS is ~230 KB lower: the collection that used to run mid-bootstrap copied the bootstrap's own live set and then immediately had it all survive anyway.
cargo test -p perry-runtime: 1637 passed, 0 failed (--test-threads=1, which is how CI runs this crate —RUST_TEST_THREADS=1at.github/workflows/test.yml:565). The crate's parallel-mode flakes are pre-existing and measured at the same 2–4/run with and without the new tests.The gate
crates/perry-runtime/src/gc/tests/global_bootstrap.rs, in the per-PRcargo-testsurface (a--libunit test, not atests/*.rsintegration suite, which per CLAUDE.md would only run nightly).It arms one pending collection, runs the bootstrap, and asserts it was not serviced, that the request is deferred rather than dropped (leaving it unserviced-and-unset would disable the trigger for the rest of the thread), that the window spans at least one arena block (so
arena_alloc_gcgenuinely reachedgc_check_triggerinside it), and that the window closed.Then it runs the control: the same armed request, on the same thread, must be serviced by ordinary allocation once the window is over. Without that second half the test would pass on a tree where nothing was ever due — CLAUDE.md's fourth way a gate cannot fail.
Sabotage-checked in the failing direction:
The two witnesses this does NOT fix, and why their triage is retargeted rather than deleted
test-parity/gc_repsel_triage.txtsays "DELETE ALL TEN when #7217 is fixed" for each. Both are still red, so the twenty entries are retargeted with their real causes — and one of the two triage texts was asserting a cause that is now known to be wrong.GC: js_regexp_new holds
&strborrows into movable StringHeaders across its whole body (residual #7217 allocation-point witness) #7247 —test_gap_gc_regexp_receiver_rooting, unchanged by this PR (exit=139 10/10 both arms).js_regexp_new(regex.rs:616) takesstring_as_str(pattern)/string_as_str(flags)—&strborrows into the movableStringHeaderpayload — and holds them across its whole body including theREGEX_CACHEprobe's.to_string()calls. The GC: js_object_assign_one's string-source arm held a raw borrow into the source StringHeader across its allocating copy loop #7215 borrow shape, quarantine backtrace in the issue.codegen: js_eq's left operand is a stale register across its sibling's allocating lowering (residual #7217 allocation-point witness) #7248 —
test_gap_gc_assign_string_source_rooting, improved 3→1. The old triage blamed "allocation-point relocation insidejs_object_assign_one/object_assign_string_source". It is not there. The residual failure is in the test's own assertion, and the IR says so:%r97may well be a rewritten shadow slot, but%r136is a register taken before the collection point and never re-read — the third property fix(gc): close the three residual root-store holes — the PutValueSet key, js_object_assign_one, and the inline-ctor this slot #7207 says a root must buy. The fix(codegen): root a call receiver and a computed-read base across their sibling operands #7206/fix(codegen): root the callee,thisand every argument of js_closure_callN #7214 operand family, atjs_eq.Both remain hard gates on
loop_polls, whichgc-moving-witnesses.ymlruns on every collector-touching PR, and both are green there 5/5.#7161 / #7154
Not claimed, and here is exactly what was and was not measured.
I could not find the
sfw-registryworkload anywhere on this machine, so #7154's own symptom was not run and I am not asserting it is resolved. That check still belongs to whoever proposes the #7161 revert.What is measured: in the configuration a #7161 revert would ship — compiled and run with
PERRY_GC_MOVING_LOOP_POLLS=1, plusPERRY_GC_FORCE_EVACUATE=1— all fivetest_gap_gc_*_rootingwitnesses are green 5/5 on this branch. And this PR removes one of the two reasons #7217 gave for calling the revert-readiness statement incomplete: the object-spread path is now verified on the allocation-point route, not merely at safepoints. #7247 and #7248 remain open on that route, but neither is a shipped configuration — the allocation-point relocating arm requires an explicitPERRY_CONSERVATIVE_STACK_SCAN=off, which the matrix header itself calls a measurement configuration and not one anyone ships.Summary by CodeRabbit
Bug Fixes
Tests
Documentation