Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/6801-stable-shape-identity-phase-c3a.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
perf(runtime): #6759 Phase C3a — stable shape identity. Shape records gain a monotonic `shape_id`; an owned keys array's grow-realloc migrates the record (slot map + id) to the new address instead of orphaning it, so a wide object's key→slot index survives capacity doublings; GC evacuation rekeys shape records to their keys array's to-space address via a metadata-rewrite scanner. Runtime-only, validation-on-hit trust model unchanged.
4 changes: 4 additions & 0 deletions crates/perry-runtime/src/gc/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,10 @@ pub fn gc_init() {
// reflect-metadata store were invisible to GC — values swept/moved under
// live references, owner keys stale after evacuation.
gc_register_mutable_root_scanner(crate::object::descriptor_state::scan_descriptor_roots_mut);
// #6759 Phase C3a: shape records follow their keys array across
// evacuation (metadata-rewrite rekey only; the records hold no heap
// references and mark nothing).
gc_register_mutable_root_scanner(crate::object::shapes::scan_shape_table_rekey_mut);
gc_register_mutable_root_scanner(crate::proxy::scan_proxy_roots_mut);
// Object/string-valued `err.<prop> = v` user props live as raw bits in
// ERROR_USER_PROPS — invisible to GC without this scanner (collectable
Expand Down
63 changes: 63 additions & 0 deletions crates/perry-runtime/src/gc/tests/dead_owner_side_tables.rs
Original file line number Diff line number Diff line change
Expand Up @@ -788,6 +788,69 @@ fn test_descriptor_meta_summary_survives_copied_minor_move() {
js_shadow_slot_set(0, 0);
}

/// #6759 Phase C3a: an owned keys array's grow-realloc migrates the shape
/// record (slot map + stable shape_id) to the new address instead of
/// orphaning it.
#[test]
fn test_shape_record_migrates_on_owned_grow() {
let _global = global_side_table_test_lock();
let old_addr: usize = 0xC3A0_0000_0000_1010;
let new_addr: usize = 0xC3A0_0000_0000_2020;
crate::object::shapes::test_seed_shape_entry(old_addr);
let id = crate::object::shapes::test_shape_id_for_keys(old_addr)
.expect("seeded entry must have an id");
assert!(id != 0, "shape ids are 1-based");

crate::object::shapes::shape_keys_grown(old_addr, new_addr as *const crate::array::ArrayHeader);

assert!(
!crate::object::shapes::test_shape_entry_exists(old_addr),
"grown-away address must no longer hold the record"
);
assert_eq!(
crate::object::shapes::test_shape_id_for_keys(new_addr),
Some(id),
"the record — including its stable shape_id — must move to the new address"
);
// Cleanup so the seeded address can't leak into later tests.
crate::object::shapes::shape_drop(new_addr as *const crate::array::ArrayHeader);
}

/// #6759 Phase C3a: GC evacuation MOVES a live keys array — the shape
/// table's metadata-rewrite scanner must rekey the record to the array's
/// to-space address (same pattern as the descriptor owner rekey), so a
/// wide object's slot map survives a copied minor.
#[test]
fn test_shape_record_rekeys_on_copied_minor_move() {
let _guard = CopyingNurseryTestGuard::new(2);
// The scoped registry starts empty — install the C3a rekey scanner.
gc_register_mutable_root_scanner(crate::object::shapes::scan_shape_table_rekey_mut);

let keys = unsafe { alloc_nursery_test_array() };
let old_addr = keys as usize;
crate::object::shapes::test_seed_shape_entry(old_addr);
let id = crate::object::shapes::test_shape_id_for_keys(old_addr)
.expect("seeded entry must have an id");
js_shadow_slot_set(0, ptr_bits(old_addr));

let _ = gc_collect_minor();

let new_addr = (js_shadow_slot_get(0) & POINTER_MASK) as usize;
assert_ne!(new_addr, old_addr, "test premise: the keys array must move");
assert!(
!crate::object::shapes::test_shape_entry_exists(old_addr),
"from-space address must no longer key the record"
);
assert_eq!(
crate::object::shapes::test_shape_id_for_keys(new_addr),
Some(id),
"the shape record must be rekeyed to the moved keys array"
);

crate::object::shapes::shape_drop(new_addr as *const crate::array::ArrayHeader);
js_shadow_slot_set(0, 0);
}

/// #6759 Phase B: the meta record is kept alive by its owner (the header
/// edge is a traced child slot) across a full non-moving collection, and an
/// explicit-null recording is preserved.
Expand Down
35 changes: 24 additions & 11 deletions crates/perry-runtime/src/object/field_set_by_name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1502,6 +1502,13 @@ pub extern "C" fn js_object_set_field_by_name(
refresh_roots_after_alloc!();
set_object_keys_array(obj, new_keys);
super::mark_object_dynamic_shape_unknown(obj);
// #6759 Phase C3a: an owned grow keeps its shape identity —
// migrate the record instead of orphaning it (a shared
// fork must NOT migrate: the old address still describes
// the siblings' live shape).
if !keys_shared {
super::shapes::shape_keys_grown(prev_keys_usize, new_keys);
}
overflow_set(obj as usize, new_index, vbits);
mirror_class_object_static_write(obj, key, value);
transition_cache_insert(
Expand Down Expand Up @@ -1529,6 +1536,11 @@ pub extern "C" fn js_object_set_field_by_name(
refresh_roots_after_alloc!();
set_object_keys_array(obj, new_keys);
super::mark_object_dynamic_shape_unknown(obj);
// #6759 Phase C3a: owned grow keeps its shape identity (see the
// overflow branch above).
if !keys_shared {
super::shapes::shape_keys_grown(prev_keys_usize, new_keys);
}
js_object_set_field(obj, new_index as u32, JSValue::from_bits(value.to_bits()));
mirror_class_object_static_write(obj, key, value);
if new_index as u32 >= (*obj).field_count {
Expand All @@ -1540,17 +1552,10 @@ pub extern "C" fn js_object_set_field_by_name(
new_keys as usize,
new_index as u32,
);
// The sidecar is keyed on the OBJECT pointer (see
// `keys_index_lookup`, which probes `obj as usize`), NOT the
// keys-array pointer — shape-sharing clones the keys array on
// every insert, so a keys-keyed entry would be orphaned each
// iteration. Previously this inline-slot append registered
// under `new_keys as usize`, so the obj-keyed lookup never
// found it and rebuilt the full O(key_count) index on every
// write — turning a wide build that stays on the inline-slot
// path (e.g. a class instance whose pre-sized inline capacity
// keeps appends below the overflow threshold) into O(n²). Use
// the object address to match the lookup + the overflow path.
// #6759 C1 note: `keys_index_insert` delegates to the keys-keyed
// shape records and takes the POST-append keys_array — with the
// C3a migration above, an owned grow lands the append on the
// migrated record rather than forcing a rebuild.
keys_index_insert(
(*obj).keys_array,
(new_index + 1) as u32,
Expand Down Expand Up @@ -1695,6 +1700,10 @@ pub extern "C" fn js_object_set_field_by_name(
refresh_roots_after_alloc!();
set_object_keys_array(obj, new_keys);
super::mark_object_dynamic_shape_unknown(obj);
// #6759 Phase C3a: owned grow keeps its shape identity.
if !keys_shared {
super::shapes::shape_keys_grown(prev_keys_usize, new_keys);
}
overflow_set(obj as usize, new_index, vbits);
mirror_class_object_static_write(obj, key, value);
// Record the shape transition so the next object sharing
Expand Down Expand Up @@ -1722,6 +1731,10 @@ pub extern "C" fn js_object_set_field_by_name(
// Update the object's keys_array pointer in case js_array_push reallocated
set_object_keys_array(obj, new_keys);
super::mark_object_dynamic_shape_unknown(obj);
// #6759 Phase C3a: owned grow keeps its shape identity.
if !keys_shared {
super::shapes::shape_keys_grown(prev_keys_usize, new_keys);
}

// Set the field at the new index and update logical field_count
js_object_set_field(obj, new_index as u32, JSValue::from_bits(value.to_bits()));
Expand Down
93 changes: 91 additions & 2 deletions crates/perry-runtime/src/object/shapes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,21 +26,40 @@ pub(crate) struct Shape {
/// incrementally (append-only while shared); shorter ⟹ a delete
/// compacted it — drop and rebuild on next lookup.
indexed_len: u32,
/// #6759 Phase C3a: stable shape identity, allocated once at record
/// birth and preserved by [`shape_keys_grown`] when an owned keys
/// array reallocates — the identity consumers re-key on in C3b
/// (FIELD_CACHE, typed_feedback exactness) so they stop churning on
/// capacity doublings and GC moves. 0 is never allocated ("no id").
#[allow(dead_code)]
shape_id: u32,
/// FNV-1a content hash of key bytes → candidate slots (collisions
/// resolved by the per-hit content validation).
slots: HashMap<u64, Vec<u32>>,
}

pub(crate) struct ShapeTable {
entries: RefCell<crate::fast_hash::PtrHashMap<usize, Shape>>,
/// #6759 Phase C3a: monotonic ShapeId allocator (1-based; 0 = none).
/// u32 wrap is theoretical (one id per shape BIRTH, not per object);
/// on wrap the allocator skips 0 and collision risk is bounded by the
/// validation-on-hit trust model like every other accelerator here.
next_id: std::cell::Cell<u32>,
}

impl ShapeTable {
pub(crate) fn new() -> Self {
ShapeTable {
entries: RefCell::new(crate::fast_hash::new_ptr_hash_map()),
next_id: std::cell::Cell::new(1),
}
}

fn alloc_shape_id(&self) -> u32 {
let id = self.next_id.get();
self.next_id.set(id.wrapping_add(1).max(1));
id
Comment on lines +58 to +61

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Prevent shape_id reuse on counter wrap.

After u32::MAX, this wraps and allocates 1 again. That breaks the promised monotonic, unique shape identity and would alias future identity-keyed consumers. Use a non-wrapping wider ID (preferably u64) or fail explicitly before reuse.

Proposed fix
-    shape_id: u32,
+    shape_id: u64,
...
-    next_id: std::cell::Cell<u32>,
+    next_id: std::cell::Cell<u64>,
...
-    fn alloc_shape_id(&self) -> u32 {
+    fn alloc_shape_id(&self) -> u64 {
         let id = self.next_id.get();
-        self.next_id.set(id.wrapping_add(1).max(1));
+        self.next_id
+            .set(id.checked_add(1).expect("shape id space exhausted"));
         id
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fn alloc_shape_id(&self) -> u32 {
let id = self.next_id.get();
self.next_id.set(id.wrapping_add(1).max(1));
id
fn alloc_shape_id(&self) -> u64 {
let id = self.next_id.get();
self.next_id
.set(id.checked_add(1).expect("shape id space exhausted"));
id
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/perry-runtime/src/object/shapes.rs` around lines 58 - 61, Update
ShapeRegistry::alloc_shape_id so the shape counter cannot wrap and reuse an
existing ID after u32::MAX. Use a wider non-wrapping counter, preferably u64,
and update the associated shape ID storage/types as needed; otherwise fail
explicitly before allocation would reuse an ID. Preserve monotonic, unique IDs
for all successful allocations.

}
}

/// Build (or extend) the slot map for `keys` covering `key_count` keys.
Expand Down Expand Up @@ -73,7 +92,8 @@ pub(crate) unsafe fn shape_slot_lookup(
build: bool,
) -> Option<u32> {
let keys_id = keys as usize;
let mut entries = crate::state::state().shapes.entries.borrow_mut();
let table = &crate::state::state().shapes;
let mut entries = table.entries.borrow_mut();
let shape = match entries.get_mut(&keys_id) {
Some(s) => {
if s.indexed_len > key_count {
Expand All @@ -87,8 +107,10 @@ pub(crate) unsafe fn shape_slot_lookup(
if !build {
return None;
}
let shape_id = table.alloc_shape_id();
entries.entry(keys_id).or_insert(Shape {
indexed_len: 0,
shape_id,
slots: HashMap::with_capacity(key_count as usize),
})
}
Expand Down Expand Up @@ -140,6 +162,30 @@ pub(crate) fn shape_note_hit(keys: *const ArrayHeader, key_hash: u64, slot: u32)
}
}

/// #6759 Phase C3a: an OWNED (non-`GC_FLAG_SHAPE_SHARED`) keys array was
/// reallocated by `js_array_push` — the SAME logical shape now lives at a
/// new address. Migrate the record (slot map, indexed_len, shape_id) so it
/// survives the capacity doubling; pre-C3a the record was orphaned at the
/// old address and the next lookup rebuilt it O(key_count), making every
/// doubling of a wide object's build pay a full re-index.
///
/// Callers must pass the OWNED-grow pair only: a shared array's fork is a
/// genuine transition (the clone starts a NEW identity and the old address
/// still describes the siblings' live shape — migrating it would corrupt
/// them). Safety net: a wrong or stale migration cannot produce wrong
/// results — every hit re-validates key bytes against the live array —
/// it only wastes the rebuild this exists to save.
pub(crate) fn shape_keys_grown(old_keys: usize, new_keys: *const ArrayHeader) {
let new_id = new_keys as usize;
if old_keys == 0 || new_id == 0 || old_keys == new_id {
return;
}
let mut entries = crate::state::state().shapes.entries.borrow_mut();
if let Some(shape) = entries.remove(&old_keys) {
entries.insert(new_id, shape);
}
}

/// Drop the shape for a keys_array that was compacted/retired in place
/// (delete path). Address-recycled arrays need no eager drop — validation
/// rejects them — but the delete path knows the map is stale NOW.
Expand All @@ -160,6 +206,36 @@ pub(crate) fn prune_dead_shape_keys(is_dead_owner: &dyn Fn(usize) -> bool) {
}
}

/// #6759 Phase C3a: rekey shape records when GC evacuation MOVES their
/// keys array, so a wide object's slot map (and its stable `shape_id`)
/// survives a copied minor instead of being orphaned at the from-space
/// address and rebuilt O(key_count) on the next lookup. Metadata-rewrite
/// only — the records hold no heap references (slot indexes + an address
/// used as identity), so outside that phase this scanner is a no-op and
/// marks nothing. Same pattern as the descriptor-table owner rekey.
pub(crate) fn scan_shape_table_rekey_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) {
if !visitor.is_metadata_rewrite_phase() {
return;
}
let mut entries = crate::state::state().shapes.entries.borrow_mut();
if entries.is_empty() {
return;
}
let moved: Vec<(usize, usize)> = entries
.keys()
.filter_map(|&keys_id| {
let mut addr = keys_id;
visitor.visit_metadata_usize_slot(&mut addr);
(addr != keys_id).then_some((keys_id, addr))
})
.collect();
for (old, new) in moved {
if let Some(shape) = entries.remove(&old) {
entries.insert(new, shape);
}
}
}

#[cfg(test)]
pub(crate) fn test_shape_entry_exists(keys_id: usize) -> bool {
crate::state::state()
Expand All @@ -172,11 +248,24 @@ pub(crate) fn test_shape_entry_exists(keys_id: usize) -> bool {

#[cfg(test)]
pub(crate) fn test_seed_shape_entry(keys_id: usize) {
crate::state::state().shapes.entries.borrow_mut().insert(
let table = &crate::state::state().shapes;
let shape_id = table.alloc_shape_id();
table.entries.borrow_mut().insert(
keys_id,
Shape {
indexed_len: 0,
shape_id,
slots: HashMap::new(),
},
);
}

#[cfg(test)]
pub(crate) fn test_shape_id_for_keys(keys_id: usize) -> Option<u32> {
crate::state::state()
.shapes
.entries
.borrow()
.get(&keys_id)
.map(|s| s.shape_id)
}
Loading