diff --git a/changelog.d/keystroke-shape-family-append.md b/changelog.d/keystroke-shape-family-append.md new file mode 100644 index 0000000000..af11fad955 --- /dev/null +++ b/changelog.d/keystroke-shape-family-append.md @@ -0,0 +1,14 @@ +### Fixed + +- Interning a shape descriptor no longer scans the keys array's whole + descriptor history. `ShapeTableInner::family_push_back` / `facts_push_back` + answer "is this id already here?" with a linear scan of the family, and a + family accumulates every descriptor ever created for one keys array — so + interning the *n*-th descriptor for a keys array cost O(n) and a render that + keeps bumping a shape's semantic generation paid quadratic time. The two + interning sites append ids that `alloc_shape_id` has just handed out, and + that allocator never reuses a value, so the scan was provably dead work: + they now use `IdList::append_unchecked`. On the compiled claude-code TUI + `IdList::contains` was 6.2 % of main-thread leaf samples during a streamed + reply and 5.9 % in the window after it, 95 % of it under + `family_push_back`. diff --git a/crates/perry-runtime/src/object/shapes.rs b/crates/perry-runtime/src/object/shapes.rs index 049f72295f..8d26b9c024 100644 --- a/crates/perry-runtime/src/object/shapes.rs +++ b/crates/perry-runtime/src/object/shapes.rs @@ -264,6 +264,15 @@ impl ShapeTableInner { self.families.entry(keys).or_default().push_back(id); } + /// Append a FRESHLY allocated id (see [`IdList::append_unchecked`]): the + /// id came from `alloc_shape_id`, which never reuses a value, so the + /// membership scan `family_push_back` would run is dead work that is + /// linear in the number of descriptors this keys array has ever had. + #[inline] + fn family_append_fresh(&mut self, keys: u64, id: u32) { + self.families.entry(keys).or_default().append_unchecked(id); + } + #[inline] fn family_push_front(&mut self, keys: u64, id: u32) { self.families.entry(keys).or_default().push_front(id); @@ -287,6 +296,12 @@ impl ShapeTableInner { self.by_facts.entry(facts).or_default().push_back(id); } + /// Fresh-id twin of [`ShapeTableInner::facts_push_back`]; same argument. + #[inline] + fn facts_append_fresh(&mut self, facts: u64, id: u32) { + self.by_facts.entry(facts).or_default().append_unchecked(id); + } + #[inline] fn facts_push_front(&mut self, facts: u64, id: u32) { self.by_facts.entry(facts).or_default().push_front(id); @@ -507,8 +522,12 @@ pub(crate) fn shape_descriptor_ensure_with_holes( // complete descriptor. // SAFETY: no slab reference is held; `slab()` above went out of scope. unsafe { table.slab_mut().insert(id, record) }; - inner.facts_push_back(facts, id); - inner.family_push_back(keys_id, id); + // `id` was just handed out by `alloc_shape_id`, which never reuses a + // value, so neither accelerator can already hold it: append without the + // membership scan, whose cost is linear in this keys array's descriptor + // history (see `IdList::append_unchecked`). + inner.facts_append_fresh(facts, id); + inner.family_append_fresh(keys_id, id); Ok(id) } diff --git a/crates/perry-runtime/src/object/shapes_slot_list.rs b/crates/perry-runtime/src/object/shapes_slot_list.rs index 2dd6d74518..b93ddd35e3 100644 --- a/crates/perry-runtime/src/object/shapes_slot_list.rs +++ b/crates/perry-runtime/src/object/shapes_slot_list.rs @@ -401,7 +401,9 @@ pub(crate) unsafe fn rekey_stable_tombstone_shape_after_squeeze( .get_mut(&record.keys) .is_some_and(|ids| ids.replace(old_id, new_id)); if !replaced { - inner.family_push_back(record.keys, new_id); + // `new_id` came from `alloc_shape_id` a few lines above and is in no + // list yet (see `IdList::append_unchecked`). + inner.family_append_fresh(record.keys, new_id); } inner.indices.remove(&(record.keys as usize)); drop(inner); diff --git a/crates/perry-runtime/src/object/shapes_store.rs b/crates/perry-runtime/src/object/shapes_store.rs index ec9d0b812a..d5cb498ce7 100644 --- a/crates/perry-runtime/src/object/shapes_store.rs +++ b/crates/perry-runtime/src/object/shapes_store.rs @@ -537,6 +537,27 @@ impl IdList { if self.contains(id) { return; } + self.append_unchecked(id); + } + + /// Append an id the caller knows is not in this list. + /// + /// `alloc_shape_id` hands out a strictly increasing counter that is never + /// reused (it parks at `SHAPE_ID_END` rather than wrapping), so an id that + /// was allocated after this list was built cannot be in it, in this family + /// or in any other. The membership scan in [`push_back`] is therefore dead + /// work at the two interning sites, and it is not O(1) dead work: a family + /// holds every descriptor ever created for one keys array, so the scan is + /// linear in the history of that keys array and interning the *n*-th + /// descriptor for it costs O(n) — quadratic over a render that keeps + /// bumping a shape's semantic generation. `IdList::contains` was 6.2 % of + /// main-thread leaf samples on a claude-code streamed reply, 95 % of it + /// under `ShapeTableInner::family_push_back`. + /// + /// Callers that re-file an EXISTING id (the metadata rekey when a keys + /// array moves) must keep using [`push_back`]: those ids can already be in + /// the destination list. + pub(super) fn append_unchecked(&mut self, id: u32) { match self { IdList::Inline { len, ids } if (*len as usize) < ids.len() => { ids[*len as usize] = id; diff --git a/crates/perry-runtime/src/object/shapes_tests.rs b/crates/perry-runtime/src/object/shapes_tests.rs index 43475708a2..cd3a56feee 100644 --- a/crates/perry-runtime/src/object/shapes_tests.rs +++ b/crates/perry-runtime/src/object/shapes_tests.rs @@ -551,6 +551,43 @@ mod descriptor_tests_8067 { test_drop_shape_descriptors(keys); } + #[test] + fn interning_appends_each_new_descriptor_to_the_family_exactly_once() { + // `shape_descriptor_ensure` appends a FRESHLY allocated id with + // `IdList::append_unchecked`, skipping the membership scan whose cost + // is linear in the family's history. The scan is skippable only + // because `alloc_shape_id` never reuses a value; this pins the + // observable consequence — every distinct descriptor for one keys + // array appears in its family exactly once, in birth order — so a + // later change that feeds a recycled id through the fresh path fails + // here instead of silently duplicating a family entry. + let _lock = crate::gc::global_side_table_test_lock(); + let keys = 0x8067_0000_0000_2900usize; + let mut born = Vec::new(); + for n in 1..=6u32 { + born.push( + shape_descriptor_ensure(keys as *const ArrayHeader, n, n) + .expect("shape range unexpectedly exhausted"), + ); + } + assert_eq!( + test_shape_ids_for_keys(keys), + born, + "each new descriptor is appended once, in birth order" + ); + // Re-interning the same facts must hit the accelerator and add nothing. + for (i, n) in (1..=6u32).enumerate() { + assert_eq!( + shape_descriptor_ensure(keys as *const ArrayHeader, n, n).unwrap(), + born[i], + "an existing descriptor must be reused, not re-appended" + ); + } + assert_eq!(test_shape_ids_for_keys(keys), born); + + test_drop_shape_descriptors(keys); + } + #[test] fn a_foreign_agent_id_misses_instead_of_aliasing_same_address() { let _lock = crate::gc::global_side_table_test_lock();