diff --git a/changelog.d/8900-transition-cache-weak-target.md b/changelog.d/8900-transition-cache-weak-target.md new file mode 100644 index 0000000000..a24f45de95 --- /dev/null +++ b/changelog.d/8900-transition-cache-weak-target.md @@ -0,0 +1,34 @@ +Stopped the transition cache from keeping shapes alive: its `next_keys` edge is +now weak, and an entry whose target died is reaped. + +`scan_transition_cache_roots_mut` visited `next_keys` with `visit_usize_slot`, +which **marks**. With 16384 slots the cache could therefore pin 16384 keys +arrays — and, through them, their shape descriptors — whether or not any live +object still had that shape. + +That is a direct contributor to the shape table growing without bound between +full collections, measured at **786,205 descriptors on a workload holding under +400 live objects**, with the shape scanner's cost tracking it (3.6 ms → 490 ms +per call). + +A transition entry is a pure cache: it answers *"adding key k to shape S yields +shape T"*. If nothing has shape T any more, the answer is worthless, so pinning +T's keys array to keep it answerable is backwards. `key_ptr` in the same entry +was already weak and metadata-only for exactly this reason; this makes the pair +consistent. + +Both halves move together, and have to: + +* `scan_transition_cache_roots_mut` now visits `next_keys` rewrite-only, so a + surviving target's address stays correct but a dead one is not resurrected; +* `prune_dead_transition_cache_entries` gains `is_dead_owner(entry.next_keys)`, + so an entry whose target did not survive is dropped rather than left dangling. + +Weakening the edge without the reaping half would leave a stale pointer in the +cache. + +The regression test seeds an entry with a **live** `prev_shape_id` on purpose. +An earlier version used `prev_shape_id = 0`, which the prune's pre-existing +`shape_descriptor_by_id(..).is_none()` clause already treats as dead — so it +passed with the new clause deleted and proved nothing. Sabotage-checked in its +final form: removing `is_dead_owner(entry.next_keys)` fails it. 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 acaf11a5fa..1cae375459 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 @@ -1812,3 +1812,58 @@ fn object_meta_expando_is_appended_not_inserted() { "expando must sit after every field codegen has an offset contract on" ); } + +/// #6759 phase 3: a transition-cache entry must NOT keep its target keys array +/// alive, and an entry whose target died must be reaped. +/// +/// `next_keys` used to be visited with `visit_usize_slot`, which MARKS. With +/// 16384 slots the cache could pin 16384 keys arrays — and through them their +/// shape descriptors — whether or not any live object still had that shape, +/// feeding unbounded shape-table growth. A transition entry is a pure cache +/// ("adding key k to shape S yields shape T"); if nothing has shape T, the +/// answer is worthless, so pinning T to keep it answerable is backwards. +/// `key_ptr` was already weak; this makes the pair consistent. +/// +/// The two halves must move together — weakening the edge without reaping dead +/// targets leaves a dangling `next_keys`. This pins the reaping half. +/// +/// The entry is seeded with a LIVE `prev_shape_id` on purpose. An earlier +/// version of this test used `prev_shape_id = 0`, which the prune's +/// pre-existing `shape_descriptor_by_id(..).is_none()` clause already treats as +/// dead — so it passed with the new clause deleted, proving nothing. Sabotage +/// check: removing `is_dead_owner(entry.next_keys)` must fail this test. +#[test] +fn transition_cache_entry_does_not_pin_its_target() { + let _lock = crate::gc::global_side_table_test_lock(); + unsafe { + // A real object gives a real, live shape id, so the ONLY thing that can + // make the seeded entry dead is its target. + let obj = crate::object::js_object_alloc(0, 0); + let keys = crate::object::object_keys_array(obj); + let live_shape = crate::object::shapes::test_shape_id_for_keys(keys as usize) + .expect("a freshly allocated object must have a registered shape"); + assert!( + crate::object::shapes::shape_descriptor_by_id(live_shape).is_some(), + "test premise: prev_shape_id must be LIVE, or the prune's existing \ + dead-shape clause decides the outcome and this test is vacuous" + ); + + let before = crate::object::test_transition_cache_occupancy(); + let dead_target = 0xDEAD_0000_1000usize; + crate::object::test_seed_transition_cache_entry(live_shape, 0, dead_target); + assert!( + crate::object::test_transition_cache_occupancy() > before, + "test premise: the entry must actually be installed" + ); + + // Only the target address is dead. + crate::object::prune_dead_transition_cache_entries(&|addr| addr == dead_target); + + assert_eq!( + crate::object::test_transition_cache_occupancy(), + before, + "an entry whose target keys array is dead must be dropped — without \ + this, weakening `next_keys` leaves a dangling pointer in the cache" + ); + } +} diff --git a/crates/perry-runtime/src/object/mod.rs b/crates/perry-runtime/src/object/mod.rs index 3a8cbc1198..bd070d04bf 100644 --- a/crates/perry-runtime/src/object/mod.rs +++ b/crates/perry-runtime/src/object/mod.rs @@ -1024,7 +1024,26 @@ pub fn scan_transition_cache_roots_mut(visitor: &mut crate::gc::RuntimeRootVisit if entry.next_keys != 0 { let mut invalidate = false; invalidate |= visitor.visit_metadata_usize_slot(&mut entry.key_ptr); - visitor.visit_usize_slot(&mut entry.next_keys); + // #6759 phase 3: `next_keys` is WEAK, not a strong root. + // + // `visit_usize_slot` MARKS. With 16384 slots this cache was + // therefore keeping up to 16384 keys arrays — and, through + // them, their shape descriptors — alive whether or not any live + // object still had that shape. That is a direct contributor to + // the shape table growing without bound between full + // collections (measured: 786k descriptors on a workload holding + // under 400 live objects). + // + // A transition entry is a pure cache: it answers "adding key k + // to shape S yields shape T". If nothing has shape T any more, + // the answer is worthless, so pinning T's keys array to keep it + // answerable is backwards. `key_ptr` was already weak for the + // same reason; this makes the pair consistent. + // + // Rewrite-only keeps a surviving target's address correct; + // `prune_dead_transition_cache_entries` drops the entry when the + // target did not survive. + visitor.visit_metadata_usize_slot(&mut entry.next_keys); if invalidate { *entry = TransitionEntry { key_ptr: 0, @@ -1066,6 +1085,10 @@ pub(crate) fn prune_dead_transition_cache_entries(is_dead_owner: &dyn Fn(usize) continue; } let dead = (entry.key_ptr != 0 && is_dead_owner(entry.key_ptr)) + // #6759 phase 3: `next_keys` stopped being a strong root, so a + // dead target is now possible and must be reaped here — this is + // the half that makes weakening it safe. + || is_dead_owner(entry.next_keys) || shapes::shape_descriptor_by_id(entry.prev_shape_id).is_none() || (entry.target_shape_id != 0 && shapes::shape_descriptor_by_id(entry.target_shape_id).is_none());