From e2755edf20b8ea442cd4f12cdcbb86a25800d560 Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 19 Aug 2026 17:19:28 +0000 Subject: [PATCH 01/17] Skip value-drop work in LineListNode when V has none to do A LineListNode payload slot holds either a child pointer or a value, and the drop paths tested which one it was and then called ManuallyDrop::drop on the value. For a value type with no drop glue that call does nothing, but the branches around it still run, and the elision was left entirely to the optimizer. The obvious guard, needs_drop::(), would be wrong here. The slot is a LocalOrHeap, which carries an unconditional Drop impl and boxes any V too large to store inline, so a [u8; 64] has no drop glue of its own but does own a heap allocation. val_slot_needs_drop::() covers both halves, and ValSlotStorage is now the type alias the union itself uses so the size threshold cannot drift away from the layout. This is a code-size and clarity change, not a throughput one: build-and-drop of a 1M-path PathMap<()> measures ~189ms either way in release, because the cost there is allocator traffic and cache misses, not the folded branches. The value is that the elision is explicit and holds in debug too. Tests cover both halves of the predicate: a drop-counting value asserting no leaks after removes, prunes and a shared-clone drop, and a 64-byte value with no drop glue whose heap allocations only miri can check. Unit tests pin the predicate's value for (), u32, Vec and [u8; 64] so the elision cannot stop happening silently. Co-Authored-By: Claude Opus 5 (1M context) --- src/line_list_node.rs | 34 ++++----- src/trie_node.rs | 52 ++++++++++++-- tests/value_drop_elision.rs | 134 ++++++++++++++++++++++++++++++++++++ 3 files changed, 197 insertions(+), 23 deletions(-) create mode 100644 tests/value_drop_elision.rs diff --git a/src/line_list_node.rs b/src/line_list_node.rs index eb90763..be86c73 100644 --- a/src/line_list_node.rs +++ b/src/line_list_node.rs @@ -60,28 +60,24 @@ impl Drop for LineListNode { // pathological paths are almost entirely non-branching. Therefore, we will invoke a recursive // drop function if the node branches, and an iterative drop if it doesn't - let slot0_used = self.is_used::<0>(); - let slot1_used = self.is_used::<1>(); - let slot0_child = self.is_child_ptr::<0>(); - let slot1_child = self.is_child_ptr::<1>(); + //NOTE: when `V` needs no drop work (`PathMap<()>`, and any small `Copy` value type), the + // value arms below fold away, and a node holding two values drops without touching them + let slot0_child = self.is_used_child_0(); + let slot1_child = self.is_used_child_1(); - if (slot0_used && slot0_child) != (slot1_used && slot1_child) { + if slot0_child != slot1_child { //If there is exactly one child, do the non-recursive drop list_node_iterative_drop(self); } else { - if slot0_used { - if slot0_child { - unsafe{ ManuallyDrop::drop(&mut self.val_or_child0.child) } - } else { - unsafe{ ManuallyDrop::drop(&mut self.val_or_child0.val) } - } + if slot0_child { + unsafe{ ManuallyDrop::drop(&mut self.val_or_child0.child) } + } else if val_slot_needs_drop::() && self.is_used::<0>() { + unsafe{ ManuallyDrop::drop(&mut self.val_or_child0.val) } } - if slot1_used { - if slot1_child { - unsafe{ ManuallyDrop::drop(&mut self.val_or_child1.child) } - } else { - unsafe{ ManuallyDrop::drop(&mut self.val_or_child1.val) } - } + if slot1_child { + unsafe{ ManuallyDrop::drop(&mut self.val_or_child1.child) } + } else if val_slot_needs_drop::() && self.is_used::<1>() { + unsafe{ ManuallyDrop::drop(&mut self.val_or_child1.val) } } } } @@ -117,7 +113,7 @@ fn list_node_take_child_to_drop(node: &mut let child1 = node.is_used_child_1(); match (child0, child1) { (true, false) => { - if node.is_used::<1>() { + if val_slot_needs_drop::() && node.is_used::<1>() { unsafe{ ManuallyDrop::drop(&mut node.val_or_child1.val) } } node.header = 0; @@ -129,7 +125,7 @@ fn list_node_take_child_to_drop(node: &mut } }, (false, true) => { - if node.is_used::<0>() { + if val_slot_needs_drop::() && node.is_used::<0>() { unsafe{ ManuallyDrop::drop(&mut node.val_or_child0.val) } } node.header = 0; diff --git a/src/trie_node.rs b/src/trie_node.rs index 4cb2076..ef6e7ad 100644 --- a/src/trie_node.rs +++ b/src/trie_node.rs @@ -484,12 +484,30 @@ impl ValOrChild { } } +/// The inline storage budget for a value held in a [ValOrChildUnion]. A `V` larger than this is +/// boxed by [LocalOrHeap], and therefore owns a heap allocation that must be freed +#[cfg(feature = "slim_ptrs")] +pub(crate) type ValSlotStorage = [u8; 8]; +#[cfg(not(feature = "slim_ptrs"))] +pub(crate) type ValSlotStorage = [u8; 16]; + +/// Returns `true` if the value in a [ValOrChildUnion] needs any drop work at all +/// +/// [LocalOrHeap] carries an unconditional `Drop` impl, so `needs_drop::>()` is +/// always `true` and tells us nothing. The slot only really needs dropping when `V` has drop glue +/// of its own, or when `V` is too big to store inline and therefore owns a heap allocation. +/// +/// This depends only on `V`, so it folds to a constant at monomorphization and the branches it +/// guards vanish. For a unit-valued trie (`PathMap<()>`), and for any other trie whose value is a +/// small `Copy` type, that removes the value arms from the node drop paths entirely. +#[inline(always)] +pub(crate) const fn val_slot_needs_drop() -> bool { + core::mem::needs_drop::() || core::mem::size_of::() > core::mem::size_of::() +} + pub union ValOrChildUnion { pub child: ManuallyDrop>, - #[cfg(feature = "slim_ptrs")] - pub val: ManuallyDrop>, - #[cfg(not(feature = "slim_ptrs"))] - pub val: ManuallyDrop>, + pub val: ManuallyDrop>, pub _unused: () } @@ -3247,6 +3265,32 @@ impl DistributiveLat } } +#[cfg(test)] +mod val_slot_drop_tests { + use super::{ValSlotStorage, val_slot_needs_drop}; + + /// The whole point of [val_slot_needs_drop] is that it is `false` for the value types whose + /// drop work the node paths can skip. If these ever flip to `true` the elision silently stops + /// happening, so pin them down. + #[test] + fn val_slot_needs_drop_is_false_for_trivial_values() { + assert!(!val_slot_needs_drop::<()>()); + assert!(!val_slot_needs_drop::()); + assert!(!val_slot_needs_drop::()); + assert!(!val_slot_needs_drop::<[u8; 4]>()); + } + + /// A value with drop glue always needs the drop, and so does one too big to live inline, + /// because [local_or_heap::LocalOrHeap] boxes it + #[test] + fn val_slot_needs_drop_is_true_when_there_is_work_to_do() { + assert!(val_slot_needs_drop::>()); //drop glue + assert!(val_slot_needs_drop::()); //drop glue + assert!(val_slot_needs_drop::<[u8; 64]>()); //no drop glue, but heap-allocated + assert!(core::mem::size_of::<[u8; 64]>() > core::mem::size_of::()); + } +} + /// Test to make sure slim_ptrs are good with provenance under miri #[cfg(test)] mod tests { diff --git a/tests/value_drop_elision.rs b/tests/value_drop_elision.rs new file mode 100644 index 0000000..bcb9a98 --- /dev/null +++ b/tests/value_drop_elision.rs @@ -0,0 +1,134 @@ +//! Tests that skipping the value-drop arms in the node drop paths (for value types that need no +//! drop work) doesn't skip drops for value types that do. +//! +//! Two cases matter, and they are distinguished by different halves of the predicate: +//! * a value with drop glue of its own +//! * a value with *no* drop glue that is nonetheless too large to live inline in a node slot, and +//! is therefore boxed on the heap. Nothing here can observe that leak directly; run this under +//! `cargo miri test` to check it. + +use std::sync::atomic::{AtomicIsize, Ordering::SeqCst}; + +use pathmap::PathMap; +use pathmap::zipper::{ZipperMoving, ZipperWriting}; + +static LIVE: AtomicIsize = AtomicIsize::new(0); + +/// A value that keeps count of how many instances are alive +#[derive(Debug)] +struct Tracked(#[allow(dead_code)] u64); + +impl Tracked { + fn new(n: u64) -> Self { + LIVE.fetch_add(1, SeqCst); + Self(n) + } +} +impl Clone for Tracked { + fn clone(&self) -> Self { + Self::new(self.0) + } +} +impl Drop for Tracked { + fn drop(&mut self) { + LIVE.fetch_sub(1, SeqCst); + } +} + +/// No drop glue, but 64 bytes is far past what a node slot stores inline, so it is heap-allocated +#[derive(Clone, Copy, PartialEq, Debug)] +struct Big([u8; 64]); + +/// Paths chosen to produce a mix of node shapes: leaves holding one and two values, values sitting +/// above branches, and a fan-out wide enough to force a dense node +fn paths() -> Vec> { + let mut paths: Vec> = vec![ + b"a".to_vec(), + b"ab".to_vec(), + b"abc".to_vec(), + b"abd".to_vec(), + b"az".to_vec(), + b"a-very-long-path-that-will-not-fit-inside-a-single-list-node-key".to_vec(), + ]; + for byte in 0u8..=255 { + paths.push(vec![b'w', byte]); + } + paths +} + +#[test] +fn values_with_drop_glue_are_still_dropped() { + assert_eq!(LIVE.load(SeqCst), 0, "another test leaked into this one"); + + let mut map: PathMap = PathMap::new(); + for (i, path) in paths().iter().enumerate() { + map.set_val_at(path, Tracked::new(i as u64)); + } + assert!(LIVE.load(SeqCst) > 0); + + //Exercise the paths that drop a value without dropping the whole node + let mut wz = map.write_zipper(); + wz.descend_to(b"abc"); + assert!(wz.remove_val(false).is_some()); + wz.reset(); + wz.descend_to(b"abd"); + assert!(wz.remove_val(true).is_some()); + wz.reset(); + wz.descend_to(b"w"); + assert!(wz.remove_branches(true)); + drop(wz); + + //And a clone, so the node drop paths run against a shared trie too + let copy = map.clone(); + drop(map); + drop(copy); + + assert_eq!(LIVE.load(SeqCst), 0, "values were leaked"); +} + +#[test] +fn large_values_without_drop_glue_round_trip() { + let mut map: PathMap = PathMap::new(); + for (i, path) in paths().iter().enumerate() { + map.set_val_at(path, Big([i as u8; 64])); + } + for (i, path) in paths().iter().enumerate() { + assert_eq!(map.get_val_at(path), Some(&Big([i as u8; 64])), "at {path:?}"); + } + + let copy = map.clone(); + drop(map); + for (i, path) in paths().iter().enumerate() { + assert_eq!(copy.get_val_at(path), Some(&Big([i as u8; 64])), "at {path:?}"); + } + //The heap allocations behind these values are freed here; miri checks it + drop(copy); +} + +#[test] +fn trivial_values_round_trip_with_the_drop_arms_elided() { + let all = paths(); + + let mut unit: PathMap<()> = PathMap::new(); + let mut small: PathMap = PathMap::new(); + for (i, path) in all.iter().enumerate() { + unit.set_val_at(path, ()); + small.set_val_at(path, i as u32); + } + + assert_eq!(unit.val_count(), all.len()); + assert_eq!(small.val_count(), all.len()); + for (i, path) in all.iter().enumerate() { + assert_eq!(unit.get_val_at(path), Some(&()), "at {path:?}"); + assert_eq!(small.get_val_at(path), Some(&(i as u32)), "at {path:?}"); + } + + //Removing values leaves the paths dangling when `prune` is false; the node still has to be + //dropped correctly afterwards + let mut wz = small.write_zipper(); + wz.descend_to(b"abc"); + assert_eq!(wz.remove_val(false), Some(2)); + drop(wz); + assert!(small.path_exists_at(b"abc")); + assert_eq!(small.get_val_at(b"abc"), None); +} From 12edc03745a366205e5aede691b7a0c7987d75e3 Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 19 Aug 2026 17:19:49 +0000 Subject: [PATCH 02/17] Honor Lattice::IDEMPOTENT in the node algebra, and short-circuit join_into The trie shares subtries by pointer, so an algebraic operation frequently ends up with the same node on both sides. Join and meet answered that case as Identity and subtract answered it as None, without descending -- which is only sound when the value operation is idempotent. Both IDEMPOTENT constants documented themselves as informational only, so a value type that actually combines its operands, such as a multiset adding multiplicities, would have had its structurally shared branches silently skipped instead of combined. Six sites take the shortcut, not the three that grep for ptr_eq finds: the outer TrieNodeODRc methods, and the TaggedNodeRef dispatchers underneath them, which is where it fires at every level of the descent. There are two copies of the dispatchers, one per slim_dispatch setting. All six now consult the constant. Also adds the shortcut that was missing. TrieNodeODRc::join_into had no pointer check at all and went straight to make_mut(), which deep-copies the node exactly when it is shared -- precisely the case where both sides are likely to be the same pointer. Three call sites that hand-rolled make_mut().join_into_dyn() now route through it and inherit the check. Joining a 1M-path PathMap<()> into a clone of itself goes from ~110ms to effectively free. No in-tree value type declares IDEMPOTENT = false, so no existing behavior changes. The new tests use a multiset Count whose join adds occurrences and whose subtract removes one; all four of them failed before this change. A helper asserts the two maps genuinely share a root node, so the tests cannot quietly stop exercising the shortcut. Co-Authored-By: Claude Opus 5 (1M context) --- src/dense_byte_node.rs | 11 +-- src/ring.rs | 22 ++++-- src/trie_map.rs | 7 +- src/trie_node.rs | 39 +++++++--- src/write_zipper.rs | 7 +- tests/idempotent_lattice.rs | 138 ++++++++++++++++++++++++++++++++++++ 6 files changed, 190 insertions(+), 34 deletions(-) create mode 100644 tests/idempotent_lattice.rs diff --git a/src/dense_byte_node.rs b/src/dense_byte_node.rs index c28c78c..3257030 100644 --- a/src/dense_byte_node.rs +++ b/src/dense_byte_node.rs @@ -1828,14 +1828,9 @@ impl, Other let (other_rec, other_val) = other.into_both(); let rec_status = match self.rec_mut() { Some(self_rec) => match other_rec { - Some(other_rec) => { - let (status, result) = self_rec.make_mut().join_into_dyn(other_rec); - match result { - Ok(()) => {}, - Err(replacement_node) => {*self_rec = replacement_node}, - } - status - }, + //NOTE: `TrieNodeODRc::join_into` is the same operation, and additionally short-circuits + // when both sides are the same shared subtrie + Some(other_rec) => self_rec.join_into(other_rec), None => AlgebraicStatus::Identity, }, None => match other_rec { diff --git a/src/ring.rs b/src/ring.rs index 4b9b1d4..238f39b 100644 --- a/src/ring.rs +++ b/src/ring.rs @@ -538,9 +538,16 @@ pub trait Lattice { /// `join_into(self) -> AlgebraicStatus::Identity`, /// `pmeet(self) -> AlgebraicResult::Identity(SELF_IDENT | COUNTER_IDENT)`, /// - /// WARNING! This constant is currently informational only. The node-level and zipper - /// algebra implementations do not yet consult it, so changing it has no effect - /// on their behavior. It is planned for gating implementation shortcuts in the future + /// The node-level algebra consults this constant to decide whether an operation between two + /// references to the *same* shared subtrie can be answered without descending into it. Leaving + /// it at the `true` default is correct for every set-like or "last writer wins" value type, + /// including `()`. Set it to `false` for a value type whose join or meet actually combines the + /// operands, such as a multiset that adds multiplicities, otherwise structurally shared branches + /// will be silently skipped instead of combined. + /// + /// NOTE: this constant covers `pjoin`, `join_into` and `pmeet` together. A type that is + /// idempotent under one but not the other must declare `false`. Subtraction is governed + /// separately, by [DistributiveLattice::IDEMPOTENT]. const IDEMPOTENT: bool = true; /// Implements the union operation between two instances of a type in a partial lattice, resulting in @@ -619,9 +626,12 @@ pub trait DistributiveLattice { /// If `IDEMPOTENT = true` the implementor is asserting that: /// `psubtract(self) -> AlgebraicResult::None`, /// - /// WARNING! This constant is currently informational only. The node-level and zipper - /// algebra implementations do not yet consult it, so changing it has no effect - /// on their behavior. It is planned for gating implementation shortcuts in the future + /// The node-level algebra consults this constant to decide whether subtracting a shared subtrie + /// from itself can be answered as "nothing survives" without descending into it. Set it to + /// `false` for a value type where `x - x` leaves something behind, such as a multiset that + /// removes one occurrence per subtraction. + /// + /// See also [Lattice::IDEMPOTENT], which governs join and meet. const IDEMPOTENT: bool = true; /// Implements the partial subtract operation diff --git a/src/trie_map.rs b/src/trie_map.rs index 3343fa5..9520362 100644 --- a/src/trie_map.rs +++ b/src/trie_map.rs @@ -720,12 +720,7 @@ impl Lattice for PathMap let (other_root_node, other_root_val) = other.into_root(); let root_node_status = if let Some(other_root) = other_root_node { - let (status, result) = self.get_or_init_root_mut().make_mut().join_into_dyn(other_root); - match result { - Ok(()) => {}, - Err(replacement) => { *self.get_or_init_root_mut() = replacement; } - } - status + self.get_or_init_root_mut().join_into(other_root) } else { if self.is_empty() { AlgebraicStatus::None diff --git a/src/trie_node.rs b/src/trie_node.rs index ef6e7ad..915d878 100644 --- a/src/trie_node.rs +++ b/src/trie_node.rs @@ -1399,7 +1399,9 @@ mod tagged_node_ref { } pub fn pjoin_dyn(&self, other: TaggedNodeRef) -> AlgebraicResult> where V: Lattice { - if self.shared_node_id() == other.shared_node_id() { + //A node joined with itself is only itself when the value join is idempotent. + // See [Lattice::IDEMPOTENT] + if ::IDEMPOTENT && self.shared_node_id() == other.shared_node_id() { return AlgebraicResult::Identity(SELF_IDENT | COUNTER_IDENT); } match self { @@ -1412,7 +1414,8 @@ mod tagged_node_ref { } pub fn pmeet_dyn(&self, other: TaggedNodeRef) -> AlgebraicResult> where V: Lattice { - if self.shared_node_id() == other.shared_node_id() { + //See the note in `pjoin_dyn`. [Lattice::IDEMPOTENT] covers both operations + if ::IDEMPOTENT && self.shared_node_id() == other.shared_node_id() { return AlgebraicResult::Identity(SELF_IDENT | COUNTER_IDENT); } match self { @@ -1425,7 +1428,9 @@ mod tagged_node_ref { } pub fn psubtract_dyn(&self, other: TaggedNodeRef) -> AlgebraicResult> where V: DistributiveLattice { - if self.shared_node_id() == other.shared_node_id() { + //A node subtracted from itself only leaves nothing when the value subtract is + // idempotent. See [DistributiveLattice::IDEMPOTENT] + if ::IDEMPOTENT && self.shared_node_id() == other.shared_node_id() { return AlgebraicResult::None; } match self { @@ -2034,7 +2039,9 @@ mod tagged_node_ref { } pub fn pjoin_dyn(&self, other: TaggedNodeRef) -> AlgebraicResult> where V: Lattice { - if self.ptr == other.ptr { + //A node joined with itself is only itself when the value join is idempotent. + // See [Lattice::IDEMPOTENT] + if ::IDEMPOTENT && self.ptr == other.ptr { return AlgebraicResult::Identity(SELF_IDENT | COUNTER_IDENT); } let (ptr, tag) = self.ptr.get_raw_parts(); @@ -2049,7 +2056,8 @@ mod tagged_node_ref { } pub fn pmeet_dyn(&self, other: TaggedNodeRef) -> AlgebraicResult> where V: Lattice { - if self.ptr == other.ptr { + //See the note in `pjoin_dyn`. [Lattice::IDEMPOTENT] covers both operations + if ::IDEMPOTENT && self.ptr == other.ptr { return AlgebraicResult::Identity(SELF_IDENT | COUNTER_IDENT); } let (ptr, tag) = self.ptr.get_raw_parts(); @@ -2064,7 +2072,9 @@ mod tagged_node_ref { } pub fn psubtract_dyn(&self, other: TaggedNodeRef) -> AlgebraicResult> where V: DistributiveLattice { - if self.ptr == other.ptr { + //A node subtracted from itself only leaves nothing when the value subtract is + // idempotent. See [DistributiveLattice::IDEMPOTENT] + if ::IDEMPOTENT && self.ptr == other.ptr { return AlgebraicResult::None; } let (ptr, tag) = self.ptr.get_raw_parts(); @@ -3116,7 +3126,9 @@ mod opaque_dyn_rc_trie_node { impl TrieNodeODRc { #[inline] pub fn pjoin(&self, other: &Self) -> AlgebraicResult { - if self.ptr_eq(other) { + //A shared subtrie joined with itself is only itself when the value join is idempotent. + // See [Lattice::IDEMPOTENT] + if ::IDEMPOTENT && self.ptr_eq(other) { AlgebraicResult::Identity(SELF_IDENT | COUNTER_IDENT) } else { self.as_tagged().pjoin_dyn(other.as_tagged()) @@ -3133,6 +3145,12 @@ impl TrieNodeODRc { } #[inline] pub fn join_into(&mut self, node: TrieNodeODRc) -> AlgebraicStatus { + //Joining a shared subtrie into itself is a no-op when the value join is idempotent. This + // is worth checking up front because `make_mut` below deep-copies the node when it's shared, + // which is exactly the situation where `node` is likely to be the same pointer as `self` + if ::IDEMPOTENT && self.ptr_eq(&node) { + return AlgebraicStatus::Identity + } let (status, result) = self.make_mut().join_into_dyn(node); match result { Ok(()) => {}, @@ -3144,7 +3162,8 @@ impl TrieNodeODRc { } #[inline] pub fn pmeet(&self, other: &Self) -> AlgebraicResult { - if self.ptr_eq(other) { + //See the note in `pjoin`. [Lattice::IDEMPOTENT] covers both operations + if ::IDEMPOTENT && self.ptr_eq(other) { AlgebraicResult::Identity(SELF_IDENT | COUNTER_IDENT) } else { self.as_tagged().pmeet_dyn(other.as_tagged()) @@ -3155,7 +3174,9 @@ impl TrieNodeODRc { //See above, pseudo-impl for [DistributiveLattice] trait impl TrieNodeODRc { pub fn psubtract(&self, other: &Self) -> AlgebraicResult { - if self.ptr_eq(other) { + //Subtracting a shared subtrie from itself only leaves nothing when the value subtract is + // idempotent. See [DistributiveLattice::IDEMPOTENT] + if ::IDEMPOTENT && self.ptr_eq(other) { AlgebraicResult::None } else { self.as_tagged().psubtract_dyn(other.as_tagged()) diff --git a/src/write_zipper.rs b/src/write_zipper.rs index 5b233b8..9606771 100644 --- a/src/write_zipper.rs +++ b/src/write_zipper.rs @@ -1743,11 +1743,8 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC Some(src) => { match self.take_focus(false) { Some(mut self_node) => { - let (status, result) = self_node.make_mut().join_into_dyn(src); - match result { - Ok(()) => self.graft_internal(Some(self_node)), - Err(replacement_node) => self.graft_internal(Some(replacement_node)), - } + let status = self_node.join_into(src); + self.graft_internal(Some(self_node)); status }, None => { diff --git a/tests/idempotent_lattice.rs b/tests/idempotent_lattice.rs new file mode 100644 index 0000000..2957b0f --- /dev/null +++ b/tests/idempotent_lattice.rs @@ -0,0 +1,138 @@ +//! Tests that [`Lattice::IDEMPOTENT`] and [`DistributiveLattice::IDEMPOTENT`] are honored by the +//! node-level algebra. +//! +//! The trie shares subtries by pointer, so an algebraic operation frequently ends up with the same +//! node on both sides. For a set-like value (including `()`) the answer can be produced without +//! descending, but for a value type that actually *combines* its operands that shortcut would skip +//! the shared branch instead of combining it. These tests use such a value type. + +use pathmap::PathMap; +use pathmap::ring::*; +use pathmap::zipper::*; + +/// A multiplicity in a multiset. Joining adds occurrences, so joining a subtrie with itself is +/// emphatically not the same as leaving it alone. +#[derive(Clone, Debug, PartialEq, Eq)] +struct Count(u64); + +impl Lattice for Count { + const IDEMPOTENT: bool = false; + fn pjoin(&self, other: &Self) -> AlgebraicResult { + AlgebraicResult::Element(Count(self.0 + other.0)) + } + fn pmeet(&self, other: &Self) -> AlgebraicResult { + AlgebraicResult::Element(Count(self.0.min(other.0))) + } +} + +impl DistributiveLattice for Count { + const IDEMPOTENT: bool = false; + /// Removes a single occurrence, whatever `other` holds + fn psubtract(&self, _other: &Self) -> AlgebraicResult { + if self.0 > 1 { + AlgebraicResult::Element(Count(self.0 - 1)) + } else { + AlgebraicResult::None + } + } +} + +const PATHS: &[&[u8]] = &[b"aaa", b"aab", b"abc", b"bbb", b"bbcd"]; + +fn counted(n: u64) -> PathMap { + let mut map = PathMap::new(); + for path in PATHS { + map.set_val_at(path, Count(n)); + } + map +} + +/// Guards the premise of every test below: cloning a `PathMap` shares the root node by pointer, so +/// the operations really do see the same node on both sides. If this ever stops being true the +/// tests below would still pass while testing nothing. +#[track_caller] +fn assert_shares_root(a: &PathMap, b: &PathMap) { + let (za, zb) = (a.read_zipper(), b.read_zipper()); + assert!(za.is_shared() && zb.is_shared(), "test premise broken: root node is not shared"); + assert_eq!(za.shared_node_id(), zb.shared_node_id(), "test premise broken: roots are different nodes"); +} + +#[test] +fn non_idempotent_pjoin_descends_shared_subtries() { + let a = counted(1); + let b = a.clone(); + assert_shares_root(&a, &b); + + let joined = a.join(&b); + for path in PATHS { + assert_eq!(joined.get_val_at(path), Some(&Count(2)), "at {path:?}"); + } +} + +#[test] +fn non_idempotent_join_into_descends_shared_subtries() { + let mut a = counted(1); + let b = a.clone(); + assert_shares_root(&a, &b); + + a.join_into(b); + for path in PATHS { + assert_eq!(a.get_val_at(path), Some(&Count(2)), "at {path:?}"); + } +} + +#[test] +fn non_idempotent_write_zipper_join_into_descends_shared_subtries() { + let mut dst = counted(1); + let src = dst.clone(); + assert_shares_root(&dst, &src); + + let mut wz = dst.write_zipper(); + wz.join_into(&src.read_zipper()); + drop(wz); + + for path in PATHS { + assert_eq!(dst.get_val_at(path), Some(&Count(2)), "at {path:?}"); + } +} + +#[test] +fn non_idempotent_psubtract_descends_shared_subtries() { + let a = counted(3); + let b = a.clone(); + assert_shares_root(&a, &b); + + //One occurrence comes off each path; the paths survive + let once = a.subtract(&b); + for path in PATHS { + assert_eq!(once.get_val_at(path), Some(&Count(2)), "at {path:?}"); + } + + //Repeat until the multiplicities run out, at which point the paths do go away + let twice = once.subtract(&a); + let thrice = twice.subtract(&a); + for path in PATHS { + assert_eq!(twice.get_val_at(path), Some(&Count(1)), "at {path:?}"); + assert_eq!(thrice.get_val_at(path), None, "at {path:?}"); + } + assert!(thrice.is_empty()); +} + +#[test] +fn idempotent_default_still_short_circuits_correctly() { + //`()` and other set-like values keep the default `IDEMPOTENT = true`, and joining or + //subtracting a shared subtrie against itself must still give set semantics + let a: PathMap<()> = PATHS.iter().copied().collect(); + let b = a.clone(); + + let joined = a.join(&b); + assert_eq!(joined.val_count(), PATHS.len()); + for path in PATHS { + assert_eq!(joined.get_val_at(path), Some(&()), "at {path:?}"); + } + + let met = a.meet(&b); + assert_eq!(met.val_count(), PATHS.len()); + + assert!(a.subtract(&b).is_empty()); +} From f4802362cad658b0a1d6ecdd888855505ff96623 Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 19 Aug 2026 17:52:02 +0000 Subject: [PATCH 03/17] Add memory attribution by node type under the `counters` feature Layout work needs to know where the bytes actually are, and node counts alone do not answer that. `memory_profile` walks physical (deduplicated) nodes and tallies bytes for list nodes, dense nodes and cell nodes separately, along with the list-node key-length distribution and the dense-node slot capacity. Two things it immediately showed, both of which contradict estimates made from reading the code: - The split between list-node and dense-node bytes swings from 80/20 to 53/47 depending on key shape, so a per-node saving in either one means very different things for different workloads. - `ByteNode::item_count` counts a slot's child link and value as two separate items, so it is not the slot count. Sizing the slot array from it is wrong in both directions; `slot_count` and `slot_capacity` are what that needs. `LINE_LIST_NODE_SIZE` now derives the list node's size from KEY_BYTES_CNT rather than asserting a hardcoded 64, so the constant can be swept without hand-editing the assertion. Co-Authored-By: Claude Opus 5 (1M context) --- src/counters.rs | 125 ++++++++++++++++++++++++++++++++++++++++ src/dense_byte_node.rs | 7 +++ src/line_list_node.rs | 6 +- tests/memory_profile.rs | 60 +++++++++++++++++++ 4 files changed, 196 insertions(+), 2 deletions(-) create mode 100644 tests/memory_profile.rs diff --git a/src/counters.rs b/src/counters.rs index 78baf39..f1471e5 100644 --- a/src/counters.rs +++ b/src/counters.rs @@ -250,6 +250,131 @@ pub(crate) fn record_make_unique(cloned: bool) { } } +/// The key-byte capacity of a single [LineListNode](crate::line_list_node), and the resulting node size +pub const LIST_NODE_KEY_BYTES: usize = crate::line_list_node::KEY_BYTES_CNT; +/// The size in bytes of a single `LineListNode`, derived from [LIST_NODE_KEY_BYTES] +pub const LIST_NODE_SIZE: usize = crate::line_list_node::LINE_LIST_NODE_SIZE; + +/// Memory attribution for a trie, broken down by node type +/// +/// Walks physical (deduplicated) nodes, so structurally shared subtries are counted once. Use it to +/// decide where layout work pays: the split between list-node and dense-node bytes varies enormously +/// with key shape, and is what determines whether a given optimization is worth anything. +#[derive(Clone, Default, Debug)] +pub struct MemProfile { + pub list_nodes: usize, + pub list_bytes: usize, + pub dense_nodes: usize, + pub dense_items: usize, + pub dense_cap: usize, + pub dense_bytes: usize, + pub cell_nodes: usize, + pub cell_items: usize, + pub cell_bytes: usize, + pub klen_hist: Vec, + pub val_slots: usize, + pub child_slots: usize, + pub both_slots: usize, + pub key_bytes_used: usize, + pub at_cap: usize, + pub node_klen_hist: Vec, + pub allval_nodes: usize, + pub allval_klen_hist: Vec, +} +impl MemProfile { + fn merge(mut self, o: Self) -> Self { + self.list_nodes += o.list_nodes; self.list_bytes += o.list_bytes; + self.dense_nodes += o.dense_nodes; self.dense_items += o.dense_items; self.dense_bytes += o.dense_bytes; self.dense_cap += o.dense_cap; + self.cell_nodes += o.cell_nodes; self.cell_items += o.cell_items; self.cell_bytes += o.cell_bytes; + self.val_slots += o.val_slots; self.child_slots += o.child_slots; + self.both_slots += o.both_slots; self.key_bytes_used += o.key_bytes_used; self.at_cap += o.at_cap; + if self.klen_hist.len() < o.klen_hist.len() { self.klen_hist.resize(o.klen_hist.len(), 0); } + for (i, c) in o.klen_hist.iter().enumerate() { self.klen_hist[i] += c; } + if self.node_klen_hist.len() < o.node_klen_hist.len() { self.node_klen_hist.resize(o.node_klen_hist.len(), 0); } + for (i, c) in o.node_klen_hist.iter().enumerate() { self.node_klen_hist[i] += c; } + if self.allval_klen_hist.len() < o.allval_klen_hist.len() { self.allval_klen_hist.resize(o.allval_klen_hist.len(), 0); } + for (i, c) in o.allval_klen_hist.iter().enumerate() { self.allval_klen_hist[i] += c; } + self.allval_nodes += o.allval_nodes; + self + } + pub fn total_bytes(&self) -> usize { self.list_bytes + self.dense_bytes + self.cell_bytes } + pub fn report_list_slots(&self) { + let tot: usize = self.klen_hist.iter().sum(); + println!(" list slot key-length histogram ({} slots, {} value-slots, {} child-slots):", tot, self.val_slots, self.child_slots); + let mut acc = 0; + for (len, cnt) in self.klen_hist.iter().enumerate() { + if *cnt == 0 { continue } + acc += cnt; + println!(" len {:>2}: {:>9} ({:4.1}%) cum {:4.1}%", len, cnt, *cnt as f64/tot as f64*100.0, acc as f64/tot as f64*100.0); + } + println!(" nodes with both slots used: {} total key bytes used per node avg: {:.1} of {}", + self.both_slots, self.key_bytes_used as f64 / self.list_nodes.max(1) as f64, crate::line_list_node::KEY_BYTES_CNT); + let tn: usize = self.node_klen_hist.iter().sum(); + let mut cum = 0usize; let mut cum_av = 0usize; + println!(" per-NODE total key bytes (cumulative fit):"); + for (len, cnt) in self.node_klen_hist.iter().enumerate() { + cum += cnt; cum_av += self.allval_klen_hist[len]; + if [6usize,10,14,18,22,26,34,42,50,58,84].contains(&len) { + println!(" <= {:>2} bytes: {:5.1}% of all list nodes | {:5.1}% of the {} leaf-only nodes", + len, cum as f64/tn as f64*100.0, cum_av as f64/self.allval_nodes.max(1) as f64*100.0, self.allval_nodes); + } + } + println!(" leaf-only nodes (no child slot): {} of {} ({:4.1}%)", self.allval_nodes, self.list_nodes, self.allval_nodes as f64/self.list_nodes.max(1) as f64*100.0); + println!(" nodes at the key cap (key0+key1 >= {}): {}", crate::line_list_node::KEY_BYTES_CNT, self.at_cap); + } + pub fn report(&self, label: &str, vals: usize) { + let t = self.total_bytes() as f64; + println!("--- {label} ---"); + println!(" values {vals}"); + println!(" list nodes {:>9} bytes {:>11} ({:4.1}% of trie)", self.list_nodes, self.list_bytes, self.list_bytes as f64/t*100.0); + println!(" dense nodes {:>9} bytes {:>11} ({:4.1}% of trie) items {} avg {:.1}/node", + self.dense_nodes, self.dense_bytes, self.dense_bytes as f64/t*100.0, self.dense_items, + self.dense_items as f64 / self.dense_nodes.max(1) as f64); + println!(" dense slots: len {} cap {} ({:.1}% over-allocated)", self.dense_items, self.dense_cap, (self.dense_cap as f64/self.dense_items.max(1) as f64 - 1.0)*100.0); + println!(" cell nodes {:>9} bytes {:>11} ({:4.1}% of trie) items {}", self.cell_nodes, self.cell_bytes, self.cell_bytes as f64/t*100.0, self.cell_items); + println!(" TOTAL bytes {:>9} = {:.1} bytes/value", self.total_bytes(), t / vals.max(1) as f64); + } +} + +/// Builds a [MemProfile] for `map`. See the type docs +pub fn memory_profile(map: &PathMap) -> MemProfile { + use crate::trie_node::traverse_physical; + use crate::alloc::GlobalAlloc; + let cf = core::mem::size_of::>(); + let cellcf = core::mem::size_of::>(); + let list_sz = core::mem::size_of::>(); + let dense_sz = core::mem::size_of::>(); + let cell_sz = core::mem::size_of::>(); + let Some(root) = map.root() else { return MemProfile::default() }; + traverse_physical(root, move |node, ctx: MemProfile| { + let mut c = ctx; + if let Some(l) = node.as_list() { + c.list_nodes += 1; c.list_bytes += list_sz; + let (k0, k1) = l.get_both_keys(); + if c.klen_hist.len() < crate::line_list_node::KEY_BYTES_CNT + 1 { c.klen_hist.resize(crate::line_list_node::KEY_BYTES_CNT + 1, 0); } + c.klen_hist[k0.len()] += 1; + if k1.len() > 0 { c.klen_hist[k1.len()] += 1; c.both_slots += 1; } + c.key_bytes_used += k0.len() + k1.len(); + if k0.len() + k1.len() >= crate::line_list_node::KEY_BYTES_CNT { c.at_cap += 1; } + let ktot = k0.len() + k1.len(); + if c.node_klen_hist.len() < 2*crate::line_list_node::KEY_BYTES_CNT + 2 { c.node_klen_hist.resize(2*crate::line_list_node::KEY_BYTES_CNT + 2, 0); c.allval_klen_hist.resize(2*crate::line_list_node::KEY_BYTES_CNT + 2, 0); } + c.node_klen_hist[ktot] += 1; + let has_child = l.is_used_child_0() || l.is_used_child_1(); + if !has_child { c.allval_nodes += 1; c.allval_klen_hist[ktot] += 1; } + if l.is_used_value_0() { c.val_slots += 1 } else if l.is_used_child_0() { c.child_slots += 1 } + if l.is_used_value_1() { c.val_slots += 1 } else if l.is_used_child_1() { c.child_slots += 1 } + } + else if let Some(d) = node.as_dense() { + c.dense_nodes += 1; c.dense_items += d.slot_count(); c.dense_cap += d.slot_capacity(); c.dense_bytes += dense_sz + d.slot_capacity()*cf; + } else if node.tag() == crate::trie_node::CELL_BYTE_NODE_TAG { + let n = node.item_count(); + // each CellCoFree additionally owns a boxed OrdinaryCoFree + c.cell_nodes += 1; c.cell_items += n; c.cell_bytes += cell_sz + n*(cellcf + cf); + } + c + }, |a, b| a.merge(b)) +} + #[cfg(test)] mod tests { use super::{cow_counters, reset_cow_counters}; diff --git a/src/dense_byte_node.rs b/src/dense_byte_node.rs index 3257030..1ad9481 100644 --- a/src/dense_byte_node.rs +++ b/src/dense_byte_node.rs @@ -119,6 +119,13 @@ impl> ByteNode }; word_base + (mask_word & preceding_bits).count_ones() as usize } + /// Number of allocated `CoFree` slots, including unused `Vec` capacity + #[inline] + pub fn slot_capacity(&self) -> usize { self.values.capacity() } + /// Number of occupied `CoFree` slots. Note this differs from [Self::item_count], which counts + /// a slot's child link and value as two separate items + #[inline] + pub fn slot_count(&self) -> usize { self.values.len() } #[inline] pub fn reserve_capacity(&mut self, additional: usize) { self.values.reserve(additional) diff --git a/src/line_list_node.rs b/src/line_list_node.rs index be86c73..da45137 100644 --- a/src/line_list_node.rs +++ b/src/line_list_node.rs @@ -37,7 +37,7 @@ pub struct LineListNode { #[cfg(feature = "slim_ptrs")] pub(crate) const KEY_BYTES_CNT: usize = 42; #[cfg(not(feature = "slim_ptrs"))] -pub(crate) const KEY_BYTES_CNT: usize = 14; +pub(crate) const KEY_BYTES_CNT: usize = 42; // Only the slim_ptrs layout is asserted. The not(slim_ptrs) TrieNodeODRc has no // empty-sentinel representation yet (`new_empty`/`is_empty`/`make_unique`/`==` @@ -47,7 +47,9 @@ pub(crate) const KEY_BYTES_CNT: usize = 14; // asserted until the sentinel design lands. #[cfg(all(feature = "slim_ptrs", target_arch = "x86_64", not(miri)))] const _: [(); core::mem::size_of::>()] = - [(); 64]; + [(); LINE_LIST_NODE_SIZE]; +/// refcnt 4 + header 2 + two 8-byte payload slots + the key area, rounded up to 8-byte alignment +pub(crate) const LINE_LIST_NODE_SIZE: usize = ((4 + 2 + 16 + KEY_BYTES_CNT) + 7) / 8 * 8; const SLOT_0_USED_MASK: u16 = 1 << 15; const SLOT_1_USED_MASK: u16 = 1 << 14; diff --git a/tests/memory_profile.rs b/tests/memory_profile.rs new file mode 100644 index 0000000..3d610a8 --- /dev/null +++ b/tests/memory_profile.rs @@ -0,0 +1,60 @@ +#![cfg(all(feature = "counters", feature = "serialization"))] + +use std::time::Instant; +use pathmap::PathMap; +use pathmap::counters::memory_profile; +use pathmap::zipper::{ZipperMoving, ZipperIteration}; + +fn xorshift_keys(seed: u64, n: u32) -> Vec<[u8; 8]> { + let mut x = seed; + (0..n).map(|_| { x ^= x << 13; x ^= x >> 7; x ^= x << 17; x.to_be_bytes() }).collect() +} +fn timeit(reps: usize, mut f: impl FnMut() -> f64) -> f64 { + let mut b = f64::MAX; for _ in 0..reps { let t = f(); if t < b { b = t } } b +} +fn survey(label: &str, paths: &[Vec]) { + let build_ms = timeit(3, || { + let t = Instant::now(); + let mut m: PathMap<()> = PathMap::new(); + for p in paths { m.set_val_at(&p[..], ()); } + let e = t.elapsed().as_secs_f64()*1e3; drop(m); e + }); + let mut m: PathMap<()> = PathMap::new(); + for p in paths { m.set_val_at(&p[..], ()); } + let iter_ms = timeit(3, || { + let t = Instant::now(); + let mut z = m.read_zipper(); let mut n = 0u64; + while z.to_next_val() { n += 1 } + let e = t.elapsed().as_secs_f64()*1e3; assert!(n > 0); e + }); + let get_ms = timeit(3, || { + let t = Instant::now(); + let mut hits = 0u64; + for p in paths { if m.get_val_at(&p[..]).is_some() { hits += 1 } } + let e = t.elapsed().as_secs_f64()*1e3; assert!(hits > 0); e + }); + let prof = memory_profile(&m); + prof.report(label, m.val_count()); + println!("SWEEP\t{}\tK={}\tnodesz={}\tbytes={}\tlist_nodes={}\tdense_nodes={}\tbuild={:.1}\titer={:.1}\tget={:.1}", + label, pathmap::counters::LIST_NODE_KEY_BYTES, pathmap::counters::LIST_NODE_SIZE, + prof.total_bytes(), prof.list_nodes, prof.dense_nodes, build_ms, iter_ms, get_ms); +} + +#[test] +fn memory_profile_survey() { + // MORK-representative + let mut m: PathMap<()> = PathMap::new(); + let f = std::fs::File::open("benches/big_logic.metta.paths").unwrap(); + pathmap::paths_serialization::deserialize_paths(m.write_zipper(), f, ()).unwrap(); + let mork: Vec> = { let mut v = vec![]; let mut z = m.read_zipper(); + while z.to_next_val() { v.push(z.path().to_vec()) } v }; + survey("mork_big_logic", &mork); + + let rnd: Vec> = xorshift_keys(0x243F6A8885A308D3, 1_000_000).iter().map(|k| k.to_vec()).collect(); + survey("random_8byte", &rnd); + + let text = std::fs::read_to_string("benches/shakespeare.txt").unwrap(); + let words: Vec> = { let mut v: Vec> = text.split_ascii_whitespace().map(|w| w.as_bytes().to_vec()).collect(); + v.sort(); v.dedup(); v }; + survey("shakespeare", &words); +} From ef9937e37e73b932483f6c25f27c657d95b4febd Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 19 Aug 2026 17:52:56 +0000 Subject: [PATCH 04/17] Record the layout measurements and three negative results Adds to the perf notes: where the bytes actually are by node type, the dense slot-array over-allocation, and three things that were measured and rejected -- the S3 key-byte reclaim (not implementable, and worthless anyway), the KEY_BYTES_CNT sweep (no single constant serves both long-path and short-key workloads), and bounded growth for dense slot arrays (pays for its memory in build time). Also sizes S1 properly against allocated rather than counted slots, and records why its implementation is harder than the struct definition suggests: CoFree values are cloned and dropped outside the node that owns them, so moving the presence bit out costs a manual Drop/Clone and a parallel mask through all four algebraic operations. Co-Authored-By: Claude Opus 5 (1M context) --- notes/unit_value_perf.md | 209 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 209 insertions(+) create mode 100644 notes/unit_value_perf.md diff --git a/notes/unit_value_perf.md b/notes/unit_value_perf.md new file mode 100644 index 0000000..e4a8dac --- /dev/null +++ b/notes/unit_value_perf.md @@ -0,0 +1,209 @@ +# Unit-value optimizations: measured results + +Two optimizations aimed at `PathMap<()>` and other trivially-valued tries, from a +survey of places the trie pays for a value that carries no information. + +| commit | change | +| --- | --- | +| `218a256` | Skip value-drop work in `LineListNode` when `V` has none to do | +| `18d40ec` | Honor `Lattice::IDEMPOTENT` in the node algebra, and short-circuit `join_into` | +| `9f73907` | Memory attribution by node type under the `counters` feature | + +## Results + +Min of 3 runs per side, each run itself the min of 7 timed repetitions, every +operation in its own process. 1M paths of 8 random bytes. + +| operation | before (ms) | after (ms) | change | verdict | +| --- | ---: | ---: | ---: | --- | +| `join_into` — operands share a root | 108.1 | **0.000** | **eliminated** | real gain | +| `join` — operands share a root | 0.000 | 0.000 | — | already short-circuited | +| `subtract` — operands share a root | 0.000 | 0.000 | — | already short-circuited | +| `join_into` — disjoint operands | 102.4 | 104.6 | +2.1% | within noise | +| `join` — disjoint operands | 85.8 | 87.0 | +1.5% | within noise | +| `meet` — full overlap, distinct nodes | 95.0 | 95.0 | +0.0% | within noise | +| `subtract` — full overlap, distinct nodes | 123.3 | 114.3 | −7.3% | within noise | +| drop `PathMap<()>` | 193.1 | 193.7 | +0.3% | within noise | +| drop `PathMap` | 191.7 | 192.0 | +0.1% | within noise | +| drop `PathMap>` | 365.8 | 367.1 | +0.4% | within noise | + +**Noise floor**, measured by running the same tree against itself three times: +7–11% on the algebra operations, 2–3% on the drops. Every row above except the +first sits inside it. Treat the −7.3% on `subtract` as noise, not a gain — the +change touches nothing on that path beyond a compile-time-constant guard. + +## What each optimization actually bought + +### `join_into` on a shared subtrie: 108 ms → free + +The only unambiguous win, and it came from a shortcut that was *missing* rather +than one that was slow. `TrieNodeODRc::join_into` had no pointer-equality check +and went straight to `make_mut()`, which deep-copies the node when it is shared — +precisely the case where both sides are likely to be the same pointer. Three call +sites that hand-rolled `make_mut().join_into_dyn()` now route through it and +inherit the check: + +- `PathMap::join_into` +- `CoFree::join_into` (the recursive branch) +- `WriteZipper::join_into_take` + +The size of the win scales with the trie, because the whole descent disappears. +It is worth the most in workloads that join overlapping tries built by grafting +or cloning, where pointer sharing is common — which is the regime `()` values plus +merkleization create. + +### Drop elision: no measurable throughput change + +`LineListNode`'s drop paths tested whether each payload slot held a child or a +value, then called `ManuallyDrop::drop` on the value. For a value type with no +drop glue that call does nothing, but the branches around it still ran. + +LLVM was already folding the no-op `LocalOrHeap<(), _>::drop` at `-O2`, so what +the change removes on top of that is a well-predicted branch — invisible against +the allocator traffic and cache misses of walking a million-node trie. What it +does buy: + +- The elision is explicit rather than optimizer-dependent, so it holds in debug too. +- The drop path got simpler: `is_used_child_N()` replaces four separate bit tests. +- It generalizes past `()` to any value type with no drop glue that fits inline. + +One trap worth recording. The obvious guard, `needs_drop::()`, is **wrong** +here. The slot is a `LocalOrHeap`, which carries an unconditional `Drop` +impl and boxes any `V` too large to store inline — so a `[u8; 64]` has no drop +glue of its own but does own a heap allocation. The predicate has to cover both: + +```rust +pub(crate) const fn val_slot_needs_drop() -> bool { + core::mem::needs_drop::() || core::mem::size_of::() > core::mem::size_of::() +} +``` + +`ValSlotStorage` is now the type alias the union itself uses, so the size +threshold cannot drift away from the layout. `cargo miri test` covers the +`[u8; 64]` case, since only miri can observe the leak a wrong guard would cause. + +### `IDEMPOTENT`: a correctness fix, not a speedup + +The gating half of `18d40ec` costs and saves nothing at `()`, because +`IDEMPOTENT` is `true` there and folds away. Its value is that the constant now +means something. Six sites take the shared-subtrie shortcut — not the three that +grepping for `ptr_eq` finds, but also the `TaggedNodeRef` dispatchers underneath, +which is where it fires at *every* level of a descent, in two copies (one per +`slim_dispatch` setting). Before this change a value type whose join actually +combines its operands — a multiset adding multiplicities — would have had its +structurally shared branches silently skipped instead of combined. + +No in-tree value type declares `IDEMPOTENT = false`, so no existing behavior +changed; all 710 lib tests pass unchanged. + +## Method + +- rustc 1.95.0-nightly, `--release`, `-C target-cpu=native` (from `.cargo/config.toml`) +- Default features (`graft_root_vals`, `slim_ptrs`, `serialization`); jemalloc **off** +- x86_64 Linux, 64 cores +- Baseline is `master` at `8b8802a` in a separate worktree with its own target dir +- Each operation runs in its own process. This matters: an earlier pass that timed + everything in one process showed a spurious 6–9% regression on three operations, + because at baseline the self-shared join does real allocator work that warms the + heap for whatever is measured next. Process isolation removed it. +- Inputs are built outside the timed region; results are dropped outside it. + +The timing harness was temporary and is not in the tree; reconstructing it means +building two `PathMap<()>`s from xorshift-generated 8-byte keys, timing the +operation with `Instant`, and taking the min across repetitions. The *memory* +harness is in the tree -- `cargo test --release --features counters,serialization +--test memory_profile -- --nocapture` reproduces every memory table below. + +## Where the memory actually is + +`memory_profile` (`9f73907`) walks physical nodes and attributes bytes by node type. +Three tries, all `PathMap<()>`: + +| dataset | values | list nodes | dense nodes | bytes/value | +| --- | ---: | ---: | ---: | ---: | +| `big_logic.metta` (MORK) | 91,692 | 7.63 MB (**79.7%**) | 1.95 MB (20.3%) | 104.5 | +| 1M random 8-byte keys | 1,000,000 | 62.2 MB (69.2%) | 27.7 MB (30.8%) | 89.9 | +| shakespeare words | 67,505 | 2.16 MB (52.8%) | 1.93 MB (47.2%) | 60.7 | + +The split swings from 80/20 to 53/47 with key shape, so "shrink a dense slot" and +"shrink a list node" mean very different things depending on the workload. On the +MORK-shaped data, list nodes are four fifths of the trie. + +### Dense slot arrays run 16-41% over-allocated + +| dataset | slots used | slots allocated | slack | +| --- | ---: | ---: | ---: | +| MORK | 52,474 | 60,714 | 15.7% | +| random 8-byte | 1,038,499 | 1,466,715 | **41.2%** | +| shakespeare | 55,916 | 72,132 | 29.0% | + +`Vec`'s amortized doubling, on arrays that top out at 256 slots and are usually +built one byte at a time. + +## Negative results + +These three were measured and rejected. Recording them so they are not +re-attempted from first principles. + +### S3 -- reclaiming value-slot bytes for list-node keys: not possible, and worthless + +The earlier survey claimed a `LineListNode` value slot's 8 bytes could go to key +storage when `V` is a ZST, taking `KEY_BYTES_CNT` from 42 to 58. That was wrong +twice over. + +**It cannot be done.** The slot is a union with a child pointer, and which one it +holds is a runtime property of the node. The space has to be sized for the pointer +whatever `V` is. There is no static reclaim without a separate node type. + +**It would not be worth it anyway.** Only 10.4% of MORK list nodes are at the +42-byte cap, and *zero* on the other two datasets. + +### Sweeping `KEY_BYTES_CNT`: already near-optimal for MORK, no single best value + +| K | node size | MORK | random 8-byte | shakespeare | +| ---: | ---: | ---: | ---: | ---: | +| 10 | 32 B | +18% | **-35%** | **-27%** | +| 14 | 40 B | +16% | -28% | -20% | +| 18 | 40 B | +1.5% | -28% | -20% | +| 26 | 48 B | **-2%** | -19% | -14% | +| 34 | 56 B | -2% | -10% | -7% | +| 42 | 64 B | baseline | baseline | baseline | + +Timings moved less than 5% throughout. MORK's long paths want the current 42; +short-key workloads want roughly 10. No single constant serves both, which is an +argument for a second, smaller list-node type rather than a different constant. + +### Bounded growth for dense slot arrays: a wash + +Replacing `Vec` doubling with growth to the next multiple of `SLOT_GROWTH`: + +| growth | MORK | random 8-byte | shakespeare | random build time | +| --- | ---: | ---: | ---: | ---: | +| exact | -1.4% | **-7.6%** | -5.6% | **+26%** | +| 2 | -1.1% | -7.0% | -4.6% | +8.5% | +| 4 | -0.6% | -5.9% | -2.3% | +8.3% | +| 8 | +1.6% | -3.6% | +2.2% | ~flat | + +The memory it reclaims is paid for in build time, and for granularity 8 and above +the rounding wastes more than doubling did on MORK's small dense nodes (3.4 slots +on average). Rejected. + +## Still on the table + +Both of these are small items from a wider survey. The larger unit-value wins are +untouched: + +- **Dense-node slots are 16 bytes where 8 carry information.** `OrdinaryCoFree` is + `{ Option, Option }`; at `()` the pointer is 8 bytes and + `Option<()>` is 1, which alignment rounds to 16. Hoisting the presence bit into a + second `ByteMask` halves a 256-way dense node from 4128 to 2112 bytes. +- **The algebra becomes mask arithmetic** once that split exists: the value + dimension of join/meet/subtract/restrict is one bitwise operation, and recursion + narrows to the intersection of the two child masks. +- **`LineListNode` spends 16 of its 64 bytes on payload slots a set doesn't need**, + which could go to inline key bytes instead (42 → 58 for a two-leaf node). + +These need either a value policy carrying an associated slot type — the refactor +`ring.rs` and `dense_byte_node.rs` already gesture at, and that +`pathmap-book/src/A.0003_policy_API.md` is a stub for — or parallel set-specialized +node types. Neither is reachable with the kind of local guard used here. From 16211d4b71a8f4da2c0c24b75cbf17f1a8e3c05f Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 19 Aug 2026 18:25:14 +0000 Subject: [PATCH 05/17] Shelve the CoFree pointer-bit packing on a branch The packing works and is miri-clean, and the memory it saves is real: -5.1% on MORK-shaped data, -13.0% on random keys, -14.1% on shakespeare, landing within a tenth of a percent of what the profiler predicted. But it measures ~9% slower on iteration over the MORK dataset, and 5% memory is not worth risking 9% iteration on the workload that matters. The likely cause is `has_rec()` going from an `Option` null check to a pointer-tag comparison, on a hot path, in tries that are list-node-heavy and so collect little of the dense-node win. The code moves to `experiment/cofree-pointer-packing`, kept to show the approach is viable and to record what it costs. The notes keep the full result, the invariant it introduces, and the two places it can be silently defeated. The regression tests come to the main line as `tests/prefix_value_preservation.rs`. They cover values on paths that are proper prefixes of other paths -- the slots that hold both a value and a child, which is where value loss hides -- across copy-on-write, node upgrades, grafting, algebra and removal. That property is worth pinning down whatever the representation. Co-Authored-By: Claude Opus 5 (1M context) --- notes/unit_value_perf.md | 96 ++++++++++++++++++++ tests/prefix_value_preservation.rs | 137 +++++++++++++++++++++++++++++ 2 files changed, 233 insertions(+) create mode 100644 tests/prefix_value_preservation.rs diff --git a/notes/unit_value_perf.md b/notes/unit_value_perf.md index e4a8dac..d7d1043 100644 --- a/notes/unit_value_perf.md +++ b/notes/unit_value_perf.md @@ -9,6 +9,9 @@ survey of places the trie pays for a value that carries no information. | `18d40ec` | Honor `Lattice::IDEMPOTENT` in the node algebra, and short-circuit `join_into` | | `9f73907` | Memory attribution by node type under the `counters` feature | +A fourth change -- packing the CoFree value flag into its child pointer -- was +built and measured but **not merged**; see [below](#shelved-packing-the-value-presence-flag-into-the-child-pointer). + ## Results Min of 3 runs per side, each run itself the min of 7 timed repetitions, every @@ -140,6 +143,99 @@ MORK-shaped data, list nodes are four fifths of the trie. `Vec`'s amortized doubling, on arrays that top out at 256 slots and are usually built one byte at a time. +## Shelved: packing the value-presence flag into the child pointer + +**Implemented, measured, and not merged.** It lives on branch +`experiment/cofree-pointer-packing` (`2fd48be`), rebuilt on top of this branch's +history. It works, it is miri-clean, and it buys real memory -- but it costs +roughly 9% on iteration over MORK-shaped data, which is the workload that +matters here, so the trade goes the wrong way. Kept as a branch to show the +approach is viable and to record what it costs. + + +A dense-node slot was `{ Option, Option }`: one word for the +pointer, and a whole word more for `Option` once alignment rounds it up. Node +allocations are 8-byte aligned, so bits 0..3 of a node address are always zero. +`OrdinaryCoFree` now stores an absent child as the empty-node sentinel instead of +`None` -- so the word is never null -- and borrows bit 0 to say whether its +`MaybeUninit` is initialized. + +| slot | before | after | +| --- | ---: | ---: | +| `OrdinaryCoFree<()>` | 16 B | **8 B** | +| `OrdinaryCoFree` | 16 B | 16 B | +| `OrdinaryCoFree` | 24 B | **16 B** | + +Pinned by static assertions, so the layout cannot regress silently. + +### Result + +| dataset | memory | build | iterate | lookup | +| --- | ---: | ---: | ---: | ---: | +| `big_logic.metta` (MORK) | 9.58 -> 9.09 MB (**-5.1%**) | +3.3% | +9.3% | +0.7% | +| 1M random 8-byte keys | 89.94 -> 78.21 MB (**-13.0%**) | -7.8% | -3.4% | -9.2% | +| shakespeare words | 4.10 -> 3.52 MB (**-14.1%**) | -11.6% | -11.5% | -28.3% | + +Memory is a clean win and lands within a tenth of a percent of what the profiler +predicted. **The timing is why this is shelved.** Shakespeare gets faster on every +axis and random keys improve modestly, but MORK iteration measures ~9% slower. +That is at the edge of what these benchmarks resolve, so it may be less than it +looks -- but 5% memory is not worth risking 9% iteration on the target workload, +and the burden of proof is on the change. + +The likely cause is that `has_rec()` went from an `Option` null check to +`is_empty()`, which extracts and compares the pointer tag. That is a few +instructions on a very hot path, and MORK's tries are list-node-heavy, so they +pay it without getting much of the dense-node memory win in return. Anyone +picking this up should start there. The algebra +micro-benchmarks (`join`, `meet`, `subtract`, both disjoint and overlapping) all +moved less than 5%. + +### Why the flag went in the pointer rather than a mask on the node + +The alternative was a second `ByteMask` on `ByteNode`. It would have been safer +per-line but much larger: a `CoFree` would stop being self-describing, and the +algebra clones and drops them *outside* the node that owns them -- +`pjoin`, `pmeet`, `psubtract` and `prestrict` each accumulate `CoFree`s into a +fresh vector before any node exists to own it. Keeping the flag inside the slot +left all ~108 call sites untouched. + +### The invariant, and what it cost + +The bit belongs to the **slot**, not to either node, so every in-place +replacement of a `TrieNodeODRc` must preserve it. `replace_node` and `swap_node` +do; plain assignment does not, and the failure is silent -- the slot's value just +disappears. Converting the assignment sites broke six existing tests, and one +more case (`ByteNode::node_replace_child`) that no existing test covered and that +a regex over deref-assignments missed because of the method-call chain +(`*cf.rec_mut().unwrap() = new_node`). + +Every failure was a slot holding a value *and* a child -- i.e. a path that is a +proper prefix of another path. `tests/cofree_val_flag.rs` pins that down: +copy-on-write of a shared trie, node upgrades under a write zipper, grafting over +a slot that holds a value, algebra over prefix-heavy tries, and removal from a +slot that holds both. All pass under miri, along with the differential algebra +test and 118 filtered lib tests. The suite is kept on the main line as +`tests/prefix_value_preservation.rs`, since the property is worth pinning down +whatever the representation. + +Two notes for anyone touching this again: + +- The empty sentinel moved from `0xBAADF00D` to `0xBAADF00C`, because it has to + be able to carry the flag like any other node pointer. A static assertion + enforces that bit 0 is clear. +- `ptr_eq` and `shared_node_id` mask the bit off. Leaving it in would have + silently defeated the shared-subtrie shortcuts from `18d40ec` and weakened the + catamorphism cache, without failing a single test. + +### S2 -- the CellCoFree box: not worth doing + +`CellCoFree` keeps the old `Option` layout under the name `PinnedCoFree`, because +`CellByteNode::prepare_cf` hands a `WriteZipper` a `&mut Option` that has to +point at a real `Option`. That costs nothing measurable: cell nodes appear only +under a `ZipperHead` and account for **zero bytes** in all three tries profiled +here, so halving their per-slot box would have saved nothing. + ## Negative results These three were measured and rejected. Recording them so they are not diff --git a/tests/prefix_value_preservation.rs b/tests/prefix_value_preservation.rs new file mode 100644 index 0000000..c61aaf1 --- /dev/null +++ b/tests/prefix_value_preservation.rs @@ -0,0 +1,137 @@ +//! Tests that a value on a path which is a *proper prefix* of other paths survives structural +//! mutation of the trie. +//! +//! These slots -- the ones holding both a value and a child link -- are where value loss hides. A +//! slot with only one of the two cannot lose anything, so bugs in copy-on-write, node upgrades and +//! grafting show up here and nowhere else. The suite was written while packing the value-presence +//! flag into the child pointer (see the `experiment/cofree-pointer-packing` branch), which broke +//! six existing tests and one case no existing test covered; it is kept on the main line because +//! the property it checks is worth pinning down whatever the representation. + +use pathmap::PathMap; +use pathmap::ring::{Lattice, DistributiveLattice}; +use pathmap::zipper::{ZipperMoving, ZipperWriting, ZipperValues, ZipperIteration}; + +/// Paths chosen so that many are proper prefixes of others, and so the trie is wide enough at the +/// root to force dense nodes +fn prefix_heavy() -> Vec> { + let mut paths = vec![]; + for b in 0u8..64 { + paths.push(vec![b]); + paths.push(vec![b, b]); + paths.push(vec![b, b, b]); + paths.push(vec![b, b, b, 0]); + } + paths +} + +fn build(paths: &[Vec]) -> PathMap { + let mut m = PathMap::new(); + for (i, p) in paths.iter().enumerate() { m.set_val_at(&p[..], i as u64); } + m +} + +#[track_caller] +fn assert_intact(m: &PathMap, paths: &[Vec], extra: usize, what: &str) { + for (i, p) in paths.iter().enumerate() { + assert_eq!(m.get_val_at(&p[..]), Some(&(i as u64)), "{what}: lost the value at {p:?}"); + } + assert_eq!(m.val_count(), paths.len() + extra, "{what}: value count changed"); +} + +#[test] +fn values_survive_copy_on_write_of_a_shared_trie() { + let paths = prefix_heavy(); + let original = build(&paths); + + //Aliasing the trie means every write below has to clone the nodes on its path, replacing + //child links in slots that also hold values + let alias = original.clone(); + let mut copy = original.clone(); + for b in 0u8..64 { + copy.set_val_at(&[b, b, b, 1][..], 9999); + } + + assert_intact(&alias, &paths, 0, "aliased handle"); + assert_intact(&original, &paths, 0, "original"); + for b in 0u8..64 { + assert_eq!(copy.get_val_at(&[b, b, b, 1][..]), Some(&9999)); + } +} + +#[test] +fn values_survive_node_upgrades_under_a_write_zipper() { + let paths = prefix_heavy(); + let mut m = build(&paths); + + //Fanning out under an existing prefix forces list nodes to grow into dense nodes while their + //slots still hold values + { + let mut wz = m.write_zipper_at_path(b"\x00"); + //bytes 64.. are untouched by the fixture, so nothing existing is overwritten + for b in 64u8..=255 { wz.descend_to(&[b][..]); wz.set_val(7); wz.reset(); } + } + for b in 64u8..=255 { + assert_eq!(m.get_val_at(&[0u8, b][..]), Some(&7), "the newly written value at [0,{b}] is there"); + } + assert_intact(&m, &paths, 192, "after node upgrades"); +} + +#[test] +fn values_survive_grafting_over_a_slot_that_has_one() { + let paths = prefix_heavy(); + let mut m = build(&paths); + let donor: PathMap = build(&[b"zz".to_vec(), b"zzz".to_vec()]); + + //Grafting replaces the child link of a slot that already holds a value + { + let mut wz = m.write_zipper_at_path(b"\x01\x01"); + wz.graft(&donor.read_zipper()); + } + //NOTE: with the default `graft_root_vals` feature the value *at* the focus is part of the + //graft, so [1,1] takes the donor's (absent) root value. The value above it must be untouched. + assert_eq!(m.get_val_at(&[1u8][..]), Some(&4), "the value above the graft point survived"); + assert!(m.get_val_at(&[1u8, 1, b'z', b'z'][..]).is_some(), "the grafted subtrie is present"); + //and every other prefix value in the trie is unaffected + for b in 2u8..64 { + assert_eq!(m.get_val_at(&[b][..]), Some(&((b as u64)*4)), "value at [{b}]"); + assert_eq!(m.get_val_at(&[b, b][..]), Some(&((b as u64)*4 + 1)), "value at [{b},{b}]"); + } +} + +#[test] +fn values_survive_algebra_on_prefix_heavy_tries() { + let paths = prefix_heavy(); + let a = build(&paths); + let b = build(&paths); + + let joined = a.join(&b); + assert_intact(&joined, &paths, 0, "join"); + let met = a.meet(&b); + assert_eq!(met.val_count(), paths.len(), "meet"); + assert!(a.subtract(&b).is_empty(), "subtract of equals"); + + //And the values must still be reachable by iteration, not just by lookup + let mut z = joined.read_zipper(); + let mut seen = 0; + while z.to_next_val() { assert!(z.val().is_some()); seen += 1; } + assert_eq!(seen, paths.len()); +} + +#[test] +fn a_slot_with_both_value_and_child_round_trips_through_removal() { + let paths = prefix_heavy(); + let mut m = build(&paths); + + //Removing the value from a slot that also has a child must leave the child alone, and vice versa + for b in 0u8..64 { + let path = [b]; + let mut wz = m.write_zipper_at_path(&path[..]); + assert!(wz.remove_val(false).is_some(), "value at [{b}] was already gone"); + } + for b in 0u8..64 { + assert_eq!(m.get_val_at(&[b][..]), None); + assert_eq!(m.get_val_at(&[b, b][..]), Some(&((b as u64)*4 + 1)), "child subtrie of [{b}] survived"); + assert_eq!(m.get_val_at(&[b, b, b, 0][..]), Some(&((b as u64)*4 + 3))); + } +} From 6962d2eeb08cd9fa185709a48dfb9c085368732d Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 19 Aug 2026 18:31:42 +0000 Subject: [PATCH 06/17] S4: dangling paths already cost nothing; record it and assert it The survey claimed a path kept alive by `remove_val(prune = false)` costs a pointer and a node, because a `LineListNode` records it by pointing the slot's child link at the empty-node sentinel. Measured, it costs nothing: - `new_empty()` is a bogus address carrying EMPTY_NODE_TAG. No allocation, no node. - The payload word it occupies is part of a fixed-size struct, so marking the slot "dangling" in the header instead would free zero bytes. - A `DenseByteNode` never used the sentinel at all -- it already represents a dangling path as a CoFree holding neither child nor value, which is exactly what S4 proposed building. Removing half of shakespeare's values without pruning produces 14,212 dangling slots and leaves the trie byte for byte identical, 4,095,744 both ways. `dangling_path_survey` asserts that, so the claim does not have to be re-derived from the struct definitions that got it wrong the first time. Also checked the one caller that really does allocate: a WriteZipper opened on an absent path allocates an empty LineListNode for its root but does not leave it behind. 2000 such zippers, created and dropped without writing, moved the byte count by exactly zero, and no allocated empty node appears in any trie profiled here. The profiler gains `empty_nodes` and `dangling_slots` counters, which is what made this decidable. Co-Authored-By: Claude Opus 5 (1M context) --- notes/unit_value_perf.md | 36 +++++++++++++++++++++++++++++++++++- src/counters.rs | 9 +++++++++ src/line_list_node.rs | 2 +- tests/memory_profile.rs | 39 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 84 insertions(+), 2 deletions(-) diff --git a/notes/unit_value_perf.md b/notes/unit_value_perf.md index d7d1043..54a099c 100644 --- a/notes/unit_value_perf.md +++ b/notes/unit_value_perf.md @@ -12,6 +12,13 @@ survey of places the trie pays for a value that carries no information. A fourth change -- packing the CoFree value flag into its child pointer -- was built and measured but **not merged**; see [below](#shelved-packing-the-value-presence-flag-into-the-child-pointer). +**A note on the survey these came from.** Its estimates were made by reading +struct definitions, and three of them did not survive measurement: S3 was not +implementable *and* worthless, S4 turned out to cost nothing to begin with, and +S1's headline ("2x node memory") was true per-node but meant 5% on +MORK-shaped data, because dense nodes are only a fifth of those bytes. Measure +first; the profiler in `9f73907` exists for that. + ## Results Min of 3 runs per side, each run itself the min of 7 timed repetitions, every @@ -238,9 +245,36 @@ here, so halving their per-slot box would have saved nothing. ## Negative results -These three were measured and rejected. Recording them so they are not +These four were measured and rejected. Recording them so they are not re-attempted from first principles. +### S4 -- replacing the dangling-path sentinel with a bit: nothing to save + +The earlier survey claimed that a path kept alive by `remove_val(prune = false)` +costs "a pointer + node per pruned leaf", because a `LineListNode` records it by +pointing the slot's child link at `TrieNodeODRc::new_empty()`. Measured, it costs +nothing at all: + +- **The sentinel never allocates.** `new_empty()` is a bogus address + (`0xBAADF00D`) carrying `EMPTY_NODE_TAG`. There is no node. +- **The payload word exists regardless.** A `LineListNode` is a fixed-size + struct, so the union the sentinel sits in is there whether the slot uses it or + not. Marking the slot "dangling" in the header instead would free zero bytes. +- **A `DenseByteNode` already does what S4 proposed.** It represents a dangling + path as a CoFree holding neither a child nor a value -- there is no sentinel in + a dense node to begin with. + +The measurement: taking shakespeare and removing half its values with +`prune = false` produces 14,212 dangling slots and leaves the trie **byte for +byte identical**, 4,095,744 before and after. `tests/memory_profile.rs` asserts +this so the claim does not have to be re-derived. + +Also checked, since it was the only caller that really allocates: a `WriteZipper` +opened on a path that does not exist allocates an empty `LineListNode` for its +root, but does **not** leave it behind -- creating 2000 such zippers and dropping +them without writing moved the byte count by exactly 0, and no allocated empty +node was ever observed in any trie profiled here. + ### S3 -- reclaiming value-slot bytes for list-node keys: not possible, and worthless The earlier survey claimed a `LineListNode` value slot's 8 bytes could go to key diff --git a/src/counters.rs b/src/counters.rs index f1471e5..e0dd865 100644 --- a/src/counters.rs +++ b/src/counters.rs @@ -277,6 +277,8 @@ pub struct MemProfile { pub both_slots: usize, pub key_bytes_used: usize, pub at_cap: usize, + pub empty_nodes: usize, + pub dangling_slots: usize, pub node_klen_hist: Vec, pub allval_nodes: usize, pub allval_klen_hist: Vec, @@ -295,6 +297,7 @@ impl MemProfile { if self.allval_klen_hist.len() < o.allval_klen_hist.len() { self.allval_klen_hist.resize(o.allval_klen_hist.len(), 0); } for (i, c) in o.allval_klen_hist.iter().enumerate() { self.allval_klen_hist[i] += c; } self.allval_nodes += o.allval_nodes; + self.empty_nodes += o.empty_nodes; self.dangling_slots += o.dangling_slots; self } pub fn total_bytes(&self) -> usize { self.list_bytes + self.dense_bytes + self.cell_bytes } @@ -332,6 +335,7 @@ impl MemProfile { self.dense_items as f64 / self.dense_nodes.max(1) as f64); println!(" dense slots: len {} cap {} ({:.1}% over-allocated)", self.dense_items, self.dense_cap, (self.dense_cap as f64/self.dense_items.max(1) as f64 - 1.0)*100.0); println!(" cell nodes {:>9} bytes {:>11} ({:4.1}% of trie) items {}", self.cell_nodes, self.cell_bytes, self.cell_bytes as f64/t*100.0, self.cell_items); + println!(" empty (allocated) nodes {} dangling sentinel slots {}", self.empty_nodes, self.dangling_slots); println!(" TOTAL bytes {:>9} = {:.1} bytes/value", self.total_bytes(), t / vals.max(1) as f64); } } @@ -348,8 +352,13 @@ pub fn memory_profile(map: &PathMap let Some(root) = map.root() else { return MemProfile::default() }; traverse_physical(root, move |node, ctx: MemProfile| { let mut c = ctx; + if node.item_count() == 0 { c.empty_nodes += 1; } if let Some(l) = node.as_list() { c.list_nodes += 1; c.list_bytes += list_sz; + //A slot whose child link is the empty sentinel is a dangling path: the path exists but + //carries no value and leads nowhere + if l.is_used_child_0() && unsafe{ l.child_in_slot::<0>() }.is_empty() { c.dangling_slots += 1 } + if l.is_used_child_1() && unsafe{ l.child_in_slot::<1>() }.is_empty() { c.dangling_slots += 1 } let (k0, k1) = l.get_both_keys(); if c.klen_hist.len() < crate::line_list_node::KEY_BYTES_CNT + 1 { c.klen_hist.resize(crate::line_list_node::KEY_BYTES_CNT + 1, 0); } c.klen_hist[k0.len()] += 1; diff --git a/src/line_list_node.rs b/src/line_list_node.rs index da45137..0dc1b7d 100644 --- a/src/line_list_node.rs +++ b/src/line_list_node.rs @@ -412,7 +412,7 @@ impl LineListNode { } } #[inline] - unsafe fn child_in_slot(&self) -> &TrieNodeODRc { + pub(crate) unsafe fn child_in_slot(&self) -> &TrieNodeODRc { match SLOT { 0 => unsafe{ &*self.val_or_child0.child }, 1 => unsafe{ &*self.val_or_child1.child }, diff --git a/tests/memory_profile.rs b/tests/memory_profile.rs index 3d610a8..ef6af91 100644 --- a/tests/memory_profile.rs +++ b/tests/memory_profile.rs @@ -40,6 +40,45 @@ fn survey(label: &str, paths: &[Vec]) { prof.total_bytes(), prof.list_nodes, prof.dense_nodes, build_ms, iter_ms, get_ms); } +/// Dangling paths: paths that exist but carry no value. `remove_val(prune=false)` is how they are +/// made, and a `LineListNode` records one by pointing the slot at the empty-node sentinel. +#[test] +fn dangling_path_survey() { + use pathmap::zipper::ZipperWriting; + let words: Vec> = std::fs::read_to_string("benches/shakespeare.txt").unwrap() + .split_ascii_whitespace().map(|w| w.as_bytes().to_vec()).collect::>() + .into_iter().collect(); + + let mut m: PathMap<()> = PathMap::new(); + for w in &words { m.set_val_at(&w[..], ()); } + let before = memory_profile(&m); + before.report("shakespeare, no dangling paths", m.val_count()); + + // strip every other value without pruning -- the maximal dangling-path case + for (i, w) in words.iter().enumerate() { + if i % 2 == 0 { + let mut wz = m.write_zipper_at_path(&w[..]); + wz.remove_val(false); + } + } + let after = memory_profile(&m); + after.report("shakespeare, half the values removed with prune=false", m.val_count()); + println!("DANGLING bytes {} -> {} empty nodes {} -> {} sentinel slots {} -> {}", + before.total_bytes(), after.total_bytes(), + before.empty_nodes, after.empty_nodes, before.dangling_slots, after.dangling_slots); + + //A dangling path costs nothing over the value it replaced. The empty-node sentinel is a bogus + //address rather than an allocation, and the payload word it sits in is part of a fixed-size + //`LineListNode` whether it is used or not -- so turning 14k values into dangling paths moves + //the byte count not at all. A `DenseByteNode` does not use the sentinel in the first place: it + //represents a dangling path as a CoFree holding neither a child nor a value. + assert!(after.dangling_slots > 10_000, "the fixture should have produced plenty of dangling paths"); + assert_eq!(after.total_bytes(), before.total_bytes(), + "dangling paths must cost nothing over the values they replaced"); + assert_eq!(after.empty_nodes, 0, "the sentinel must never become an allocated node"); + assert_eq!(before.empty_nodes, 0); +} + #[test] fn memory_profile_survey() { // MORK-representative From 8b91e87e0c8e2f9c826489fb716481863eb4e93c Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 19 Aug 2026 18:45:32 +0000 Subject: [PATCH 07/17] F3: there is nothing to elide in merkleize's value hashing The survey said merkleize "requires V: Hash to hash nothing", implying a const branch around the value hash. Both hash sites take `Option<&V>`, not `V`, and that distinction is the whole story: - `().hash()` writes zero bytes, so the payload already costs nothing and the call folds away. There is no branch to add. - `Option<&()>` writes 8 bytes, and None and Some(&()) hash differently. That is the discriminant, and for a set trie it is the only information present -- whether the path is a member. Eliding it would make a path with a value hash identically to one without, and merkleize would merge structurally distinct tries. The V: Hash bound cannot come off either: it is needed for a general V, and () satisfies it for free. The one adjacent remnant -- the "value, no child" branch building a fresh hasher per leaf to compute what is a constant for any zero-sized V -- was priced at 3.03ns x 91,692 leaves = 0.28ms against a 21.03ms merkleize, or 1.3%. That is below the noise floor, in the routine where a subtle change silently corrupts structural sharing. No code change; recording the measurements so this is not re-attempted. Co-Authored-By: Claude Opus 5 (1M context) --- notes/unit_value_perf.md | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/notes/unit_value_perf.md b/notes/unit_value_perf.md index 54a099c..b531fc8 100644 --- a/notes/unit_value_perf.md +++ b/notes/unit_value_perf.md @@ -245,9 +245,45 @@ here, so halving their per-slot box would have saved nothing. ## Negative results -These four were measured and rejected. Recording them so they are not +These five were measured and rejected. Recording them so they are not re-attempted from first principles. +### F3 -- eliding the value hash in `merkleize`: nothing to elide, and the obvious version is a bug + +The survey said `merkleize` "requires `V: Hash` to hash nothing". The natural +reading -- put a const branch around `val.hash(&mut hasher)` -- is wrong twice. + +Both sites hash `Option<&V>`, not `V`, and the `Option` is what carries the +information: + +| expression | bytes written | hash | +| --- | ---: | --- | +| `().hash(h)` | **0** | -- | +| `Option::<&()>::None` | 8 | `0` | +| `Option::<&()>::Some(&())` | 8 | `27512614111` | + +So the value payload *already* costs nothing at `()` -- `impl Hash for ()` writes +zero bytes and the call folds away. What the 8 bytes carry is the discriminant, +and for a set trie that discriminant is the only information there is: whether +the path is a member. Skipping it would make a path with a value hash identically +to one without, and `merkleize` would merge structurally distinct tries. That is +a correctness bug, not an optimization. + +The `V: Hash` bound cannot come off either -- it is needed for a general `V`, and +`()` implements `Hash`, so it costs nothing to satisfy. + +One adjacent remnant is real but too small to take: the "value, no child" branch +builds a fresh `GxHasher` per leaf value to compute what is, for any zero-sized +`V`, a constant. Priced directly: + + merkleize, big_logic.metta 21.03 ms for 91,692 values (229 ns/value) + merkleize, shakespeare 5.65 ms for 67,505 values ( 84 ns/value) + the per-leaf hasher 3.03 ns x 91,692 leaves = 0.28 ms + +0.28 ms of 21.03 ms is **1.3%** -- below this machine's measured noise floor, in +the one routine where a subtle change silently corrupts structural sharing. +Not worth it. + ### S4 -- replacing the dangling-path sentinel with a bit: nothing to save The earlier survey claimed that a path kept alive by `remove_val(prune = false)` From 305658318604e18269ee0317735c293a04d7661e Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 19 Aug 2026 18:52:13 +0000 Subject: [PATCH 08/17] S5: the payload union is sized by the child pointer, not by LocalOrHeap The survey said `LocalOrHeap` is why a payload slot is 8 bytes wide. It is not. `LocalOrHeap` is a fixed-size cell that boxes anything too large to fit, so the value arm is the same width for every V, and the union is 8 bytes whether V is `()` or `[u8; 1024]` -- because the child pointer arm needs 8 bytes regardless. Deleting the value arm would save nothing, and for `()` it already costs nothing at runtime. `val_slot_layout_tests` asserts both. What the investigation did turn up is real, though it is not a unit-value concern. Values live overwhelmingly in list-node slots -- 99.93% on MORK-shaped data, 100% on fixed-width random keys -- and those are exactly the slots that box. So for a value over the threshold, one heap allocation per value. Measured on 91,692 paths, going from an 8-byte value to a 9-byte one costs +19% on insertion and +36% on drop, entirely allocator traffic. Widening the cell would fix that only by growing every node for every V, including the ones that need none of it, and sizing it per-V needs generic_const_exprs. So the fix here is documentation: TrieValue now describes the cliff, since it is invisible in the type system and lands on whoever picks a value type. `val_is_boxed` gives the threshold one home, which `val_slot_needs_drop` now shares. The profiler gains a `dense_val_slots` counter, which is what made the list-vs-dense split measurable. Co-Authored-By: Claude Opus 5 (1M context) --- notes/unit_value_perf.md | 50 +++++++++++++++++++++++++++++++++++++++- src/counters.rs | 8 ++++++- src/dense_byte_node.rs | 3 +++ src/lib.rs | 14 +++++++++++ src/trie_node.rs | 40 +++++++++++++++++++++++++++++++- 5 files changed, 112 insertions(+), 3 deletions(-) diff --git a/notes/unit_value_perf.md b/notes/unit_value_perf.md index b531fc8..179f0a2 100644 --- a/notes/unit_value_perf.md +++ b/notes/unit_value_perf.md @@ -245,9 +245,57 @@ here, so halving their per-slot box would have saved nothing. ## Negative results -These five were measured and rejected. Recording them so they are not +These six were measured and rejected. Recording them so they are not re-attempted from first principles. +### S5 -- `LocalOrHeap` "pins the payload union at 8 bytes": it does not + +The survey said the `LocalOrHeap` arm is why a payload slot is 8 +bytes wide, and that a set-specialized node would have to replace it. Both halves +are wrong. + +`LocalOrHeap` is a *fixed-size cell* -- it boxes anything too big to fit -- so +the value arm is the same width for every `V`, and the union's width is set by +its **other** arm, the child pointer: + +| `V` | `size_of::()` | union | boxed? | +| --- | ---: | ---: | --- | +| `()` | 0 | 8 | no | +| `u64` | 8 | 8 | no | +| `String` | 24 | 8 | **yes** | +| `[u8; 1024]` | 1024 | 8 | **yes** | + +Deleting the value arm entirely would not shrink the union by a byte, because the +child pointer needs those 8 regardless. And for `()` the arm costs nothing at +runtime either: `size_of::<()>() == 0`, so nothing is boxed. +`val_slot_layout_tests` asserts both facts. + +**What it did surface, which is real but not a unit-value concern.** Values live +overwhelmingly in list-node slots, and those are the ones that box: + +| dataset | values in list slots | in dense slots | +| --- | ---: | ---: | +| `big_logic.metta` (MORK) | **99.93%** | 0.07% | +| 1M random 8-byte keys | **100.00%** | 0.00% | +| shakespeare words | 53.44% | 46.56% | + +So for a `V` over the threshold, MORK-shaped data pays roughly one heap +allocation *per value*. Priced on 91,692 paths: + +| value | size | boxed | build | drop | +| --- | ---: | --- | ---: | ---: | +| `u64` | 8 | no | 23.66 ms | 6.94 ms | +| `[u8; 8]` | 8 | no | 23.76 ms | 6.96 ms | +| `[u8; 9]` | 9 | **yes** | 28.22 ms (**+19%**) | 9.43 ms (**+36%**) | +| `[u8; 16]` | 16 | yes | 26.02 ms | 9.13 ms | + +One byte over the line costs ~19% on insertion and ~36% on drop. Widening the +cell would fix it, but only by making every node bigger for every `V` -- +including `()`, which needs none of it -- and sizing the cell per-`V` needs +`generic_const_exprs`. So the actionable part is documentation: the cliff is now +described on [`TrieValue`](../src/lib.rs), where someone choosing a value type +will see it. + ### F3 -- eliding the value hash in `merkleize`: nothing to elide, and the obvious version is a bug The survey said `merkleize` "requires `V: Hash` to hash nothing". The natural diff --git a/src/counters.rs b/src/counters.rs index e0dd865..fa80e14 100644 --- a/src/counters.rs +++ b/src/counters.rs @@ -279,6 +279,7 @@ pub struct MemProfile { pub at_cap: usize, pub empty_nodes: usize, pub dangling_slots: usize, + pub dense_val_slots: usize, pub node_klen_hist: Vec, pub allval_nodes: usize, pub allval_klen_hist: Vec, @@ -298,6 +299,7 @@ impl MemProfile { for (i, c) in o.allval_klen_hist.iter().enumerate() { self.allval_klen_hist[i] += c; } self.allval_nodes += o.allval_nodes; self.empty_nodes += o.empty_nodes; self.dangling_slots += o.dangling_slots; + self.dense_val_slots += o.dense_val_slots; self } pub fn total_bytes(&self) -> usize { self.list_bytes + self.dense_bytes + self.cell_bytes } @@ -336,6 +338,10 @@ impl MemProfile { println!(" dense slots: len {} cap {} ({:.1}% over-allocated)", self.dense_items, self.dense_cap, (self.dense_cap as f64/self.dense_items.max(1) as f64 - 1.0)*100.0); println!(" cell nodes {:>9} bytes {:>11} ({:4.1}% of trie) items {}", self.cell_nodes, self.cell_bytes, self.cell_bytes as f64/t*100.0, self.cell_items); println!(" empty (allocated) nodes {} dangling sentinel slots {}", self.empty_nodes, self.dangling_slots); + let vs = self.val_slots + self.dense_val_slots; + println!(" values by home: list-node slots {} ({:.2}%) dense-node slots {} ({:.2}%)", + self.val_slots, self.val_slots as f64 / vs.max(1) as f64 * 100.0, + self.dense_val_slots, self.dense_val_slots as f64 / vs.max(1) as f64 * 100.0); println!(" TOTAL bytes {:>9} = {:.1} bytes/value", self.total_bytes(), t / vals.max(1) as f64); } } @@ -374,7 +380,7 @@ pub fn memory_profile(map: &PathMap if l.is_used_value_1() { c.val_slots += 1 } else if l.is_used_child_1() { c.child_slots += 1 } } else if let Some(d) = node.as_dense() { - c.dense_nodes += 1; c.dense_items += d.slot_count(); c.dense_cap += d.slot_capacity(); c.dense_bytes += dense_sz + d.slot_capacity()*cf; + c.dense_nodes += 1; c.dense_items += d.slot_count(); c.dense_val_slots += d.val_slot_count(); c.dense_cap += d.slot_capacity(); c.dense_bytes += dense_sz + d.slot_capacity()*cf; } else if node.tag() == crate::trie_node::CELL_BYTE_NODE_TAG { let n = node.item_count(); // each CellCoFree additionally owns a boxed OrdinaryCoFree diff --git a/src/dense_byte_node.rs b/src/dense_byte_node.rs index 1ad9481..0d8d528 100644 --- a/src/dense_byte_node.rs +++ b/src/dense_byte_node.rs @@ -122,6 +122,9 @@ impl> ByteNode /// Number of allocated `CoFree` slots, including unused `Vec` capacity #[inline] pub fn slot_capacity(&self) -> usize { self.values.capacity() } + /// Number of slots holding a value + #[inline] + pub fn val_slot_count(&self) -> usize { self.values.iter().filter(|cf| cf.has_val()).count() } /// Number of occupied `CoFree` slots. Note this differs from [Self::item_count], which counts /// a slot's child link and value as two separate items #[inline] diff --git a/src/lib.rs b/src/lib.rs index 8df862b..44df25d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -143,6 +143,20 @@ mod bridge_node; mod old_cursor; /// A supertrait that encapsulates the bounds for a value that can be put in a [PathMap] +/// +/// # Value size, and a cliff at 8 bytes +/// +/// Most values in a trie live in a list node, whose payload slot is a fixed-size cell shared with +/// the child pointer. A `V` that fits in that cell is stored inline; a larger one is boxed +/// individually, one heap allocation per value. The threshold is the size of a node pointer, so +/// **8 bytes** on 64-bit targets with the default `slim_ptrs` feature. +/// +/// The cliff is sharp and does not show up in the type system. On a trie of ~92k paths where 99.9% +/// of values sit in list-node slots, going from an 8-byte value to a 9-byte one costs about 19% on +/// insertion and 36% on drop, entirely in allocator traffic. +/// +/// If your value is close to the limit it is worth keeping it under: pack it, shrink an +/// enum's discriminant, or store an index into a side table rather than the payload itself. pub trait TrieValue: Clone + Send + Sync + Unpin {} impl TrieValue for T where T : Clone + Send + Sync + Unpin {} diff --git a/src/trie_node.rs b/src/trie_node.rs index 915d878..35c0910 100644 --- a/src/trie_node.rs +++ b/src/trie_node.rs @@ -500,9 +500,16 @@ pub(crate) type ValSlotStorage = [u8; 16]; /// This depends only on `V`, so it folds to a constant at monomorphization and the branches it /// guards vanish. For a unit-valued trie (`PathMap<()>`), and for any other trie whose value is a /// small `Copy` type, that removes the value arms from the node drop paths entirely. +/// `true` if a `V` is too large to live inline in a [ValOrChildUnion], and so is boxed individually +/// by [LocalOrHeap] in every list-node slot that holds one +#[inline(always)] +pub(crate) const fn val_is_boxed() -> bool { + core::mem::size_of::() > core::mem::size_of::() +} + #[inline(always)] pub(crate) const fn val_slot_needs_drop() -> bool { - core::mem::needs_drop::() || core::mem::size_of::() > core::mem::size_of::() + core::mem::needs_drop::() || val_is_boxed::() } pub union ValOrChildUnion { @@ -3286,6 +3293,37 @@ impl DistributiveLat } } +#[cfg(test)] +mod val_slot_layout_tests { + use super::*; + use crate::alloc::GlobalAlloc; + + /// The payload union is the width of its *child* arm, not its value arm + /// + /// [LocalOrHeap] is a fixed-size cell -- it boxes any `V` too big to fit -- so the value arm is + /// the same width for every `V`, and the union is 8 bytes whether `V` is `()` or `[u8; 1024]`. + /// Removing the value arm entirely would not shrink it, because the child pointer needs those 8 + /// bytes regardless. + #[test] + fn payload_union_width_is_set_by_the_child_pointer() { + let child = core::mem::size_of::>(); + assert_eq!(core::mem::size_of::>(), child); + assert_eq!(core::mem::size_of::>(), child); + assert_eq!(core::mem::size_of::>(), child); + assert_eq!(core::mem::size_of::>(), child); + } + + /// Values larger than [ValSlotStorage] are boxed one at a time in list-node slots. This is a + /// sharp, silent cliff -- see the note on [`TrieValue`](crate::TrieValue) + #[test] + fn the_inline_value_budget_is_where_it_is_documented() { + assert!(core::mem::size_of::() >= 8); + assert!(!crate::trie_node::val_is_boxed::()); + assert!(crate::trie_node::val_is_boxed::<[u8; 16]>()); + assert!(!crate::trie_node::val_is_boxed::<()>()); + } +} + #[cfg(test)] mod val_slot_drop_tests { use super::{ValSlotStorage, val_slot_needs_drop}; From af247eb784d55c8b7bacd2bde60666b8802d04b0 Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 19 Aug 2026 19:04:59 +0000 Subject: [PATCH 09/17] F4: the merge machinery is ~4% of runtime; the allocator is ~30% The survey said the generic result plumbing -- AlgebraicResult, merge, combine_algebraic_results, the Hetero* traits -- is where a PathMap<()> spends its time. Profiled with perf on 400k-path joins, meets and subtracts, with cycles attributed by source line so inlined code lands on its own line, ring.rs accounts for 1.06% and combine_algebraic_results for 2.86%, against 29.72% in malloc. The proposed rewrite would have chased a twenty-fifth of the runtime, and depended on the shelved S1 besides. What the profile did find is worth more than F4 was. ByteNode::pjoin allocates the result slot vector and clones every slot into it before knowing whether the result is a new node or simply one of its operands. When the answer is Identity that work is discarded: on 400k-path joins, 99.7% of calls discard it when one operand is a subset of the other, and 100% at 95% overlap, wasting 94.8% and 99.6% of all slot clones respectively -- each an atomic refcount increment and a matching decrement, plus a Vec allocation and free per call. End to end this inverts the expected ordering: joining two disjoint 400k tries into a new 800k one takes 48.75ms, while joining a trie with a 95% overlapping one -- whose answer is essentially the first operand -- takes 74.96ms. The join with nothing to do is 1.5x slower than the one that doubles the trie. Recorded with the fix sketched (defer materialization until the first slot result that is neither SELF_IDENT nor COUNTER_IDENT, back-filling the prefix from whichever side was still identity). Not attempted; it is general rather than unit-value work. Co-Authored-By: Claude Opus 5 (1M context) --- notes/unit_value_perf.md | 63 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 62 insertions(+), 1 deletion(-) diff --git a/notes/unit_value_perf.md b/notes/unit_value_perf.md index 179f0a2..74d0078 100644 --- a/notes/unit_value_perf.md +++ b/notes/unit_value_perf.md @@ -245,9 +245,70 @@ here, so halving their per-slot box would have saved nothing. ## Negative results -These six were measured and rejected. Recording them so they are not +These seven were measured and rejected. Recording them so they are not re-attempted from first principles. +### F4 -- "the result-merging machinery is what actually costs": off by an order of magnitude + +The survey claimed the generic algebra's result plumbing -- `AlgebraicResult`, +`merge`, `combine_algebraic_results`, the `Hetero*` traits -- is where a +`PathMap<()>` spends its time, because the identity masks keep it from folding +away. Profiled with `perf` on 400k-path joins, meets and subtracts, attributing +cycles by source line so inlined code lands on its own line: + +| source | cycles | +| --- | ---: | +| `malloc.c` | **29.72%** | +| `dense_byte_node.rs` | 17.71% | +| `trie_node.rs` | 13.31% | +| `option.rs` | 3.82% | +| `atomic.rs` (refcounts) | 2.71% | +| `line_list_node.rs` | 2.48% | +| **`ring.rs`** -- all of `AlgebraicResult` and the `Lattice` impls | **1.06%** | + +`combine_algebraic_results` shows up separately at 2.86%. So the merging +machinery is roughly **4%**, against **30% in the allocator**. Rewriting it as +bit arithmetic -- which is what the survey proposed, and which depended on the +shelved S1 anyway -- would have chased about a twenty-fifth of the runtime. + +## The thing the F4 profile actually found + +Where the allocator time goes is worth its own section, because it is a real and +fixable inefficiency, and it is not unit-value-specific. + +`ByteNode::pjoin` allocates the result slot vector and clones every slot into it +*before* it knows whether the result is a new node or just one of its operands. +When the answer turns out to be `Identity`, all of that is discarded. Instrumented +on 400k-path joins: + +| operands | calls returning `Identity` | slot clones wasted | +| --- | ---: | ---: | +| disjoint | 0.0% | 0 of 789,291 (0.0%) | +| one is a subset of the other | **99.7%** | 159,078 of 167,800 (**94.8%**) | +| 95% overlap | **100.0%** | 449,799 of 451,600 (**99.6%**) | + +Each wasted slot clone is an atomic refcount increment plus a matching decrement +on drop, and each wasted call is a `Vec` allocation and free. The effect is +visible end to end, and it inverts what you would expect: + +| join | time | +| --- | ---: | +| disjoint operands -- builds a genuinely new 800k-path trie | 48.75 ms | +| one operand a subset -- result equals the larger operand | 42.64 ms | +| 95% overlap -- result is essentially the larger operand | **74.96 ms** | + +The join that has almost nothing to do takes **1.5x longer** than the one that +doubles the trie, because it does the full copy and then throws it away. Joining +a trie with something it already mostly contains is a common shape -- it is what +incremental ingestion looks like -- so this is worth fixing. + +The fix is to defer materializing the vector until the first slot result that is +neither `SELF_IDENT` nor `COUNTER_IDENT`. While both node-level identity flags +are alive, the result prefix is exactly one operand's slots, so it can be +back-filled at the point the flags die, using the side that was still identity. +Recursive sub-joins that returned `Identity` allocated nothing themselves, so the +saving is the whole per-node cost. Not attempted here. + ### S5 -- `LocalOrHeap` "pins the payload union at 8 bytes": it does not The survey said the `LocalOrHeap` arm is why a payload slot is 8 From bdd7a255a7077833baea80b7375d31e44ee782af Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 19 Aug 2026 19:24:31 +0000 Subject: [PATCH 10/17] F6: ValOrChild is already niche-packed; PayloadRef is not where the time goes The survey wanted both payload enums collapsed to a nullable pointer plus a bit. ValOrChild<()> is already one word: `Val(())` carries nothing, so rustc encodes it in the child pointer's null niche. Nothing to do, and a third variant would silently undo it -- hence a test. PayloadRef cannot use that niche, having two pointer-carrying variants plus None, so one word goes to the discriminant. But it is 16 bytes for every V, not just for (), and it is a transient built on the stack during meets and never stored in a trie, so its width costs memory nowhere. The path is hot, which is the part the survey got right: on list-node-heavy set algebra, node_get_payloads plus pmeet_generic and its inner loop are 17.7% of cycles. The cycles are not in the enum, though -- 4.38% is `is_used` header bit tests, 3.03% key-length extraction, 3.29% key-slice construction, and 1.97% the inner function's prologue spilling its arrays. Only that last part scales with payload width, and shrinking a two-element array from 48 to 32 bytes reclaims a fraction of it. Header decoding and key slicing is where that path actually spends its time, and is the thing to look at if it ever needs to be faster. Co-Authored-By: Claude Opus 5 (1M context) --- notes/unit_value_perf.md | 48 +++++++++++++++++++++++++++++++++++++++- src/trie_node.rs | 30 +++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/notes/unit_value_perf.md b/notes/unit_value_perf.md index 74d0078..0845ee8 100644 --- a/notes/unit_value_perf.md +++ b/notes/unit_value_perf.md @@ -245,9 +245,55 @@ here, so halving their per-slot box would have saved nothing. ## Negative results -These seven were measured and rejected. Recording them so they are not +These eight were measured and rejected. Recording them so they are not re-attempted from first principles. +### F6 -- shrinking the payload enums: one is already optimal, the other is not where the time goes + +The survey wanted `ValOrChild` and `PayloadRef` collapsed to "a nullable pointer +plus a bool -- one word". Measured: + +| `V` | `ValOrChild` | `PayloadRef` | +| --- | ---: | ---: | +| `()` | **8** | 16 | +| `u64` | 16 | 16 | +| `String` | 24 | 16 | + +**`ValOrChild<()>` is already one word.** `Val(())` carries nothing, so rustc +encodes it in the child pointer's null niche -- no discriminant, no padding. +There was nothing to do. + +**`PayloadRef` cannot use that niche**, because it has two pointer-carrying +variants plus `None`, so one word goes to the discriminant. But it is 16 bytes +for *every* `V`, so it is not a unit-value issue, and it is a transient -- built +on the stack during meets, never stored in a trie -- so its width costs memory +nowhere. `payload_layout_tests` pins both facts, since a third variant on +`ValOrChild` would silently double it. + +The path *is* hot, which is the one place the survey was right. On a +list-node-heavy set-algebra workload (long shared prefixes, meet/subtract/restrict): + + node_get_payloads (LineListNode) 8.09% + pmeet_generic 5.75% + pmeet_generic_internal 3.54% + ------ + 17.7% + +But the cycles inside it are not the enum: + +| line | what it is | cycles | +| --- | --- | ---: | +| `line_list_node.rs:248/249` | `is_used::` header bit tests | 4.38% | +| `:391`, `:396` | key-length extraction from the header | 3.03% | +| `:406`, `:551`, `:556` | building key slices, `get_both_keys` | 3.29% | +| `trie_node.rs:620` | `pmeet_generic_internal` prologue, i.e. array spills | 1.97% | + +Only the last line is attributable to payload width at all, and shrinking a +two-element `(usize, PayloadRef)` array from 48 bytes to 32 would reclaim a +fraction of it. The rest is header decoding and key slicing -- inherent work for +a node that packs two variable-length keys into a 16-bit header, and the thing to +look at if this path ever needs to be faster. + ### F4 -- "the result-merging machinery is what actually costs": off by an order of magnitude The survey claimed the generic algebra's result plumbing -- `AlgebraicResult`, diff --git a/src/trie_node.rs b/src/trie_node.rs index 35c0910..9261193 100644 --- a/src/trie_node.rs +++ b/src/trie_node.rs @@ -3293,6 +3293,36 @@ impl DistributiveLat } } +#[cfg(test)] +mod payload_layout_tests { + use super::*; + use crate::alloc::GlobalAlloc; + + /// [ValOrChild] is already as small as it can be, courtesy of the niche in the child pointer + /// + /// `Val(())` carries nothing, so rustc encodes it in the child pointer's null niche and the + /// whole enum is one word -- no discriminant, no padding. Adding a third variant, or any field + /// that defeats the niche, would silently double it for every unit-valued trie. + #[test] + fn val_or_child_is_niche_packed_for_a_unit_value() { + assert_eq!( + core::mem::size_of::>(), + core::mem::size_of::>(), + "ValOrChild<()> should fit in the child pointer's niche" + ); + } + + /// [PayloadRef] cannot use that niche: it has *two* pointer-carrying variants plus `None`, so + /// one word goes to the discriminant regardless of `V`. It is a transient -- built on the + /// stack during meets and never stored in a trie -- so its width costs memory nowhere. + #[test] + fn payload_ref_costs_a_discriminant_word_for_every_value_type() { + let w = core::mem::size_of::<&'static TrieNodeODRc<(), GlobalAlloc>>(); + assert_eq!(core::mem::size_of::>(), 2 * w); + assert_eq!(core::mem::size_of::>(), 2 * w); + } +} + #[cfg(test)] mod val_slot_layout_tests { use super::*; From 67004f8fcd8a6d30b4f6811724c26d4a6963c807 Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 19 Aug 2026 19:51:00 +0000 Subject: [PATCH 11/17] Don't clone a dense node until a subtraction actually changes it `ByteNode::psubtract` opened with `let mut btn = self.clone()` -- the whole node including its slot array, with a refcount bump per slot -- before it knew whether the subtraction would remove anything. When the answer came back `Identity`, all of that was discarded. That is the common case, not the corner case. Instrumented on 400k-path subtractions: disjoint operands 100.0% of calls return Identity; 431,714 of 431,714 slot clones wasted (100%) 5% overlap 71.2% return Identity; 251,481 of 432,028 wasted (58.2%) a 25% subset 0.0% return Identity; nothing wasted `btn` is now an `Option`, left `None` while the result is still identical to `self` and cloned at the first actual change. Indexing stays correct because the clone is taken before any modification, so `btn` still matches `self` everywhere the loop has already been. Min of 3 runs, 400k paths: subtract, disjoint 30.04ms -> 17.18ms -42.8% subtract, 5% overlap 34.87ms -> 25.18ms -27.8% subtract, 25% subset 40.43ms -> 39.20ms -3.0% subtract, 95% subset 69.96ms -> 69.44ms -0.7% The last two are cases with nothing to reclaim, and land inside the noise floor; `meet`, whose code is untouched, moved -8.9% to +0.1% across the same operands, which is that floor. This is the pattern A1 complained about, minus the mask arithmetic it proposed: the descent is already narrowed to the intersection of the two masks in all four operations, and the value dimension cannot become mask ops without the shelved slot split. `pjoin` and `prestrict` waste work the same way but build a fresh slot vector rather than cloning, so deferring them needs a back-fill; not attempted here. Verified with the differential algebra test and miri: 4 + 5 + 5 integration tests and 76 filtered lib tests across subtract, restrict, meet and join, all clean. Co-Authored-By: Claude Opus 5 (1M context) --- src/dense_byte_node.rs | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/src/dense_byte_node.rs b/src/dense_byte_node.rs index 0d8d528..073b145 100644 --- a/src/dense_byte_node.rs +++ b/src/dense_byte_node.rs @@ -15,6 +15,7 @@ use crate::line_list_node::LineListNode; //NOTE: This: `core::array::from_fn(|i| i as u8);` ought to work, but https://github.com/rust-lang/rust/issues/109341 const ALL_BYTES: [u8; 256] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135, 136, 137, 138, 139, 140, 141, 142, 143, 144, 145, 146, 147, 148, 149, 150, 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, 161, 162, 163, 164, 165, 166, 167, 168, 169, 170, 171, 172, 173, 174, 175, 176, 177, 178, 179, 180, 181, 182, 183, 184, 185, 186, 187, 188, 189, 190, 191, 192, 193, 194, 195, 196, 197, 198, 199, 200, 201, 202, 203, 204, 205, 206, 207, 208, 209, 210, 211, 212, 213, 214, 215, 216, 217, 218, 219, 220, 221, 222, 223, 224, 225, 226, 227, 228, 229, 230, 231, 232, 233, 234, 235, 236, 237, 238, 239, 240, 241, 242, 243, 244, 245, 246, 247, 248, 249, 250, 251, 252, 253, 254, 255]; + /// A ByteNode with insides that **cannot** be shared across threads pub type DenseByteNode = ByteNode, A>; @@ -2294,8 +2295,11 @@ impl, Other // `other` be differently parameterized types impl> ByteNode { fn psubtract>(&self, other: &ByteNode) -> AlgebraicResult where Self: Sized { - let mut is_identity = true; - let mut btn = self.clone(); + //`btn` stays `None` for as long as the result is still identical to `self`, so a subtraction + //that removes nothing from this node -- the common case when the operands barely overlap -- + //never clones it. The eager version cloned every node it visited, including its whole slot + //array with a refcount bump per slot, and then discarded the lot on the `Identity` return. + let mut btn: Option = None; for i in 0..4 { let mut lm = self.mask.0[i]; @@ -2303,19 +2307,21 @@ impl { - is_identity = false; - btn.remove(64*(i as u8) + (index as u8)); + btn.get_or_insert_with(|| self.clone()).remove(byte); }, AlgebraicResult::Identity(mask) => { debug_assert_eq!(mask, SELF_IDENT); //subtract is non-commutative }, AlgebraicResult::Element(jv) => { - is_identity = false; - let dst = unsafe { btn.get_unchecked_mut(64*(i as u8) + (index as u8)) }; + //NOTE: the clone is taken at the first change, so `btn` still matches + //`self` everywhere the loop has already been, and indexing it by byte + //stays correct even after a `remove` has shifted its slots + let dst = unsafe { btn.get_or_insert_with(|| self.clone()).get_unchecked_mut(byte) }; *dst = jv; }, } @@ -2325,14 +2331,19 @@ impl if self.is_empty() { + AlgebraicResult::None + } else { AlgebraicResult::Identity(SELF_IDENT) + }, + Some(btn) => if btn.is_empty() { + AlgebraicResult::None } else { AlgebraicResult::Element(btn) - } + }, } } } From 156bdf5e4dd09b46bfa675ce8aab53cd8c447151 Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 19 Aug 2026 19:51:18 +0000 Subject: [PATCH 12/17] Record the A1 result Two thirds of A1 was already done or blocked: the descent is already narrowed to the mask intersection in all four operations, and the value dimension needs the shelved slot split. Its aside about psubtract's speculative clone was the whole prize -- 42.8% off a disjoint subtract, 27.8% at 5% overlap, neutral where there is nothing to reclaim. Co-Authored-By: Claude Opus 5 (1M context) --- notes/unit_value_perf.md | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/notes/unit_value_perf.md b/notes/unit_value_perf.md index 0845ee8..1d45042 100644 --- a/notes/unit_value_perf.md +++ b/notes/unit_value_perf.md @@ -8,6 +8,7 @@ survey of places the trie pays for a value that carries no information. | `218a256` | Skip value-drop work in `LineListNode` when `V` has none to do | | `18d40ec` | Honor `Lattice::IDEMPOTENT` in the node algebra, and short-circuit `join_into` | | `9f73907` | Memory attribution by node type under the `counters` feature | +| `d75b0fb` | Don't clone a dense node until a subtraction actually changes it (**A1**) | A fourth change -- packing the CoFree value flag into its child pointer -- was built and measured but **not merged**; see [below](#shelved-packing-the-value-presence-flag-into-the-child-pointer). @@ -317,6 +318,45 @@ machinery is roughly **4%**, against **30% in the allocator**. Rewriting it as bit arithmetic -- which is what the survey proposed, and which depended on the shelved S1 anyway -- would have chased about a twenty-fifth of the runtime. +## A1 -- landed, as the fix the profile pointed at rather than the one proposed + +A1 proposed rewriting the four dense-node operations as mask arithmetic. Checked +against the code, two thirds of it was either impossible or already done: + +- **The narrowed descent already exists.** `pmeet` and `prestrict` iterate + `self.mask & other.mask`; `pjoin` iterates the union but only *recurses* on the + intersection; `psubtract` only recurses where `other` has the bit. +- **The value dimension cannot become mask ops** without the per-node `val_mask` + from S1, which is shelved. + +What was left was A1's aside about `psubtract` opening with `self.clone()` before +knowing the answer -- and that turned out to be the whole prize, and needs no mask +split. Instrumented on 400k-path subtractions: + +| operands | calls returning `Identity` | slot clones wasted | +| --- | ---: | ---: | +| disjoint | **100.0%** | 431,714 of 431,714 (**100%**) | +| 5% overlap | 71.2% | 251,481 of 432,028 (58.2%) | +| a 25% subset | 0.0% | nothing to waste | + +`btn` is now an `Option`, left `None` while the result is still identical to +`self` and cloned at the first real change. Min of 3 runs, 400k paths: + +| subtract | before | after | change | +| --- | ---: | ---: | ---: | +| disjoint operands | 30.04 ms | 17.18 ms | **-42.8%** | +| 5% overlap | 34.87 ms | 25.18 ms | **-27.8%** | +| a 25% subset | 40.43 ms | 39.20 ms | -3.0% | +| a 95% subset | 69.96 ms | 69.44 ms | -0.7% | + +The last two have nothing to reclaim and sit in the noise floor. `meet`, whose +code is untouched, moved -8.9% to +0.1% across the same operands -- that is the +floor. Verified with the differential algebra test and miri. + +`pjoin` and `prestrict` waste work identically but build a fresh slot vector +instead of cloning, so deferring them needs a back-fill of the prefix from +whichever operand was still identity. Not attempted. + ## The thing the F4 profile actually found Where the allocator time goes is worth its own section, because it is a real and From ad675841d3946bcda3bb0c6e399762098935afde Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 19 Aug 2026 20:16:23 +0000 Subject: [PATCH 13/17] Deferring the slot vector does not pay for pjoin or prestrict; and a restrict bug Both were implemented the same way psubtract was -- hold off building the slot vector until the first result that is neither SELF_IDENT nor COUNTER_IDENT, and back-fill the prefix from whichever operand was still identity, which the flags make recoverable since a live flag implies that operand's mask equals the result mask. Correct, and reverted anyway. Interleaved, min of 4 rounds per side, with meet and subtract as untouched controls, join went +10.6% on disjoint operands and -13.4% on a 25% subset. Swapping Vec::push for an unchecked write into the reserved slot cut the disjoint regression to +3.7% but did not grow the wins. prestrict landed inside the noise in both directions across two runs. psubtract pays because it cloned the whole node up front and barely recurses on disjoint operands, so the clone was nearly the whole call. These two only skip the slot vector, and the recursive descent -- which still visits every overlapping byte -- dominates. F4 measured 99.6% of pjoin's slot clones wasted at 95% overlap, and removing all of them bought about 6%. Separately: writing a differential oracle for restrict turned up a correctness bug. restrict(a, a) should be a, and is not when a path both carries a value and branches -- {ab, abc, abd} loses `ab`, {a, ab, abc} loses `a`. One child is fine, two or more and the value goes. It reproduces on master, so it predates this work; the existing differential tests cover join, meet and subtract but not restrict. The oracle and the minimal repro are committed, #[ignore]d so the suite stays green. Co-Authored-By: Claude Opus 5 (1M context) --- notes/unit_value_perf.md | 63 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 3 deletions(-) diff --git a/notes/unit_value_perf.md b/notes/unit_value_perf.md index 1d45042..d7d6c07 100644 --- a/notes/unit_value_perf.md +++ b/notes/unit_value_perf.md @@ -353,9 +353,66 @@ The last two have nothing to reclaim and sit in the noise floor. `meet`, whose code is untouched, moved -8.9% to +0.1% across the same operands -- that is the floor. Verified with the differential algebra test and miri. -`pjoin` and `prestrict` waste work identically but build a fresh slot vector -instead of cloning, so deferring them needs a back-fill of the prefix from -whichever operand was still identity. Not attempted. +### The same fix does not pay for `pjoin` or `prestrict` + +Both were implemented -- deferring the slot vector until the first result that is +neither `SELF_IDENT` nor `COUNTER_IDENT`, back-filling the prefix from whichever +operand was still identity, which the flags make recoverable because a live flag +implies that operand's mask equals the result mask, so the slots line up by index. +Correct, and reverted anyway. + +Interleaved runs, min of 4 per side, with `meet` and `subtract` as untouched +controls: + +| join | before | after | | +| --- | ---: | ---: | --- | +| disjoint | 46.28 ms | 51.19 ms | **+10.6%** | +| 5% overlap | 49.25 ms | 52.25 ms | +6.1% | +| 25% subset | 40.33 ms | 34.91 ms | **-13.4%** | +| 95% subset | 60.90 ms | 57.13 ms | -6.2% | +| identical | 61.43 ms | 59.67 ms | -2.9% | + +Replacing `Vec::push` with an unchecked write into the reserved slot brought the +disjoint regression from +10.6% to +3.7%, but did not grow the wins. `prestrict` +landed inside the noise in both directions across two runs. + +**Why it pays for `psubtract` and not these two.** `psubtract` cloned the *whole +node* up front, and on disjoint operands it barely recurses -- few bytes are in +both masks -- so the clone was nearly the entire cost of the call and removing it +took 42% off. `pjoin` and `prestrict` only skip the *slot vector*; the recursive +descent still happens on every overlapping byte and dominates. F4 measured 99.6% +of `pjoin`'s slot clones as wasted at 95% overlap, and eliminating all of them +bought ~6%, which is where the recursion leaves room. + +A measurement note worth keeping: a first pass showed everything regressing 11-24% +*including the two controls*, which share their code with the baseline exactly. +That was the machine drifting between the two measurement windows. Any comparison +here has to interleave the two sides and carry an untouched control, or it will +measure the room temperature. + +## A correctness bug in `restrict`, found while testing the above + +`restrict(a, a)` should be `a` for any `a`: every path of `a` has a value at +itself in `a`, and the `CoFree` rule keeps everything below a value in the other +operand. It does not hold when a path both carries a value and branches: + +| input | `restrict(a, a)` | dropped | +| --- | --- | --- | +| `{ab, abc}` | `{ab, abc}` | -- | +| `{ab, abc, abd}` | `{abc, abd}` | **`ab`** | +| `{a, ab, abc}` | `{ab, abc}` | **`a`** | +| `{ab, abc, abd, abe, abf}` | 4 of 5 | **`ab`** | + +One child is fine; two or more and the value is silently dropped. This is not +from any change in this branch -- it reproduces identically on `master` +(`8b8802a`). It went unnoticed because the existing differential tests cover +join, meet and subtract but not restrict, which reaches `Identity` down a +different route: a dense node can only answer `self` when *both* operand masks +equal their intersection. + +`tests/pathmap_algebra_differential.rs` carries the repro and a `BTreeSet` oracle +for `restrict`, `#[ignore]`d so it does not fail the suite. Run it with +`cargo test -- --ignored`. ## The thing the F4 profile actually found From 6a9e3f8316d02c445eb16fa189fb752712b5a5fa Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 19 Aug 2026 20:40:17 +0000 Subject: [PATCH 14/17] Record the restrict fix Cause, cost, and why the bug survived: restrict was the only one of the four algebraic operations without a differential test. A standalone repro lives on branch restrict-branching-value-repro, off master. Co-Authored-By: Claude Opus 5 (1M context) --- notes/unit_value_perf.md | 42 ++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/notes/unit_value_perf.md b/notes/unit_value_perf.md index d7d6c07..b9c8936 100644 --- a/notes/unit_value_perf.md +++ b/notes/unit_value_perf.md @@ -9,6 +9,7 @@ survey of places the trie pays for a value that carries no information. | `18d40ec` | Honor `Lattice::IDEMPOTENT` in the node algebra, and short-circuit `join_into` | | `9f73907` | Memory attribution by node type under the `counters` feature | | `d75b0fb` | Don't clone a dense node until a subtraction actually changes it (**A1**) | +| `424ea5e` | Fix `restrict` dropping the value on a path that branches (**correctness**) | A fourth change -- packing the CoFree value flag into its child pointer -- was built and measured but **not merged**; see [below](#shelved-packing-the-value-presence-flag-into-the-child-pointer). @@ -390,11 +391,11 @@ That was the machine drifting between the two measurement windows. Any compariso here has to interleave the two sides and carry an untouched control, or it will measure the room temperature. -## A correctness bug in `restrict`, found while testing the above +## A correctness bug in `restrict`, found while testing the above -- fixed -`restrict(a, a)` should be `a` for any `a`: every path of `a` has a value at -itself in `a`, and the `CoFree` rule keeps everything below a value in the other -operand. It does not hold when a path both carries a value and branches: +`restrict(a, a)` must be `a` for any `a`: every path of `a` has a value at itself +in `a`, and the rule is to keep everything at or below a value in the other +operand. It did not hold when a path both carried a value and branched: | input | `restrict(a, a)` | dropped | | --- | --- | --- | @@ -403,16 +404,29 @@ operand. It does not hold when a path both carries a value and branches: | `{a, ab, abc}` | `{ab, abc}` | **`a`** | | `{ab, abc, abd, abe, abf}` | 4 of 5 | **`ab`** | -One child is fine; two or more and the value is silently dropped. This is not -from any change in this branch -- it reproduces identically on `master` -(`8b8802a`). It went unnoticed because the existing differential tests cover -join, meet and subtract but not restrict, which reaches `Identity` down a -different route: a dense node can only answer `self` when *both* operand masks -equal their intersection. - -`tests/pathmap_algebra_differential.rs` carries the repro and a `BTreeSet` oracle -for `restrict`, `#[ignore]`d so it does not fail the suite. Run it with -`cargo test -- --ignored`. +One child was fine; two or more and the value was silently dropped. It predates +this branch -- it reproduced identically on `master` (`8b8802a`). + +**The cause.** `follow_path_to_value` walked the other operand following onward +links, and only looked for a value once the walk ran out of them. A node can hold +a value *and* a child under the same key -- a path that both ends there and +continues -- and in that case the walk took the link and reported "no value, path +continues", so `restrict_slot_contents` discarded the value. The check now happens +at every node before the link is taken, which also covers a value sitting at a +prefix of the key being followed. Fixed in `424ea5e`. + +**Cost:** none measurable. Interleaved against the untouched `meet` as a control, +restrict moves -1.5% to +0.6% across five operand shapes; the control moves -1.4% +to +1.6%. + +**Why it survived.** `restrict` was the only one of the four algebraic operations +without a differential test. It now has a `BTreeSet` oracle -- a path of `self` +survives when some prefix of it, the empty prefix and the path itself included, +carries a value in `other` -- run over prefix-free and prefix-heavy sets and the +minimal shapes above. + +A standalone repro lives on branch `restrict-branching-value-repro`, branched +from `master`: three tests, two of which fail there and all of which pass here. ## The thing the F4 profile actually found From 2398bf7ce669bed7e196d632a590cad9018e265e Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 19 Aug 2026 20:59:36 +0000 Subject: [PATCH 15/17] Join a dense node in place when the other side brings no new bytes `ByteNode::join_into` allocated a slot array sized to the union of the two masks and moved every slot into it, even when the union equals `self.mask` -- when `other` contributes no byte `self` does not already have. In that case nothing needs inserting and the overlapping slots can be joined where they sit. That case is the norm for an incremental ingest. Instrumented on a 400k-path trie joined with batches of varying overlap: disjoint 0.3% of calls, 5.2% of slot moves avoidable batch, 1% new keys 95.3% of calls, 95.3% of slot moves avoidable already contained (5%) 100% of calls, 100% avoidable already contained (95%) 100% of calls, 451,600 slot moves avoidable The time it buys is modest, because these are slot *moves* -- a memcpy of the CoFrees -- rather than the clones that made the same fix worth 42.8% in `psubtract`, where each clone was an atomic refcount bump. What is avoided here is one allocation and one memcpy per node. Interleaved, min of 8 rounds, with `join` as an untouched control: join_into, disjoint 61.55ms -> 61.81ms +0.4% join_into, 1% new keys 2.37ms -> 2.21ms -6.8% join_into, contained (5%) 12.81ms -> 12.51ms -2.3% join_into, contained (95%) 80.42ms -> 76.54ms -4.8% join (control), all four -0.8% to +0.5% The direction is consistent across all four shapes with the control flat, and there is no regression on the disjoint case, which is the one the new branch cannot help. The 1% figure has a 22-43% run-to-run spread on a 2ms measurement, so read it as "no worse"; the 95% case, at 8% spread, is the reliable one. Verified with the differential algebra tests and miri. Co-Authored-By: Claude Opus 5 (1M context) --- src/dense_byte_node.rs | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/src/dense_byte_node.rs b/src/dense_byte_node.rs index 073b145..ab9a4d8 100644 --- a/src/dense_byte_node.rs +++ b/src/dense_byte_node.rs @@ -16,6 +16,7 @@ use crate::line_list_node::LineListNode; const ALL_BYTES: [u8; 256] = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127, 128, 129, 130, 131, 132, 133, 134, 135, 136, 137, 138, 139, 140, 141, 142, 143, 144, 145, 146, 147, 148, 149, 150, 151, 152, 153, 154, 155, 156, 157, 158, 159, 160, 161, 162, 163, 164, 165, 166, 167, 168, 169, 170, 171, 172, 173, 174, 175, 176, 177, 178, 179, 180, 181, 182, 183, 184, 185, 186, 187, 188, 189, 190, 191, 192, 193, 194, 195, 196, 197, 198, 199, 200, 201, 202, 203, 204, 205, 206, 207, 208, 209, 210, 211, 212, 213, 214, 215, 216, 217, 218, 219, 220, 221, 222, 223, 224, 225, 226, 227, 228, 229, 230, 231, 232, 233, 234, 235, 236, 237, 238, 239, 240, 241, 242, 243, 244, 245, 246, 247, 248, 249, 250, 251, 252, 253, 254, 255]; + /// A ByteNode with insides that **cannot** be shared across threads pub type DenseByteNode = ByteNode, A>; @@ -2110,6 +2111,46 @@ impl, Other let mut is_identity = self.mask == jm; + //When `other` brings no new bytes the result mask is `self.mask`, so nothing has to be + //inserted and the slots can be joined where they sit. The general path below allocates a + //fresh slot array and moves every slot into it, which for an incremental ingest -- a trie + //joined with a batch of paths it mostly already covers -- is the entire cost of the call. + if jm == self.mask { + let mut is_identity = true; + let mut l = 0; + let mut r = 0; + for i in 0..4 { + let mut lm = self.mask.0[i]; + let om = other.mask.0[i]; + while lm != 0 { + let index = lm.trailing_zeros(); + if (om >> index) & 1 != 0 { + //SAFETY: `other`'s slots are all consumed here, because its mask is a + // subset of `self`'s, and its length is zeroed below so they are not + // dropped a second time + let rv = unsafe { std::ptr::read(other.values.get_unchecked(r)) }; + let lv = unsafe { self.values.get_unchecked_mut(l) }; + match lv.join_into(rv) { + AlgebraicStatus::Identity => { }, + AlgebraicStatus::Element => { is_identity = false; }, + AlgebraicStatus::None => unreachable!(), //Some.join(Some) shouldn't create None + } + r += 1; + } + l += 1; + lm ^= 1u64 << index; + } + } + unsafe { other.values.set_len(0) } + return if self.mask.is_empty_mask() { + AlgebraicStatus::None + } else if is_identity { + AlgebraicStatus::Identity + } else { + AlgebraicStatus::Element + } + } + let jmc = [jm.0[0].count_ones(), jm.0[1].count_ones(), jm.0[2].count_ones(), jm.0[3].count_ones()]; let l = (jmc[0] + jmc[1] + jmc[2] + jmc[3]) as usize; From e6250847c7cd4523919dbc208c98db08615b7a97 Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 19 Aug 2026 20:59:53 +0000 Subject: [PATCH 16/17] Record the A5 result In-place join_into when the other operand brings no new bytes: 95-100% of calls on incremental-ingest shapes, -4.8% on the most reliable case, no regression on disjoint operands. Modest because these are slot moves rather than the clones that made the same idea worth 42.8% in psubtract. Co-Authored-By: Claude Opus 5 (1M context) --- notes/unit_value_perf.md | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/notes/unit_value_perf.md b/notes/unit_value_perf.md index b9c8936..9d4753f 100644 --- a/notes/unit_value_perf.md +++ b/notes/unit_value_perf.md @@ -10,6 +10,7 @@ survey of places the trie pays for a value that carries no information. | `9f73907` | Memory attribution by node type under the `counters` feature | | `d75b0fb` | Don't clone a dense node until a subtraction actually changes it (**A1**) | | `424ea5e` | Fix `restrict` dropping the value on a path that branches (**correctness**) | +| `8440428` | Join a dense node in place when the other side brings no new bytes (**A5**) | A fourth change -- packing the CoFree value flag into its child pointer -- was built and measured but **not merged**; see [below](#shelved-packing-the-value-presence-flag-into-the-child-pointer). @@ -354,6 +355,43 @@ The last two have nothing to reclaim and sit in the noise floor. `meet`, whose code is untouched, moved -8.9% to +0.1% across the same operands -- that is the floor. Verified with the differential algebra test and miri. +### A5 -- `join_into` in place when the other side brings no new bytes + +`ByteNode::join_into` allocated a slot array sized to the union of the two masks +and moved every slot into it, even when the union equals `self.mask`. Then nothing +needs inserting and the overlapping slots can be joined where they sit. A5 +proposed this behind the shelved val-mask; it needs no such thing. + +The case is the norm for an incremental ingest -- a trie joined with a batch of +paths it mostly already covers: + +| operands | calls where other's mask is a subset | slot moves avoidable | +| --- | ---: | ---: | +| disjoint | 0.3% | 5.2% | +| batch, 1% new keys | **95.3%** | **95.3%** | +| already contained (5%) | **100%** | 100% | +| already contained (95%) | **100%** | 451,600 | + +Interleaved, min of 8 rounds, `join` as an untouched control: + +| join_into | before | after | | +| --- | ---: | ---: | --- | +| disjoint | 61.55 ms | 61.81 ms | +0.4% | +| 1% new keys | 2.37 ms | 2.21 ms | -6.8% | +| contained (5%) | 12.81 ms | 12.51 ms | -2.3% | +| contained (95%) | 80.42 ms | 76.54 ms | **-4.8%** | +| *join (control)* | | | *-0.8% to +0.5%* | + +Consistent in direction across all four shapes with the control flat, and no +regression on the disjoint case the branch cannot help. The 1% figure sits on a +2 ms measurement with 22-43% spread -- read it as "no worse"; the 95% case, at 8% +spread, is the one to believe. + +Modest, and the reason is worth keeping: these are slot **moves**, a memcpy of the +`CoFree`s, not the **clones** that made the same idea worth 42.8% in `psubtract` +where each was an atomic refcount bump. What A5 avoids is one allocation and one +memcpy per node. + ### The same fix does not pay for `pjoin` or `prestrict` Both were implemented -- deferring the slot vector until the first result that is From de7116b6391721ae7ecedc0b27e877625021768c Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 19 Aug 2026 21:24:17 +0000 Subject: [PATCH 17/17] Point the notes at the rebased commits The branch was rebased onto master now that the restrict fix has landed there (b2a0c09), so every commit hash in the notes had moved. The restrict fix drops out of the branch's own table -- it is upstream, and git dropped the duplicate during the rebase as already applied. Co-Authored-By: Claude Opus 5 (1M context) --- notes/unit_value_perf.md | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/notes/unit_value_perf.md b/notes/unit_value_perf.md index 9d4753f..0674ccf 100644 --- a/notes/unit_value_perf.md +++ b/notes/unit_value_perf.md @@ -5,12 +5,11 @@ survey of places the trie pays for a value that carries no information. | commit | change | | --- | --- | -| `218a256` | Skip value-drop work in `LineListNode` when `V` has none to do | -| `18d40ec` | Honor `Lattice::IDEMPOTENT` in the node algebra, and short-circuit `join_into` | -| `9f73907` | Memory attribution by node type under the `counters` feature | -| `d75b0fb` | Don't clone a dense node until a subtraction actually changes it (**A1**) | -| `424ea5e` | Fix `restrict` dropping the value on a path that branches (**correctness**) | -| `8440428` | Join a dense node in place when the other side brings no new bytes (**A5**) | +| `e2755ed` | Skip value-drop work in `LineListNode` when `V` has none to do | +| `12edc03` | Honor `Lattice::IDEMPOTENT` in the node algebra, and short-circuit `join_into` | +| `f480236` | Memory attribution by node type under the `counters` feature | +| `67004f8` | Don't clone a dense node until a subtraction actually changes it (**A1**) | +| `2398bf7` | Join a dense node in place when the other side brings no new bytes (**A5**) | A fourth change -- packing the CoFree value flag into its child pointer -- was built and measured but **not merged**; see [below](#shelved-packing-the-value-presence-flag-into-the-child-pointer). @@ -20,7 +19,7 @@ struct definitions, and three of them did not survive measurement: S3 was not implementable *and* worthless, S4 turned out to cost nothing to begin with, and S1's headline ("2x node memory") was true per-node but meant 5% on MORK-shaped data, because dense nodes are only a fifth of those bytes. Measure -first; the profiler in `9f73907` exists for that. +first; the profiler in `f480236` exists for that. ## Results @@ -97,7 +96,7 @@ threshold cannot drift away from the layout. `cargo miri test` covers the ### `IDEMPOTENT`: a correctness fix, not a speedup -The gating half of `18d40ec` costs and saves nothing at `()`, because +The gating half of `12edc03` costs and saves nothing at `()`, because `IDEMPOTENT` is `true` there and folds away. Its value is that the constant now means something. Six sites take the shared-subtrie shortcut — not the three that grepping for `ptr_eq` finds, but also the `TaggedNodeRef` dispatchers underneath, @@ -129,7 +128,7 @@ harness is in the tree -- `cargo test --release --features counters,serializatio ## Where the memory actually is -`memory_profile` (`9f73907`) walks physical nodes and attributes bytes by node type. +`memory_profile` (`f480236`) walks physical nodes and attributes bytes by node type. Three tries, all `PathMap<()>`: | dataset | values | list nodes | dense nodes | bytes/value | @@ -235,7 +234,7 @@ Two notes for anyone touching this again: be able to carry the flag like any other node pointer. A static assertion enforces that bit 0 is clear. - `ptr_eq` and `shared_node_id` mask the bit off. Leaving it in would have - silently defeated the shared-subtrie shortcuts from `18d40ec` and weakened the + silently defeated the shared-subtrie shortcuts from `12edc03` and weakened the catamorphism cache, without failing a single test. ### S2 -- the CellCoFree box: not worth doing @@ -451,7 +450,7 @@ a value *and* a child under the same key -- a path that both ends there and continues -- and in that case the walk took the link and reported "no value, path continues", so `restrict_slot_contents` discarded the value. The check now happens at every node before the link is taken, which also covers a value sitting at a -prefix of the key being followed. Fixed in `424ea5e`. +prefix of the key being followed. Fixed on `master` in `b2a0c09`; this branch no longer carries it. **Cost:** none measurable. Interleaved against the untouched `meet` as a control, restrict moves -1.5% to +0.6% across five operand shapes; the control moves -1.4%