perf(runtime): compact an owned keys array in place on delete (populated delete −26.8%)

changelog.d/9013-delete-inplace-owned-keys.md

@@ -0,0 +1,32 @@
+`delete obj[k]` stops allocating when the keys array is owned.
+
+The delete cloned the keys array unconditionally, because one array is shared
+by every object that built the shape through a `transition_cache_lookup` hit —
+mutating it in place would drop entries from siblings that never deleted
+anything.
+
+Sharing is tracked, though: the caches stamp `GC_FLAG_SHAPE_SHARED` when they
+publish an array, and both the ordinary `[[Set]]` growth path and
+`object_ops::keys_array` already treat that bit as authoritative for exactly
+this clone-or-mutate decision. An array without it has a single owner and can
+be compacted in place, which removes the last per-delete allocation here — a
+~500-element clone, 200k of them on `bench_populated_delete.ts` — and keeps
+the array's address, so the key index needs only a slot shift rather than a
+migration to a new id.
+
+**The shape must be re-published** for the same array at its new key count.
+Without that the object keeps a stamped ShapeId whose descriptor still claims
+the pre-delete count, and the shape-facts audit catches it outright
+("published ShapeId disagrees with authoritative ObjectHeader facts") — an
+earlier version of this change failed precisely there.
+`publish_object_shape_from` versions a same-pointer change internally, and
+`keys_changed` is false, so the typed layout is preserved rather than marked
+unknown.
+
+Interleaved A/B, min-of-13 at quiet load (~1.0): `bench_populated_delete`
+2768 → **2026 ms, −26.8%** (mean −27.0%). Combined overwrite and
+realistic-name read unchanged.
+
+Cumulative this session: **5938 → 2026 ms, −65.9%**, i.e. ~280× node → ~96×.
+Suite 2788 passed, no warnings; adversarial property differential and
+computed-key differential both byte-identical to node.

crates/perry-runtime/src/object/delete_rest.rs

@@ -332,58 +332,100 @@ pub extern "C" fn js_object_delete_field(
         let alloc_limit = std::cmp::max(field_count as usize, crate::object::INLINE_SLOT_FLOOR);
         let new_count = key_count - 1;
 
-        // CRITICAL: clone the keys_array before mutating it. The same
-        // keys_array is shared across all objects that built the same
-        // shape via `transition_cache_lookup`-hit fast paths. Without
-        // cloning, mutating its length / contents to remove the deleted
-        // key would corrupt every other object that picks up this
-        // shape — they'd silently lose entries they never deleted.
-        let keys_cloned = crate::array::js_array_alloc(new_count.max(1) as u32 + 4);
-        let src_elements =
-            (keys as *const u8).add(std::mem::size_of::<crate::ArrayHeader>()) as *const f64;
-        let dst_elements =
-            (keys_cloned as *mut u8).add(std::mem::size_of::<crate::ArrayHeader>()) as *mut f64;
-        // Copy keys [0..i) ++ [i+1..N) into [0..new_count) as two contiguous
-        // runs. These were scalar element loops, which is O(resident keys) of
-        // load/store pairs on a path that already allocates and rebuilds a
-        // layout per delete — and `delete obj[k]` on a populated object is
-        // perry's worst object-model gap against node (~200x on
-        // `bench_populated_delete.ts`).
+        // The clone below exists because one keys_array is shared by every
+        // object that built this shape through a `transition_cache_lookup`
+        // hit: mutating it in place would silently drop entries from siblings
+        // that never deleted anything.
         //
-        // GC_STORE_AUDIT(INIT): the destination is a freshly allocated, still
-        // UNPUBLISHED keys array whose layout is rebuilt before it is
-        // published — which is why the per-element writes carried no barrier
-        // either. Source and destination are distinct allocations, so the
-        // copies cannot overlap.
-        if i > 0 {
-            std::ptr::copy_nonoverlapping(src_elements, dst_elements, i);
-        }
-        if new_count > i {
-            // GC_STORE_AUDIT(INIT): same unpublished destination as the run
-            // above — freshly allocated keys array, layout rebuilt before
-            // `set_object_keys_array` publishes it, distinct allocations.
-            std::ptr::copy_nonoverlapping(
-                src_elements.add(i + 1),
-                dst_elements.add(i),
-                new_count - i,
+        // Sharing is TRACKED. The caches stamp `GC_FLAG_SHAPE_SHARED` when
+        // they publish an array (`transition_cache_insert`), and both the
+        // ordinary `[[Set]]` growth path and `object_ops::keys_array` already
+        // treat that bit as authoritative for exactly this decision. An array
+        // without it has a single owner, so it can be compacted in place —
+        // removing the last per-delete allocation on this path (a ~500-element
+        // clone, 200k of them on `bench_populated_delete.ts`) and keeping the
+        // array's ADDRESS, so the key index only needs its slots shifted.
+        let keys_gc_header =
+            (keys as *const u8).sub(crate::gc::GC_HEADER_SIZE) as *const crate::gc::GcHeader;
+        let keys_owned = (*keys_gc_header).gc_flags & crate::gc::GC_FLAG_SHAPE_SHARED == 0;
+        let index_migrated = if keys_owned {
+            let elements =
+                (keys as *mut u8).add(std::mem::size_of::<crate::ArrayHeader>()) as *mut f64;
+            // Overlapping ranges inside ONE allocation: `copy` (memmove).
+            //
+            // Unlike the clone arm below, this destination is the LIVE,
+            // PUBLISHED array, so that arm's "unpublished, so no barrier" does
+            // not carry over. No new referent is introduced -- every value
+            // moved was already in this array one slot higher.
+            // Same shape as `Array.prototype.splice`'s tail memmove
+            // (`array/splice_slice.rs`), and safe for the same reason.
+            if new_count > i {
+                // GC_STORE_AUDIT(BARRIERED): `rebuild_array_layout_from_slots`
+                // runs just below and, for an old-gen array, re-runs
+                // `runtime_write_barrier_slot` over every slot; `length` is set
+                // first so it covers exactly the compacted range.
+                std::ptr::copy(elements.add(i + 1), elements.add(i), new_count - i);
+            }
+            (*keys).length = new_count as u32;
+            super::rebuild_array_layout_from_slots(keys);
+            // Re-publish the shape for the SAME array at its new key count.
+            // Without this the object keeps a stamped ShapeId whose descriptor
+            // still claims the pre-delete count, which the shape-facts audit
+            // catches as "published ShapeId disagrees with authoritative
+            // ObjectHeader facts". `publish_object_shape_from` versions a
+            // same-pointer change internally, and `keys_changed` is false here
+            // so the typed layout is preserved rather than marked unknown.
+            set_object_keys_array(obj, keys as *mut crate::ArrayHeader);
+            super::shapes::shape_index_shift_in_place(keys as usize, i as u32, key_count as u32)
+        } else {
+            let keys_cloned = crate::array::js_array_alloc(new_count.max(1) as u32 + 4);
+            let src_elements =
+                (keys as *const u8).add(std::mem::size_of::<crate::ArrayHeader>()) as *const f64;
+            let dst_elements =
+                (keys_cloned as *mut u8).add(std::mem::size_of::<crate::ArrayHeader>()) as *mut f64;
+            // Copy keys [0..i) ++ [i+1..N) into [0..new_count) as two contiguous
+            // runs. These were scalar element loops, which is O(resident keys) of
+            // load/store pairs on a path that already allocates and rebuilds a
+            // layout per delete — and `delete obj[k]` on a populated object is
+            // perry's worst object-model gap against node (~200x on
+            // `bench_populated_delete.ts`).
+            //
+            // GC_STORE_AUDIT(INIT): the destination is a freshly allocated, still
+            // UNPUBLISHED keys array whose layout is rebuilt before it is
+            // published — which is why the per-element writes carried no barrier
+            // either. Source and destination are distinct allocations, so the
+            // copies cannot overlap.
+            if i > 0 {
+                std::ptr::copy_nonoverlapping(src_elements, dst_elements, i);
+            }
+            if new_count > i {
+                // GC_STORE_AUDIT(INIT): same unpublished destination as the run
+                // above — freshly allocated keys array, layout rebuilt before
+                // `set_object_keys_array` publishes it, distinct allocations.
+                std::ptr::copy_nonoverlapping(
+                    src_elements.add(i + 1),
+                    dst_elements.add(i),
+                    new_count - i,
+                );
+            }
+            (*keys_cloned).length = new_count as u32;
+            super::rebuild_array_layout_from_slots(keys_cloned);
+            // Carry the key index onto the clone by shifting slots, instead of
+            // letting the new address miss `indices` and re-hash every surviving
+            // property name. On a 500-key object that rebuild ran on EVERY delete.
+            // A wrong index can only cause a miss — `shape_slot_lookup` validates
+            // the stored key against the requested bytes before returning a slot.
+            let index_migrated = super::shapes::shape_index_migrate_after_delete(
+                keys as usize,
+                keys_cloned as usize,
+                i as u32,
+                key_count as u32,
             );
-        }
-        (*keys_cloned).length = new_count as u32;
-        super::rebuild_array_layout_from_slots(keys_cloned);
-        // Carry the key index onto the clone by shifting slots, instead of
-        // letting the new address miss `indices` and re-hash every surviving
-        // property name. On a 500-key object that rebuild ran on EVERY delete.
-        // A wrong index can only cause a miss — `shape_slot_lookup` validates
-        // the stored key against the requested bytes before returning a slot.
-        let index_migrated = super::shapes::shape_index_migrate_after_delete(
-            keys as usize,
-            keys_cloned as usize,
-            i as u32,
-            key_count as u32,
-        );
-        // `set_object_keys_array` publishes the cloned edge while preserving
-        // the predecessor's semantic generation and object kind.
-        set_object_keys_array(obj, keys_cloned);
+            // `set_object_keys_array` publishes the cloned edge while preserving
+            // the predecessor's semantic generation and object kind.
+            set_object_keys_array(obj, keys_cloned);
+            index_migrated
+        };
 
         // 1) Shift values down: for slot j in i..new_count, copy slot j+1
         //    into slot j. Inline reads/writes for j < alloc_limit;

crates/perry-runtime/src/object/shapes.rs

@@ -34,7 +34,8 @@ use std::cell::RefCell;
 #[path = "shapes_slot_list.rs"]
 mod shapes_slot_list;
 pub(crate) use shapes_slot_list::{
-    record_shape_scan_outcome, shape_index_migrate_after_delete, SlotList,
+    record_shape_scan_outcome, shape_index_migrate_after_delete, shape_index_shift_in_place,
+    SlotList,
 };
 
 pub(crate) struct ShapeIndex {

crates/perry-runtime/src/object/shapes_slot_list.rs

@@ -112,6 +112,41 @@ pub(crate) fn record_shape_scan_outcome(
     }
 }
 
+/// Shift a key index in place after an IN-PLACE delete.
+///
+/// Twin of [`shape_index_migrate_after_delete`] for an OWNED keys array (no
+/// `GC_FLAG_SHAPE_SHARED`), which is compacted in place and therefore keeps
+/// its address — and hence its `indices` key. Same shift, same safety net:
+/// `shape_slot_lookup` re-validates the stored key against the requested
+/// bytes, so a wrong index yields a miss, never a wrong property.
+///
+/// Returns whether the index is now current, so the caller can skip the
+/// `shape_drop` that would otherwise discard it.
+#[must_use]
+pub(crate) fn shape_index_shift_in_place(
+    keys_id: usize,
+    removed_slot: u32,
+    old_key_count: u32,
+) -> bool {
+    if keys_id == 0 {
+        return false;
+    }
+    let mut inner = crate::state::state().shapes.inner.borrow_mut();
+    let Some(index) = inner.indices.get_mut(&keys_id) else {
+        return false;
+    };
+    if index.indexed_len < old_key_count {
+        inner.indices.remove(&keys_id);
+        return false;
+    }
+    index.slots.retain(|_, list| {
+        list.retain_shift(removed_slot);
+        !list.is_empty()
+    });
+    index.indexed_len = old_key_count - 1;
+    true
+}
+
 /// Carry a key index across a delete, instead of re-hashing every key name.
 ///
 /// `delete obj[k]` clones the keys array, so the result has a new address and