From 01313fc2e1861678218c7586529e2ef45ddfa357 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 6 Sep 2026 14:27:56 +0200 Subject: [PATCH 1/2] perf(gc): swap-remove for keys-array families, with a spill index IdList::remove was Vec::remove(pos), which shifts every element past the removed position. The removals that matter come from the dead-owner prune (prune_dead_owner_side_tables_post_trace -> remove_descriptor_indexed_under) against a `families` list -- the descriptor ids indexed under one keys-array address. Measured on the compiled claude-code TUI, single 3300-char replies, two hosts, fourteen draws: * the removals sit at position ~0.31 of the list -- essentially always the FRONT, which is the worst case for a tail shift; * one owned-keys array per process grows its family to 404k-514k while every other list stays small; * so the same ~3.7M removals memmove up to 848 GB in a single turn; * 100.000% of those bytes are in `families`. `by_facts` moves ZERO: its longest list is 1 in every draw, exactly as its doc claims. * retire_owned_shape_siblings is NOT involved -- it never sees a family longer than 16. THE ORDER QUESTION, because a swap-remove is only available if order is not load-bearing, and the answer differs per index: * `by_facts` IS ordered -- facts_push_front installs a process-global id as the canonical answer ahead of an equivalent local one, read first-wins. It keeps remove_ordered, and that costs nothing: length 1. * `families` is NOT. Its only order-touching reader is the "one descriptor stands for the family" choice in the two rekey walks, which breaks on the first carrier and otherwise takes any present member -- and the chosen descriptor feeds exactly one expression, old_carrier || cache_carrier, whose value is the same for every carrier and the same for every non-carrier. The outcome is a function of the SET. So removal comes in two flavours and THE CALLER DECLARES THE CONTRACT, because the caller is the one that knows whether its order matters; a single remove() that guessed would be the bug. A spilled list gains an id -> index map, built once it passes SPILL_INDEX_MIN (32) and empty below it, where a scan of a few entries is one cache line and a hash probe is not. The index makes position/contains/remove O(1) on the lists that get long, which also removes the linear membership scan in family_push_back -- recorded at this file's own append_unchecked as 6.2% of main-thread leaf samples, 95% of it under that one caller. Three tests. The guard asserts its bound as a MULTIPLE of N, so it is about the complexity class rather than about one N: 2,000 front removals may move at most 4N elements, where the ordered path moves N(N-1)/2 = 1,999,000. Sabotage: point remove_unordered at remove_ordered (250x the bound); raising SPILL_INDEX_MIN above N fails the scan half of the same test. The second checks the index against a plain Vec oracle after front, middle and back removals, because an index that drifts is a WRONG ANSWER -- a descriptor that cannot be found -- not a slow one; its sabotage is dropping the fixup for the element the swap relocated. The third pins that a short spilled list builds no index at all. [gc-idlist] under PERRY_GC_DIAG=1 reports removals, elements moved and positions scanned. NOT CLAIMED: that the memmove explains the bimodal turn CPU. On the pre-fix binary one draw moved 335 GB and was as fast as one that moved 16 GB, so bytes moved is necessary but not sufficient for the slow mode. What this removes is unambiguously wasted work. NOT ADDRESSED: why one family reaches half a million descriptors. That is a separate defect, still being measured, and will be a separate change. Claude-Session: https://claude.ai/code/session_014knX724SYDogwzsXybCGxp --- .../9879-gc-family-list-swap-remove.md | 29 ++ crates/perry-runtime/src/gc/copying.rs | 1 + crates/perry-runtime/src/object/shapes.rs | 24 +- .../perry-runtime/src/object/shapes_store.rs | 466 ++++++++++++++++-- 4 files changed, 466 insertions(+), 54 deletions(-) create mode 100644 changelog.d/9879-gc-family-list-swap-remove.md diff --git a/changelog.d/9879-gc-family-list-swap-remove.md b/changelog.d/9879-gc-family-list-swap-remove.md new file mode 100644 index 0000000000..d27b0ef599 --- /dev/null +++ b/changelog.d/9879-gc-family-list-swap-remove.md @@ -0,0 +1,29 @@ +### Fixed + +- **gc:** a keys-array family's descriptor list no longer memmoves its whole + tail on every removal, and no longer scans linearly to find an id. + + `IdList::remove` was `Vec::remove(pos)`, which shifts everything past the + removed position. Measured on the compiled claude-code TUI, one 3300-char + reply, ten draws across two hosts: the removals sit at position **~0.31** of + the list — i.e. essentially always the front — and the longest list reaches + **514,030** entries, so the same ~3.7 M removals memmove up to **848 GB** in + a single turn. The removals come from the dead-owner prune + (`prune_dead_owner_side_tables_post_trace`). + + No claim is made that this explains the turn's bimodal CPU: one draw moved + 335 GB and was as fast as one that moved 16 GB, so bytes moved is necessary + but not sufficient for the slow mode. What is removed here is unambiguously + wasted work; how much time that is worth is for the A/B to say. + + A spilled list now carries an `id -> index` map, built once it passes 32 + entries, and `families` removes through a swap-remove that moves one element + regardless of position. `by_facts` keeps the order-preserving removal it + needs (its first entry is the canonical answer for exact-facts interning) and + is unaffected — measured at max length **1**, so it never builds an index. + + The same index also removes the linear membership scan in + `family_push_back`, previously **6.2 %** of main-thread leaf samples. + + This does not address why one family reaches half a million descriptors, + which is a separate defect and a separate change. diff --git a/crates/perry-runtime/src/gc/copying.rs b/crates/perry-runtime/src/gc/copying.rs index d42daabea7..6781762f7f 100644 --- a/crates/perry-runtime/src/gc/copying.rs +++ b/crates/perry-runtime/src/gc/copying.rs @@ -1896,6 +1896,7 @@ pub(super) fn run_copied_minor_attempt( } crate::arena::alloc_sample::report("minor"); super::diag_sites::report_primitive_dispatch("minor"); + crate::object::shapes::id_list_report(); report_forwarding_refusals("copying_minor"); super::scanner_profile::report_and_reset("copying_minor"); CopiedMinorAttempt::Done(Some(CopiedMinorFastPathOutcome { diff --git a/crates/perry-runtime/src/object/shapes.rs b/crates/perry-runtime/src/object/shapes.rs index 3e0ebf1a01..b0d47aac53 100644 --- a/crates/perry-runtime/src/object/shapes.rs +++ b/crates/perry-runtime/src/object/shapes.rs @@ -264,6 +264,15 @@ struct ShapeTableInner { const SHAPE_YOUNG_LOG_NAME: &str = "shapes.families+indices"; +/// Re-export of the id-list operation counters' report, so the collector does +/// not have to name a private sibling module. One `[gc-idlist]` line per +/// copying minor under `PERRY_GC_DIAG=1`; `elems_moved` is the falsifier for +/// the swap-remove change. +#[inline] +pub(crate) fn id_list_report() { + shapes_store::id_list_report(); +} + impl ShapeTableInner { /// Rule 1 of `gc/young_log.rs`: log a keys address BEFORE a family or a /// slot index is published under it, when the keys array is not old. @@ -305,7 +314,14 @@ impl ShapeTableInner { let Some(ids) = self.families.get_mut(&keys) else { return false; }; - let removed = ids.remove(id); + // UNORDERED: a family's readers are set-valued (see `IdList`'s type + // doc), and the ordered removal was memmoving the whole tail of a list + // measured at up to 514,030 entries, from position ~0.31, 3.7 M times + // per 3300-char reply. The dominant caller is the dead-owner prune + // (`prune_dead_owner_side_tables_post_trace` -> + // `remove_descriptor_indexed_under`); `retire_owned_shape_siblings` + // never sees a family longer than 16. + let removed = ids.remove_unordered(id); if ids.is_empty() { self.families.remove(&keys); } @@ -334,7 +350,11 @@ impl ShapeTableInner { let Some(ids) = self.by_facts.get_mut(&facts) else { return false; }; - let removed = ids.remove(id); + // ORDERED, and it must stay ordered: `facts_push_front` is how an + // installed process-global id becomes the canonical answer ahead of an + // equivalent local one, and this list is read first-wins. Measured at + // max length 1 on cc, so the order costs nothing to keep. + let removed = ids.remove_ordered(id); if ids.is_empty() { self.by_facts.remove(&facts); } diff --git a/crates/perry-runtime/src/object/shapes_store.rs b/crates/perry-runtime/src/object/shapes_store.rs index d5cb498ce7..fdba57fe38 100644 --- a/crates/perry-runtime/src/object/shapes_store.rs +++ b/crates/perry-runtime/src/object/shapes_store.rs @@ -467,23 +467,225 @@ impl ShapeSlab { } } +/// MEASUREMENT that this structure is judged on, and the test's instrument. +/// +/// Two counters on the id-list mutation path, kept unconditionally because the +/// rig falsifier and the unit guard both read them and a `cfg(test)` counter +/// can only prove the test's own arithmetic. Deliberately a THREE-field struct +/// in one `Cell`: a thread-local `Cell` get/set copies `T` on every +/// operation, and this path runs millions of times per turn, so the width of +/// this type is itself a cost. +#[derive(Default, Clone, Copy)] +pub(crate) struct IdListOpStats { + /// Removals that found their id. + pub(crate) removals: u64, + /// Elements shifted by a removal. Bytes = this x 4. Swap-remove moves + /// none; `Vec::remove` moves the whole tail past the removed position. + pub(crate) elems_moved: u64, + /// Entries touched by a linear membership or position scan. The other + /// half of the same defect: the index removes this too. + pub(crate) positions_scanned: u64, +} + +thread_local! { + pub(crate) static ID_LIST_OP_STATS: std::cell::Cell = + const { + std::cell::Cell::new(IdListOpStats { + removals: 0, + elems_moved: 0, + positions_scanned: 0, + }) + }; +} + +#[inline] +fn note_scan(entries: usize) { + ID_LIST_OP_STATS.with(|c| { + let mut st = c.get(); + st.positions_scanned += entries as u64; + c.set(st); + }); +} + +#[inline] +fn note_removal(elems_moved: usize) { + ID_LIST_OP_STATS.with(|c| { + let mut st = c.get(); + st.removals += 1; + st.elems_moved += elems_moved as u64; + c.set(st); + }); +} + +/// One `[gc-idlist]` line per copying minor under `PERRY_GC_DIAG=1`, +/// cumulative. `elems_moved` is the rig falsifier for this change. +pub(crate) fn id_list_report() { + if !crate::gc::gc_diag_enabled() { + return; + } + let st = ID_LIST_OP_STATS.with(std::cell::Cell::get); + if st.removals == 0 { + return; + } + eprintln!( + "[gc-idlist] removals={} elems_moved={} bytes_moved={} positions_scanned={}", + st.removals, + st.elems_moved, + st.elems_moved * 4, + st.positions_scanned, + ); +} + +/// A spilled id list: the ids, plus an `id -> index` map built once the list +/// is large enough for a linear scan to cost more than a hash probe. +/// +/// The index is what makes `remove_unordered`, `contains` and `position` O(1) +/// on the lists that actually get long. Below [`SPILL_INDEX_MIN`] it stays +/// empty and every operation is the linear scan it always was, because for a +/// handful of entries the scan is a single cache line and the map is not. +#[derive(Clone, Debug, Default)] +pub(super) struct SpillList { + ids: Vec, + /// Empty while `ids.len() < SPILL_INDEX_MIN`; complete above it. + /// + /// `PtrHasher` (#8125) is the right hasher here for the same reason it is + /// on the maps around it: shape ids come from a monotonic counter, so the + /// key is a small dense integer and the avalanche step is what keeps every + /// one of them off bucket 0. + pos: crate::fast_hash::PtrHashMap, +} + +/// Where the index starts paying. Measured shape of the problem: `families` +/// lists reach 514,030 entries on a claude-code reply while `by_facts` lists +/// are length 1, so anything in the low tens is far below the case that hurts +/// and far above the case where the map would be pure overhead. +const SPILL_INDEX_MIN: usize = 32; + +impl SpillList { + #[inline] + fn indexed(&self) -> bool { + !self.pos.is_empty() + } + + /// Build the index if the list has just crossed the threshold. Called + /// after every growth, so the map exists from the first entry past it. + #[inline] + fn maybe_build_index(&mut self) { + if self.pos.is_empty() && self.ids.len() >= SPILL_INDEX_MIN { + self.pos.reserve(self.ids.len()); + for (i, &id) in self.ids.iter().enumerate() { + self.pos.insert(id, i as u32); + } + } + } + + /// Position of `id`, O(1) when indexed and a counted linear scan below the + /// threshold. + #[inline] + fn position(&self, id: u32) -> Option { + if self.indexed() { + return self.pos.get(&id).map(|&i| i as usize); + } + note_scan(self.ids.len()); + self.ids.iter().position(|&x| x == id) + } + + #[inline] + fn push(&mut self, id: u32) { + let i = self.ids.len(); + self.ids.push(id); + if self.indexed() { + self.pos.insert(id, i as u32); + } else { + self.maybe_build_index(); + } + } + + /// ORDER-PRESERVING removal, for a list whose order is load-bearing. + /// O(n) in the tail by construction — that is what "preserve the order" + /// costs — and it reindexes the shifted suffix. + fn remove_ordered(&mut self, id: u32) -> Option { + let pos = self.position(id)?; + let moved = self.ids.len() - 1 - pos; + note_removal(moved); + self.ids.remove(pos); + if self.indexed() { + self.pos.remove(&id); + for (i, &other) in self.ids.iter().enumerate().skip(pos) { + self.pos.insert(other, i as u32); + } + } + Some(pos) + } + + /// UNORDERED removal: the last element takes the removed one's slot. + /// Moves ONE element regardless of position, which is the whole point — + /// the measured removals sit at position ~0.31 of a list up to 514,030 + /// long, so `Vec::remove` was shifting essentially the entire list every + /// time. + /// + /// What this does NOT claim: that the memmove explains the bimodal turn + /// CPU. On perrymaster one draw moved 335 GB and was as fast as a draw + /// that moved 16 GB, so bytes moved is necessary but not sufficient for + /// the slow mode. This removes work that is unambiguously wasted; how much + /// TIME it removes is the A/B's to say. + fn remove_unordered(&mut self, id: u32) -> Option { + let pos = self.position(id)?; + note_removal(if pos + 1 == self.ids.len() { 0 } else { 1 }); + let last = self.ids.len() - 1; + self.ids.swap_remove(pos); + if self.indexed() { + self.pos.remove(&id); + if pos != last { + // The element that was last now lives at `pos`. + self.pos.insert(self.ids[pos], pos as u32); + } + } + Some(pos) + } + + #[inline] + fn replace(&mut self, old: u32, new: u32) -> bool { + let Some(pos) = self.position(old) else { + return false; + }; + self.ids[pos] = new; + if self.indexed() { + self.pos.remove(&old); + self.pos.insert(new, pos as u32); + } + true + } +} + /// A compact list of descriptor ids: up to three inline, then a spilled -/// `Vec`. Sized so a family-index bucket is `(u64, IdList)` = 24 bytes. +/// `Vec` with an `id -> index` map (see [`SpillList`]). Sized so a family-index +/// bucket is `(u64, IdList)` = 24 bytes. +/// +/// # Order +/// Order is meaningful **for `by_facts` only**: [`IdList::push_front`] is how +/// an installed process-global id becomes the canonical answer for exact-facts +/// interning ahead of an equivalent local id (`install_external_shape_id`), and +/// that list is read first-wins. `families` is NOT order-sensitive: its only +/// order-touching reader is the "one descriptor stands for the family" choice +/// in the two rekey walks, which breaks on the first carrier and otherwise +/// takes any present member — and the chosen descriptor feeds exactly one +/// expression, `old_carrier || cache_carrier`, whose value is the same for +/// every carrier and the same for every non-carrier. The outcome is a function +/// of the SET, not of the order. /// -/// Order is meaningful: [`IdList::push_front`] is how an installed -/// process-global id becomes the canonical answer for exact-facts interning -/// ahead of an equivalent local id (`install_external_shape_id`). +/// That asymmetry is why removal comes in two flavours: +/// [`IdList::remove_ordered`] for `by_facts` and [`IdList::remove_unordered`] +/// for `families`. **The caller declares the contract**, because the caller is +/// the one that knows whether its order is load-bearing; a single `remove` that +/// guessed would be the bug. #[derive(Clone, Debug)] pub(super) enum IdList { - Inline { - len: u8, - ids: [u32; 3], - }, - // The `Box` is the point: an inline `Vec` is 24 bytes and would make every - // bucket 32; the spill is the rare case, so its extra indirection is - // cheaper than eight bytes on every family. - #[allow(clippy::box_collection)] - Spill(Box>), + Inline { len: u8, ids: [u32; 3] }, + // The `Box` is the point: an inline `SpillList` is far wider and would make + // every bucket pay for it; the spill is the rare case, so its extra + // indirection is cheaper than those bytes on every family. + Spill(Box), } const _: () = assert!(std::mem::size_of::() == 16); @@ -502,7 +704,7 @@ impl IdList { pub(super) fn as_slice(&self) -> &[u32] { match self { IdList::Inline { len, ids } => &ids[..*len as usize], - IdList::Spill(v) => v.as_slice(), + IdList::Spill(v) => v.ids.as_slice(), } } @@ -518,12 +720,18 @@ impl IdList { #[inline] pub(super) fn contains(&self, id: u32) -> bool { - self.as_slice().contains(&id) + match self { + IdList::Inline { len, ids } => ids[..*len as usize].contains(&id), + IdList::Spill(v) => v.position(id).is_some(), + } } - fn spill(&mut self) -> &mut Vec { + fn spill(&mut self) -> &mut SpillList { if let IdList::Inline { len, ids } = self { - let v = ids[..*len as usize].to_vec(); + let v = SpillList { + ids: ids[..*len as usize].to_vec(), + pos: crate::fast_hash::new_ptr_hash_map(), + }; *self = IdList::Spill(Box::new(v)); } match self { @@ -554,6 +762,11 @@ impl IdList { /// main-thread leaf samples on a claude-code streamed reply, 95 % of it /// under `ShapeTableInner::family_push_back`. /// + /// The spill index now removes that scan for the callers that cannot use + /// this entry point, which is why `contains` is O(1) above + /// [`SPILL_INDEX_MIN`]. This function stays because skipping the probe + /// entirely is still cheaper than performing it. + /// /// 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. @@ -567,7 +780,9 @@ impl IdList { } } - /// Prepend `id` unless already present. + /// Prepend `id` unless already present. Order-preserving by definition, so + /// it stays O(n) on a spilled list; only `by_facts` and the external-id + /// install use it, and neither is on a hot path. pub(super) fn push_front(&mut self, id: u32) { if self.contains(id) { return; @@ -578,39 +793,62 @@ impl IdList { ids[0] = id; *len += 1; } - _ => self.spill().insert(0, id), + _ => { + let v = self.spill(); + v.ids.insert(0, id); + if v.indexed() { + v.pos.clear(); + } + v.maybe_build_index(); + } } } - /// Drop `id` if present; returns whether it was. - pub(super) fn remove(&mut self, id: u32) -> bool { + /// Drop `id` if present, PRESERVING the order of what remains; returns + /// whether it was there. For a list whose order is load-bearing — + /// `by_facts`, where the first entry is the canonical answer. + pub(super) fn remove_ordered(&mut self, id: u32) -> bool { match self { - IdList::Inline { len, ids } => { - let n = *len as usize; - let Some(pos) = ids[..n].iter().position(|&x| x == id) else { - return false; - }; - ids.copy_within(pos + 1..n, pos); - ids[n - 1] = 0; - *len -= 1; - true - } - IdList::Spill(v) => { - let Some(pos) = v.iter().position(|&x| x == id) else { - return false; - }; - v.remove(pos); - true - } + IdList::Inline { len, ids } => Self::remove_inline(len, ids, id), + IdList::Spill(v) => v.remove_ordered(id).is_some(), + } + } + + /// Drop `id` if present, WITHOUT preserving order; returns whether it was + /// there. For `families`, whose readers are set-valued (see the type doc). + /// + /// This is the change: on a spilled list it moves ONE element instead of + /// the whole tail. + pub(super) fn remove_unordered(&mut self, id: u32) -> bool { + match self { + // Three entries: the inline shift is a single register move and + // there is nothing to gain from disturbing the order. + IdList::Inline { len, ids } => Self::remove_inline(len, ids, id), + IdList::Spill(v) => v.remove_unordered(id).is_some(), } } + #[inline] + fn remove_inline(len: &mut u8, ids: &mut [u32; 3], id: u32) -> bool { + let n = *len as usize; + note_scan(n); + let Some(pos) = ids[..n].iter().position(|&x| x == id) else { + return false; + }; + note_removal(n - 1 - pos); + ids.copy_within(pos + 1..n, pos); + ids[n - 1] = 0; + *len -= 1; + true + } + /// Replace `old` with `new` in place (keeps its position); returns /// whether `old` was present. pub(super) fn replace(&mut self, old: u32, new: u32) -> bool { match self { IdList::Inline { len, ids } => { let n = *len as usize; + note_scan(n); match ids[..n].iter().position(|&x| x == old) { Some(pos) => { ids[pos] = new; @@ -619,21 +857,20 @@ impl IdList { None => false, } } - IdList::Spill(v) => match v.iter().position(|&x| x == old) { - Some(pos) => { - v[pos] = new; - true - } - None => false, - }, + IdList::Spill(v) => v.replace(old, new), } } - /// Bytes held outside the containing bucket. pub(super) fn heap_bytes(&self) -> usize { match self { IdList::Inline { .. } => 0, - IdList::Spill(v) => std::mem::size_of::>() + v.capacity() * 4, + IdList::Spill(v) => { + std::mem::size_of::() + + v.ids.capacity() * 4 + // The index is the structure's memory cost and is reported + // rather than hidden: it exists only above SPILL_INDEX_MIN. + + v.pos.capacity() * (std::mem::size_of::<(u32, u32)>() + 1) + } } } } @@ -782,8 +1019,10 @@ mod tests { assert_eq!(list.as_slice(), &[1, 2, 3, 4]); list.push_front(0); assert_eq!(list.as_slice(), &[0, 1, 2, 3, 4]); - assert!(list.remove(2)); - assert!(!list.remove(2)); + // The ORDERED removal keeps this list's order, which is what + // `by_facts` depends on. + assert!(list.remove_ordered(2)); + assert!(!list.remove_ordered(2)); assert_eq!(list.as_slice(), &[0, 1, 3, 4]); assert!(list.replace(3, 30)); assert!(!list.replace(3, 300)); @@ -794,13 +1033,136 @@ mod tests { inline.push_back(7); inline.push_back(8); inline.push_back(9); - assert!(inline.remove(8)); + assert!(inline.remove_ordered(8)); assert_eq!(inline.as_slice(), &[7, 9]); assert!(inline.replace(9, 10)); assert_eq!(inline.as_slice(), &[7, 10]); - assert!(inline.remove(7)); - assert!(inline.remove(10)); + assert!(inline.remove_ordered(7)); + assert!(inline.remove_ordered(10)); assert!(inline.is_empty()); assert_eq!(inline.heap_bytes(), 0); } + + /// THE GUARD for this change, and it is an asymmetric one: the unordered + /// removal must move O(1) elements per call, and the ordered one is + /// allowed to move O(n) because that is what preserving the order costs. + /// + /// Front removal is the measured shape of the defect — removals sit at + /// position ~0.31 of a list up to 514,030 long — so the test removes from + /// the front, which is the worst case for `Vec::remove` and the best case + /// for nothing. + /// + /// **Sabotage: point `remove_unordered` at `remove_ordered`.** The bound + /// below is `4 * N`; the O(n) path moves `N * (N - 1) / 2` = 1,999,000 + /// elements for N = 2,000, i.e. 250x the bound, and this fails. A bound + /// expressed as a MULTIPLE of N rather than an absolute is what makes the + /// assertion about the complexity class instead of about one N. + #[test] + fn unordered_removal_moves_o1_elements_and_scans_o1_entries() { + const N: u32 = 2_000; + + let baseline = ID_LIST_OP_STATS.with(std::cell::Cell::get); + let mut list = IdList::default(); + for id in 1..=N { + // The interning sites' entry point: no membership probe. + list.append_unchecked(id); + } + assert_eq!(list.len(), N as usize); + assert!(matches!(list, IdList::Spill(_))); + + // Remove every id from the FRONT of the list, in insertion order. + for id in 1..=N { + assert!(list.remove_unordered(id), "id {id} was not present"); + } + assert!(list.is_empty()); + + let after = ID_LIST_OP_STATS.with(std::cell::Cell::get); + let moved = after.elems_moved - baseline.elems_moved; + let scanned = after.positions_scanned - baseline.positions_scanned; + let removals = after.removals - baseline.removals; + assert_eq!(removals, u64::from(N)); + + // O(1) per removal, with room for the swap itself. + assert!( + moved <= 4 * u64::from(N), + "unordered removal moved {moved} elements for {N} removals — that \ + is the O(n) tail shift this structure exists to remove \ + (the ordered path would move {})", + u64::from(N) * (u64::from(N) - 1) / 2 + ); + // The index answers `position`, so no linear scan may be charged for + // a list this long. Sabotage: raise SPILL_INDEX_MIN above N and this + // fails with ~N*N/2 scanned entries. + assert!( + scanned <= 4 * u64::from(N), + "unordered removal scanned {scanned} entries for {N} removals — \ + the spill index is not answering `position`" + ); + } + + /// The index must agree with the vector after every operation, including + /// the swap that moves a third element nobody named. Checked exhaustively + /// against a plain `Vec` oracle, because an index that drifts is a wrong + /// ANSWER (a descriptor that cannot be found, or one found under the wrong + /// id), not a slow one. + /// + /// Sabotage: drop the `self.pos.insert(self.ids[pos], pos as u32)` fixup + /// in `remove_unordered` — the element the swap relocated keeps a stale + /// index and the `contains` check below fails. + #[test] + fn the_spill_index_agrees_with_the_vector_after_every_operation() { + let mut list = IdList::default(); + let mut oracle: Vec = Vec::new(); + for id in 1..=200u32 { + list.append_unchecked(id); + oracle.push(id); + } + // Remove a scattered third of them, front, middle and back. + for &id in &[1u32, 2, 3, 100, 101, 199, 200, 50, 150, 7] { + assert!(list.remove_unordered(id)); + oracle.retain(|&x| x != id); + } + // Same SET, whatever the order. + let mut got = list.as_slice().to_vec(); + got.sort_unstable(); + let mut want = oracle.clone(); + want.sort_unstable(); + assert_eq!(got, want); + // And every survivor is still findable through the index. + for &id in &want { + assert!(list.contains(id), "id {id} lost its index entry"); + } + for &id in &[1u32, 2, 3, 100, 101, 199, 200, 50, 150, 7] { + assert!(!list.contains(id), "removed id {id} is still findable"); + } + // `replace` must keep the index coherent too. + let survivor = want[0]; + assert!(list.replace(survivor, 9_999)); + assert!(!list.contains(survivor)); + assert!(list.contains(9_999)); + } + + /// A list that never reaches `SPILL_INDEX_MIN` must not allocate an index + /// — the map is the structure's memory cost and it is only worth paying + /// where the scan hurts. `by_facts` lists, measured at length 1 on cc, + /// live entirely in this regime. + #[test] + fn a_short_spilled_list_builds_no_index() { + let mut list = IdList::default(); + for id in 1..=8u32 { + list.append_unchecked(id); + } + assert!(matches!(list, IdList::Spill(_))); + match &list { + IdList::Spill(v) => assert!( + !v.indexed(), + "a list of 8 built an index; SPILL_INDEX_MIN is {SPILL_INDEX_MIN}" + ), + IdList::Inline { .. } => unreachable!(), + } + // Still correct without one. + assert!(list.remove_unordered(4)); + assert!(!list.contains(4)); + assert!(list.contains(8)); + } } From f9eb62c58ccc500a52132cba7419499a470839ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 6 Sep 2026 16:05:43 +0200 Subject: [PATCH 2/2] test(gc): the retirement test asserts membership, not family order perrymaster's run of the PR tree: 3192 tests, 1 failed -- owned_key_count_versions_are_retired_behind_the_current_one, at the post-retirement assert_eq!(test_shape_ids_for_keys(keys), vec![cached, current]); swap-removing stale_a/stale_b moved `current` in front of `cached`. THE TEST OVER-SPECIFIES, and the evidence is an enumeration of every production reader of `families` on this base, done with this test's subject in mind: shapes.rs:1408 retire_owned_shape_siblings filter all-but-keep SET shapes.rs:1906 prune_dead_shape_keys_young snapshot, remove all SET shapes.rs:1946 scan_shape_table_rekey_mut first-carrier-else-any SET shapes.rs:2067 move_shape_family wholesale remove/insert n/a shapes.rs:2092 relevant_shape_keys KEYS, sort+dedup normalised shapes.rs:2182 scan_shape_keys_address as 1946 SET shapes.rs:2337 census heap_bytes sum aggregate shapes.rs:2390 census len/max aggregate slot_list:402 retire all but `current` filter SET slot_list:597 ids.replace(old, new) position-PRESERVING n/a slot_list:686 retire all but `id` filter SET The two rekey walks are the only order-TOUCHING readers, and what they take from the list is a single choice fed to exactly one expression, old_carrier || cache_carrier, whose value is the same for every carrier and the same for every non-carrier -- so the outcome is a function of the set. Nothing else reads a position. And the helper the assertion uses, test_shape_ids_for_keys, is #[cfg(test)]: it renders families.as_slice().to_vec() for tests only. Its .first() sibling, test_shape_id_for_keys, is also #[cfg(test)] and is only ever called on families the caller has seeded with one descriptor. #9706's contract is about WHICH ids survive a same-address retirement, not the order they survive in; the assertion compared against a Vec because the helper returns one. So the post-retirement assertion becomes a sorted comparison, with the contract and the reader enumeration written at it. The pre-retirement assertion is left as-is -- no removal has happened there and it legitimately documents that adds append -- with a comment saying that is a property of the add path and not a contract. Corroborating: the suite ran 3192 with exactly ONE failure, so no other test in the tree asserts a family's order across a removal. --- .../perry-runtime/src/object/shapes_tests.rs | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/crates/perry-runtime/src/object/shapes_tests.rs b/crates/perry-runtime/src/object/shapes_tests.rs index cd3a56feee..dd06428773 100644 --- a/crates/perry-runtime/src/object/shapes_tests.rs +++ b/crates/perry-runtime/src/object/shapes_tests.rs @@ -777,8 +777,9 @@ mod descriptor_tests_8067 { .expect("shape range unexpectedly exhausted"); let unrelated = shape_descriptor_ensure(unrelated_keys as *const ArrayHeader, 1, 1) .expect("shape range unexpectedly exhausted"); - // Before retirement every version is resolvable and the family lists - // them in mint order. + // Before retirement every version is resolvable. Adds append, so the + // family happens to be in mint order here; that is a property of the + // ADD path, not a contract (see the retirement assertion below). assert_eq!( test_shape_ids_for_keys(keys), vec![stale_a, stale_b, cached, current] @@ -795,7 +796,26 @@ mod descriptor_tests_8067 { ); assert!(shape_descriptor_by_id(current).is_some()); assert!(shape_descriptor_by_id(unrelated).is_some()); - assert_eq!(test_shape_ids_for_keys(keys), vec![cached, current]); + // MEMBERSHIP, not order. #9706's contract is "the growth history is + // retired behind the version its owner now carries, except one an + // optimization cache permanently owns" — a statement about WHICH ids + // survive. The order they survive in is not part of it, and no + // production reader of `families` depends on it: every one either + // filters the whole list, snapshots the whole list, aggregates it, or + // (the two rekey walks) picks "a carrier if the family has one, else + // any present member" and feeds that single choice to exactly one + // expression, `old_carrier || cache_carrier`, whose value is the same + // for every carrier and the same for every non-carrier. The two + // helpers this test uses are `#[cfg(test)]` renderings of the list. + // + // This assertion compared against a `Vec` because the helper returns + // one, which pinned mint order by accident; `families` now removes by + // swapping the last element into the hole, so a survivor can move. + let mut survivors = test_shape_ids_for_keys(keys); + survivors.sort_unstable(); + let mut expected = vec![cached, current]; + expected.sort_unstable(); + assert_eq!(survivors, expected); // Retired facts re-intern as FRESH ids: nothing can resolve the old ones. let reminted = shape_descriptor_ensure(keys as *const ArrayHeader, 1, 1) .expect("shape range unexpectedly exhausted");