diff --git a/changelog.d/9013-delete-inplace-owned-keys.md b/changelog.d/9013-delete-inplace-owned-keys.md new file mode 100644 index 0000000000..dec4eb240b --- /dev/null +++ b/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. diff --git a/crates/perry-runtime/src/object/delete_rest.rs b/crates/perry-runtime/src/object/delete_rest.rs index d942ec8048..38faaf6301 100644 --- a/crates/perry-runtime/src/object/delete_rest.rs +++ b/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::()) as *const f64; - let dst_elements = - (keys_cloned as *mut u8).add(std::mem::size_of::()) 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::()) 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::()) as *const f64; + let dst_elements = + (keys_cloned as *mut u8).add(std::mem::size_of::()) 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; diff --git a/crates/perry-runtime/src/object/shapes.rs b/crates/perry-runtime/src/object/shapes.rs index 400e69df04..50c88cb9c9 100644 --- a/crates/perry-runtime/src/object/shapes.rs +++ b/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 { diff --git a/crates/perry-runtime/src/object/shapes_slot_list.rs b/crates/perry-runtime/src/object/shapes_slot_list.rs index bc2a1ae521..eefdba9125 100644 --- a/crates/perry-runtime/src/object/shapes_slot_list.rs +++ b/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