Skip to content

Commit 4609344

Browse files
author
Ralph Küpper
committed
fix(gc): run the globalThis bootstrap in a no-move window (#7217)
`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.
1 parent 8b02495 commit 4609344

6 files changed

Lines changed: 439 additions & 84 deletions

File tree

Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,139 @@
1+
### Fixed
2+
3+
- **GC: the lazy realm bootstraps now run in a no-move window (#7217).**
4+
`test_gap_gc_spread_accessor_rooting`#7207's reproducer for #7200
5+
SIGSEGV'd 10/10 deterministically under
6+
`PERRY_GC_HEAP_LIMIT=8 PERRY_GC_INCREMENTAL=0 PERRY_CONSERVATIVE_STACK_SCAN=off`
7+
(the allocation-point route) months after three separate rooting fixes had
8+
been verified green on the safepoint route. **The failing collection was not
9+
in the code any of those fixes touched.**
10+
11+
`js_get_global_this()` builds the whole realm on first use, and it is reached
12+
*lazily* — in this program from `js_object_set_field_by_name`
13+
`object_prototype_addr_matches``js_get_global_this_builtin_value`, i.e.
14+
from an ordinary property write several hundred loop iterations in, after
15+
~8 MB of churn. The bootstrap then allocates ~1.15 MB of its own, so under an
16+
8 MB heap limit **minor #0 lands in the middle of it**. #6982 rooted the
17+
`globalThis` singleton — one pointer. The bootstrap builds a *graph*:
18+
`intl::install_constructor` threads `ctor`, `proto` and `ns_obj` as bare
19+
`*mut ObjectHeader` locals across dozens of allocating installs, and so do the
20+
error, typed-array, generator, Reflect, Atomics and WebAssembly installers,
21+
across a dozen files. Every one is a slot the collector does not rewrite.
22+
23+
`PERRY_GC_PROTECT_FROMSPACE=1` (#7196) named it without inference:
24+
`set_builtin_property_attrs``intl::install_function`
25+
`install_constructor``install_intl_namespace`
26+
`populate_global_this_builtins``js_get_global_this`, on an address with
27+
`retired_by_minor=#0`. Confirmed before any code changed: adding
28+
`const __warm = typeof (globalThis as any).Intl;` at the top of the
29+
*unmodified* reproducer — so the bootstrap runs while the arena is nearly
30+
empty — makes it clean 5/5 with 6 copying minors and 4 613–5 797 objects
31+
copied each.
32+
33+
**Why the safepoint route could not see it, which is the general finding.** A
34+
loop back-edge poll fires only while user JS is running, and the bootstrap
35+
runs no user JS, so none of those locals is ever live across a collection
36+
there. On the allocation-point route the bootstrap's *own* allocations are the
37+
collection points, so the entire graph is exposed at once. The two routes are
38+
not two chances to catch the same bug: `loop_polls` cannot expose an unrooted
39+
local in any runtime code that does not re-enter user JS, which is most of the
40+
runtime.
41+
42+
**The invariant: a bootstrap that builds an IMMORTAL object graph through raw
43+
pointers held across its own allocations must run in a NO-MOVE WINDOW.**
44+
Rooting each holder individually is unbounded (hundreds of sites) and
45+
ungateable — `scripts/gc_root_dominance_check.py` reads emitted LLVM IR and is
46+
structurally blind to all of them. The window is one line and provably enough,
47+
and it costs nothing a collection would have recovered: every object born in
48+
it is reachable from `globalThis` for the life of the thread.
49+
50+
The fix is one line: `crate::gc::GcSuppressScope` (the existing nesting-safe
51+
RAII no-move window, already used by `descriptor_state.rs`) at the top of
52+
`populate_global_this_builtins`. `GC_FLAG_SUPPRESSED` gates
53+
`gc_check_trigger`, the budgeted stepper **and** `gc_safepoint_moving_minor`,
54+
so the window is comprehensive rather than allocation-point-only. No
55+
installer's rooting was touched — adding a `RuntimeHandleScope` to one of
56+
fifty installers would imply the other forty-nine are fine. No env knob is
57+
added and no collector behaviour changes anywhere else.
58+
59+
Measured on the allocation-point arm, same host, idle, one target dir, 10 runs
60+
per cell. The base arm was produced by reverting the source change and
61+
rebuilding, and came back **bit-identical** to the pre-change build
62+
(`perry` md5 `6142b49f…` both times) against `068af604…` for the fix, so the
63+
two arms are demonstrably different binaries. Every row was then re-run
64+
against the final shipped tree (`f1f002f8…`, after the two #7251 windows were
65+
dropped) and is unchanged:
66+
67+
| witness | base `8b024958f` | fixed |
68+
|---|---|---|
69+
| `test_gap_gc_spread_accessor_rooting` | **exit=139, no output, 10/10** | **`bad plain 0 hot 0 tail 0` 10/10** |
70+
| `test_gap_gc_static_block_this_rooting` | `bad 1` 10/10 | **`bad 0` 10/10** |
71+
| `test_gap_gc_inline_ctor_this_rooting` | green 10/10 | green 10/10 |
72+
73+
`loop_polls` (compiled **and** run with `PERRY_GC_MOVING_LOOP_POLLS=1` plus
74+
`PERRY_GC_FORCE_EVACUATE=1`): all five `test_gap_gc_*_rooting` witnesses green
75+
5/5, and all five still relocate (1–5 cycles, 224–26 063 objects copied), so
76+
none went inert. Shipped default: clean 3/3, byte-exact against
77+
`node --experimental-strip-types`. `PERRY_GC_PROTECT_FROMSPACE=1` on the
78+
reproducer now reports **no fault at all**. The static
79+
`gc_root_dominance_check.py` reports 0 violations before *and* after, in both
80+
its default and `--unrooted-allocas` modes — it reads emitted LLVM IR, so it
81+
is structurally blind to a bug that lives in the runtime's Rust locals, which
82+
is worth recording as a limit of that gate rather than as a clean bill.
83+
84+
**The window defers a collection, it does not add one.** On
85+
`console.log("hi", typeof globalThis.Intl)`, peak RSS drops from
86+
10 878 976 / 10 895 360 / 10 878 976 bytes to
87+
10 649 600 / 10 649 600 / 10 633 216, and GC cycles at `HEAP_LIMIT=8` go from
88+
2 to 1 — the collection that used to run mid-bootstrap copied the bootstrap's
89+
own live set and then had all of it survive anyway.
90+
91+
### Testing
92+
93+
- **`crates/perry-runtime/src/gc/tests/global_bootstrap.rs`** — a `--lib` unit
94+
test, so it runs in the per-PR `cargo-test` gate rather than in a
95+
nightly-only `tests/*.rs` suite. It arms **one** pending collection, runs
96+
the bootstrap, and asserts it was not serviced, that the request is
97+
**deferred rather than dropped**, that the window spans at least one arena
98+
block (so `arena_alloc_gc` genuinely reached `gc_check_trigger` inside it),
99+
and that the window closed. Each then runs **the control**: the *same* armed
100+
request, on the *same* thread, must be serviced by ordinary allocation once
101+
the window is over. Without that second half the test would pass on a tree
102+
where nothing was ever due — CLAUDE.md's fourth way a gate cannot fail.
103+
Sabotage-checked in the failing direction: removing the `GcSuppressScope`
104+
reddens it with `left: 1, right: 0` and the message naming the installer
105+
locals.
106+
107+
### Known issues
108+
109+
- Two of the five `test_gap_gc_*_rooting` witnesses remain red on the
110+
allocation-point route for **unrelated** reasons, and their twenty
111+
`test-parity/gc_repsel_triage.txt` entries are retargeted rather than deleted
112+
— one of the two triage texts was asserting a cause now known to be wrong.
113+
Both are green on `loop_polls`, which `gc-moving-witnesses.yml` gates.
114+
- **#7247**`test_gap_gc_regexp_receiver_rooting`, unchanged (exit=139 10/10
115+
on both arms). `js_regexp_new` holds `string_as_str(pattern)` /
116+
`string_as_str(flags)``&str` borrows into a movable `StringHeader`
117+
payload — across its whole body. The #7215 borrow shape.
118+
- **#7248**`test_gap_gc_assign_string_source_rooting`, improved from
119+
`bad char 3 count 3` to `bad char 1 count 1` (10/10 each). The residual
120+
failure is a stale `js_eq` left operand in the test's own
121+
`got !== ALPHA[i % 26]` assertion — a register loaded above the allocating
122+
`js_string_index_get_boxed` sibling and never re-read — not anything in
123+
`js_object_assign_one`. The #7206/#7214 operand family.
124+
- **#7251** — the same defect shape exists in `ensure_generator_intrinsics` and
125+
`ensure_typed_array_intrinsic`, which build the same kind of immortal tower
126+
through the same kind of raw locals and are *also* reachable lazily ahead of
127+
the bootstrap. Windows for them were written and then **deliberately dropped
128+
from this PR**: a tower is three orders of magnitude smaller than the
129+
bootstrap, fits inside one arena block's tail, and so may reach no
130+
`gc_check_trigger` at all — three successive versions of a gate for them
131+
passed with the window deleted. Shipping a GC-trigger change with no test that
132+
can fail without it is the thing CLAUDE.md's knob-kill policy exists to stop,
133+
so the exposure is tracked instead, with the two candidate gate designs and an
134+
unexplained observation (something may already be suppressing across part of
135+
the tower build) written up in the issue.
136+
- **#7154's `sfw-registry --help` symptom was not run** (the workload is not
137+
present on this machine) and is **not** claimed resolved. What is measured is
138+
that the five witnesses are green in the configuration a #7161 revert would
139+
ship.
Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
//! #7217: the lazy realm bootstraps must run in a NO-MOVE WINDOW.
2+
//!
3+
//! `populate_global_this_builtins` constructs an IMMORTAL object graph —
4+
//! everything it allocates is reachable from `globalThis` for the life of the
5+
//! thread. It constructs it by threading raw `*mut ObjectHeader` /
6+
//! `*mut ClosureHeader` locals through several hundred installs in a dozen
7+
//! installer modules, each of which allocates. Those locals are slots the
8+
//! collector does not rewrite, so a *relocating* collection inside the window
9+
//! leaves the rest of the bootstrap writing into from-space.
10+
//!
11+
//! This was invisible on the safepoint route (`PERRY_GC_MOVING_LOOP_POLLS=1`):
12+
//! a back-edge poll only fires while user JS runs, and the bootstrap runs no
13+
//! user JS. It is reachable on the allocation-point route, where the
14+
//! bootstrap's own block allocations are the collection points — which is why
15+
//! `test_gap_gc_spread_accessor_rooting` still SIGSEGV'd 10/10 under
16+
//! `PERRY_GC_HEAP_LIMIT=8 PERRY_GC_INCREMENTAL=0
17+
//! PERRY_CONSERVATIVE_STACK_SCAN=off` after three rooting fixes that were all
18+
//! green at safepoints.
19+
//!
20+
//! ***BOTH HALVES ARE ASSERTED*** (CLAUDE.md's fourth way a gate cannot fail).
21+
//! A test that only checked "no collection ran during the bootstrap" would pass
22+
//! on a tree where nothing was ever due. The test below therefore arms ONE
23+
//! pending collection, shows the bootstrap did not service it, and then shows
24+
//! that the SAME armed request is serviced by ordinary allocation once the
25+
//! window has closed. Same thread, same lever, same magnitude — the only
26+
//! variable is whether the allocations happened inside the window.
27+
//!
28+
//! SCOPE. Only `populate_global_this_builtins` is covered, and deliberately so.
29+
//! `ensure_generator_intrinsics` / `ensure_typed_array_intrinsic` build the same
30+
//! shape of immortal graph through the same kind of raw locals and are ALSO
31+
//! reachable lazily, ahead of the bootstrap — but a tower is three orders of
32+
//! magnitude smaller than the bootstrap, fits inside one arena block's tail, and
33+
//! so may reach no `gc_check_trigger` at all. A version of this file that armed a
34+
//! collection around them PASSED with their windows deleted. Rather than ship a
35+
//! GC-trigger change with no test that can fail without it — the exact thing
36+
//! CLAUDE.md's knob-kill policy exists to stop — those two windows were dropped
37+
//! and the exposure is tracked separately.
38+
39+
use super::super::*;
40+
use super::support::*;
41+
42+
/// Run `body` on a thread that has never touched `globalThis` or the intrinsic
43+
/// towers, so the lazy bootstrap really runs instead of returning a cache hit.
44+
/// `THREAD_GLOBAL_THIS`, the arena, `GC_STATS` and `GC_OLD_RECLAIM_PENDING` are
45+
/// all thread-local, so the arming and the measurement stay on this thread.
46+
fn on_a_fresh_thread(body: impl FnOnce() + Send + 'static) {
47+
std::thread::Builder::new()
48+
.stack_size(16 << 20)
49+
.spawn(body)
50+
.expect("spawn bootstrap test thread")
51+
.join()
52+
.expect("bootstrap test thread panicked");
53+
}
54+
55+
/// Make one collection due at the very next `gc_check_trigger()` — which
56+
/// `arena_alloc_gc` calls every time the current block fills. This is the same
57+
/// lever `scan_fallback.rs` uses, and it completes synchronously on the
58+
/// allocation-point arm rather than deferring to a safepoint.
59+
fn arm_one_pending_collection() {
60+
GC_OLD_RECLAIM_PENDING.with(|pending| pending.set(true));
61+
}
62+
63+
fn pending_collection_still_owed() -> bool {
64+
GC_OLD_RECLAIM_PENDING.with(std::cell::Cell::get)
65+
}
66+
67+
fn clear_pending_collection() {
68+
GC_OLD_RECLAIM_PENDING.with(|pending| pending.set(false));
69+
GC_SAFEPOINT_PENDING.with(|pending| pending.set(false));
70+
let old_in_use = crate::arena::old_gen_in_use_bytes();
71+
GC_LAST_OLD_RECLAIM_IN_USE_BYTES.with(|bytes| bytes.set(old_in_use));
72+
}
73+
74+
/// THE CONTROL. Allocate ordinary young objects — nothing rooted, nothing
75+
/// exotic — until either the armed collection is serviced or two arena blocks
76+
/// have been consumed without it. Returns whether it was serviced.
77+
///
78+
/// Two blocks is deliberately more than the bootstrap window spans (measured:
79+
/// ~1.15 MB, i.e. just over one 1 MB block), so a `false` here means the
80+
/// arming is inert on this thread and the subject assertion above it proved
81+
/// nothing.
82+
fn ordinary_allocation_services_the_armed_collection(collections_before: u64) -> bool {
83+
let arena_before = crate::arena::arena_total_bytes();
84+
for _ in 0..500_000 {
85+
let _ = young_leaf();
86+
if gc_collection_count() > collections_before {
87+
return true;
88+
}
89+
if crate::arena::arena_total_bytes() >= arena_before + (2 << 20) {
90+
return false;
91+
}
92+
}
93+
false
94+
}
95+
96+
#[test]
97+
fn global_this_bootstrap_runs_in_a_no_move_window() {
98+
on_a_fresh_thread(|| {
99+
// Pin the shipped pacing (#7161 flipped `PERRY_GC_MOVING_LOOP_POLLS`
100+
// off) so this asserts against a declared mode rather than whatever the
101+
// process-wide OnceLock happened to resolve to.
102+
let _pacing = crate::gc::policy::force_legacy_gc_pacing();
103+
crate::gc::ensure_gc_initialized();
104+
clear_pending_collection();
105+
106+
arm_one_pending_collection();
107+
let arena_before = crate::arena::arena_total_bytes();
108+
let collections_before = gc_collection_count();
109+
110+
// THE SUBJECT: the one-shot realm bootstrap.
111+
let global = crate::object::js_get_global_this();
112+
assert!(
113+
crate::value::JSValue::from_bits(global.to_bits()).is_pointer(),
114+
"js_get_global_this must return a real singleton, else the \
115+
bootstrap never ran and this test measured nothing"
116+
);
117+
118+
let arena_after = crate::arena::arena_total_bytes();
119+
let collections_after = gc_collection_count();
120+
121+
// LIVE SUBJECT, half 1: the window really did span a block boundary, so
122+
// `arena_alloc_gc` really did reach `gc_check_trigger()` inside it.
123+
assert!(
124+
arena_after >= arena_before + (1 << 20),
125+
"the bootstrap must consume at least one arena block for this test \
126+
to say anything (before={arena_before} after={arena_after})"
127+
);
128+
// THE INVARIANT: nothing collected, and therefore nothing moved, while
129+
// the installers held raw pointers.
130+
assert_eq!(
131+
collections_after, collections_before,
132+
"a collection ran inside the globalThis bootstrap — every installer \
133+
local (`ctor`, `proto`, `ns_obj`, …) is now a from-space address"
134+
);
135+
assert!(
136+
pending_collection_still_owed(),
137+
"the window must DEFER the request, not drop it: leaving it \
138+
unserviced-and-unset would disable the trigger for the rest of \
139+
the thread"
140+
);
141+
assert!(
142+
!crate::gc::gc_is_suppressed(),
143+
"the no-move window must close when the bootstrap returns"
144+
);
145+
146+
// LIVE SUBJECT, half 2 — THE CONTROL. Same thread, same armed request,
147+
// ordinary allocation. If this does not collect, the arming was inert
148+
// and the assertion above is vacuous.
149+
assert!(
150+
ordinary_allocation_services_the_armed_collection(collections_after),
151+
"the armed collection was never serviceable on this thread, so \
152+
'the bootstrap did not collect' proved nothing"
153+
);
154+
155+
clear_pending_collection();
156+
});
157+
}

crates/perry-runtime/src/gc/tests/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ mod error_side_tables;
1313
mod evacuation;
1414
mod fromspace_protect;
1515
mod fromspace_scan;
16+
mod global_bootstrap;
1617
mod helper_stores;
1718
mod host_safepoints;
1819
mod incremental_sweep_reclaim;

crates/perry-runtime/src/object/global_this/populate.rs

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,43 @@ pub(crate) fn populate_global_this_builtins(singleton_at_entry: *mut ObjectHeade
4040
// Only reachable when the conservative native-stack scan is off, which is
4141
// production's `Auto -> SkipDisabled` resolution; the scan was masking this
4242
// by pinning the argument register.
43+
//
44+
// #7217: rooting the singleton is necessary but NOT sufficient, and the
45+
// difference is the whole point of the allocation-point route.
46+
//
47+
// The singleton is one pointer. The bootstrap it drives is a *graph*:
48+
// `install_intl_namespace` -> `install_constructor` -> `install_function`
49+
// holds `ctor`, `proto` and `ns_obj` as raw `*mut ObjectHeader` locals
50+
// across dozens of allocating installs, and so do the error, typed-array,
51+
// generator, Reflect, Atomics, WebAssembly, … installers, in a dozen files
52+
// and several hundred call sites. Every one of those is a slot the
53+
// collector does not rewrite.
54+
//
55+
// On the SAFEPOINT route none of them can be exposed: a collection reached
56+
// from a loop back-edge poll only happens while user JS is running, and the
57+
// bootstrap runs no user JS. On the ALLOCATION-POINT route every one of the
58+
// bootstrap's own ~1.15 MB of allocations is a collection point, so the
59+
// whole graph is exposed at once. That is why three separate rooting fixes
60+
// verified green on `PERRY_GC_MOVING_LOOP_POLLS=1` were still red under
61+
// `PERRY_GC_HEAP_LIMIT=8 PERRY_GC_INCREMENTAL=0
62+
// PERRY_CONSERVATIVE_STACK_SCAN=off`: the collection they were failing on
63+
// was not in the code they had fixed, it was minor #0 landing inside this
64+
// bootstrap. `PERRY_GC_PROTECT_FROMSPACE=1` names it exactly —
65+
// `set_builtin_property_attrs` <- `intl::install_function` <-
66+
// `install_constructor` <- `install_intl_namespace` <- here, on an address
67+
// `retired_by_minor=#0`.
68+
//
69+
// THE INVARIANT: a bootstrap that builds an IMMORTAL object graph through
70+
// raw pointers held across its own allocations must run in a NO-MOVE
71+
// WINDOW. Rooting each holder individually is unbounded (hundreds of sites
72+
// across a dozen installer modules) and ungateable (no checker can prove
73+
// the set complete), while the window is one line and provably enough. It
74+
// costs nothing a collection would have recovered: every object born here
75+
// is reachable from `globalThis` for the life of the process, so a
76+
// collection inside the window frees nothing. Measured footprint of the
77+
// whole window: ~1.15 MB allocated, ~410 KB of it live afterwards, once per
78+
// thread.
79+
let _no_move = crate::gc::GcSuppressScope::new();
4380
let scope = crate::gc::RuntimeHandleScope::new();
4481
let singleton_handle = scope.root_raw_mut_ptr(singleton_at_entry);
4582
let singleton = || singleton_handle.get_raw_mut_ptr::<ObjectHeader>();

0 commit comments

Comments
 (0)