diff --git a/changelog.d/7987-dead-owner-side-tables-realm-bootstrap.md b/changelog.d/7987-dead-owner-side-tables-realm-bootstrap.md new file mode 100644 index 0000000000..e1b0c3d3d5 --- /dev/null +++ b/changelog.d/7987-dead-owner-side-tables-realm-bootstrap.md @@ -0,0 +1,48 @@ +### `dead_owner_side_tables` was measuring the `globalThis` bootstrap, not the death prune + +`test_dead_arguments_object_entry_pruned_on_full_gc` and +`test_dead_owner_descriptor_entries_pruned_on_full_gc` (#7975) failed 10 of 200 +`--test-threads=10` runs of their module. Filed as a self-race — it is not one. +Every test in the module already takes the same global isolation mutex, and run +**alone** both cases fail **200/200**; they pass only when a sibling ran first. + +Two facts compose. Both cases reach an API that resolves the **process-global** +memoized `Object.prototype` address (`array::prototype_addr`) — +`set_property_attrs` for one, `js_object_set_field_by_name` for the other — and a +MISS runs the whole lazy `globalThis` bootstrap, ~1.15 MB allocated and ~410 KB +of it live-and-rooted, *inside the caller*. The cache misses exactly once per +PROCESS, so which libtest thread pays is a scheduling accident. And arena block +reset is all-or-nothing, so `gc::trace::mark_block_persisting_arena_objects` +force-MARKS every object in a block that holds one reachable object — the test's +own unrooted owner included. `dead_owner::PostTraceProbe::owner_is_dead` then +correctly reports "not dead" and the prune correctly keeps the entry: a block +that persists cannot recycle the owner's address, so the entry is not stale. The +test failed on the prune and named the prune. + +`GcTestIsolationGuard::with_realm_bootstrapped()` runs the bootstrap inside the +isolation lock but before `ScopedRootScannerRegistryGuard` takes the thread's +scanners and before `reset_global_roots()`, putting the realm graph outside the +measured window. + +The force-mark is cleared again by the sweep, so nothing about it survived a +collection for a test to read — the premise was unstateable. +`gc::trace::block_persist_force_mark_count()` is now an always-on, +O(1)-per-pass thread-local census of block-persistence force-marks, recorded by +**both** the whole-cycle pass and the budgeted `BlockPersistCycleState` arm (a +census that counted one arm would read zero on exactly the cycles that are +hardest to reason about). Same rationale as `gc::scan_fallback` (#7148). +`full_gc_with_no_block_persistence()` now fails as a *premise* instead of +letting the subject assertion mis-name it, and +`test_block_persistence_census_moves_when_a_block_has_a_live_tenant` plants the +confounder's exact shape — one rooted object and one unrooted owner in one +block, no realm involved — asserting both that the census moves and that the +force-marked owner's side-table entry correctly survives. Without that the +"premise held" verdicts would be vacuous. + +No assertion was weakened. Verified: fixed binary 400/400 green on +`--test-threads=10 dead_owner_side_tables`, 200/200 on each case run alone, +200/200 on the new census case, and 300/300 on the full suite at default +parallelism — against `origin/main`'s 10/200, 200/200-failing alone, and +300/300 full-suite green (unchanged). Sabotage check: reverting only the guard +turns both cases into `test premise: block persistence force-marked objects +during this collection … left: 4083, right: 0`. diff --git a/crates/perry-runtime/src/gc/cycle.rs b/crates/perry-runtime/src/gc/cycle.rs index 2ca0d073f7..4bc22252fa 100644 --- a/crates/perry-runtime/src/gc/cycle.rs +++ b/crates/perry-runtime/src/gc/cycle.rs @@ -165,6 +165,7 @@ impl BlockPersistCycleState { } self.stats.marked_objects = self.stats.marked_objects.saturating_add(self.newly_marked); + super::trace::note_block_persist_force_marks(self.newly_marked); if self.newly_marked == 0 { self.subphase = BlockPersistSubphase::Done; return true; diff --git a/crates/perry-runtime/src/gc/tests/dead_owner_side_tables.rs b/crates/perry-runtime/src/gc/tests/dead_owner_side_tables.rs index e8f53d0a1a..0edf805647 100644 --- a/crates/perry-runtime/src/gc/tests/dead_owner_side_tables.rs +++ b/crates/perry-runtime/src/gc/tests/dead_owner_side_tables.rs @@ -6,6 +6,25 @@ //! and assert the entry is gone — plus the two safety inverses (a LIVE owner's //! entries survive; a TENURED owner's entries survive a MINOR trace, which //! never marks the old generation and therefore proves nothing about it). +//! +//! ★ #7975 — THE PREMISE EVERY "IT IS PRUNED" CASE HERE DEPENDS ON. Arena +//! block reset is all-or-nothing, so `gc::trace` force-MARKS every object in a +//! block that still holds one reachable object +//! (`mark_block_persisting_arena_objects`, `BLOCK_PERSIST_WINDOW`). An owner +//! sharing its block with ANY live object is therefore not dead, and the prune +//! *correctly* keeps its entry — a persisting block cannot recycle the +//! address, so the entry is not stale. A case that lets a live tenant into its +//! block does not test the prune; it fails on it. +//! +//! The tenant that actually turned up is the lazy `globalThis` realm bootstrap +//! (~1.15 MB allocated, ~410 KB live), reached through the **process-global** +//! memoized `Object.prototype` address that `set_property_attrs` and +//! `js_object_set_field_by_name` resolve. It misses once per PROCESS, so which +//! libtest thread pays for it is a scheduling accident: two cases here failed +//! 200/200 alone and 0/200 behind a sibling. Use +//! [`GcTestIsolationGuard::with_realm_bootstrapped`] in any case that reaches a +//! realm-resolving API, and collect through [`full_gc_with_no_block_persistence`] +//! so the premise fails as a premise instead of as the subject. use super::super::*; use super::support::*; @@ -70,6 +89,39 @@ unsafe fn alloc_malloc_test_object() -> *mut crate::object::ObjectHeader { obj } +/// PREMISE GUARD for the "dead owner ⇒ entry pruned" cases (#7975). +/// +/// "The entry is gone" only says something about the death prune when the owner +/// was genuinely DEAD. Arena block reset is all-or-nothing, so +/// `gc::trace::mark_block_persisting_arena_objects` force-MARKS every object in +/// a block that still holds one reachable object — and a force-marked owner is +/// *correctly* left registered, because a block that persists cannot recycle +/// its address, so the entry is not stale. A case whose block picked up an +/// unrelated live tenant therefore fails on the prune assertion and blames the +/// prune for something the prune got right. +/// +/// The mark itself is cleared again by the sweep, so nothing about it survives +/// the collection for a test to read. `gc::trace`'s force-mark census is the +/// one quantity that does, and it has exactly one satisfying path: it moves iff +/// some block in the persist window held a reachable object. +/// +/// Wrap the collection so the premise fails as a premise, before the subject +/// assertion gets a chance to mis-name it. +fn full_gc_with_no_block_persistence() { + let force_marks_before = crate::gc::block_persist_force_mark_count(); + full_gc(); + assert_eq!( + crate::gc::block_persist_force_mark_count(), + force_marks_before, + "test premise: block persistence force-marked objects during this \ + collection, so an unrooted owner in one of those blocks is NOT dead \ + and the prune correctly declines to drop its entry. Something \ + reachable shares the owner's arena block — the lazy `globalThis` \ + realm bootstrap is the one that turned up (#7975); see \ + `GcTestIsolationGuard::with_realm_bootstrapped`" + ); +} + fn util_types_is_map_iterator(addr: usize) -> bool { crate::object::js_util_types_is_map_iterator(f64::from_bits(ptr_bits(addr))).to_bits() == crate::value::TAG_TRUE @@ -259,7 +311,10 @@ fn test_array_named_dead_set_iterator_marker_does_not_brand_exact_eden_reuse() { #[test] fn test_dead_owner_descriptor_entries_pruned_on_full_gc() { - let _guard = GcTestIsolationGuard::new(); + // `set_property_attrs` below resolves the process-global memoized + // `Object.prototype` address, and a MISS runs the whole lazy `globalThis` + // bootstrap inside this test — see `with_realm_bootstrapped` (#7975). + let _guard = GcTestIsolationGuard::with_realm_bootstrapped(); let (obj, _) = unsafe { alloc_nursery_test_object(0) }; let addr = obj as usize; crate::object::set_property_attrs( @@ -275,7 +330,7 @@ fn test_dead_owner_descriptor_entries_pruned_on_full_gc() { assert!(crate::object::get_property_attrs(addr, "frozenKey").is_some()); // No roots: the owner is dead at the full trace. - full_gc(); + full_gc_with_no_block_persistence(); assert!( crate::object::get_property_attrs(addr, "frozenKey").is_none(), @@ -393,6 +448,54 @@ fn test_dead_owner_symbol_property_entries_pruned_on_full_gc() { ); } +/// SABOTAGE CHECK for [`full_gc_with_no_block_persistence`] (#7975). Every +/// "the premise held" verdict it gives is vacuous unless the census it reads +/// can move — CLAUDE.md's fourth way a gate cannot fail. +/// +/// Plant the exact shape the #7975 confounder had, minus the realm: one ROOTED +/// object and one unrooted owner in the same arena block. Nothing here resolves +/// `globalThis` (`closure_set_dynamic_prop` does not), so the planted tenant is +/// the only reachable object in the persist window and the census has exactly +/// one satisfying path. +/// +/// Both halves are asserted: the census moves, AND the semantic consequence +/// that makes the premise guard necessary in the first place — a force-marked +/// owner is not dead, so the death prune correctly leaves its entry alone. +#[test] +fn test_block_persistence_census_moves_when_a_block_has_a_live_tenant() { + let _guard = CopyingNurseryTestGuard::new(1); + crate::arena::arena_reset_all_blocks_to_zero(); + + let (live, _) = unsafe { alloc_nursery_test_object(0) }; + js_shadow_slot_set(0, ptr_bits(live as usize)); + + let ptr = crate::arena::arena_alloc_gc( + std::mem::size_of::(), + 8, + GC_TYPE_CLOSURE, + ); + unsafe { init_test_closure(ptr) }; + let dead = ptr as usize; + crate::closure::closure_set_dynamic_prop(dead, "memo", 42.0); + assert!(crate::closure::closure_get_own_dynamic_prop(dead, "memo").is_some()); + + let force_marks_before = crate::gc::block_persist_force_mark_count(); + full_gc(); + + assert!( + crate::gc::block_persist_force_mark_count() > force_marks_before, + "block persistence must force-mark the unrooted neighbour of a rooted \ + object — a census that cannot move makes every \ + `full_gc_with_no_block_persistence` verdict in this file vacuous" + ); + assert!( + crate::closure::closure_get_own_dynamic_prop(dead, "memo").is_some(), + "and the consequence the premise guard exists to name: a force-marked \ + owner is NOT dead, so its side-table entry correctly survives — a case \ + that hits this shape is failing on its premise, not on the prune" + ); +} + #[test] fn test_dead_closure_side_table_entries_pruned_on_full_gc() { let _guard = GcTestIsolationGuard::new(); @@ -477,13 +580,17 @@ fn test_object_dead_payload_arm_clears_keys_index() { #[test] fn test_dead_arguments_object_entry_pruned_on_full_gc() { - let _guard = GcTestIsolationGuard::new(); + // `js_arguments_object_alloc` reaches `js_object_set_field_by_name`, which + // resolves the process-global memoized `Object.prototype` address; a MISS + // runs the lazy `globalThis` bootstrap inside this test — see + // `with_realm_bootstrapped` (#7975). + let _guard = GcTestIsolationGuard::with_realm_bootstrapped(); let undefined = f64::from_bits(crate::value::TAG_UNDEFINED); let obj = crate::object::js_arguments_object_alloc(undefined, undefined, 0); let addr = obj as usize; assert!(crate::object::test_arguments_object_registered(addr)); - full_gc(); + full_gc_with_no_block_persistence(); assert!( !crate::object::test_arguments_object_registered(addr), diff --git a/crates/perry-runtime/src/gc/tests/support.rs b/crates/perry-runtime/src/gc/tests/support.rs index 2e2ad07efe..9b022a22cd 100644 --- a/crates/perry-runtime/src/gc/tests/support.rs +++ b/crates/perry-runtime/src/gc/tests/support.rs @@ -324,7 +324,56 @@ pub(super) struct GcTestIsolationGuard { impl GcTestIsolationGuard { pub(super) fn new() -> Self { + Self::build(RealmBootstrap::LeaveLazy) + } + + /// [`GcTestIsolationGuard::new`] plus: run the lazy `globalThis` realm + /// bootstrap BEFORE the measured window opens (#7975). + /// + /// REQUIRED by any test that asserts an unrooted object is DEAD at a + /// collection and reaches the runtime through an API that can resolve the + /// realm. Two facts compose into a scheduling-dependent false failure: + /// + /// 1. `object::set_property_attrs` and `js_object_set_field_by_name` both + /// consult the **process-global** memoized `Object.prototype` address + /// (`array::prototype_addr`), and a MISS runs the whole lazy + /// `globalThis` bootstrap — ~1.15 MB allocated, ~410 KB of it live, + /// rooted for the life of the thread — inside the caller. The cache + /// misses exactly once per PROCESS, so WHICH libtest thread pays is a + /// scheduling accident. + /// 2. Arena block reset is all-or-nothing, so + /// `gc::trace::mark_block_persisting_arena_objects` force-MARKS every + /// object in a block that holds one reachable object. A test owner that + /// shares its block with a freshly-bootstrapped realm is therefore NOT + /// dead — and the death prune *correctly* declines to drop its + /// side-table entry, because a block that persists cannot recycle the + /// owner's address. + /// + /// Measured on `origin/main` before this existed: the two affected cases in + /// `dead_owner_side_tables` failed **200/200** runs when scheduled first and + /// **0/200** when any sibling resolved the cache first — 10/200 at + /// `--test-threads=10` over the whole module (#7975). + /// + /// Bootstrapping here — inside the isolation lock, but before + /// [`ScopedRootScannerRegistryGuard`] takes the thread's scanners and + /// before `reset_global_roots` — puts the realm graph OUTSIDE the window: + /// the guard then un-roots it, so it cannot keep the test's own block + /// alive. + pub(super) fn with_realm_bootstrapped() -> Self { + Self::build(RealmBootstrap::RunItNow) + } + + fn build(realm: RealmBootstrap) -> Self { let lock = copying_nursery_isolation_lock(); + if matches!(realm, RealmBootstrap::RunItNow) { + let global = crate::object::js_get_global_this(); + assert!( + crate::value::JSValue::from_bits(global.to_bits()).is_pointer(), + "the realm bootstrap must have produced a singleton — otherwise \ + it did not run here, and the confounder this guard exists to \ + move out of the window is still inside it" + ); + } let scanner_guard = ScopedRootScannerRegistryGuard::new(); reset_copying_nursery_runtime_test_state(); reset_shadow_stack(); @@ -337,6 +386,14 @@ impl GcTestIsolationGuard { } } +/// Whether [`GcTestIsolationGuard::build`] forces the lazy `globalThis` +/// bootstrap before opening the window. See +/// [`GcTestIsolationGuard::with_realm_bootstrapped`]. +enum RealmBootstrap { + LeaveLazy, + RunItNow, +} + impl Drop for GcTestIsolationGuard { fn drop(&mut self) { reset_copying_nursery_runtime_test_state(); diff --git a/crates/perry-runtime/src/gc/trace.rs b/crates/perry-runtime/src/gc/trace.rs index aa7ffb450a..13ae67f2a6 100644 --- a/crates/perry-runtime/src/gc/trace.rs +++ b/crates/perry-runtime/src/gc/trace.rs @@ -865,6 +865,46 @@ pub(super) fn trace_marked_objects(valid_ptrs: &ValidPointerSet) { /// old-block neighbors as new block-persist candidates. pub(super) const BLOCK_PERSIST_WINDOW: usize = 5; +thread_local! { + /// Objects this thread's block-persistence pass has FORCE-MARKED since + /// process start — i.e. kept alive for no reason other than sharing a + /// block with something reachable. + /// + /// The census exists for the same reason `gc::scan_fallback`'s does + /// (#7148): force-marking is a correctness measure with a *semantic* side + /// effect — an unrooted object in a persisting block is not dead, so every + /// death-keyed consumer (the `dead_owner` side-table prunes, the Map/Set + /// registry sweeps) legitimately declines to act on it. A test that + /// asserts "this unrooted owner is dead" is therefore asserting something + /// about the tenancy of its arena block, and without this counter it has no + /// way to say so: the mark is cleared again by the sweep, so nothing + /// observable survives the collection. #7975 is what that costs — two + /// `dead_owner_side_tables` cases failed 200/200 when the lazy `globalThis` + /// bootstrap landed in their block, and the failure named the prune. + /// + /// Updated once per pass (per fixed-point round), never per object. + static BLOCK_PERSIST_FORCE_MARKS: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +/// Running count of [`BLOCK_PERSIST_FORCE_MARKS`] on this thread. Compare +/// across a collection: an unchanged value means block persistence force-marked +/// nothing, so an object left unmarked by that collection was genuinely +/// unreachable rather than merely a neighbour of something reachable. +pub(crate) fn block_persist_force_mark_count() -> u64 { + BLOCK_PERSIST_FORCE_MARKS.with(std::cell::Cell::get) +} + +/// Record one pass's force-mark count. BOTH arms call this: the whole-cycle +/// pass below and the budgeted `BlockPersistCycleState` in `gc::cycle`, which +/// force-marks through its own loop. A census that only counted one of them +/// would read zero on exactly the cycles that are hardest to reason about. +pub(super) fn note_block_persist_force_marks(marked: usize) { + if marked != 0 { + BLOCK_PERSIST_FORCE_MARKS + .with(|count| count.set(count.get().saturating_add(marked as u64))); + } +} + pub(super) fn mark_block_persisting_arena_objects( valid_ptrs: &ValidPointerSet, ) -> BlockPersistTraceStats { @@ -940,6 +980,7 @@ pub(super) fn mark_block_persisting_arena_objects( }, ); stats.marked_objects += newly_marked; + note_block_persist_force_marks(newly_marked); if newly_marked == 0 { break;