From 1e3b1c3f7ea377db48696e9ddc5c81423e87d34b Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 16 Sep 2026 23:25:42 +0000 Subject: [PATCH 1/6] Fix join_into overwriting a destination that holds the source Joining with a TinyRefNode swapped operands without inverting the identity mask, so an unchanged destination reported COUNTER_IDENT and join_into replaced it with the source. merge_guts and the integer pjoin also under-reported identities, giving Element for unchanged joins. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019R2H8fnco29asY2v3TPbtF --- src/dense_byte_node.rs | 6 +++++- src/line_list_node.rs | 36 ++++++++++++++++--------------- src/ring.rs | 16 +++++++++----- src/write_zipper.rs | 48 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 83 insertions(+), 23 deletions(-) diff --git a/src/dense_byte_node.rs b/src/dense_byte_node.rs index 65568d9c..2b69e152 100644 --- a/src/dense_byte_node.rs +++ b/src/dense_byte_node.rs @@ -1323,8 +1323,12 @@ impl> TrieNode self.pjoin(other_byte_node).map(|new_node| TrieNodeODRc::new_in(new_node, self.alloc.clone())) }, TINY_REF_NODE_TAG => { + //Expand the tiny node and keep `self` on the left, so the identity mask stays ours let tiny_node = unsafe{ other.as_tiny_unchecked() }; - tiny_node.pjoin_dyn(self.as_tagged()) + match tiny_node.into_full() { + Some(full_node) => self.pjoin_dyn(full_node.as_tagged()), + None => AlgebraicResult::Identity(SELF_IDENT), + } } EMPTY_NODE_TAG => { AlgebraicResult::Identity(SELF_IDENT) diff --git a/src/line_list_node.rs b/src/line_list_node.rs index 6013ef25..6cd8afac 100644 --- a/src/line_list_node.rs +++ b/src/line_list_node.rs @@ -1345,14 +1345,13 @@ fn merge_guts<'a, V: Clone + Lattice + Send + Sync, A: Allocator, const ASLOT: u unsafe{ intermediate_node.set_payload_owned::<0>(&a_key[overlap..], a_payload); } debug_assert!(validate_node(&intermediate_node)); let intermediate_node = TrieNodeODRc::new_in(intermediate_node, a.alloc.clone()); - let joined = b_child.pjoin(&intermediate_node).unwrap_or_else(|which_arg| { - match which_arg { - 0 => b_child.clone(), - 1 => intermediate_node, - _ => unreachable!() - } - }, || panic!()); - return AlgebraicResult::Element((&a_key[0..overlap], ValOrChild::Child(joined))) + return match b_child.pjoin(&intermediate_node) { + AlgebraicResult::Element(joined) => AlgebraicResult::Element((&a_key[0..overlap], ValOrChild::Child(joined))), + //`b`'s child already held `a`'s payload, so `b`'s slot is the result + AlgebraicResult::Identity(mask) if mask & SELF_IDENT > 0 => AlgebraicResult::Identity(COUNTER_IDENT), + AlgebraicResult::Identity(_) => AlgebraicResult::Element((&a_key[0..overlap], ValOrChild::Child(intermediate_node))), + AlgebraicResult::None => unreachable!(), //`intermediate_node` is never empty + } } if a_key_len == overlap && a.is_child_ptr::() && b_key_len > overlap { let a_child = unsafe{ a.child_in_slot::() }; @@ -1361,14 +1360,13 @@ fn merge_guts<'a, V: Clone + Lattice + Send + Sync, A: Allocator, const ASLOT: u unsafe{ intermediate_node.set_payload_owned::<0>(&b_key[overlap..], b_payload); } debug_assert!(validate_node(&intermediate_node)); let intermediate_node = TrieNodeODRc::new_in(intermediate_node, a.alloc.clone()); - let joined = a_child.pjoin(&intermediate_node).unwrap_or_else(|which_arg| { - match which_arg { - 0 => a_child.clone(), - 1 => intermediate_node, - _ => unreachable!() - } - }, || panic!()); - return AlgebraicResult::Element((&a_key[0..overlap], ValOrChild::Child(joined))) + return match a_child.pjoin(&intermediate_node) { + AlgebraicResult::Element(joined) => AlgebraicResult::Element((&a_key[0..overlap], ValOrChild::Child(joined))), + //Mirror of the case above: `a`'s slot is the result + AlgebraicResult::Identity(mask) if mask & SELF_IDENT > 0 => AlgebraicResult::Identity(SELF_IDENT), + AlgebraicResult::Identity(_) => AlgebraicResult::Element((&a_key[0..overlap], ValOrChild::Child(intermediate_node))), + AlgebraicResult::None => unreachable!(), //`intermediate_node` is never empty + } } //If we have overlapping initial bytes that can be joined together, make a new prefix node @@ -2649,8 +2647,12 @@ impl TrieNode for LineListNode } }, TINY_REF_NODE_TAG => { + //Expand the tiny node and keep `self` on the left (see DenseByteNode::pjoin_dyn) let tiny_node = unsafe{ other.as_tiny_unchecked() }; - tiny_node.pjoin_dyn(self.as_tagged()) + match tiny_node.into_full() { + Some(full_node) => self.pjoin_dyn(full_node.as_tagged()), + None => AlgebraicResult::Identity(SELF_IDENT), + } } EMPTY_NODE_TAG => { AlgebraicResult::Identity(SELF_IDENT) diff --git a/src/ring.rs b/src/ring.rs index 4b9b1d45..100edaed 100644 --- a/src/ring.rs +++ b/src/ring.rs @@ -851,15 +851,21 @@ impl Lattice for () { fn pmeet(&self, _other: &Self) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT | COUNTER_IDENT) } } +/// Left-biased join; equal values are also `other`'s identity +#[inline] +fn left_biased_pjoin(a: &T, b: &T) -> AlgebraicResult { + if a == b { AlgebraicResult::Identity(SELF_IDENT | COUNTER_IDENT) } else { AlgebraicResult::Identity(SELF_IDENT) } +} + //GOAT trash impl Lattice for usize { - fn pjoin(&self, _other: &usize) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT) } + fn pjoin(&self, other: &usize) -> AlgebraicResult { left_biased_pjoin(self, other) } fn pmeet(&self, _other: &usize) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT) } } //GOAT trash impl Lattice for u64 { - fn pjoin(&self, _other: &u64) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT) } + fn pjoin(&self, other: &u64) -> AlgebraicResult { left_biased_pjoin(self, other) } fn pmeet(&self, _other: &u64) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT) } } @@ -873,13 +879,13 @@ impl DistributiveLattice for u64 { //GOAT trash impl Lattice for u32 { - fn pjoin(&self, _other: &u32) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT) } + fn pjoin(&self, other: &u32) -> AlgebraicResult { left_biased_pjoin(self, other) } fn pmeet(&self, _other: &u32) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT) } } //GOAT trash impl Lattice for u16 { - fn pjoin(&self, _other: &u16) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT) } + fn pjoin(&self, other: &u16) -> AlgebraicResult { left_biased_pjoin(self, other) } fn pmeet(&self, _other: &u16) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT) } } @@ -893,7 +899,7 @@ impl DistributiveLattice for u16 { //GOAT trash impl Lattice for u8 { - fn pjoin(&self, _other: &u8) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT) } + fn pjoin(&self, other: &u8) -> AlgebraicResult { left_biased_pjoin(self, other) } fn pmeet(&self, _other: &u8) -> AlgebraicResult { AlgebraicResult::Identity(SELF_IDENT) } } diff --git a/src/write_zipper.rs b/src/write_zipper.rs index 0892e475..8e68fce9 100644 --- a/src/write_zipper.rs +++ b/src/write_zipper.rs @@ -6672,4 +6672,52 @@ mod tests { } assert_eq!(keys(&m), ["cx", "cy", "d"]); } + + /// `join_into` from a source focus partway into a line node (a `TinyRefNode`) + #[test] + fn write_zipper_join_into_mid_key_source_keeps_destination() { + fn mk(ps: &[(&[u8], u64)]) -> PathMap { let mut m = PathMap::new(); for (p, v) in ps { m.set_val_at(p, *v); } m } + fn vals(m: &PathMap) -> Vec<(Vec, u64)> { m.iter().map(|(k, v)| (k.to_vec(), *v)).collect() } + let src = mk(&[(&[0, 0, 0], 7)]); + + //Dense destination + let mut dst = mk(&[(&[0], 7), (&[1], 1), (&[2], 2), (&[3], 3)]); + let before = vals(&dst); + let st = { let mut wz = dst.write_zipper(); let mut rz = src.read_zipper(); rz.descend_to(&[0, 0]); wz.join_into(&rz) }; + assert_eq!(st, AlgebraicStatus::Identity); + assert_eq!(vals(&dst), before); + + //List destination + let mut dst = mk(&[(&[0], 7), (&[0, 0], 0)]); + let before = vals(&dst); + let st = { let mut wz = dst.write_zipper(); let mut rz = src.read_zipper(); rz.descend_to(&[0, 0]); wz.join_into(&rz) }; + assert_eq!(st, AlgebraicStatus::Identity); + assert_eq!(vals(&dst), before); + + //And a join that does add something still says so, with the destination intact + let mut dst = mk(&[(&[1], 1), (&[2], 2), (&[3], 3)]); + let st = { let mut wz = dst.write_zipper(); let mut rz = src.read_zipper(); rz.descend_to(&[0, 0]); wz.join_into(&rz) }; + assert_eq!(st, AlgebraicStatus::Element); + assert_eq!(vals(&dst), vec![(vec![0], 7), (vec![1], 1), (vec![2], 2), (vec![3], 3)]); + } + + /// `join_into` of a source already contained under a destination child is `Identity` + #[test] + fn write_zipper_join_into_contained_under_child_is_identity() { + fn mk(ps: &[(&[u8], u64)]) -> PathMap { let mut m = PathMap::new(); for (p, v) in ps { m.set_val_at(p, *v); } m } + fn vals(m: &PathMap) -> Vec<(Vec, u64)> { m.iter().map(|(k, v)| (k.to_vec(), *v)).collect() } + let mut dst = mk(&[(&[0, 0], 0), (&[0, 1], 0)]); + let before = vals(&dst); + let src = mk(&[(&[0, 0], 0)]); + let st = { let mut wz = dst.write_zipper(); wz.join_into(&src.read_zipper()) }; + assert_eq!(st, AlgebraicStatus::Identity); + assert_eq!(vals(&dst), before); + + //The mirror image: the source holds the child, the destination the longer key + let mut dst = mk(&[(&[0, 0], 0)]); + let src = mk(&[(&[0, 0], 0), (&[0, 1], 0)]); + let st = { let mut wz = dst.write_zipper(); wz.join_into(&src.read_zipper()) }; + assert_eq!(st, AlgebraicStatus::Element); + assert_eq!(vals(&dst), vals(&src)); + } } From 1be6c5516545ff7103172fe1c6e1c2542dd73666 Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Tue, 15 Sep 2026 23:38:18 +0000 Subject: [PATCH 2/6] Make meet and join value bias independent of node layout Meets and joins keep the left operand's value on a collision, but a byte node paired with a list node ran the operation with operands swapped and kept the list node's values. join_k_path_into also folded k-paths in reverse. Values now follow operand order and path order. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019R2H8fnco29asY2v3TPbtF --- src/dense_byte_node.rs | 111 ++++++++++++++++++++++++++++++---- src/line_list_node.rs | 132 +++++++++++++++++++++++------------------ src/trie_node.rs | 113 +++++++++++++++++++++++++++++++---- 3 files changed, 277 insertions(+), 79 deletions(-) diff --git a/src/dense_byte_node.rs b/src/dense_byte_node.rs index 2b69e152..827f08de 100644 --- a/src/dense_byte_node.rs +++ b/src/dense_byte_node.rs @@ -260,6 +260,93 @@ impl> ByteNode } } + /// [Self::join_child_into] with `node` as the left operand; status relative to `self` + pub(crate) fn join_child_into_left(&mut self, k: u8, node: TrieNodeODRc) -> AlgebraicStatus where V: Clone + Lattice { + let ix = self.mask.index_of(k) as usize; + if self.mask.test_bit(k) { + let cf = unsafe { self.values.get_unchecked_mut(ix) }; + match cf.rec_mut() { + Some(existing_node) => { + match node.pjoin(existing_node) { + //`COUNTER_IDENT` means the result is `existing_node`: nothing to do + AlgebraicResult::Identity(mask) if mask & COUNTER_IDENT > 0 => AlgebraicStatus::Identity, + AlgebraicResult::Identity(_) => { + *existing_node = node; + AlgebraicStatus::Element + }, + AlgebraicResult::Element(joined) => { + *existing_node = joined; + AlgebraicStatus::Element + }, + //Only two empty nodes join to nothing, and then there is nothing to change + AlgebraicResult::None => AlgebraicStatus::Identity, + } + }, + None => { + cf.set_rec(node); + AlgebraicStatus::Element + } + } + } else { + self.mask.set_bit(k); + let new_cf = CoFree::new(Some(node), None); + self.values.insert(ix, new_cf); + AlgebraicStatus::Element + } + } + + /// [Self::join_val_into] with `val` as the *left* operand of the join; see [Self::join_child_into_left] + pub(crate) fn join_val_into_left(&mut self, k: u8, val: V) -> AlgebraicStatus where V: Lattice { + let ix = self.mask.index_of(k) as usize; + if self.mask.test_bit(k) { + let cf = unsafe { self.values.get_unchecked_mut(ix) }; + match cf.val_mut() { + Some(existing_val) => { + match val.pjoin(existing_val) { + AlgebraicResult::Identity(mask) if mask & COUNTER_IDENT > 0 => AlgebraicStatus::Identity, + AlgebraicResult::Identity(_) => { + *existing_val = val; + AlgebraicStatus::Element + }, + AlgebraicResult::Element(joined) => { + *existing_val = joined; + AlgebraicStatus::Element + }, + //A join of two present values never has an empty result; see `Lattice::join_into` + AlgebraicResult::None => AlgebraicStatus::Identity, + } + } + None => { + cf.set_val(val); + AlgebraicStatus::Element + } + } + } else { + self.mask.set_bit(k); + let new_cf = CoFree::new(None, Some(val)); + self.values.insert(ix, new_cf); + AlgebraicStatus::Element + } + } + + /// Dispatches to [Self::join_child_into] or [Self::join_child_into_left] + #[inline] + pub(crate) fn join_child_into_oriented(&mut self, k: u8, node: TrieNodeODRc, incoming_is_left: bool) -> AlgebraicStatus where V: Clone + Lattice { + if incoming_is_left { self.join_child_into_left(k, node) } else { self.join_child_into(k, node) } + } + + /// Dispatches to [Self::join_payload_into] or its left-biased counterpart + #[inline] + pub(crate) fn join_payload_into_oriented(&mut self, k: u8, payload: ValOrChild, incoming_is_left: bool) -> AlgebraicStatus where V: Clone + Lattice { + if !incoming_is_left { + return self.join_payload_into(k, payload) + } + match payload { + ValOrChild::Child(child) => self.join_child_into_left(k, child), + ValOrChild::Val(val) => self.join_val_into_left(k, val), + } + } + /// Internal method to remove a CoFree from the node #[inline] fn remove(&mut self, k: u8) -> Option { @@ -545,7 +632,8 @@ impl> ByteNode } /// Merges the entries in the ListNode into the ByteNode - pub fn merge_from_list_node(&mut self, list_node: &LineListNode) -> AlgebraicStatus where V: Clone + Lattice { + /// Joins `list_node` into `self`; `list_is_left` says which operand wins collisions + pub fn merge_from_list_node(&mut self, list_node: &LineListNode, list_is_left: bool) -> AlgebraicStatus where V: Clone + Lattice { let self_was_empty = self.is_empty(); self.reserve_capacity(2); @@ -555,9 +643,9 @@ impl> ByteNode if key.len() > 1 { let mut child_node = LineListNode::::new_in(self.alloc.clone()); unsafe{ child_node.set_payload_owned::<0>(&key[1..], payload); } - self.join_child_into(key[0], TrieNodeODRc::new_in(child_node, self.alloc.clone())) + self.join_child_into_oriented(key[0], TrieNodeODRc::new_in(child_node, self.alloc.clone()), list_is_left) } else { - self.join_payload_into(key[0], payload) + self.join_payload_into_oriented(key[0], payload, list_is_left) } } else { if self_was_empty { @@ -573,9 +661,9 @@ impl> ByteNode if key.len() > 1 { let mut child_node = LineListNode::::new_in(self.alloc.clone()); unsafe{ child_node.set_payload_owned::<0>(&key[1..], payload); } - self.join_child_into(key[0], TrieNodeODRc::new_in(child_node, self.alloc.clone())) + self.join_child_into_oriented(key[0], TrieNodeODRc::new_in(child_node, self.alloc.clone()), list_is_left) } else { - self.join_payload_into(key[0], payload) + self.join_payload_into_oriented(key[0], payload, list_is_left) } } else { if self_was_empty { @@ -1308,7 +1396,7 @@ impl> TrieNode LINE_LIST_NODE_TAG => { let other_list_node = unsafe{ other.as_list_unchecked() }; let mut new_node = self.clone(); - let status = new_node.merge_from_list_node(other_list_node); + let status = new_node.merge_from_list_node(other_list_node, false); AlgebraicResult::from_status(status, || TrieNodeODRc::new_in(new_node, self.alloc.clone())) }, #[cfg(feature = "bridge_nodes")] @@ -1353,7 +1441,7 @@ impl> TrieNode let other_list_node = unsafe{ other_node.into_list_unchecked() }; //GOAT, optimization opportunity to take the contents from the list, rather than cloning // them, to turn around and drop the ListNode and free them / decrement the refcounts - self.merge_from_list_node(other_list_node) + self.merge_from_list_node(other_list_node, false) }, #[cfg(feature = "bridge_nodes")] TaggedNodeRefMut::BridgeNode(_other_bridge_node) => { @@ -1391,7 +1479,8 @@ impl> TrieNode }, _ => { let mut new_node = Self::new_in(self.alloc.clone()); - while let Some(cf) = self.values.pop() { + //Ascending order, accumulated node on the left: the first path's value wins + for cf in self.values.drain(..) { let child = cf.into_rec().filter(|child| !child.is_empty()); let child = if byte_cnt > 1 { child.and_then(|mut child| child.make_mut().drop_head_dyn(byte_cnt-1)) @@ -1424,7 +1513,8 @@ impl> TrieNode }, LINE_LIST_NODE_TAG => { let other_list_node = unsafe { other.as_list_unchecked() }; - other_list_node.pmeet_dyn(self.as_tagged()).invert_identity() + //`self` is the left operand, hence `swapped` + other_list_node.pmeet_dyn_oriented(self.as_tagged(), true).invert_identity() }, #[cfg(feature = "bridge_nodes")] TaggedNodeRef::BridgeNode(other_bridge_node) => { @@ -1436,7 +1526,8 @@ impl> TrieNode }, TINY_REF_NODE_TAG => { let tiny_node = unsafe { other.as_tiny_unchecked() }; - tiny_node.pmeet_dyn(self.as_tagged()).invert_identity() + let full_node = tiny_node.into_full().unwrap(); + self.pmeet_dyn(full_node.as_tagged()) }, EMPTY_NODE_TAG => AlgebraicResult::None, _ => unsafe{ unreachable_unchecked() } diff --git a/src/line_list_node.rs b/src/line_list_node.rs index 6cd8afac..19e2499d 100644 --- a/src/line_list_node.rs +++ b/src/line_list_node.rs @@ -1304,6 +1304,30 @@ fn try_merge<'a, V: Clone + Lattice + Send + Sync, A: Allocator, const ASLOT: us } /// The part of `try_merge` that we probably shouldn't inline +/// One slot's subtrie with `byte_cnt` bytes dropped from its paths, or `None` +fn drop_head_from_payload(key: &[u8], payload: ValOrChild, byte_cnt: usize, alloc: &A) -> Option> { + let key_len = key.len(); + if byte_cnt < key_len { + let mut new_node = LineListNode::new_in(alloc.clone()); + unsafe { new_node.set_payload_owned::<0>(&key[byte_cnt..], payload); } + debug_assert!(validate_node(&new_node)); + return Some(TrieNodeODRc::new_in(new_node, alloc.clone())) + } + match payload { + ValOrChild::Val(_) => None, + ValOrChild::Child(mut child) => { + if child.is_empty() { + return None + } + if byte_cnt == key_len { + Some(child) + } else { + child.make_mut().drop_head_dyn(byte_cnt - key_len) + } + } + } +} + fn merge_guts<'a, V: Clone + Lattice + Send + Sync, A: Allocator, const ASLOT: usize, const BSLOT: usize>(mut overlap: usize, a_key: &'a[u8], a: &LineListNode, b_key: &'a[u8], b: &LineListNode) -> AlgebraicResult<(&'a[u8], ValOrChild)> { debug_assert!(overlap > 0); let a_key_len = a_key.len(); @@ -1345,10 +1369,11 @@ fn merge_guts<'a, V: Clone + Lattice + Send + Sync, A: Allocator, const ASLOT: u unsafe{ intermediate_node.set_payload_owned::<0>(&a_key[overlap..], a_payload); } debug_assert!(validate_node(&intermediate_node)); let intermediate_node = TrieNodeODRc::new_in(intermediate_node, a.alloc.clone()); - return match b_child.pjoin(&intermediate_node) { + //`a` is the left operand of the join + return match intermediate_node.pjoin(b_child) { AlgebraicResult::Element(joined) => AlgebraicResult::Element((&a_key[0..overlap], ValOrChild::Child(joined))), //`b`'s child already held `a`'s payload, so `b`'s slot is the result - AlgebraicResult::Identity(mask) if mask & SELF_IDENT > 0 => AlgebraicResult::Identity(COUNTER_IDENT), + AlgebraicResult::Identity(mask) if mask & COUNTER_IDENT > 0 => AlgebraicResult::Identity(COUNTER_IDENT), AlgebraicResult::Identity(_) => AlgebraicResult::Element((&a_key[0..overlap], ValOrChild::Child(intermediate_node))), AlgebraicResult::None => unreachable!(), //`intermediate_node` is never empty } @@ -2619,7 +2644,7 @@ impl TrieNode for LineListNode DENSE_BYTE_NODE_TAG => { let other_dense_node = unsafe{ other.as_dense_unchecked() }; let mut new_node = other_dense_node.clone(); - match new_node.merge_from_list_node(self) { + match new_node.merge_from_list_node(self, true) { //Both nodes were empty so the join is empty too AlgebraicStatus::None => { debug_assert!(self.node_is_empty() && other_dense_node.node_is_empty()); @@ -2636,7 +2661,7 @@ impl TrieNode for LineListNode CELL_BYTE_NODE_TAG => { let other_dense_node = unsafe{ other.as_dense_unchecked() }; let mut new_node = other_dense_node.clone(); - match new_node.merge_from_list_node(self) { + match new_node.merge_from_list_node(self, true) { //See the DENSE_BYTE_NODE_TAG arm: two empty nodes join to an empty result AlgebraicStatus::None => { debug_assert!(self.node_is_empty() && other_dense_node.node_is_empty()); @@ -2672,7 +2697,7 @@ impl TrieNode for LineListNode DENSE_BYTE_NODE_TAG => { let other_dense_node = unsafe{ other_node.as_dense_unchecked() }; let mut new_node = other_dense_node.clone(); - let status = new_node.merge_from_list_node(self); + let status = new_node.merge_from_list_node(self, true); debug_assert!(!status.is_none()); (AlgebraicStatus::Element, Err(TrieNodeODRc::new_in(new_node, self.alloc.clone()))) }, @@ -2683,7 +2708,7 @@ impl TrieNode for LineListNode CELL_BYTE_NODE_TAG => { let other_dense_node = unsafe{ other_node.as_cell_unchecked() }; let mut new_node = other_dense_node.clone(); - let status = new_node.merge_from_list_node(self); + let status = new_node.merge_from_list_node(self, true); debug_assert!(!status.is_none()); (AlgebraicStatus::Element, Err(TrieNodeODRc::new_in(new_node, self.alloc.clone()))) }, @@ -2843,45 +2868,55 @@ impl TrieNode for LineListNode return Some(TrieNodeODRc::new_in(temp_node, self.alloc.clone())) } - //The final case is to construct a brand new node from the remaining parts of the key after we have - // discarded what we can discard and then merged together what's left. And then call this function - // recursively on the newly merged nodes - let chop_bytes = key0_len.min(key1_len); - debug_assert!(chop_bytes <= byte_cnt); - debug_assert!(chop_bytes > 0); - let new_key0 = &key0[chop_bytes-1..]; - let new_key1 = &key1[chop_bytes-1..]; - - let overlap = find_prefix_overlap(&key0[chop_bytes..], &key1[chop_bytes..]); - let merged_payload = match merge_guts::(overlap+1, new_key0, &temp_node, new_key1, &temp_node) { - AlgebraicResult::Element((_shared_key, merged_payload)) => merged_payload, - AlgebraicResult::Identity(mask) => { - if mask & SELF_IDENT > 0 { - temp_node.clone_payload::<0>().unwrap() - } else { - debug_assert_eq!(mask, COUNTER_IDENT); - temp_node.clone_payload::<1>().unwrap() - } - }, - AlgebraicResult::None => unreachable!() //`merge_guts` shouldn't return AlgebraicResult::None because that should have been caught by an earlier case + //Drop from each slot separately and join, slot 0 on the left, so the first k-path's value wins + let mut key0_buf: [MaybeUninit; KEY_BYTES_CNT] = [MaybeUninit::new(0); KEY_BYTES_CNT]; + let mut key1_buf: [MaybeUninit; KEY_BYTES_CNT] = [MaybeUninit::new(0); KEY_BYTES_CNT]; + let (key0, key1) = unsafe { + core::ptr::copy_nonoverlapping(key0.as_ptr(), key0_buf.as_mut_ptr().cast::(), key0_len); + core::ptr::copy_nonoverlapping(key1.as_ptr(), key1_buf.as_mut_ptr().cast::(), key1_len); + (core::slice::from_raw_parts(key0_buf.as_ptr().cast::(), key0_len), + core::slice::from_raw_parts(key1_buf.as_ptr().cast::(), key1_len)) }; - - if let ValOrChild::Child(mut child_node) = merged_payload { - //A dangling child (the empty sentinel) has nothing below the dropped bytes and can't be made mutable - if child_node.is_empty() { - return None - } - if chop_bytes == byte_cnt { - return Some(child_node) - } else { - return child_node.make_mut().drop_head_dyn(byte_cnt-chop_bytes) + //Take slot 1 first: taking slot 0 would shift slot 1 into its place. + let payload1 = temp_node.take_payload::<1>().unwrap(); + let payload0 = temp_node.take_payload::<0>().unwrap(); + let dropped0 = drop_head_from_payload(key0, payload0, byte_cnt, &self.alloc); + let dropped1 = drop_head_from_payload(key1, payload1, byte_cnt, &self.alloc); + match (dropped0, dropped1) { + (None, None) => None, + (Some(node), None) | (None, Some(node)) => Some(node), + (Some(node0), Some(node1)) => match node0.pjoin(&node1) { + AlgebraicResult::Element(joined) => Some(joined), + AlgebraicResult::Identity(mask) => Some(if mask & SELF_IDENT > 0 { node0 } else { node1 }), + AlgebraicResult::None => None, } } - - unreachable!() } fn pmeet_dyn(&self, other: TaggedNodeRef) -> AlgebraicResult> where V: Lattice { + self.pmeet_dyn_oriented(other, false) + } + fn psubtract_dyn(&self, other: TaggedNodeRef) -> AlgebraicResult> where V: DistributiveLattice { + debug_assert!(validate_node(self)); + let slot0_result = self.subtract_from_slot_contents::<0>(other); + let slot1_result = self.subtract_from_slot_contents::<1>(other); + self.combine_slot_results_into_node_result(slot0_result, slot1_result) + } + fn prestrict_dyn(&self, other: TaggedNodeRef) -> AlgebraicResult> { + debug_assert!(validate_node(self)); + let slot0_result = self.restrict_slot_contents::<0>(other); + let slot1_result = self.restrict_slot_contents::<1>(other); + self.combine_slot_results_into_node_result(slot0_result, slot1_result) + } + fn clone_self(&self) -> TrieNodeODRc { + TrieNodeODRc::new_in(self.clone(), self.alloc.clone()) + } +} + +impl LineListNode { + /// [TrieNode::pmeet_dyn]; `swapped` means `self` is the right operand. Each slot keeps the + /// deepest prefix of its key that `other` also has + pub(crate) fn pmeet_dyn_oriented(&self, other: TaggedNodeRef, swapped: bool) -> AlgebraicResult> where V: Lattice { debug_assert!(validate_node(self)); let mut self_payloads_buf: [(&[u8], PayloadRef); 2] = [(&[], PayloadRef::None); 2]; @@ -2906,7 +2941,7 @@ impl TrieNode for LineListNode _ => unsafe{ unreachable_unchecked() } }; - pmeet_generic::<2, V, A, _>(self_payloads, other, |payloads| { + pmeet_generic::<2, V, A, _>(self_payloads, other, swapped, |payloads| { debug_assert_eq!(payloads.len(), self_payloads.len()); let slot0_payload = payloads.get_mut(0).and_then(|p| core::mem::take(p)).map(|p| p.into()); let slot1_payload = payloads.get_mut(1).and_then(|p| core::mem::take(p)).map(|p| p.into()); @@ -2914,24 +2949,7 @@ impl TrieNode for LineListNode TrieNodeODRc::new_in(new_node, self.alloc.clone()) }) } - fn psubtract_dyn(&self, other: TaggedNodeRef) -> AlgebraicResult> where V: DistributiveLattice { - debug_assert!(validate_node(self)); - let slot0_result = self.subtract_from_slot_contents::<0>(other); - let slot1_result = self.subtract_from_slot_contents::<1>(other); - self.combine_slot_results_into_node_result(slot0_result, slot1_result) - } - fn prestrict_dyn(&self, other: TaggedNodeRef) -> AlgebraicResult> { - debug_assert!(validate_node(self)); - let slot0_result = self.restrict_slot_contents::<0>(other); - let slot1_result = self.restrict_slot_contents::<1>(other); - self.combine_slot_results_into_node_result(slot0_result, slot1_result) - } - fn clone_self(&self) -> TrieNodeODRc { - TrieNodeODRc::new_in(self.clone(), self.alloc.clone()) - } -} -impl LineListNode { /// Part of the implementation of methods the remove subtries from a node fn remove_subtries(&mut self, remove_0: bool, remove_1: bool, key0_starts_with: bool, prune: bool, key_len: usize) { //NOTE: the order here is important because removing slot_0 first might shift the diff --git a/src/trie_node.rs b/src/trie_node.rs index 27366659..bea4d08f 100644 --- a/src/trie_node.rs +++ b/src/trie_node.rs @@ -642,7 +642,16 @@ impl ValOrChildUnion { // was observed. Therefore the the ~20% slowdown is simply the higher overheads of this generic function. // //The next port of call for optimization is probably to remove the recursion -pub(crate) fn pmeet_generic(self_payloads: &[(&[u8], PayloadRef)], other: TaggedNodeRef, merge_f: MergeF) -> AlgebraicResult> +// +/// `swapped` says which operand `self_payloads` came from. The meet is left-biased (a `Lattice` +/// impl resolves a collision as `left.pmeet(right)`), so when a caller enumerates the *right* +/// operand's payloads because that node type is the easier one to iterate, it passes `swapped = +/// true`: every value and every recursive node meet is then computed as `other op self` and the +/// identity masks are re-expressed relative to `self_payloads`. The caller still applies +/// `invert_identity()` to the final result to get back to its own orientation. Without this, a +/// dense-node-versus-list-node meet returned the list node's values regardless of which side it +/// was on. +pub(crate) fn pmeet_generic(self_payloads: &[(&[u8], PayloadRef)], other: TaggedNodeRef, swapped: bool, merge_f: MergeF) -> AlgebraicResult> where MergeF: FnOnce(&mut [Option>]) -> TrieNodeODRc, V: Clone + Send + Sync + Lattice @@ -657,7 +666,7 @@ pub(crate) fn pmeet_generic(self_payloads, &mut request_keys[..], &mut request_results[..], &mut element_results[..], other); + let is_exhaustive = pmeet_generic_internal::(self_payloads, &mut request_keys[..], &mut request_results[..], &mut element_results[..], other, swapped); let mut is_none = true; let mut combined_mask = SELF_IDENT | COUNTER_IDENT; let mut result_payloads = ArrayVec::>, MAX_PAYLOAD_CNT>::new(); @@ -697,7 +706,7 @@ pub(crate) fn node_count_branches_recursive(self_payloads: &[(&[u8], PayloadRef)], keys: &mut [(&[u8], bool)], request_results: &mut [(usize, PayloadRef<'trie, V, A>)], results: &mut [FatAlgebraicResult>], other_node: TaggedNodeRef<'trie, V, A>) -> bool +pub(crate) fn pmeet_generic_internal<'trie, const MAX_PAYLOAD_CNT: usize, V, A: Allocator>(self_payloads: &[(&[u8], PayloadRef)], keys: &mut [(&[u8], bool)], request_results: &mut [(usize, PayloadRef<'trie, V, A>)], results: &mut [FatAlgebraicResult>], other_node: TaggedNodeRef<'trie, V, A>, swapped: bool) -> bool where V: Clone + Send + Sync + Lattice { //If is_exhaustive gets set to `false`, then the pmeet method cannot return a `COUNTER_IDENTITY` result @@ -729,14 +738,14 @@ pub(crate) fn pmeet_generic_internal<'trie, const MAX_PAYLOAD_CNT: usize, V, A: // we have the same node as the previous time through the loop if cur_group.is_some() { if (cur_group.as_ref().unwrap().1 as *const TrieNodeODRc) != (child as *const TrieNodeODRc) { - pmeet_generic_recursive_reset::(&mut cur_group, &mut is_exhaustive, idx, self_payloads, keys, request_results, results); + pmeet_generic_recursive_reset::(&mut cur_group, &mut is_exhaustive, idx, self_payloads, keys, request_results, results, swapped); cur_group = Some((idx, child)); } } else { cur_group = Some((idx, child)); } } else { - pmeet_generic_recursive_reset::(&mut cur_group, &mut is_exhaustive, idx, self_payloads, keys, request_results, results); + pmeet_generic_recursive_reset::(&mut cur_group, &mut is_exhaustive, idx, self_payloads, keys, request_results, results, swapped); //We've arrived at a contained value or onward link that has a correspondence // to one of the values or links in `self` @@ -745,13 +754,13 @@ pub(crate) fn pmeet_generic_internal<'trie, const MAX_PAYLOAD_CNT: usize, V, A: let result = match &self_payloads[idx].1 { PayloadRef::Child(self_link) => { let other_link = payload.child(); - let result = self_link.pmeet(other_link); + let result = if swapped { other_link.pmeet(self_link).invert_identity() } else { self_link.pmeet(other_link) }; FatAlgebraicResult::from_binary_op_result(result, self_link, other_link) .map(|child| ValOrChild::Child(child)) }, PayloadRef::Val(self_val) => { let other_val = payload.val(); - let result = (*self_val).pmeet(other_val); + let result = if swapped { other_val.pmeet(*self_val).invert_identity() } else { (*self_val).pmeet(other_val) }; FatAlgebraicResult::from_binary_op_result(result, *self_val, other_val) .map(|val| ValOrChild::Val(val)) }, @@ -762,13 +771,17 @@ pub(crate) fn pmeet_generic_internal<'trie, const MAX_PAYLOAD_CNT: usize, V, A: results[idx] = result; } } else { - pmeet_generic_recursive_reset::(&mut cur_group, &mut is_exhaustive, idx, self_payloads, keys, request_results, results); + pmeet_generic_recursive_reset::(&mut cur_group, &mut is_exhaustive, idx, self_payloads, keys, request_results, results, swapped); let result = match &self_payloads[idx].1 { PayloadRef::Child(self_link) => { match other_node.get_node_at_key(keys[idx].0).into_option() { Some(other_onward_node) => { - let result = self_link.as_tagged().pmeet_dyn(other_onward_node.as_tagged()); + let result = if swapped { + other_onward_node.as_tagged().pmeet_dyn(self_link.as_tagged()).invert_identity() + } else { + self_link.as_tagged().pmeet_dyn(other_onward_node.as_tagged()) + }; FatAlgebraicResult::from_binary_op_result(result, self_link, &other_onward_node) .map(|child| ValOrChild::Child(child)) }, @@ -791,7 +804,7 @@ pub(crate) fn pmeet_generic_internal<'trie, const MAX_PAYLOAD_CNT: usize, V, A: results[idx] = result; } } - pmeet_generic_recursive_reset::(&mut cur_group, &mut is_exhaustive, keys.len(), self_payloads, keys, request_results, results); + pmeet_generic_recursive_reset::(&mut cur_group, &mut is_exhaustive, keys.len(), self_payloads, keys, request_results, results, swapped); is_exhaustive } @@ -799,7 +812,7 @@ pub(crate) fn pmeet_generic_internal<'trie, const MAX_PAYLOAD_CNT: usize, V, A: /// Effectively part of `pmeet_generic_internal`, but factored out separately because it's called in /// several different places. Resets the `cur_group` state and does a recursive call of `pmeet_generic_internal` #[inline] -fn pmeet_generic_recursive_reset<'trie, const MAX_PAYLOAD_CNT: usize, V, A: Allocator>(cur_group: &mut Option<(usize, &'trie TrieNodeODRc)>, is_exhaustive: &mut bool, idx: usize, self_payloads: &[(&[u8], PayloadRef)], keys: &mut [(&[u8], bool)], request_results: &mut [(usize, PayloadRef<'trie, V, A>)], results: &mut [FatAlgebraicResult>]) +fn pmeet_generic_recursive_reset<'trie, const MAX_PAYLOAD_CNT: usize, V, A: Allocator>(cur_group: &mut Option<(usize, &'trie TrieNodeODRc)>, is_exhaustive: &mut bool, idx: usize, self_payloads: &[(&[u8], PayloadRef)], keys: &mut [(&[u8], bool)], request_results: &mut [(usize, PayloadRef<'trie, V, A>)], results: &mut [FatAlgebraicResult>], swapped: bool) where V: Clone + Send + Sync + Lattice { match core::mem::take(cur_group) { @@ -807,7 +820,7 @@ fn pmeet_generic_recursive_reset<'trie, const MAX_PAYLOAD_CNT: usize, V, A: Allo let group_keys = &mut keys[group_start..idx]; let group_results = &mut results[group_start..idx]; let group_self_payloads = &self_payloads[group_start..idx]; - if !pmeet_generic_internal::(group_self_payloads, group_keys, request_results, group_results, next_node.as_tagged()) { + if !pmeet_generic_internal::(group_self_payloads, group_keys, request_results, group_results, next_node.as_tagged(), swapped) { *is_exhaustive = false; } }, @@ -3446,6 +3459,82 @@ mod tests { use crate::PathMap; use crate::zipper::*; + fn mk(ps: &[(&[u8], u64)]) -> PathMap { + let mut m = PathMap::::new(); + for (p, v) in ps { m.set_val_at(p, *v); } + m + } + fn vals(m: &PathMap) -> Vec<(Vec, u64)> { + m.iter().map(|(p, v)| (p.to_vec(), *v)).collect() + } + + /// A meet keeps the left operand's value, whatever the node types + #[test] + fn meet_value_bias_is_left_regardless_of_node_layout() { + let two_payload_list = mk(&[(&[0], 0), (&[0, 0], 0), (&[3, 0], 1)]); + let single_line = mk(&[(&[0], 1)]); + let dense = mk(&[(&[0], 0), (&[1], 0), (&[2], 0), (&[3], 0), (&[4], 0)]); + for (a, b, expect) in [ + (&two_payload_list, &single_line, 0u64), + (&single_line, &two_payload_list, 1), + (&dense, &single_line, 0), + (&single_line, &dense, 1), + (&dense, &two_payload_list, 0), + (&two_payload_list, &dense, 0), + ] { + let mut out = PathMap::::new(); + { let mut wz = out.write_zipper(); wz.meet_2(&a.read_zipper(), &b.read_zipper()); } + assert_eq!(out.get_val_at(&[0]), Some(&expect), "meet_2 of {:?} and {:?}", vals(a), vals(b)); + + let mut into = a.clone(); + { let mut wz = into.write_zipper(); wz.meet_into(&b.read_zipper(), false); } + assert_eq!(into.get_val_at(&[0]), Some(&expect), "meet_into of {:?} and {:?}", vals(a), vals(b)); + } + } + + /// Same for joins + #[test] + fn join_value_bias_is_left_regardless_of_node_layout() { + let line = mk(&[(&[1], 0)]); + let dense = mk(&[(&[0], 0), (&[1], 1), (&[2], 0)]); + assert_eq!(line.join(&dense).get_val_at(&[1]), Some(&0)); + assert_eq!(dense.join(&line).get_val_at(&[1]), Some(&1)); + + let mut into = line.clone(); + { let mut wz = into.write_zipper(); wz.join_into(&dense.read_zipper()); } + assert_eq!(vals(&into), vec![(vec![0], 0), (vec![1], 0), (vec![2], 0)]); + let mut into = dense.clone(); + { let mut wz = into.write_zipper(); wz.join_into(&line.read_zipper()); } + assert_eq!(vals(&into), vec![(vec![0], 0), (vec![1], 1), (vec![2], 0)]); + + //A deeper collision, so the child-node join is exercised as well as the value join + let line = mk(&[(&[1, 5], 0), (&[1, 6], 0)]); + let dense = mk(&[(&[0], 0), (&[1, 5], 1), (&[2], 0)]); + assert_eq!(line.join(&dense).get_val_at(&[1, 5]), Some(&0)); + assert_eq!(dense.join(&line).get_val_at(&[1, 5]), Some(&1)); + } + + /// `join_k_path_into` keeps the first k-path's value on a collision + #[test] + fn join_k_path_into_keeps_lexicographically_first_value() { + let mut m = mk(&[(&[0, 0, 0, 2], 0), (&[0, 1, 0, 2], 1), (&[0, 1, 0, 2, 0], 0)]); + { let mut wz = m.write_zipper(); wz.join_k_path_into(3, false); } + assert_eq!(vals(&m), vec![(vec![2], 0), (vec![2, 0], 0)]); + + let mut m = mk(&[(&[1, 0, 3], 0), (&[0], 0), (&[0, 0, 0], 0), (&[0, 0, 3], 1)]); + { let mut wz = m.write_zipper(); wz.join_k_path_into(2, false); } + assert_eq!(vals(&m), vec![(vec![0], 0), (vec![3], 1)]); + + let mut m = mk(&[(&[0, 0, 0], 0), (&[0], 0), (&[1, 0, 0], 1)]); + { let mut wz = m.write_zipper(); wz.join_k_path_into(2, false); } + assert_eq!(vals(&m), vec![(vec![0], 0)]); + + //Three-plus branches at the root make it a byte node + let mut m = mk(&[(&[0, 0, 7], 0), (&[1, 0, 7], 1), (&[2, 0, 7], 2), (&[3, 0, 7], 3)]); + { let mut wz = m.write_zipper(); wz.join_k_path_into(2, false); } + assert_eq!(vals(&m), vec![(vec![7], 0)]); + } + #[test] fn slim_ptrs_test1() { let map = PathMap::<()>::new(); From a3c64a89c855c46eb326b51fc7a33fb10abb437f Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 16 Sep 2026 22:11:19 +0000 Subject: [PATCH 3/6] Fix write zipper node stack after prune_path prune_path walked the node stack up to find where pruning stops but left it there when the zipper did not move, so the next write through the focus went to the wrong node (get_val_or_set_mut panicked). Walk the stack back down. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019R2H8fnco29asY2v3TPbtF --- src/write_zipper.rs | 47 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/src/write_zipper.rs b/src/write_zipper.rs index 8e68fce9..0e9c4346 100644 --- a/src/write_zipper.rs +++ b/src/write_zipper.rs @@ -2572,6 +2572,9 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC if should_ascend { self.key.prefix_buf.truncate(temp_path.len()); + } else if ascended { + //The zipper didn't move, so restore the node stack to the focus + self.descend_to_internal(); } pruned_bytes @@ -3710,6 +3713,50 @@ mod tests { assert_eq!(btm2.path_exists_at(&[0, 255, 1]), false); } + /// A write after `prune_path` (or `meet_into(.., true)`) must reach the focus node + #[test] + fn write_zipper_write_after_prune_path_below_a_graft() { + let build = || { + let mut m0 = PathMap::::new(); + let mut m1 = PathMap::::new(); + m1.set_val_at(&[1u8, 0, 0, 0, 0], 7); + m0.create_path(&[0u8, 0]); + m1.create_path(&[1u8]); + (m0, m1) + }; + + // prune_path directly + let (mut m0, m1) = build(); + { + let mut wz = m0.write_zipper_at_path(&[0u8, 0]); + let rz = m1.read_zipper_at_path(&[1u8]); + wz.graft(&rz); + wz.descend_last_byte(); + wz.remove_branches(false); + wz.prune_path(); + assert_eq!(wz.path(), &[0u8]); + assert_eq!(*wz.get_val_or_set_mut_with(|| 3), 3); + assert_eq!(wz.val(), Some(&3)); + } + assert_eq!(m0.get_val_at(&[0u8, 0, 0]), Some(&3)); + assert_eq!(m0.val_count(), 1); + + // through meet_into with prune + let (mut m0, m1) = build(); + { + let mut wz = m0.write_zipper_at_path(&[0u8, 0]); + let rz = m1.read_zipper_at_path(&[1u8]); + wz.graft(&rz); + wz.descend_last_byte(); + wz.meet_into(&rz, true); + assert_eq!(wz.path(), &[0u8]); + assert_eq!(*wz.get_val_or_set_mut_with(|| 3), 3); + assert_eq!(wz.val(), Some(&3)); + } + assert_eq!(m0.get_val_at(&[0u8, 0, 0]), Some(&3)); + } + + /// Tests whether the [WriteZipper::subtract_into] operation will do the right thing with the root value #[test] fn write_zipper_subtract_into_test1() { From f80187974a0b3c61e13e10236f9767529a99792f Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 16 Sep 2026 01:34:16 +0000 Subject: [PATCH 4/6] Don't report a change for dropping a shadowed dangling slot An empty link sharing its key with a value in the same list node carries nothing. Subtract dropped it and reported Element for an unchanged node; it is now Identity. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019R2H8fnco29asY2v3TPbtF --- src/line_list_node.rs | 61 +++++++++++++++++++++++++++++++++++++---- src/write_zipper.rs | 64 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 6 deletions(-) diff --git a/src/line_list_node.rs b/src/line_list_node.rs index 19e2499d..527c6974 100644 --- a/src/line_list_node.rs +++ b/src/line_list_node.rs @@ -1192,6 +1192,20 @@ impl LineListNode { AlgebraicResult::Identity(SELF_IDENT) } } + /// `true` when the slot is an empty link sharing its key with the other slot, so it carries nothing + fn slot_is_shadowed_dangling(&self, slot: usize) -> bool { + let is_used_child = if slot == 0 { self.is_used_child_0() } else { self.is_used_child_1() }; + if !is_used_child || !self.is_used::<1>() { + return false + } + let (key0, key1) = self.get_both_keys(); + if key0 != key1 { + return false + } + let child = unsafe{ if slot == 0 { self.child_in_slot::<0>() } else { self.child_in_slot::<1>() } }; + child.as_tagged().node_is_empty() + } + /// Internal method to restrict the contents of `SLOT` with the contents of the `other` node fn restrict_slot_contents(&self, other: TaggedNodeRef) -> AlgebraicResult> where V: Clone { if self.is_used::() { @@ -2900,6 +2914,18 @@ impl TrieNode for LineListNode debug_assert!(validate_node(self)); let slot0_result = self.subtract_from_slot_contents::<0>(other); let slot1_result = self.subtract_from_slot_contents::<1>(other); + + //Dropping a shadowed empty link is not a change + match (&slot0_result, &slot1_result) { + (AlgebraicResult::None, AlgebraicResult::Identity(_)) if self.slot_is_shadowed_dangling(0) => { + return AlgebraicResult::Identity(SELF_IDENT) + }, + (AlgebraicResult::Identity(_), AlgebraicResult::None) if self.slot_is_shadowed_dangling(1) => { + return AlgebraicResult::Identity(SELF_IDENT) + }, + _ => {} + } + self.combine_slot_results_into_node_result(slot0_result, slot1_result) } fn prestrict_dyn(&self, other: TaggedNodeRef) -> AlgebraicResult> { @@ -2921,16 +2947,31 @@ impl LineListNode { let mut self_payloads_buf: [(&[u8], PayloadRef); 2] = [(&[], PayloadRef::None); 2]; + //A shadowed dangling slot carries nothing, so skip it + let skipped = if self.slot_is_shadowed_dangling(0) { + Some(0) + } else if self.slot_is_shadowed_dangling(1) { + Some(1) + } else { + None + }; + let self_slot_count = self.used_slot_count(); - let self_payloads = match self_slot_count { - 0 => return AlgebraicResult::None, - 1 => { + let self_payloads = match (self_slot_count, skipped) { + (0, _) => return AlgebraicResult::None, + (_, Some(0)) => { + let key = unsafe{ self.key_unchecked::<1>() }; + let payload = unsafe{ self.payload_in_slot::<1>() }; + self_payloads_buf[0] = (key, payload); + &self_payloads_buf[..1] + }, + (1, _) | (_, Some(1)) => { let key = unsafe{ self.key_unchecked::<0>() }; let payload = unsafe{ self.payload_in_slot::<0>() }; self_payloads_buf[0] = (key, payload); &self_payloads_buf[..1] }, - 2 => { + (2, None) => { let (key0, key1) = self.get_both_keys(); let payload0 = unsafe{ self.payload_in_slot::<0>() }; let payload1 = unsafe{ self.payload_in_slot::<1>() }; @@ -2943,8 +2984,16 @@ impl LineListNode { pmeet_generic::<2, V, A, _>(self_payloads, other, swapped, |payloads| { debug_assert_eq!(payloads.len(), self_payloads.len()); - let slot0_payload = payloads.get_mut(0).and_then(|p| core::mem::take(p)).map(|p| p.into()); - let slot1_payload = payloads.get_mut(1).and_then(|p| core::mem::take(p)).map(|p| p.into()); + //With a slot skipped, the single result belongs to the slot that stayed in, and the + // skipped one is dropped -- which is what the meet would have done with it anyway. + let (slot0_payload, slot1_payload) = match skipped { + Some(0) => (None, payloads.get_mut(0).and_then(|p| core::mem::take(p)).map(|p| p.into())), + Some(_) => (payloads.get_mut(0).and_then(|p| core::mem::take(p)).map(|p| p.into()), None), + None => ( + payloads.get_mut(0).and_then(|p| core::mem::take(p)).map(|p| p.into()), + payloads.get_mut(1).and_then(|p| core::mem::take(p)).map(|p| p.into()), + ), + }; let new_node = self.clone_with_updated_payloads(slot0_payload, slot1_payload).unwrap(); TrieNodeODRc::new_in(new_node, self.alloc.clone()) }) diff --git a/src/write_zipper.rs b/src/write_zipper.rs index 0e9c4346..28336150 100644 --- a/src/write_zipper.rs +++ b/src/write_zipper.rs @@ -6767,4 +6767,68 @@ mod tests { assert_eq!(st, AlgebraicStatus::Element); assert_eq!(vals(&dst), vals(&src)); } + + /// Dropping an empty link that shares its key with a value is not a change + #[test] + fn write_zipper_shadowed_dangling_slot_is_identity() { + fn mk(ps: &[(&[u8], u64)]) -> PathMap { let mut m = PathMap::new(); for (p, v) in ps { m.set_val_at(p, *v); } m } + fn vals(m: &PathMap) -> Vec<(Vec, u64)> { m.iter().map(|(k, v)| (k.to_vec(), *v)).collect() } + + //Value and empty link, both at `[0]` + fn dst_with_shadowed_dangling() -> PathMap { + let seed = mk(&[(&[0, 0], 0)]); + let mut dst = PathMap::::new(); + { + let mut wz = dst.write_zipper(); + wz.graft(&seed.read_zipper()); + wz.descend_to_byte(0); + wz.insert_prefix(&[0]); + wz.get_val_or_set_mut(1); + wz.remove_branches(false); + } + assert_eq!(vals(&dst), vec![(vec![0], 1)]); + dst + } + + //Nothing of `src` collides with the value at `[0]`, so the subtraction takes nothing away + let mut dst = dst_with_shadowed_dangling(); + let src = mk(&[(&[0, 0], 9)]); + let st = { let mut wz = dst.write_zipper(); wz.subtract_into(&src.read_zipper(), false) }; + assert_eq!(st, AlgebraicStatus::Identity); + assert_eq!(vals(&dst), vec![(vec![0], 1)]); + + //The meet keeps the value at `[0]` and drops the dangling link, which changes nothing + let mut dst = dst_with_shadowed_dangling(); + let src = mk(&[(&[0], 9)]); + let st = { let mut wz = dst.write_zipper(); wz.meet_into(&src.read_zipper(), false) }; + assert_eq!(st, AlgebraicStatus::Identity); + assert_eq!(vals(&dst), vec![(vec![0], 1)]); + + //A subtraction that really does annihilate the value beside the dangling link still says so + let mut dst = dst_with_shadowed_dangling(); + let src = mk(&[(&[0], 1)]); + let st = { let mut wz = dst.write_zipper(); wz.subtract_into(&src.read_zipper(), false) }; + assert_eq!(st, AlgebraicStatus::None); + assert_eq!(vals(&dst), vec![]); + + //...and so does a meet that drops it + let mut dst = dst_with_shadowed_dangling(); + let src = mk(&[(&[1], 9)]); + let st = { let mut wz = dst.write_zipper(); wz.meet_into(&src.read_zipper(), false) }; + assert_eq!(st, AlgebraicStatus::None); + assert_eq!(vals(&dst), vec![]); + + //An unshadowed empty link at `[0, 0]` is a real change + let mut dst = mk(&[(&[0], 1), (&[0, 0], 2)]); + { + let empty = PathMap::::new(); + let mut wz = dst.write_zipper_at_path(&[0, 0]); + wz.graft(&empty.read_zipper()); + } + assert_eq!(vals(&dst), vec![(vec![0], 1)]); + let src = mk(&[(&[0, 0], 9)]); + let st = { let mut wz = dst.write_zipper(); wz.subtract_into(&src.read_zipper(), false) }; + assert_eq!(st, AlgebraicStatus::Element); + assert_eq!(vals(&dst), vec![(vec![0], 1)]); + } } From e356da7f6d6ff76ea46b5cdb4c36b8db91318083 Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Thu, 17 Sep 2026 00:26:18 +0000 Subject: [PATCH 5/6] Meet as an intersection of locations A path survives a meet exactly when both sides have it, dangling or not, and a value exactly when both hold one. Dense bytes that meet to nothing stay as dangling paths, list nodes meet slot by slot keeping the shared key prefix, and meet_into never removes its focus. With prune, dangling paths below the focus are dropped, possibly skipping nodes shared with the source. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019R2H8fnco29asY2v3TPbtF --- src/dense_byte_node.rs | 202 +++++++++--------- src/empty_node.rs | 3 - src/lib.rs | 4 +- src/line_list_node.rs | 207 +++++++++++-------- src/ring.rs | 30 --- src/tiny_node.rs | 27 --- src/trie_node.rs | 459 +++++++++++++++++++---------------------- src/write_zipper.rs | 436 ++++++++++++++++++++++++++++++++++---- 8 files changed, 818 insertions(+), 550 deletions(-) diff --git a/src/dense_byte_node.rs b/src/dense_byte_node.rs index 827f08de..d4264417 100644 --- a/src/dense_byte_node.rs +++ b/src/dense_byte_node.rs @@ -808,102 +808,6 @@ impl> TrieNode let cf = self.get_mut(key[0]).unwrap(); *cf.rec_mut().unwrap() = new_node; } - fn node_get_payloads<'node, 'res>(&'node self, keys: &[(&[u8], bool)], results: &'res mut [(usize, PayloadRef<'node, V, A>)]) -> bool { - //DISCUSSION: This function appears overly complicated primarily because it needs to track - // whether or not a both the val and the rec each cofree are requested, but we don't have a bitmask - // in advance that records vals and rec links separately. Since we don't want nested loops, we leverage - // the fact that a rec must be requested before a val, to stash the val for the next trip through the - // loop. The loop body therefore is an annoying state-machine. But at least it's not that much code. - - //Becomes true if only half of `(Some, Some)` CoFree is requested, without requesting the other half - // This flag never gets unset once it gets set - let mut unrequested_cofree_half = false; - //Temporary state that bridges across multiple requests into a `(Some, Some)` CoFree, by holding the - // val until it's requested, leveraging the fact that values are requested after rec links - let mut stashed_val: Option<&V> = None; - //Tracks whether the current CoFree's val has been taken. So, `last_byte` toggles to `Some` and stays - // at `Some` until we move onto a different CF, while `stashed_val` toggles to `Some`, and toggles back - // as soon as the value is requested. - let mut last_byte: Option = None; - //Tracks which CoFrees have yet to be requested from the node - let mut requested_mask = ByteMask::from(self.mask); - - debug_assert!(results.len() >= keys.len()); - for ((key, expect_val), (result_key_len, payload_ref)) in keys.into_iter().zip(results.iter_mut()) { - if key.len() > 0 { - let byte = key[0]; - - //Check to see if we had a Val from the CoFree that we aren't going to request - match &last_byte { - Some(prev_byte) => { - if byte != *prev_byte { - if stashed_val.is_some() { - unrequested_cofree_half = true; - } - stashed_val = None; - last_byte = None; - } - }, - None => {} - } - - //Check to see if this trip through the loop is the request for the stashed val - match stashed_val { - Some(val) => { - if key.len() == 1 && *expect_val { - *result_key_len = 1; - *payload_ref = PayloadRef::Val(val); - stashed_val = None; - continue; - } - }, - None => {} - } - - requested_mask.clear_bit(byte); - match self.get(byte) { - Some(cf) => { - // An exact value-only request does not enumerate an onward link stored in the - // same CoFree. The preceding stashed-value fast path means this branch is - // reached only when that link was not requested separately. - if key.len() == 1 && *expect_val && cf.has_rec() { - unrequested_cofree_half = true; - } - - //A key longer than 1 byte or an explicit request for a rec link can be answered with a Child - if key.len() > 1 || !*expect_val { - match cf.rec() { - Some(rec) => { - *result_key_len = 1; - *payload_ref = PayloadRef::Child(rec); - }, - None => {} - } - } - match cf.val() { - Some(val) => { - //Answer an explicit request for this val, or stash the val for - if key.len() == 1 && *expect_val { - debug_assert!(stashed_val.is_none()); - *result_key_len = 1; - *payload_ref = PayloadRef::Val(val); - } else { - if last_byte.is_none() { - stashed_val = Some(val); - last_byte = Some(byte); - } - } - }, - None => {} - } - }, - None => {} - } - } - } - - !unrequested_cofree_half && stashed_val.is_none() && requested_mask.is_empty_mask() - } fn node_contains_val(&self, key: &[u8]) -> bool { if key.len() == 1 { match self.get(key[0]) { @@ -1970,18 +1874,54 @@ impl, Other rec_status.merge(val_status, true, true) } fn pmeet(&self, other: &OtherCf) -> AlgebraicResult { - //If one or the other cofree is dangling, it's an identity result for the dangling cofree - let mut identity_flag = 0; - if !self.has_rec() && !self.has_val() {identity_flag = SELF_IDENT;} - if !other.has_rec() && !other.has_val() {identity_flag |= COUNTER_IDENT;} - if identity_flag > 0 { - return AlgebraicResult::Identity(identity_flag) + //The location exists on both sides, so it survives even if nothing below does + let self_rec = self.rec().filter(|node| !node.as_tagged().node_is_empty()); + let other_rec = other.rec().filter(|node| !node.as_tagged().node_is_empty()); + let self_dangling = self_rec.is_none() && !self.has_val(); + let other_dangling = other_rec.is_none() && !other.has_val(); + if self_dangling || other_dangling { + //The meet is the bare location, which is exactly what a dangling side holds + let mut mask = 0; + if self_dangling { mask |= SELF_IDENT; } + if other_dangling { mask |= COUNTER_IDENT; } + return AlgebraicResult::Identity(mask) + } + + let rec = match (self_rec, other_rec) { + (Some(l), Some(r)) => l.pmeet(r), + _ => AlgebraicResult::None, + }; + let val = self.val().pmeet(&other.val()); + + //A part that meets to nothing equals the side that had nothing there + let (rec_self, rec_counter) = match &rec { + AlgebraicResult::Identity(mask) => (mask & SELF_IDENT > 0, mask & COUNTER_IDENT > 0), + AlgebraicResult::None => (self_rec.is_none(), other_rec.is_none()), + AlgebraicResult::Element(_) => (false, false), + }; + let (val_self, val_counter) = match &val { + AlgebraicResult::Identity(mask) => (mask & SELF_IDENT > 0, mask & COUNTER_IDENT > 0), + AlgebraicResult::None => (!self.has_val(), !other.has_val()), + AlgebraicResult::Element(_) => (false, false), + }; + let mut mask = 0; + if rec_self && val_self { mask |= SELF_IDENT; } + if rec_counter && val_counter { mask |= COUNTER_IDENT; } + if mask > 0 { + return AlgebraicResult::Identity(mask) } - //Otherwise actually work with what the cofrees contain - let rec = self.rec().pmeet(&other.rec()); - let val = self.val().pmeet(&other.val()); - self.combine_algebraic_results(other, rec, val) + let new_rec = match rec { + AlgebraicResult::Element(node) => Some(node), + AlgebraicResult::Identity(mask) => if mask & SELF_IDENT > 0 { self_rec.cloned() } else { other_rec.cloned() }, + AlgebraicResult::None => None, + }; + let new_val = match val { + AlgebraicResult::Element(val) => val, + AlgebraicResult::Identity(mask) => if mask & SELF_IDENT > 0 { self.val().cloned() } else { other.val().cloned() }, + AlgebraicResult::None => None, + }; + AlgebraicResult::Element(Self::new(new_rec, new_val)) } //GOAT, HeteroLattice will totally disappear when we do the policy refactor // fn join_all(_xs: &[&Self]) -> Self where Self: Sized { @@ -2445,6 +2385,56 @@ impl> ByteNode where Self: TrieNodeDowncast { + /// See [node_drop_dangling] + pub(crate) fn drop_dangling(&self, src: Option>) -> DropDangling { + let mut new_node: Option = None; + for (idx, byte) in self.mask.iter().enumerate() { + let cf = unsafe{ self.values.get_unchecked(idx) }; + let rec = match cf.rec() { + Some(child) => node_drop_dangling(child, meet_src_child(src, &[byte])), + None => DropDangling::Empty, + }; + let unchanged = match (&rec, cf.rec()) { + (DropDangling::Unchanged, _) => true, + (DropDangling::Empty, None) => cf.has_val(), + _ => false, + }; + if unchanged && new_node.is_none() { + continue + } + let new_node = new_node.get_or_insert_with(|| { + let mut node = Self::with_capacity_in(self.values.len(), self.alloc.clone()); + for (prev_idx, prev_byte) in self.mask.iter().enumerate().take(idx) { + node.set_cf(prev_byte, unsafe{ self.values.get_unchecked(prev_idx) }.rec().cloned(), unsafe{ self.values.get_unchecked(prev_idx) }.val().cloned()); + } + node + }); + let new_rec = match rec { + DropDangling::Unchanged => cf.rec().cloned(), + DropDangling::Empty => None, + DropDangling::New(node) => Some(node), + }; + if new_rec.is_some() || cf.has_val() { + new_node.set_cf(byte, new_rec, cf.val().cloned()); + } + } + match new_node { + None => DropDangling::Unchanged, + Some(node) if node.values.len() == 0 => DropDangling::Empty, + Some(node) => DropDangling::New(TrieNodeODRc::new_in(node, self.alloc.clone())), + } + } + fn set_cf(&mut self, byte: u8, rec: Option>, val: Option) { + if let Some(rec) = rec { + self.set_child(byte, rec); + } + if let Some(val) = val { + self.set_val(byte, val); + } + } +} + impl> ByteNode { fn prestrict>(&self, other: &ByteNode) -> AlgebraicResult where Self: Sized { // Iterate the overlap mask directly. Slot indexes are recovered with diff --git a/src/empty_node.rs b/src/empty_node.rs index ce336dd1..0cf79d06 100644 --- a/src/empty_node.rs +++ b/src/empty_node.rs @@ -26,9 +26,6 @@ impl TrieNode for EmptyNode { fn node_replace_child(&mut self, _key: &[u8], _new_node: TrieNodeODRc) { unreachable!() //Should not be called unless it's known that the node being replaced exists } - fn node_get_payloads<'node, 'res>(&'node self, _keys: &[(&[u8], bool)], _results: &'res mut [(usize, PayloadRef<'node, V, A>)]) -> bool { - true - } fn node_contains_val(&self, _key: &[u8]) -> bool { false } diff --git a/src/lib.rs b/src/lib.rs index 6d38dc03..f8ffb636 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -593,8 +593,10 @@ mod tests { assert_eq!(met, l); } + //No common values, but the shared path prefix survives as dangling let met = met.meet(&r); - assert!(met.is_empty()); + assert_eq!(met.val_count(), 0); + assert!(!met.is_empty()); } #[test] diff --git a/src/line_list_node.rs b/src/line_list_node.rs index 527c6974..c705a317 100644 --- a/src/line_list_node.rs +++ b/src/line_list_node.rs @@ -1781,52 +1781,6 @@ impl TrieNode for LineListNode debug_assert!(consumed_bytes == key.len()); *child_node = new_node; } - fn node_get_payloads<'node, 'res>(&'node self, keys: &[(&[u8], bool)], results: &'res mut [(usize, PayloadRef<'node, V, A>)]) -> bool { - let mut slot_0_requested = !self.is_used::<0>(); - let mut slot_1_requested = !self.is_used::<1>(); - let (node_key_0, node_key_1) = self.get_both_keys(); - - debug_assert!(results.len() >= keys.len()); - for ((key, expect_val), (result_key_len, payload_ref)) in keys.into_iter().zip(results.iter_mut()) { - if self.is_used::<0>() { - if starts_with(key, node_key_0) { - let node_key_len = node_key_0.len(); - if self.is_child_ptr::<0>() { - if !*expect_val || node_key_len < key.len() { - slot_0_requested = true; - *result_key_len = node_key_len; - *payload_ref = PayloadRef::Child(unsafe{ &*self.val_or_child0.child }); - } - } else { - if *expect_val && node_key_len == key.len() { - slot_0_requested = true; - *result_key_len = node_key_len; - *payload_ref = PayloadRef::Val(unsafe{ &**self.val_or_child0.val }); - } - } - } - } - if self.is_used::<1>() { - if starts_with(key, node_key_1) { - let node_key_len = node_key_1.len(); - if self.is_child_ptr::<1>() { - if !*expect_val || node_key_len < key.len() { - slot_1_requested = true; - *result_key_len = node_key_len; - *payload_ref = PayloadRef::Child(unsafe{ &*self.val_or_child1.child }); - } - } else { - if *expect_val && node_key_len == key.len() { - slot_1_requested = true; - *result_key_len = node_key_len; - *payload_ref = PayloadRef::Val(unsafe{ &**self.val_or_child1.val }); - } - } - } - } - } - slot_0_requested && slot_1_requested - } fn node_contains_val(&self, key: &[u8]) -> bool { self.contains_val(key) } @@ -2945,58 +2899,129 @@ impl LineListNode { pub(crate) fn pmeet_dyn_oriented(&self, other: TaggedNodeRef, swapped: bool) -> AlgebraicResult> where V: Lattice { debug_assert!(validate_node(self)); - let mut self_payloads_buf: [(&[u8], PayloadRef); 2] = [(&[], PayloadRef::None); 2]; - //A shadowed dangling slot carries nothing, so skip it - let skipped = if self.slot_is_shadowed_dangling(0) { - Some(0) - } else if self.slot_is_shadowed_dangling(1) { - Some(1) + let (use0, use1) = match self.used_slot_count() { + 0 => return AlgebraicResult::None, + 1 => (true, false), + _ => { + if self.slot_is_shadowed_dangling(0) { + (false, true) + } else if self.slot_is_shadowed_dangling(1) { + (true, false) + } else { + (true, true) + } + } + }; + let (key0, key1) = self.get_both_keys(); + + let out0 = if use0 { + meet_list_slot(key0, unsafe{ self.payload_in_slot::<0>() }, other, swapped) } else { - None + (true, true, SlotMeet::Skipped) + }; + let out1 = if use1 { + meet_list_slot(key1, unsafe{ self.payload_in_slot::<1>() }, other, swapped) + } else { + (true, true, SlotMeet::Skipped) }; + if out0.2.is_nothing() && out1.2.is_nothing() { + return AlgebraicResult::None + } - let self_slot_count = self.used_slot_count(); - let self_payloads = match (self_slot_count, skipped) { - (0, _) => return AlgebraicResult::None, - (_, Some(0)) => { - let key = unsafe{ self.key_unchecked::<1>() }; - let payload = unsafe{ self.payload_in_slot::<1>() }; - self_payloads_buf[0] = (key, payload); - &self_payloads_buf[..1] - }, - (1, _) | (_, Some(1)) => { - let key = unsafe{ self.key_unchecked::<0>() }; - let payload = unsafe{ self.payload_in_slot::<0>() }; - self_payloads_buf[0] = (key, payload); - &self_payloads_buf[..1] + let mut mask = 0; + if out0.0 && out1.0 { + mask |= SELF_IDENT; + } + if swapped && out0.1 && out1.1 { + let slots = [ + if use0 { Some((key0, self.is_child_ptr::<0>(), out0.2.reach(key0.len()))) } else { None }, + if use1 { Some((key1, self.is_child_ptr::<1>(), out1.2.reach(key1.len()))) } else { None }, + ]; + if meet_other_within_slots(other, &slots) { + mask |= COUNTER_IDENT; + } + } + if mask > 0 { + return AlgebraicResult::Identity(mask) + } + + //Build the result from what each slot contributes + let mut items: [Option<(&[u8], ValOrChild)>; 2] = [ + out0.2.into_item(key0, || self.clone_payload::<0>().unwrap()), + out1.2.into_item(key1, || self.clone_payload::<1>().unwrap()), + ]; + //Drop a dangling item the other item covers, keeping the node valid + for i in 0..2 { + let j = 1 - i; + let redundant = match (&items[i], &items[j]) { + (Some((key_i, ValOrChild::Child(child_i))), Some((key_j, payload_j))) if child_i.as_tagged().node_is_empty() && key_j.starts_with(key_i) => { + let j_dangling = matches!(payload_j, ValOrChild::Child(child_j) if child_j.as_tagged().node_is_empty()); + key_j.len() > key_i.len() || !j_dangling || i > j + }, + _ => false + }; + if redundant { + items[i] = None; + } + } + let mut new_node = Self::new_in(self.alloc.clone()); + let [item0, item1] = items; + match (item0, item1) { + (Some((key0, payload0)), Some((key1, payload1))) => { + unsafe{ new_node.set_payload_owned::<0>(key0, payload0); } + unsafe{ new_node.set_payload_owned::<1>(key1, payload1); } }, - (2, None) => { - let (key0, key1) = self.get_both_keys(); - let payload0 = unsafe{ self.payload_in_slot::<0>() }; - let payload1 = unsafe{ self.payload_in_slot::<1>() }; - self_payloads_buf[0] = (key0, payload0); - self_payloads_buf[1] = (key1, payload1); - &self_payloads_buf[..2] + (Some((key, payload)), None) | (None, Some((key, payload))) => { + unsafe{ new_node.set_payload_owned::<0>(key, payload); } }, - _ => unsafe{ unreachable_unchecked() } - }; + (None, None) => unreachable!() + } + debug_assert!(validate_node(&new_node)); + AlgebraicResult::Element(TrieNodeODRc::new_in(new_node, self.alloc.clone())) + } - pmeet_generic::<2, V, A, _>(self_payloads, other, swapped, |payloads| { - debug_assert_eq!(payloads.len(), self_payloads.len()); - //With a slot skipped, the single result belongs to the slot that stayed in, and the - // skipped one is dropped -- which is what the meet would have done with it anyway. - let (slot0_payload, slot1_payload) = match skipped { - Some(0) => (None, payloads.get_mut(0).and_then(|p| core::mem::take(p)).map(|p| p.into())), - Some(_) => (payloads.get_mut(0).and_then(|p| core::mem::take(p)).map(|p| p.into()), None), - None => ( - payloads.get_mut(0).and_then(|p| core::mem::take(p)).map(|p| p.into()), - payloads.get_mut(1).and_then(|p| core::mem::take(p)).map(|p| p.into()), - ), - }; - let new_node = self.clone_with_updated_payloads(slot0_payload, slot1_payload).unwrap(); - TrieNodeODRc::new_in(new_node, self.alloc.clone()) - }) + /// See [node_drop_dangling] + pub(crate) fn drop_dangling(&self, src: Option>) -> DropDangling { + let (key0, key1) = self.get_both_keys(); + let slot_result = |key: &[u8], is_child: bool, child: fn(&Self) -> &TrieNodeODRc| { + if is_child { + node_drop_dangling(child(self), meet_src_child(src, key)) + } else { + DropDangling::Unchanged + } + }; + let result0 = if self.is_used::<0>() { + slot_result(key0, self.is_child_ptr::<0>(), |node| unsafe{ node.child_in_slot::<0>() }) + } else { + DropDangling::Empty + }; + let result1 = if self.is_used::<1>() { + slot_result(key1, self.is_child_ptr::<1>(), |node| unsafe{ node.child_in_slot::<1>() }) + } else { + DropDangling::Empty + }; + let is_unchanged = |result: &DropDangling, used: bool| matches!(result, DropDangling::Unchanged) || (!used && matches!(result, DropDangling::Empty)); + if is_unchanged(&result0, self.is_used::<0>()) && is_unchanged(&result1, self.is_used::<1>()) { + return DropDangling::Unchanged + } + let payload = |result: DropDangling, slot: usize| match result { + DropDangling::Unchanged => if slot == 0 { self.clone_payload::<0>() } else { self.clone_payload::<1>() }, + DropDangling::Empty => None, + DropDangling::New(node) => Some(ValOrChild::Child(node)), + }; + let mut new_node = Self::new_in(self.alloc.clone()); + match (payload(result0, 0), payload(result1, 1)) { + (Some(payload0), Some(payload1)) => { + unsafe{ new_node.set_payload_owned::<0>(key0, payload0); } + unsafe{ new_node.set_payload_owned::<1>(key1, payload1); } + }, + (Some(payload), None) => unsafe{ new_node.set_payload_owned::<0>(key0, payload); }, + (None, Some(payload)) => unsafe{ new_node.set_payload_owned::<0>(key1, payload); }, + (None, None) => return DropDangling::Empty, + } + debug_assert!(validate_node(&new_node)); + DropDangling::New(TrieNodeODRc::new_in(new_node, self.alloc.clone())) } /// Part of the implementation of methods the remove subtries from a node diff --git a/src/ring.rs b/src/ring.rs index 100edaed..d54aa9b9 100644 --- a/src/ring.rs +++ b/src/ring.rs @@ -425,36 +425,6 @@ impl FatAlgebraicResult { pub(crate) const fn new(identity_mask: u64, element: Option) -> Self { Self {identity_mask, element} } - /// Converts an [AlgebraicResult] into a `FatAlgebraicResult`, assuming the source `result` was the - /// output of a binary operation (two arguments). - #[inline] - pub(crate) fn from_binary_op_result(result: AlgebraicResult, a: &V, b: &V) -> Self - where V: Clone - { - match result { - AlgebraicResult::None => FatAlgebraicResult::none(), - AlgebraicResult::Element(v) => FatAlgebraicResult::element(v), - AlgebraicResult::Identity(mask) => { - debug_assert!(mask <= (SELF_IDENT | COUNTER_IDENT)); - if mask & SELF_IDENT > 0 { - FatAlgebraicResult::new(mask, Some(a.clone())) - } else { - debug_assert_eq!(mask, COUNTER_IDENT); - FatAlgebraicResult::new(mask, Some(b.clone())) - } - } - } - } - /// Maps a `FatAlgebraicResult` to `FatAlgebraicResult` by applying a function to a contained value - #[inline] - pub fn map(self, f: F) -> FatAlgebraicResult - where F: FnOnce(V) -> U, - { - FatAlgebraicResult:: { - identity_mask: self.identity_mask, - element: self.element.map(f) - } - } /// The result of an operation between non-none arguments that results in None #[inline(always)] pub(crate) const fn none() -> Self { diff --git a/src/tiny_node.rs b/src/tiny_node.rs index 6ece3c72..c76ad615 100644 --- a/src/tiny_node.rs +++ b/src/tiny_node.rs @@ -149,33 +149,6 @@ impl<'a, V: Clone + Send + Sync, A: Allocator> TrieNode for TinyRefNode<'a } fn node_get_child_mut(&mut self, _key: &[u8]) -> Option<(usize, &mut TrieNodeODRc)> { unreachable!() } fn node_replace_child(&mut self, _key: &[u8], _new_node: TrieNodeODRc) { unreachable!() } - fn node_get_payloads<'node, 'res>(&'node self, keys: &[(&[u8], bool)], results: &'res mut [(usize, PayloadRef<'node, V, A>)]) -> bool { - if self.node_is_empty() { - return true - } - let mut requested_contained_item = false; // This node type only has one item - let self_key = self.key(); - debug_assert!(results.len() >= keys.len()); - for ((key, expect_val), (result_key_len, payload_ref)) in keys.into_iter().zip(results.into_iter()) { - if starts_with(key, self_key) { - let self_key_len = self_key.len(); - if self.is_child_ptr() { - if !*expect_val || self_key_len < key.len() { - requested_contained_item = true; - *result_key_len = self_key_len; - *payload_ref = PayloadRef::Child(unsafe{ &*self.payload.child }); - } - } else { - if *expect_val && self_key_len == key.len() { - requested_contained_item = true; - *result_key_len = self_key_len; - *payload_ref = PayloadRef::Val(unsafe{ &**self.payload.val }); - } - } - } - } - requested_contained_item - } fn node_contains_val(&self, key: &[u8]) -> bool { if self.is_used_val() { let node_key = self.key(); diff --git a/src/trie_node.rs b/src/trie_node.rs index bea4d08f..cf3a844f 100644 --- a/src/trie_node.rs +++ b/src/trie_node.rs @@ -5,7 +5,6 @@ use core::ptr::NonNull; use std::collections::HashMap; use dyn_clone::*; use local_or_heap::LocalOrHeap; -use arrayvec::ArrayVec; use crate::utils::ByteMask; use crate::alloc::Allocator; @@ -72,35 +71,6 @@ pub(crate) trait TrieNode: TrieNodeDowncas /// cheaper, but it is adequate for the places that call it fn node_replace_child(&mut self, key: &[u8], new_node: TrieNodeODRc); - /// Retrieves multiple values or child links from the node, associated with elements from `keys`, - /// and places them into the respective element in `results` - /// - /// The `bool` in `keys` indicates whether a value is expected at the requested key. `true` will be - /// passed to indicate a **value**. (WARNING: This is different from the convention in some node types) - /// - /// If a node contains both an onward link and a value at the same key, the `bool` specifies which to - /// return; however, a node may be returned for a requested value, if the path to the node is a prefix - /// to the path to the requested value. This is because the value may live within a child node. On - /// the other hand, a value will only be returned if it is an exact match with the key provided. - /// - /// The `usize` in `results` functions the same way as the returned `usize` in [TrieNode::node_get_child], - /// to indicate the number of key bytes matched by the key contained within the node. - /// - /// The implementation may assume `keys` will be in sorted order, and `false` sorts before `true` if - /// both a value and a node at the same key are requested. - /// - /// Returns `true` if the requested `keys` completely enumerate the set of elements contained within - /// the node, or `false` if the node contains additional elements that were not requested - /// - /// If a result is not found for a given key, the implementation does not guarantee the corresponding - /// element in `results` will be set to [`PayloadRef::None`], therefore the caller should init `results` - /// to default values. - /// - /// Panics if `keys.len() > results.len()` - /// - /// NOTE: It perfectly fine for multiple keys to share a prefix, and sometimes that means multiple - /// results will be identical if the node represents only the prefix portion of the key. - fn node_get_payloads<'node, 'res>(&'node self, keys: &[(&[u8], bool)], results: &'res mut [(usize, PayloadRef<'node, V, A>)]) -> bool; /// Returns `true` if the node contains a value at the specified key, otherwise returns `false` /// @@ -526,32 +496,6 @@ impl Default for PayloadRef<'_, V, A> { } } -impl<'a, V: Clone + Send + Sync, A: Allocator> PayloadRef<'a, V, A> { - pub fn is_none(&self) -> bool { - match self { - Self::None => true, - _ => false - } - } - pub fn is_val(&self) -> bool { - match self { - Self::Val(_) => true, - _ => false - } - } - pub fn child(&self) -> &'a TrieNodeODRc { - match self { - Self::Child(child) => child, - _ => panic!() - } - } - pub fn val(&self) -> &'a V { - match self { - Self::Val(val) => val, - _ => panic!() - } - } -} #[derive(Clone)] pub(crate) enum ValOrChild { @@ -630,62 +574,224 @@ impl ValOrChildUnion { } } -/// An implementation of pmeet_dyn that should be correct for any two node types, Although it -/// certainly won't be optimally efficient. -/// -/// WARNING: just like [TrieNode::node_get_payloads], the keys in `self_payloads` must be in -/// sorted order. -// -//NOTE: I have confirmed that this function behaves no more conservatively than the function it replaced. -// In other words, I have confirmed that, in tests where the old function was behaving correctly, this -// function returns *identical* results. Furthermore those same tests are the ones where the 20% slowdown -// was observed. Therefore the the ~20% slowdown is simply the higher overheads of this generic function. -// -//The next port of call for optimization is probably to remove the recursion -// -/// `swapped` says which operand `self_payloads` came from. The meet is left-biased (a `Lattice` -/// impl resolves a collision as `left.pmeet(right)`), so when a caller enumerates the *right* -/// operand's payloads because that node type is the easier one to iterate, it passes `swapped = -/// true`: every value and every recursive node meet is then computed as `other op self` and the -/// identity masks are re-expressed relative to `self_payloads`. The caller still applies -/// `invert_identity()` to the final result to get back to its own orientation. Without this, a -/// dense-node-versus-list-node meet returned the list node's values regardless of which side it -/// was on. -pub(crate) fn pmeet_generic(self_payloads: &[(&[u8], PayloadRef)], other: TaggedNodeRef, swapped: bool, merge_f: MergeF) -> AlgebraicResult> - where - MergeF: FnOnce(&mut [Option>]) -> TrieNodeODRc, - V: Clone + Send + Sync + Lattice -{ - let mut request_keys = ArrayVec::<(&[u8], bool), MAX_PAYLOAD_CNT>::new(); - let mut element_results = ArrayVec::>, MAX_PAYLOAD_CNT>::new(); - let mut request_results = ArrayVec::<(usize, PayloadRef), MAX_PAYLOAD_CNT>::new(); - for (self_key, self_payload) in self_payloads.iter() { - debug_assert!(!self_payload.is_none()); - request_keys.push((self_key, self_payload.is_val())); - element_results.push(FatAlgebraicResult::none()); - request_results.push((0, PayloadRef::default())); +/// The node holding the rest of `key` after following onward links, and that rest +#[inline] +pub(crate) fn meet_locate_key<'n, 'k, V: Clone + Send + Sync, A: Allocator>(mut node: TaggedNodeRef<'n, V, A>, mut key: &'k [u8]) -> (TaggedNodeRef<'n, V, A>, &'k [u8]) { + debug_assert!(key.len() > 0); + while let Some((consumed, child)) = node.node_get_child(key) { + if consumed >= key.len() { + break + } + node = child.as_tagged(); + key = &key[consumed..]; } + (node, key) +} - let is_exhaustive = pmeet_generic_internal::(self_payloads, &mut request_keys[..], &mut request_results[..], &mut element_results[..], other, swapped); - let mut is_none = true; - let mut combined_mask = SELF_IDENT | COUNTER_IDENT; - let mut result_payloads = ArrayVec::>, MAX_PAYLOAD_CNT>::new(); - for result in element_results { - combined_mask &= result.identity_mask; - is_none = is_none && result.element.is_none(); - result_payloads.push(result.element); +/// The onward node exactly at `key` in `src`, if there is one +#[inline] +pub(crate) fn meet_src_child<'a, V: Clone + Send + Sync, A: Allocator>(src: Option>, key: &[u8]) -> Option> { + let (node, rest) = meet_locate_key(src?, key); + match node.node_get_child(rest) { + Some((consumed, child)) if consumed == rest.len() => Some(child.as_tagged()), + _ => None } +} + +/// The outcome of [node_drop_dangling] +pub(crate) enum DropDangling { + /// The node is kept as it is + Unchanged, + /// No value is left below the node's root + Empty, + /// The node with its dangling paths dropped + New(TrieNodeODRc), +} + +/// Drops dangling paths below `node`; nodes shared with `src` are skipped +pub(crate) fn node_drop_dangling(node: &TrieNodeODRc, src: Option>) -> DropDangling { + let tagged = node.as_tagged(); + if tagged.node_is_empty() { + return DropDangling::Empty + } + if let Some(src) = src { + if tagged.shared_node_id() == src.shared_node_id() { + return DropDangling::Unchanged + } + } + match tagged.tag() { + DENSE_BYTE_NODE_TAG => unsafe{ tagged.as_dense_unchecked() }.drop_dangling(src), + LINE_LIST_NODE_TAG => unsafe{ tagged.as_list_unchecked() }.drop_dangling(src), + CELL_BYTE_NODE_TAG => unsafe{ tagged.as_cell_unchecked() }.drop_dangling(src), + _ => unreachable!() + } +} - if is_none { - return AlgebraicResult::None +/// The number of branches below `key` in the trie rooted at `node` +fn meet_count_branches_at(node: TaggedNodeRef, key: &[u8]) -> usize { + if key.is_empty() { + return node.count_branches(&[]) } - if !is_exhaustive { - combined_mask &= !COUNTER_IDENT; + let (node, rest) = meet_locate_key(node, key); + match node.node_get_child(rest) { + Some((consumed, child)) if consumed == rest.len() => child.as_tagged().count_branches(&[]), + _ => node.count_branches(rest) } - if combined_mask > 0 { - return AlgebraicResult::Identity(combined_mask) +} + +/// What one slot of a list node contributes to a meet; see [meet_list_slot] +pub(crate) enum SlotMeet { + /// The slot was left out of the meet + Skipped, + /// Not even the first byte of the slot's key exists in the other operand + Nothing, + /// The slot's own payload, unchanged + Keep, + /// A value at the slot's key + Val(V), + /// An onward node at the slot's key + Child(TrieNodeODRc), + /// A dangling path along the first `n` bytes of the slot's key + Dangling(usize), +} + +impl SlotMeet { + #[inline] + pub(crate) fn is_nothing(&self) -> bool { + matches!(self, Self::Nothing | Self::Skipped) + } + /// How many bytes of the slot's key exist in the result + #[inline] + pub(crate) fn reach(&self, key_len: usize) -> usize { + match self { + Self::Skipped | Self::Nothing => 0, + Self::Dangling(n) => *n, + _ => key_len + } + } + /// The key and payload of the contribution; `keep` supplies the slot's own payload + #[inline] + pub(crate) fn into_item<'k, F: FnOnce() -> ValOrChild>(self, key: &'k [u8], keep: F) -> Option<(&'k [u8], ValOrChild)> { + match self { + Self::Skipped | Self::Nothing => None, + Self::Keep => Some((key, keep())), + Self::Val(val) => Some((key, ValOrChild::Val(val))), + Self::Child(node) => Some((key, ValOrChild::Child(node))), + Self::Dangling(n) => Some((&key[..n], ValOrChild::Child(TrieNodeODRc::new_empty()))), + } } - AlgebraicResult::Element(merge_f(&mut result_payloads[..])) +} + +/// Meets one list-node slot against `other`. Returns `(self_ident, counter_ok, contribution)` +pub(crate) fn meet_list_slot(key: &[u8], payload: PayloadRef, other: TaggedNodeRef, swapped: bool) -> (bool, bool, SlotMeet) { + let (node, rest) = meet_locate_key(other, key); + let exact_child = match node.node_get_child(rest) { + Some((consumed, child)) if consumed == rest.len() => Some(child), + _ => None + }; + let other_val = node.node_get_val(rest); + if exact_child.is_none() && other_val.is_none() && !node.node_contains_partial_key(rest) { + //`other` stops partway along the key: the part both have is a dangling path + let reach = key.len() - rest.len() + node.node_key_overlap(rest); + return (false, true, if reach > 0 { SlotMeet::Dangling(reach) } else { SlotMeet::Nothing }) + } + + match payload { + PayloadRef::Val(self_val) => match other_val { + Some(other_val) => { + let result = if swapped { other_val.pmeet(self_val).invert_identity() } else { self_val.pmeet(other_val) }; + match result { + AlgebraicResult::Identity(mask) => if mask & SELF_IDENT > 0 { + (true, mask & COUNTER_IDENT > 0, SlotMeet::Keep) + } else { + (false, true, SlotMeet::Val(other_val.clone())) + }, + AlgebraicResult::Element(val) => (false, false, SlotMeet::Val(val)), + AlgebraicResult::None => (false, false, SlotMeet::Dangling(key.len())), + } + }, + None => (false, true, SlotMeet::Dangling(key.len())), + }, + PayloadRef::Child(self_child) => { + let onward = match exact_child { + Some(child) => AbstractNodeRef::BorrowedRc(child), + None => node.get_node_at_key(rest), + }; + let self_empty = self_child.as_tagged().node_is_empty(); + let other_empty = match onward.try_as_tagged() { + Some(below) => below.node_is_empty(), + None => true + }; + if self_empty || other_empty { + //Nothing below the key survives, which leaves the key itself + let contribution = if self_empty { SlotMeet::Keep } else { SlotMeet::Dangling(key.len()) }; + return (self_empty, other_empty, contribution) + } + let result = { + let other_below = onward.as_tagged(); + if swapped { + other_below.pmeet_dyn(self_child.as_tagged()).invert_identity() + } else { + self_child.as_tagged().pmeet_dyn(other_below) + } + }; + match result { + AlgebraicResult::Identity(mask) => if mask & SELF_IDENT > 0 { + (true, mask & COUNTER_IDENT > 0, SlotMeet::Keep) + } else { + (false, true, SlotMeet::Child(onward.into_option().unwrap())) + }, + AlgebraicResult::Element(node) => (false, false, SlotMeet::Child(node)), + AlgebraicResult::None => (false, false, SlotMeet::Dangling(key.len())), + } + }, + PayloadRef::None => unreachable!() + } +} + +/// Whether all of `other` lies within the met slots +pub(crate) fn meet_other_within_slots(other: TaggedNodeRef, slots: &[Option<(&[u8], bool, usize)>; 2]) -> bool { + //The distinct bytes the slots continue with after `prefix` + let branches_expected = |prefix: &[u8]| -> usize { + let depth = prefix.len(); + let mut first = None; + let mut count = 0; + for (key, _, reach) in slots.iter().flatten() { + if *reach > depth && &key[..depth] == prefix { + let byte = key[depth]; + if first != Some(byte) { + count += 1; + first = Some(byte); + } + } + } + count + }; + if meet_count_branches_at(other, &[]) != branches_expected(&[]) { + return false + } + for (key, _, reach) in slots.iter().flatten() { + for depth in 1..=*reach { + let prefix = &key[..depth]; + //A value slot can share its key with an onward-node slot, which then answers for below + let below_is_met = slots.iter().flatten().any(|(slot_key, slot_is_child, slot_reach)| { + *slot_is_child && *slot_reach == depth && *slot_key == prefix + }); + if !below_is_met && meet_count_branches_at(other, prefix) != branches_expected(prefix) { + return false + } + let (node, rest) = meet_locate_key(other, prefix); + if node.node_get_val(rest).is_some() { + let covered = slots.iter().flatten().any(|(slot_key, slot_is_child, slot_reach)| { + !*slot_is_child && *slot_reach == depth && *slot_key == prefix + }); + if !covered { + return false + } + } + } + } + true } pub(crate) fn node_count_branches_recursive(node: TaggedNodeRef, key: &[u8]) -> usize { @@ -705,128 +811,7 @@ pub(crate) fn node_count_branches_recursive(self_payloads: &[(&[u8], PayloadRef)], keys: &mut [(&[u8], bool)], request_results: &mut [(usize, PayloadRef<'trie, V, A>)], results: &mut [FatAlgebraicResult>], other_node: TaggedNodeRef<'trie, V, A>, swapped: bool) -> bool - where V: Clone + Send + Sync + Lattice -{ - //If is_exhaustive gets set to `false`, then the pmeet method cannot return a `COUNTER_IDENTITY` result - let mut is_exhaustive = true; - - //Get the payload results from the node - if !other_node.node_get_payloads(&keys[..], request_results) { - is_exhaustive = false; - } - - //Divide the results into groups based on the returned node. Because keys must be - // in sorted order, we can assume that query results returning the same node will - // be contiguous. - //NOTE: It's theoretically possible (although pretty unlikely) that a node will - // have multiple discontinuous internal paths leading to the same child node, however - // the TrieNodeODRc pointers will be different in that case, so this logic is still - // correct. - let mut cur_group: Option<(usize, &TrieNodeODRc)> = None; - for idx in 0..keys.len() { - let (consumed_bytes, payload) = core::mem::take(request_results.get_mut(idx).unwrap()); - if !payload.is_none() { - let is_val = keys[idx].1; - if consumed_bytes < keys[idx].0.len() { - keys[idx].0 = &keys[idx].0[consumed_bytes..]; - debug_assert!(!payload.is_val()); - let child = payload.child(); - - //Continue to grow range, or do the recursive call, depending on whether - // we have the same node as the previous time through the loop - if cur_group.is_some() { - if (cur_group.as_ref().unwrap().1 as *const TrieNodeODRc) != (child as *const TrieNodeODRc) { - pmeet_generic_recursive_reset::(&mut cur_group, &mut is_exhaustive, idx, self_payloads, keys, request_results, results, swapped); - cur_group = Some((idx, child)); - } - } else { - cur_group = Some((idx, child)); - } - } else { - pmeet_generic_recursive_reset::(&mut cur_group, &mut is_exhaustive, idx, self_payloads, keys, request_results, results, swapped); - - //We've arrived at a contained value or onward link that has a correspondence - // to one of the values or links in `self` - debug_assert_eq!(consumed_bytes, keys[idx].0.len()); - debug_assert_eq!(is_val, payload.is_val()); - let result = match &self_payloads[idx].1 { - PayloadRef::Child(self_link) => { - let other_link = payload.child(); - let result = if swapped { other_link.pmeet(self_link).invert_identity() } else { self_link.pmeet(other_link) }; - FatAlgebraicResult::from_binary_op_result(result, self_link, other_link) - .map(|child| ValOrChild::Child(child)) - }, - PayloadRef::Val(self_val) => { - let other_val = payload.val(); - let result = if swapped { other_val.pmeet(*self_val).invert_identity() } else { (*self_val).pmeet(other_val) }; - FatAlgebraicResult::from_binary_op_result(result, *self_val, other_val) - .map(|val| ValOrChild::Val(val)) - }, - _ => unreachable!() - }; - debug_assert!(results[idx].element.is_none()); - debug_assert_eq!(results[idx].identity_mask, 0); - results[idx] = result; - } - } else { - pmeet_generic_recursive_reset::(&mut cur_group, &mut is_exhaustive, idx, self_payloads, keys, request_results, results, swapped); - - let result = match &self_payloads[idx].1 { - PayloadRef::Child(self_link) => { - match other_node.get_node_at_key(keys[idx].0).into_option() { - Some(other_onward_node) => { - let result = if swapped { - other_onward_node.as_tagged().pmeet_dyn(self_link.as_tagged()).invert_identity() - } else { - self_link.as_tagged().pmeet_dyn(other_onward_node.as_tagged()) - }; - FatAlgebraicResult::from_binary_op_result(result, self_link, &other_onward_node) - .map(|child| ValOrChild::Child(child)) - }, - None => { - //Check to see if we have a dangling path, because a dangling path meet with a value should result in a path, but no value - if self_link.is_empty() && other_node.node_get_val(keys[idx].0).is_some() { - FatAlgebraicResult::new(SELF_IDENT, Some(ValOrChild::Child(TrieNodeODRc::new_empty()))) - } else { - FatAlgebraicResult::new(COUNTER_IDENT, None) - } - } - } - }, - PayloadRef::Val(_self_val) => { - //If self_payload is a val and we didn't get a corresponding val, then this result is None - FatAlgebraicResult::new(COUNTER_IDENT, None) - }, - _ => unreachable!() - }; - results[idx] = result; - } - } - pmeet_generic_recursive_reset::(&mut cur_group, &mut is_exhaustive, keys.len(), self_payloads, keys, request_results, results, swapped); - - is_exhaustive -} -/// Effectively part of `pmeet_generic_internal`, but factored out separately because it's called in -/// several different places. Resets the `cur_group` state and does a recursive call of `pmeet_generic_internal` -#[inline] -fn pmeet_generic_recursive_reset<'trie, const MAX_PAYLOAD_CNT: usize, V, A: Allocator>(cur_group: &mut Option<(usize, &'trie TrieNodeODRc)>, is_exhaustive: &mut bool, idx: usize, self_payloads: &[(&[u8], PayloadRef)], keys: &mut [(&[u8], bool)], request_results: &mut [(usize, PayloadRef<'trie, V, A>)], results: &mut [FatAlgebraicResult>], swapped: bool) - where V: Clone + Send + Sync + Lattice -{ - match core::mem::take(cur_group) { - Some((group_start, next_node)) => { - let group_keys = &mut keys[group_start..idx]; - let group_results = &mut results[group_start..idx]; - let group_self_payloads = &self_payloads[group_start..idx]; - if !pmeet_generic_internal::(group_self_payloads, group_keys, request_results, group_results, next_node.as_tagged(), swapped) { - *is_exhaustive = false; - } - }, - None => {} - } -} /// An abstracted reference to the node at the zipper's focus, returned by [`crate::zipper::ZipperInfallibleSubtries::get_focus`] /// @@ -1278,15 +1263,6 @@ mod tagged_node_ref { } } - pub(crate) fn node_get_payloads<'res>(&self, keys: &[(&[u8], bool)], results: &'res mut [(usize, PayloadRef<'a, V, A>)]) -> bool { - match self { - Self::DenseByteNode(node) => node.node_get_payloads(keys, results), - Self::LineListNode(node) => node.node_get_payloads(keys, results), - Self::CellByteNode(node) => node.node_get_payloads(keys, results), - Self::TinyRefNode(node) => node.node_get_payloads(keys, results), - Self::EmptyNode => true, - } - } pub fn node_contains_val(&self, key: &[u8]) -> bool { match self { @@ -1925,17 +1901,6 @@ mod tagged_node_ref { _ => unsafe{ unreachable_unchecked() } } } - pub fn node_get_payloads<'res>(&self, keys: &[(&[u8], bool)], results: &'res mut [(usize, PayloadRef<'a, V, A>)]) -> bool { - let (ptr, tag) = self.ptr.get_raw_parts(); - match tag { - EMPTY_NODE_TAG => true, - DENSE_BYTE_NODE_TAG => unsafe{ &*ptr.cast::>() }.node_get_payloads(keys, results), - LINE_LIST_NODE_TAG => unsafe{ &*ptr.cast::>() }.node_get_payloads(keys, results), - CELL_BYTE_NODE_TAG => unsafe{ &*ptr.cast::>() }.node_get_payloads(keys, results), - TINY_REF_NODE_TAG => unsafe{ &*ptr.cast::>() }.node_get_payloads(keys, results), - _ => unsafe{ unreachable_unchecked() } - } - } pub fn node_contains_val(&self, key: &[u8]) -> bool { let (ptr, tag) = self.ptr.get_raw_parts(); match tag { @@ -2205,7 +2170,7 @@ mod tagged_node_ref { } let (ptr, tag) = self.ptr.get_raw_parts(); match tag { - EMPTY_NODE_TAG => AlgebraicResult::None, + EMPTY_NODE_TAG => crate::empty_node::EmptyNode.pmeet_dyn(other), DENSE_BYTE_NODE_TAG => unsafe{ &*ptr.cast::>() }.pmeet_dyn(other), LINE_LIST_NODE_TAG => unsafe{ &*ptr.cast::>() }.pmeet_dyn(other), CELL_BYTE_NODE_TAG => unsafe{ &*ptr.cast::>() }.pmeet_dyn(other), diff --git a/src/write_zipper.rs b/src/write_zipper.rs index 28336150..d1555004 100644 --- a/src/write_zipper.rs +++ b/src/write_zipper.rs @@ -1916,8 +1916,21 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC } else { PathMap::new_in(self.alloc.clone()) }; + //`prune` drops the dangling paths from the meet; the focus itself is never removed + let temp_map = if prune { + let alloc = temp_map.alloc.clone(); + let (root, root_val) = temp_map.into_root(); + let root = root.and_then(|root| match node_drop_dangling(&root, None) { + DropDangling::Unchanged => Some(root), + DropDangling::Empty => None, + DropDangling::New(new_root) => Some(new_root), + }); + PathMap::new_with_root_in(root, root_val, alloc) + } else { + temp_map + }; if temp_map.is_empty() { - self.remove_branches(prune); + self.remove_branches(false); false } else { self.graft_map(temp_map); @@ -1989,6 +2002,7 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC } /// See [ZipperWriting::meet_into] pub fn meet_into>(&mut self, read_zipper: &Z, prune: bool) -> AlgebraicStatus where V: Lattice { + //The focus is never removed let src_root_val = read_zipper.val(); #[cfg(not(feature = "graft_root_vals"))] let _ = src_root_val; @@ -1997,60 +2011,76 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC (Some(self_val), Some(src_val)) => { let new_status = match self_val.pmeet(src_val) { AlgebraicResult::Element(new_val) => {self.set_val(new_val); AlgebraicStatus::Element }, - AlgebraicResult::None => {self.remove_val(prune); AlgebraicStatus::None }, + AlgebraicResult::None => {self.remove_val(false); AlgebraicStatus::None }, AlgebraicResult::Identity(_) => { AlgebraicStatus::Identity } }; (new_status, false) }, (None, Some(_)) => { (AlgebraicStatus::None, true) }, - (Some(_), None) => { self.remove_val(prune); (AlgebraicStatus::None, false) }, + (Some(_), None) => { self.remove_val(false); (AlgebraicStatus::None, false) }, (None, None) => { (AlgebraicStatus::None, true) }, }; - let node_was_none; - let node_status = match self.get_focus().try_as_tagged() { - Some(self_node) => { - if !self_node.node_is_empty() { - node_was_none = false; - let src = read_zipper.get_focus(); - if src.is_none() { - self.graft_internal(None); + let self_focus = self.get_focus(); + let node_was_none = match self_focus.try_as_tagged() { + Some(self_node) => self_node.node_is_empty(), + None => true + }; + let node_status = if node_was_none { + AlgebraicStatus::None + } else { + let src = read_zipper.get_focus(); + let result = match src.try_as_tagged() { + Some(src_node) => self_focus.as_tagged().pmeet_dyn(src_node), + None => AlgebraicResult::None, + }; + //Prune drops dangling paths; shared nodes may be skipped + let drop_dangling = |node: TrieNodeODRc, src: Option>| -> Option> { + match node_drop_dangling(&node, src) { + DropDangling::Unchanged => Some(node), + DropDangling::Empty => None, + DropDangling::New(new_node) => Some(new_node), + } + }; + let (unchanged, new_node) = match result { + AlgebraicResult::Element(intersection) => { + (false, if prune { drop_dangling(intersection, src.try_as_tagged()) } else { Some(intersection) }) + }, + AlgebraicResult::None => (false, None), + AlgebraicResult::Identity(mask) => { + if mask & SELF_IDENT > 0 { if prune { - self.prune_path(); + let self_rc = self_focus.into_option().unwrap(); + match node_drop_dangling(&self_rc, src.try_as_tagged()) { + DropDangling::Unchanged => (true, None), + DropDangling::Empty => (false, None), + DropDangling::New(new_node) => (false, Some(new_node)), + } + } else { + (true, None) } - AlgebraicStatus::None } else { - match self_node.pmeet_dyn(src.as_tagged()) { - AlgebraicResult::Element(intersection) => { - self.graft_internal(Some(intersection)); - AlgebraicStatus::Element - }, - AlgebraicResult::None => { - self.graft_internal(None); - if prune { - self.prune_path(); - } - AlgebraicStatus::None - }, - AlgebraicResult::Identity(mask) => { - if mask & SELF_IDENT > 0 { - AlgebraicStatus::Identity - } else { - debug_assert_eq!(mask, COUNTER_IDENT); //It's gotta be self or other - self.graft_internal(Some(src.into_option().unwrap())); - AlgebraicStatus::Element - } - }, - } + debug_assert_eq!(mask, COUNTER_IDENT); //It's gotta be self or other + //The source's own node is shared with the source, so pruning may skip it + let src_is_shared = matches!(src.0, AbstractNodeRef::BorrowedRc(_)); + let src_rc = src.into_option().unwrap(); + (false, if prune && !src_is_shared { drop_dangling(src_rc, None) } else { Some(src_rc) }) + } + }, + }; + if unchanged { + AlgebraicStatus::Identity + } else { + match new_node { + Some(new_node) => { + self.graft_internal(Some(new_node)); + AlgebraicStatus::Element + }, + None => { + self.graft_internal(None); + AlgebraicStatus::None } - } else { - node_was_none = true; - AlgebraicStatus::None } - }, - None => { - node_was_none = true; - AlgebraicStatus::None } }; @@ -2059,6 +2089,7 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC #[cfg(feature = "graft_root_vals")] return node_status.merge(val_status, node_was_none, val_was_none) } + /// See [WriteZipper::meet_2] pub fn meet_2, ZB: ZipperInfallibleSubtries>(&mut self, rz_a: &ZA, rz_b: &ZB) -> AlgebraicStatus where V: Lattice { let a_focus = rz_a.get_focus(); @@ -3690,7 +3721,7 @@ mod tests { assert_eq!(btm.path_exists_at(&[1, 255, 0]), true); assert_eq!(btm.path_exists_at(&[0, 255, 0]), true); - // Test 3: meet from a higher level with all dangling paths and prune=true + //Test 3: with prune, the shared dangling path goes; the focus stays let mut btm2: PathMap<()> = PathMap::new(); btm2.create_path(&[0, 255, 0]); btm2.create_path(&[0, 255, 1]); @@ -3701,16 +3732,35 @@ mod tests { let mut wz = zh2.write_zipper_at_exclusive_path(&[0]).unwrap(); let rz = zh2.read_zipper_at_path(&[1]).unwrap(); let alg_result = wz.meet_into(&rz, true); - assert_eq!(alg_result, AlgebraicStatus::Element); + assert_eq!(alg_result, AlgebraicStatus::None); drop(wz); drop(rz); drop(zh2); // Verify the meet operation did what it should have assert_eq!(btm2.path_exists_at(&[1, 255, 0]), true); - assert_eq!(btm2.path_exists_at(&[0, 255, 0]), true); + assert_eq!(btm2.path_exists_at(&[0]), true); + assert_eq!(btm2.path_exists_at(&[0, 255]), false); assert_eq!(btm2.path_exists_at(&[0, 200, 5]), false); assert_eq!(btm2.path_exists_at(&[0, 255, 1]), false); + + // Test 4: the same without prune keeps the path both sides have, and only that + let mut btm3: PathMap<()> = PathMap::new(); + btm3.create_path(&[0, 255, 0]); + btm3.create_path(&[0, 255, 1]); + btm3.create_path(&[0, 200, 5]); + btm3.create_path(&[1, 255, 0]); + let zh3 = btm3.zipper_head(); + + let mut wz = zh3.write_zipper_at_exclusive_path(&[0]).unwrap(); + let rz = zh3.read_zipper_at_path(&[1]).unwrap(); + assert_eq!(wz.meet_into(&rz, false), AlgebraicStatus::Element); + drop(wz); + drop(rz); + drop(zh3); + assert_eq!(btm3.path_exists_at(&[0, 255, 0]), true); + assert_eq!(btm3.path_exists_at(&[0, 200]), false); + assert_eq!(btm3.path_exists_at(&[0, 255, 1]), false); } /// A write after `prune_path` (or `meet_into(.., true)`) must reach the focus node @@ -3757,6 +3807,197 @@ mod tests { } + /// The tag of the root node of `map` + fn root_tag(map: &PathMap) -> usize { + map.root().unwrap().as_tagged().tag() + } + + /// Every location in `map`, dangling ones included, with its value + fn locations(map: &PathMap) -> Vec<(Vec, Option)> { + let mut rz = map.read_zipper(); + let mut locations = vec![]; + loop { + locations.push((rz.path().to_vec(), rz.val().cloned())); + if !rz.to_next_step() { break } + } + locations + } + + /// Dense bytes that meet to nothing survive as dangling paths + #[test] + fn write_zipper_meet_into_dense_keeps_bytes_that_meet_to_nothing() { + type Build = fn() -> PathMap; + type Locations = Vec<(Vec, Option)>; + const BYTES: [u8; 4] = [1, 2, 3, 4]; + let values_at_0: Build = || { let mut m = PathMap::::new(); for b in BYTES { m.set_val_at(&[b, 0], 1); } m }; + let values_at_1: Build = || { let mut m = PathMap::::new(); for b in BYTES { m.set_val_at(&[b, 1], 2); } m }; + let dangling: Build = || { let mut m = PathMap::::new(); for b in BYTES { m.create_path(&[b]); } m }; + let emptied: Build = || { + let mut m = PathMap::::new(); + for b in BYTES { m.set_val_at(&[b, 0], 1); } + for b in BYTES { m.write_zipper_at_path(&[b]).remove_branches(false); } + m + }; + for build in [values_at_0, values_at_1, dangling, emptied] { + assert_eq!(root_tag(&build()), DENSE_BYTE_NODE_TAG); + } + let bare: Locations = core::iter::once((vec![], None)).chain(BYTES.iter().map(|b| (vec![*b], None))).collect(); + assert_eq!(locations(&dangling()), bare); + assert_eq!(locations(&emptied()), bare); + + // The values below each byte meet to nothing; the bytes stay + assert_eq!(locations(&values_at_0().meet(&values_at_1())), bare); + let mut dst = values_at_0(); + assert_eq!(dst.write_zipper().meet_into(&values_at_1().read_zipper(), false), AlgebraicStatus::Element); + assert_eq!(locations(&dst), bare); + + // A dangling side is an identity for the meet, whichever form the dangling byte takes + for dangling_side in [dangling, emptied] { + for other in [values_at_0, values_at_1, dangling, emptied] { + let mut dst = dangling_side(); + assert_eq!(dst.write_zipper().meet_into(&other().read_zipper(), false), AlgebraicStatus::Identity); + assert_eq!(locations(&dst), bare); + let mut dst = other(); + let expected = if locations(&dst) == bare { AlgebraicStatus::Identity } else { AlgebraicStatus::Element }; + assert_eq!(dst.write_zipper().meet_into(&dangling_side().read_zipper(), false), expected); + assert_eq!(locations(&dst), bare); + } + } + } + + /// A list-node slot keeps the deepest key prefix the other side has, in both operand orders + #[test] + fn write_zipper_meet_into_list_keeps_shared_key_prefix() { + type Build = fn() -> PathMap; + type Locations = Vec<(Vec, Option)>; + let list_val: Build = || { let mut m = PathMap::new(); m.set_val_at(&[5u8, 6, 7], 1); m }; + let list_child: Build = || { let mut m = PathMap::new(); m.set_val_at(&[5u8, 6, 7, 8, 9], 1); m }; + let list_dangling: Build = || { let mut m = PathMap::new(); m.create_path(&[5u8, 6]); m }; + let dense_dangling: Build = || { let mut m = PathMap::new(); for b in [1u8, 2, 3] { m.set_val_at(&[b], 3); } m.create_path(&[5u8, 6]); m }; + let dense_branch: Build = || { let mut m = PathMap::new(); for b in [1u8, 2, 3] { m.set_val_at(&[b], 3); } m.set_val_at(&[5u8, 6, 0], 4); m }; + let dense_value: Build = || { let mut m = PathMap::new(); for b in [1u8, 2, 3] { m.set_val_at(&[b], 3); } m.set_val_at(&[5u8, 6], 4); m }; + assert_eq!(root_tag(&list_val()), LINE_LIST_NODE_TAG); + assert_eq!(root_tag(&dense_dangling()), DENSE_BYTE_NODE_TAG); + + let upto_6: Locations = vec![(vec![], None), (vec![5], None), (vec![5, 6], None)]; + let cases: [(&str, Build, Build, Locations); 7] = [ + ("list value, dense dangling", list_val, dense_dangling, upto_6.clone()), + ("list value, dense branch", list_val, dense_branch, upto_6.clone()), + ("list value, dense value", list_val, dense_value, upto_6.clone()), + ("list child, dense branch", list_child, dense_branch, upto_6.clone()), + ("list child, list dangling", list_child, list_dangling, upto_6.clone()), + ("list value, list dangling", list_val, list_dangling, upto_6.clone()), + ("list dangling, dense value", list_dangling, dense_value, upto_6.clone()), + ]; + let mut failures = vec![]; + for (name, a, b, expected) in cases { + for (order, dst, src) in [("a,b", a, b), ("b,a", b, a)] { + let got_map = locations(&dst().meet(&src())); + let mut d = dst(); + let status = d.write_zipper().meet_into(&src().read_zipper(), false); + let got = locations(&d); + let expected_status = if locations(&dst()) == expected { AlgebraicStatus::Identity } else { AlgebraicStatus::Element }; + if got_map != expected || got != expected || status != expected_status { + failures.push(format!("{name} ({order}): PathMap::meet {got_map:?}, meet_into {status:?} {got:?}; expected {expected_status:?} {expected:?}")); + } + } + } + assert!(failures.is_empty(), "\n{}", failures.join("\n")); + } + + /// A meet with an equal trie is `Identity`, dense destination and list source included + #[test] + fn write_zipper_meet_into_dense_with_equal_list_is_identity() { + let build_list = || { + let mut m = PathMap::::new(); + m.set_val_at(&[2u8], 9); + m.set_val_at(&[2u8, 1, 0], 7); + m.set_val_at(&[2u8, 2, 0], 8); + m + }; + let build_dense = || { + let mut m = build_list(); + for b in [1u8, 3] { m.set_val_at(&[b], 1); } + for b in [1u8, 3] { m.remove_val_at(&[b], true); } + m + }; + assert_eq!(root_tag(&build_list()), LINE_LIST_NODE_TAG); + assert_eq!(root_tag(&build_dense()), DENSE_BYTE_NODE_TAG); + assert_eq!(locations(&build_dense()), locations(&build_list())); + + let mut dst = build_dense(); + assert_eq!(dst.write_zipper().meet_into(&build_list().read_zipper(), false), AlgebraicStatus::Identity); + assert_eq!(locations(&dst), locations(&build_list())); + let mut dst = build_list(); + assert_eq!(dst.write_zipper().meet_into(&build_dense().read_zipper(), false), AlgebraicStatus::Identity); + + // Anything more in the dense node is not in the result + let mut dst = build_dense(); + dst.create_path(&[2u8, 3]); + assert_eq!(dst.write_zipper().meet_into(&build_list().read_zipper(), false), AlgebraicStatus::Element); + assert_eq!(locations(&dst), locations(&build_list())); + } + + /// `meet_into` never removes its focus; `prune` only drops dangling paths below it + #[test] + fn write_zipper_meet_into_keeps_focus() { + type Locations = Vec<(Vec, Option)>; + let dst = || { let mut m = PathMap::::new(); m.set_val_at(&[5u8], 1); m.set_val_at(&[5u8, 0], 2); m.set_val_at(&[9u8], 9); m }; + let src = || { let mut m = PathMap::::new(); m.set_val_at(&[5u8, 1], 3); m }; + let focus_left: Locations = vec![(vec![], None), (vec![5], None), (vec![9], Some(9))]; + for prune in [false, true] { + let mut d = dst(); + let s = src(); + assert_eq!(d.write_zipper_at_path(&[5u8]).meet_into(&s.read_zipper_at_path(&[5u8]), prune), AlgebraicStatus::None, "prune = {prune}"); + assert_eq!(locations(&d), focus_left, "prune = {prune}"); + } + + // The source's focus value is gone, but a dangling path both sides have survives unless pruned + let dst = || { let mut m = PathMap::::new(); m.set_val_at(&[5u8], 1); m.create_path(&[5u8, 0, 0]); m.set_val_at(&[9u8], 9); m }; + let src = || { let mut m = PathMap::::new(); m.create_path(&[5u8, 0, 0]); m }; + let mut d = dst(); + assert_eq!(d.write_zipper_at_path(&[5u8]).meet_into(&src().read_zipper_at_path(&[5u8]), false), AlgebraicStatus::Element); + assert_eq!(locations(&d), vec![(vec![], None), (vec![5], None), (vec![5, 0], None), (vec![5, 0, 0], None), (vec![9], Some(9))]); + let mut d = dst(); + assert_eq!(d.write_zipper_at_path(&[5u8]).meet_into(&src().read_zipper_at_path(&[5u8]), true), AlgebraicStatus::None); + assert_eq!(locations(&d), focus_left); + } + + /// `meet_k_path_into` keeps dangling paths, or drops them with `prune`; the focus stays + #[test] + fn write_zipper_meet_k_path_into_dangling_paths() { + let build = || { + let mut m = PathMap::::new(); + m.set_val_at(&[8u8], 8); + m.set_val_at(&[9u8, 1, 2], 5); + m.create_path(&[9u8, 1, 3]); + m.set_val_at(&[9u8, 2, 2], 6); + m.create_path(&[9u8, 2, 3]); + m + }; + let mut m = build(); + assert_eq!(m.write_zipper_at_path(&[9u8]).meet_k_path_into(1, false), true); + assert_eq!(locations(&m), vec![(vec![], None), (vec![8], Some(8)), (vec![9], None), (vec![9, 2], Some(5)), (vec![9, 3], None)]); + let mut m = build(); + assert_eq!(m.write_zipper_at_path(&[9u8]).meet_k_path_into(1, true), true); + assert_eq!(locations(&m), vec![(vec![], None), (vec![8], Some(8)), (vec![9], None), (vec![9, 2], Some(5))]); + + // Only a dangling path in common: kept without prune, and with prune nothing is left below + let build = || { + let mut m = PathMap::::new(); + m.set_val_at(&[8u8], 8); + m.create_path(&[9u8, 1, 3]); + m.create_path(&[9u8, 2, 3]); + m + }; + let mut m = build(); + assert_eq!(m.write_zipper_at_path(&[9u8]).meet_k_path_into(1, false), true); + assert_eq!(locations(&m), vec![(vec![], None), (vec![8], Some(8)), (vec![9], None), (vec![9, 3], None)]); + let mut m = build(); + assert_eq!(m.write_zipper_at_path(&[9u8]).meet_k_path_into(1, true), false); + assert_eq!(locations(&m), vec![(vec![], None), (vec![8], Some(8)), (vec![9], None)]); + } + /// Tests whether the [WriteZipper::subtract_into] operation will do the right thing with the root value #[test] fn write_zipper_subtract_into_test1() { @@ -6768,6 +7009,111 @@ mod tests { assert_eq!(vals(&dst), vals(&src)); } + /// Meet rule: a path survives iff both sides have it, a value iff both hold one. + /// With `prune`, dangling paths are dropped, except possibly inside shared nodes + #[test] + fn write_zipper_meet_into_dangling_paths() { + type Build = fn() -> PathMap; + type Locations = Vec<(Vec, Option)>; + + // meet({[0,0]:-}, {[0,1]:-}) -> {[0]:-}, and with prune nothing but the root + let a = || { let mut m = PathMap::::new(); m.create_path(&[0u8, 0]); m }; + let b = || { let mut m = PathMap::::new(); m.create_path(&[0u8, 1]); m }; + assert_eq!(locations(&a().meet(&b())), vec![(vec![], None), (vec![0], None)]); + let mut d = a(); + assert_eq!(d.write_zipper().meet_into(&b().read_zipper(), false), AlgebraicStatus::Element); + assert_eq!(locations(&d), vec![(vec![], None), (vec![0], None)]); + let mut d = a(); + assert_eq!(d.write_zipper().meet_into(&b().read_zipper(), true), AlgebraicStatus::None); + assert_eq!(locations(&d), vec![(vec![], None)]); + + // A dangling path met against a value keeps the path without the value, in both orders + let left = || { let mut m = PathMap::::new(); m.create_path([7u8, 1, 0]); m }; + let right = || { let mut m = PathMap::::new(); m.set_val_at([7u8, 1, 0], 10); m.set_val_at([7u8, 2, 0], 20); m.create_path([7u8, 3]); m }; + let expected: Locations = vec![(vec![], None), (vec![7], None), (vec![7, 1], None), (vec![7, 1, 0], None)]; + assert_eq!(locations(&left().meet(&right())), expected); + assert_eq!(locations(&right().meet(&left())), expected); + + // Meeting an equal trie: unchanged without prune, dangling paths dropped with it + let equal_cases: [(&str, Build, Locations); 3] = [ + ("create_path", || { let mut m = PathMap::new(); m.create_path(&[1u8]); m }, vec![(vec![], None)]), + ("value removed", || { let mut m = PathMap::new(); m.set_val_at(&[1u8], 5); m.remove_val_at(&[1u8], false); m }, vec![(vec![], None)]), + ("dangling beside values", || { + let mut m = PathMap::new(); + m.set_val_at(&[1u8, 2], 5); + m.set_val_at(&[1u8, 4, 4, 4], 5); + m.set_val_at(&[9u8], 5); + m.write_zipper_at_path(&[1u8, 4]).remove_branches(false); + m + }, vec![(vec![], None), (vec![1], None), (vec![1, 2], Some(5)), (vec![9], Some(5))]), + ]; + for (name, build, pruned) in equal_cases { + let expected = locations(&build()); + let mut shared = build(); + let clone = shared.clone(); + assert_eq!(shared.write_zipper().meet_into(&clone.read_zipper(), false), AlgebraicStatus::Identity, "{name}: meet with a clone"); + assert_eq!(locations(&shared), expected, "{name}: meet with a clone"); + let mut unshared = build(); + assert_eq!(unshared.write_zipper().meet_into(&build().read_zipper(), false), AlgebraicStatus::Identity, "{name}: meet with a copy"); + assert_eq!(locations(&unshared), expected, "{name}: meet with a copy"); + assert_eq!(locations(&build().meet(&build())), expected, "{name}: PathMap::meet"); + + //A clone shares every node, so prune may skip it all + let mut shared = build(); + let clone = shared.clone(); + shared.write_zipper().meet_into(&clone.read_zipper(), true); + let got = locations(&shared); + assert!(pruned.iter().all(|l| got.contains(l)) && got.iter().all(|l| expected.contains(l)), + "{name}: pruned meet with a clone: {got:?} is not between {pruned:?} and {expected:?}"); + let mut unshared = build(); + unshared.write_zipper().meet_into(&build().read_zipper(), true); + assert_eq!(locations(&unshared), pruned, "{name}: pruned meet with a copy"); + } + + // Pruning never removes the focus, below the root or at a zipper's root + let dst = || { let mut m = PathMap::::new(); m.set_val_at(&[9u8], 9); m.create_path(&[5u8, 0]); m }; + let src = || { let mut m = PathMap::::new(); m.create_path(&[5u8, 1]); m }; + let mut d = dst(); + let s = src(); + { let mut wz = d.write_zipper(); wz.descend_to(&[5u8]); assert_eq!(wz.meet_into(&s.read_zipper_at_path(&[5u8]), true), AlgebraicStatus::None); } + assert_eq!(locations(&d), vec![(vec![], None), (vec![5], None), (vec![9], Some(9))]); + let mut d = dst(); + { let mut wz = d.write_zipper_at_path(&[5u8]); assert_eq!(wz.meet_into(&s.read_zipper_at_path(&[5u8]), true), AlgebraicStatus::None); } + assert_eq!(locations(&d), vec![(vec![], None), (vec![5], None), (vec![9], Some(9))]); + + // A dangling [2] in the destination, against sources of different node types + let dense_dst: Build = || { let mut d = PathMap::new(); for b in [1u8, 3, 4] { d.set_val_at(&[b], 1); } d.create_path(&[2u8]); d }; + let list_dst: Build = || { let mut d = PathMap::new(); d.set_val_at(&[1u8], 1); d.create_path(&[2u8]); d }; + let list_src_below_2: Build = || { let mut s = PathMap::new(); s.set_val_at(&[1u8], 1); s.set_val_at(&[2u8, 0, 1], 246); s }; + let dense_src_below_2: Build = || { let mut s = PathMap::new(); for b in [1u8, 3, 4] { s.set_val_at(&[b], 1); } s.set_val_at(&[2u8, 0, 1], 246); s }; + let dense_src_without_2: Build = || { let mut s = PathMap::new(); for b in [1u8, 3, 4] { s.set_val_at(&[b], 1); } s }; + let dense_all: Locations = vec![(vec![], None), (vec![1], Some(1)), (vec![2], None), (vec![3], Some(1)), (vec![4], Some(1))]; + let cases: [(&str, Build, Build, AlgebraicStatus, Locations); 6] = [ + ("list dst, list src with [2]", list_dst, list_src_below_2, AlgebraicStatus::Identity, + vec![(vec![], None), (vec![1], Some(1)), (vec![2], None)]), + ("list dst, list src dangling at [2]", list_dst, list_dst, AlgebraicStatus::Identity, + vec![(vec![], None), (vec![1], Some(1)), (vec![2], None)]), + ("dense dst, dense src with [2]", dense_dst, dense_src_below_2, AlgebraicStatus::Identity, dense_all.clone()), + ("dense dst, dense src dangling at [2]", dense_dst, dense_dst, AlgebraicStatus::Identity, dense_all), + ("dense dst, list src with [2]", dense_dst, list_src_below_2, AlgebraicStatus::Element, + vec![(vec![], None), (vec![1], Some(1)), (vec![2], None)]), + ("dense dst, src without [2]", dense_dst, dense_src_without_2, AlgebraicStatus::Element, + vec![(vec![], None), (vec![1], Some(1)), (vec![3], Some(1)), (vec![4], Some(1))]), + ]; + let mut failures = vec![]; + for (name, dst, src, status, expected) in cases { + let mut d = dst(); + let s = src(); + let st = d.write_zipper().meet_into(&s.read_zipper(), false); + let got = locations(&d); + let whole = locations(&dst().meet(&src())); + if st != status || got != expected || whole != expected { + failures.push(format!("{name}: meet_into {st:?} {got:?}, PathMap::meet {whole:?}; expected {status:?} {expected:?}")); + } + } + assert!(failures.is_empty(), "\n{}", failures.join("\n")); + } + /// Dropping an empty link that shares its key with a value is not a change #[test] fn write_zipper_shadowed_dangling_slot_is_identity() { From 812c2d781d0bd26d2024833a6ed7858e30b30e19 Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Thu, 17 Sep 2026 02:14:53 +0000 Subject: [PATCH 6/6] Specify the meet rule in the model meet keeps a location both sides have, dangling or not, and a value both hold. meetPruned drops dangling paths; since pathmap may skip shared nodes with prune, the fuzzer compares prune = false only. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019R2H8fnco29asY2v3TPbtF --- differential/src/harness.rs | 5 +++-- lean/PathMapModel/Check.lean | 3 +++ lean/PathMapModel/Fuzz.lean | 1 + lean/PathMapModel/PathMap.lean | 14 +++++++++++--- lean/PathMapModel/Spec.lean | 15 +++++++++++++-- lean/PathMapModel/Write.lean | 30 ++++++++++++------------------ 6 files changed, 43 insertions(+), 25 deletions(-) diff --git a/differential/src/harness.rs b/differential/src/harness.rs index 0b0d72d9..a5a7d935 100644 --- a/differential/src/harness.rs +++ b/differential/src/harness.rs @@ -747,7 +747,8 @@ pub fn run_ops( ("join_map_into", s) } 38 => { - let _pr = get!(d.boolean()); // decoded for stream alignment; see `no_prune` + // `prune = true` is best-effort, so only `prune = false` is compared + let _pr = get!(d.boolean()); ("meet_into", show_status_opt((*rz).do_meet_into(&mut wz, no_prune))) } 39 => { @@ -823,7 +824,7 @@ pub fn run_ops( } 46 => { let k = get!(d.modn(4)); - let _pr = get!(d.boolean()); // decoded for stream alignment; see `no_prune` + let _pr = get!(d.boolean()); // decoded for stream alignment; see op 38 // `meet_k_path_into` spins forever when the focus has no // children, and escapes the focus subtree when k == 0. // See `Zip.meetKPathUnspecified`. diff --git a/lean/PathMapModel/Check.lean b/lean/PathMapModel/Check.lean index ea252df0..076107d2 100644 --- a/lean/PathMapModel/Check.lean +++ b/lean/PathMapModel/Check.lean @@ -147,6 +147,9 @@ def dropT1Result : T := ((zipAt dropT1 [0x31,0x32,0x33,0x3a] []).joinKPathInto o #guard fixtures.all (joinIdem ops) #guard fixtures.all (fun a => fixtures.all (fun b => fixtures.all (joinAssoc ops a b))) #guard fixtures.all (meetIdemOnVals ops) +#guard fixtures.all (meetIdem ops) +#guard fixtures.all (meetPrunedIdem ops) +#guard fixtures.all (fun a => fixtures.all (meetCommOnPaths ops a)) #guard fixtures.all (subSelfEmptyVals ops) #guard fixtures.all (restrictSelf ops) diff --git a/lean/PathMapModel/Fuzz.lean b/lean/PathMapModel/Fuzz.lean index de6c88e7..7612875c 100644 --- a/lean/PathMapModel/Fuzz.lean +++ b/lean/PathMapModel/Fuzz.lean @@ -407,6 +407,7 @@ def step (s : St) (d : Dec) : Option (St × Dec) := do | 38 => do let (_pr, d) ← d.bool if s.act then some (emit s "meet_into" skipAct, d) else + -- `prune = true` is best-effort, so only `prune = false` is compared let (st, z) := s.wz.meetInto ops s.rz noPrune some (emit { s with wz := z } "meet_into" (toString st), d) | 39 => do let (_pr, d) ← d.bool diff --git a/lean/PathMapModel/PathMap.lean b/lean/PathMapModel/PathMap.lean index 2054dfd9..783d6506 100644 --- a/lean/PathMapModel/PathMap.lean +++ b/lean/PathMapModel/PathMap.lean @@ -338,11 +338,19 @@ def join (a b : PathMap V) : PathMap V := mk' (keys.filterMap fun k => (joinVal ops (a.valAt k) (b.valAt k)).map (k, ·)) (a.paths ++ b.paths) -/-- Meet (intersection). A location survives only if it lies on the way to a -surviving value, so dangling paths never survive a meet. -/ +/-- Meet (`prune = false`): a location survives iff both sides have it, a value iff +both hold one. -/ def meet (a b : PathMap V) : PathMap V := let keys := Path.sortDedup (a.vals.map (·.1)) - mk' (keys.filterMap fun k => (meetVal ops (a.valAt k) (b.valAt k)).map (k, ·)) [] + mk' (keys.filterMap fun k => (meetVal ops (a.valAt k) (b.valAt k)).map (k, ·)) + (a.paths.filter b.pathExists) + +/-- `t` without its dangling paths. -/ +def dropDangling (t : PathMap V) : PathMap V := mk' t.vals [] + +/-- Meet with `prune = true`, fully pruned. `pathmap` may skip shared nodes, so its +result lies between this and `meet`; not compared by the fuzzer. -/ +def meetPruned (a b : PathMap V) : PathMap V := (meet ops a b).dropDangling /-- Subtract. diff --git a/lean/PathMapModel/Spec.lean b/lean/PathMapModel/Spec.lean index 44158a55..24c141ec 100644 --- a/lean/PathMapModel/Spec.lean +++ b/lean/PathMapModel/Spec.lean @@ -314,11 +314,22 @@ def joinIdem (ops : ValOps V) (a : PathMap V) : Bool := def joinAssoc (ops : ValOps V) (a b c : PathMap V) : Bool := PathMap.beqT ops (PathMap.join ops (PathMap.join ops a b) c) (PathMap.join ops a (PathMap.join ops b c)) -/-- `meet` is idempotent *on values*. It is not idempotent on locations: a meet -discards dangling paths, so `meet a a` keeps only the value-bearing skeleton. -/ +/-- `meet` is idempotent *on values*. -/ def meetIdemOnVals (ops : ValOps V) (a : PathMap V) : Bool := (PathMap.meet ops a a).vals.map (·.1) == a.vals.map (·.1) +/-- `meet` is idempotent, dangling paths included. -/ +def meetIdem (ops : ValOps V) (a : PathMap V) : Bool := + PathMap.beqT ops (PathMap.meet ops a a) a + +/-- `meetPruned a a` is `a` with its dangling paths dropped. -/ +def meetPrunedIdem (ops : ValOps V) (a : PathMap V) : Bool := + PathMap.beqT ops (PathMap.meetPruned ops a a) a.dropDangling + +/-- `meet` is commutative on locations. -/ +def meetCommOnPaths (ops : ValOps V) (a b : PathMap V) : Bool := + (PathMap.meet ops a b).paths == (PathMap.meet ops b a).paths + /-- Subtracting a map from itself leaves no values. -/ def subSelfEmptyVals (ops : ValOps V) (a : PathMap V) : Bool := (PathMap.sub ops a a).vals.isEmpty diff --git a/lean/PathMapModel/Write.lean b/lean/PathMapModel/Write.lean index 6ee3a84e..ff8d354c 100644 --- a/lean/PathMapModel/Write.lean +++ b/lean/PathMapModel/Write.lean @@ -328,9 +328,8 @@ def joinIntoTake (src : Zip V) (prune : Bool) : AlgStatus × Zip V × Zip V := /-- `ZipperWriting::meet_into`: intersect the focus's subtrie with the source's. -The value step runs first and can prune the focus out from under the node step. -A meet drops every dangling path, since a location only survives if it leads to -a surviving value. -/ +Below the focus the result is `PathMap.meet`, or `PathMap.meetPruned` with `prune` +(best-effort in `pathmap`, not compared). The focus is never removed. -/ def meetInto (src : Zip V) (prune : Bool) : AlgStatus × Zip V := let (valStatus, valWasNone, z1) := match z.val, src.val with @@ -339,26 +338,20 @@ def meetInto (src : Zip V) (prune : Bool) : AlgStatus × Zip V := (AlgStatus.ofValRes r, false, match r.resolve sv ov with | some v => (z.setVal v).2 - | none => (z.removeVal prune).2) + | none => (z.removeVal false).2) | none, some _ => (AlgStatus.none, true, z) - | some _, none => (AlgStatus.none, false, (z.removeVal prune).2) + | some _, none => (AlgStatus.none, false, (z.removeVal false).2) | none, none => (AlgStatus.none, true, z) let selfB := z1.focusNode let srcB := src.focusNode if selfB.isEmptyMap then (AlgStatus.merge .none valStatus true valWasNone, z1) else if srcB.isEmptyMap then - let z2 := z1.withTrie (z1.trie.removeBelow z1.focus) - let z3 := if prune then (z2.prunePath).2 else z2 - (AlgStatus.merge .none valStatus false valWasNone, z3) + (AlgStatus.merge .none valStatus false valWasNone, z1.withTrie (z1.trie.removeBelow z1.focus)) else - let r := PathMap.meet ops selfB srcB + let r := if prune then PathMap.meetPruned ops selfB srcB else PathMap.meet ops selfB srcB let st := nodeStatus ops selfB r - let z2 := - if st == .identity then z1 - else - let zg := z1.withTrie (z1.trie.graftBelow z1.focus r) - if st == .none && prune then (zg.prunePath).2 else zg + let z2 := if st == .identity then z1 else z1.withTrie (z1.trie.graftBelow z1.focus r) (AlgStatus.merge st valStatus false valWasNone, z2) /-- `ZipperWriting::subtract_into`: remove the source's subtrie from the focus's. @@ -396,7 +389,7 @@ def subtractInto (src : Zip V) (prune : Bool) : AlgStatus × Zip V := (AlgStatus.merge st valStatus false valWasNone, z2) /-- `ZipperWriting::meet_2`: meet two *source* subtries and write the result at -the focus. +the focus, as `PathMap.meet`. Two things separate this from `meet_into`. It does not consult what is already at the focus, so — as the implementation notes — it never reports `Identity`, @@ -486,9 +479,10 @@ def meetKPathInto (k : Nat) (prune : Bool) : Bool × Zip V := match acc with | none => some m | some a => some (PathMap.meet ops a m)) none - match result with - | some m => if m.isEmptyMap then (false, (z.removeBranches prune).2) else (true, z.graftMap m) - | none => (false, (z.removeBranches prune).2) + -- `prune` drops dangling paths below the focus + match result.map fun m => if prune then m.dropDangling else m with + | some m => if m.isEmptyMap then (false, (z.removeBranches false).2) else (true, z.graftMap m) + | none => (false, (z.removeBranches false).2) end Zip end PathMapModel