gc(shapes) phase 3: make the transition cache's target edge weak, and reap dead targets

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.

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"
+        );
+    }
+}

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());