From cb8d7d463a6a1eca24f11b7bc17eecabd557e625 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 29 Aug 2026 08:06:30 +0200 Subject: [PATCH 1/2] perf(runtime): compact an OWNED keys array in place on delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit delete obj[k] cloned the keys array unconditionally, because one array is shared by every object that built the shape via a transition_cache_lookup hit and mutating it would drop entries from siblings. But sharing is tracked: the caches stamp GC_FLAG_SHAPE_SHARED when they publish an array, and both the [[Set]] growth path and object_ops::keys_array already treat that bit as authoritative for this same decision. An array without it has a single owner and can be compacted in place — removing the last per-delete allocation here (a ~500-element clone, 200k of them on bench_populated_delete.ts) and keeping the array's address, so the key index needs only its slots shifted rather than a migration. 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, which the shape-facts audit catches outright ("published ShapeId disagrees with authoritative ObjectHeader facts") — an earlier version of this change failed exactly 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. Claude-Session: https://claude.ai/code/session_01Ay8VyLkKbm8Hkc1xmvTEsP --- changelog.d/9013-delete-inplace-owned-keys.md | 32 +++++ .../perry-runtime/src/object/delete_rest.rs | 131 +++++++++++------- crates/perry-runtime/src/object/shapes.rs | 3 +- .../src/object/shapes_slot_list.rs | 35 +++++ 4 files changed, 150 insertions(+), 51 deletions(-) create mode 100644 changelog.d/9013-delete-inplace-owned-keys.md 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..64eb077711 100644 --- a/crates/perry-runtime/src/object/delete_rest.rs +++ b/crates/perry-runtime/src/object/delete_rest.rs @@ -332,58 +332,89 @@ 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). + if new_count > i { + 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..65963f3091 100644 --- a/crates/perry-runtime/src/object/shapes_slot_list.rs +++ b/crates/perry-runtime/src/object/shapes_slot_list.rs @@ -129,6 +129,41 @@ pub(crate) fn record_shape_scan_outcome( /// /// Returns whether the index was actually carried over: the delete tail uses /// that to skip the `shape_drop` that would otherwise discard it immediately. +/// 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 +} + #[must_use] pub(crate) fn shape_index_migrate_after_delete( old_keys_id: usize, From 10fdac378a2f8e75097c289dfd4a018c4291490b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 29 Aug 2026 08:49:15 +0200 Subject: [PATCH 2/2] fix(runtime): audit the in-place delete store, and unglue two doc comments `scripts/gc_store_site_inventory.py` (a `lint` gate) rejects the new in-place memmove: it has no GC_STORE_AUDIT marker, and the clone arm's INIT reasoning does not carry over -- that destination is unpublished, this one is the LIVE PUBLISHED array. Marked BARRIERED, matching `Array.prototype.splice`'s tail memmove, which is the same shape. The claim is checkable rather than asserted: `rebuild_array_layout_from_slots` (object/gc_slots.rs) re-runs `runtime_write_barrier_slot` over every slot when the array is old-gen, and it runs immediately below with `length` already set to `new_count`, so it covers exactly the compacted range. No new referent is introduced either -- every value moved was already in this array one slot higher. Also: the new function's doc comment landed INSIDE `shape_index_migrate_after_delete`'s, so the merged block claimed "`delete obj[k]` clones the keys array, so the result has a new address" directly above `shape_index_shift_in_place`, which keeps the address -- while the migrate function was left with no docs at all. Split back apart. --- .../perry-runtime/src/object/delete_rest.rs | 11 ++++++ .../src/object/shapes_slot_list.rs | 34 +++++++++---------- 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/crates/perry-runtime/src/object/delete_rest.rs b/crates/perry-runtime/src/object/delete_rest.rs index 64eb077711..38faaf6301 100644 --- a/crates/perry-runtime/src/object/delete_rest.rs +++ b/crates/perry-runtime/src/object/delete_rest.rs @@ -352,7 +352,18 @@ pub extern "C" fn js_object_delete_field( 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; diff --git a/crates/perry-runtime/src/object/shapes_slot_list.rs b/crates/perry-runtime/src/object/shapes_slot_list.rs index 65963f3091..eefdba9125 100644 --- a/crates/perry-runtime/src/object/shapes_slot_list.rs +++ b/crates/perry-runtime/src/object/shapes_slot_list.rs @@ -112,23 +112,6 @@ pub(crate) fn record_shape_scan_outcome( } } -/// 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 -/// misses `indices` — which meant a 500-key object rebuilt its whole index on -/// every delete, decoding and FNV-hashing all ~500 property names each time. -/// The surviving keys are the same strings in the same order minus one, so the -/// index can be shifted rather than recomputed: drop the removed slot and -/// decrement every slot above it. No key bytes are touched. -/// -/// Safe against a mistake by construction: [`shape_slot_lookup`] re-validates -/// the stored key against the requested bytes before returning a slot, so an -/// index that is wrong produces a MISS and the caller's own fallback, never a -/// wrong property. Only a fully-built index is carried over; a partially built -/// one is dropped and rebuilt as before. -/// -/// Returns whether the index was actually carried over: the delete tail uses -/// that to skip the `shape_drop` that would otherwise discard it immediately. /// Shift a key index in place after an IN-PLACE delete. /// /// Twin of [`shape_index_migrate_after_delete`] for an OWNED keys array (no @@ -164,6 +147,23 @@ pub(crate) fn shape_index_shift_in_place( 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 +/// misses `indices` — which meant a 500-key object rebuilt its whole index on +/// every delete, decoding and FNV-hashing all ~500 property names each time. +/// The surviving keys are the same strings in the same order minus one, so the +/// index can be shifted rather than recomputed: drop the removed slot and +/// decrement every slot above it. No key bytes are touched. +/// +/// Safe against a mistake by construction: [`shape_slot_lookup`] re-validates +/// the stored key against the requested bytes before returning a slot, so an +/// index that is wrong produces a MISS and the caller's own fallback, never a +/// wrong property. Only a fully-built index is carried over; a partially built +/// one is dropped and rebuilt as before. +/// +/// Returns whether the index was actually carried over: the delete tail uses +/// that to skip the `shape_drop` that would otherwise discard it immediately. #[must_use] pub(crate) fn shape_index_migrate_after_delete( old_keys_id: usize,