From 66b804b6ab638261e85d7c59da44c5976a924e50 Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Thu, 17 Sep 2026 03:35:25 +0000 Subject: [PATCH 1/4] Fix ProductZipper is_shared at a factor root A secondary factor's root isn't a child of the node above it, so the core zipper's parent lookup unwrapped None. Ask the factor's TrieRef instead. --- src/product_zipper.rs | 41 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/src/product_zipper.rs b/src/product_zipper.rs index 9f2be3e9..0dce0faf 100644 --- a/src/product_zipper.rs +++ b/src/product_zipper.rs @@ -148,6 +148,14 @@ impl<'factor_z, 'trie, V: Clone + Send + Sync + Unpin, A: Allocator> ProductZipp self.enroll_next_factor(); } } + /// The secondary factor whose root node is the focus, if any. Its node is not a child of the + /// node above it, so the core zipper can't look it up. + fn factor_root(&self) -> Option<&TrieRef<'trie, V, A>> { + match self.factor_paths.last() { + Some(&start) if start == self.depth() => self.secondaries.get(self.factor_paths.len() - 1), + _ => None + } + } /// Internal method to make sure `self.factor_paths` is correct after an ascend method #[inline] fn fix_after_ascend(&mut self) { @@ -362,8 +370,19 @@ impl<'trie, V: Clone + Send + Sync + Unpin + 'trie, A: Allocator + 'trie> Zipper } impl ZipperConcrete for ProductZipper<'_, '_, V, A> { - fn shared_node_id(&self) -> Option { self.z.shared_node_id() } - fn is_shared(&self) -> bool { self.z.is_shared() } + fn shared_node_id(&self) -> Option { + match self.factor_root() { + Some(_) if self.z.is_val() => None, + Some(factor) => factor.shared_node_id(), + None => self.z.shared_node_id(), + } + } + fn is_shared(&self) -> bool { + match self.factor_root() { + Some(factor) => factor.is_shared(), + None => self.z.is_shared(), + } + } } impl<'trie, V: Clone + Send + Sync + Unpin + 'trie, A: Allocator + 'trie> ZipperPathBuffer for ProductZipper<'_, 'trie, V, A> { @@ -1975,6 +1994,24 @@ mod tests { |btm: &mut PathMap<()>, path: &[u8]| -> _ { ProductZipperG::new::<[ReadZipperUntracked<()>; 0]>(btm.read_zipper_at_path(path), []) }); + + /// `is_shared` and `shared_node_id` across factor boundaries + #[test] + fn product_zipper_is_shared_across_factors() { + let mut a = PathMap::::new(); + for p in [&[1u8, 2, 1][..], &[1, 2, 1, 0], &[1, 2, 1, 3, 3], &[0], &[2, 2]] { a.set_val_at(p, 7); } + let b = a.clone(); + let mut z = ProductZipper::new(a.read_zipper_at_path(&[1u8, 2, 1]), [b.read_zipper()]); + let mut factor_roots = 0; + while z.to_next_step() { + let _ = (z.is_shared(), z.shared_node_id()); + if z.factor_root().is_some() { + factor_roots += 1; + assert!(z.is_shared(), "{:?}", z.path()); + } + } + assert!(factor_roots > 0); + } } //POSSIBLE FUTURE DIRECTION: From 22750f8dee74fb5122be1e4e4a7415030274074b Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 23 Sep 2026 04:29:55 +0000 Subject: [PATCH 2/4] Only report ProductZipper sharing in the last factor Brings over the follow-up from fix/product-zipper-is-shared-factor-root. Only report sharing in the last factor, for both ProductZipper and ProductZipperG. In an earlier factor the subtrie below a node continues into the following factors, so the same node reached in different factors is a different subtrie of the product. A cached cata keyed by shared_node_id conflated the two and returned wrong results. --- src/product_zipper.rs | 47 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/src/product_zipper.rs b/src/product_zipper.rs index 0dce0faf..10e1b9ea 100644 --- a/src/product_zipper.rs +++ b/src/product_zipper.rs @@ -156,6 +156,12 @@ impl<'factor_z, 'trie, V: Clone + Send + Sync + Unpin, A: Allocator> ProductZipp _ => None } } + /// `true` once every factor has been entered, i.e. the focus is in the last factor's trie (including + /// its root). See the `ZipperConcrete` impl for why sharing is only reported there. + #[inline] + fn in_last_factor(&self) -> bool { + self.factor_paths.len() == self.secondaries.len() + } /// Internal method to make sure `self.factor_paths` is correct after an ascend method #[inline] fn fix_after_ascend(&mut self) { @@ -369,8 +375,14 @@ impl<'trie, V: Clone + Send + Sync + Unpin + 'trie, A: Allocator + 'trie> Zipper } } +/// Sharing is only reported in the last factor. In an earlier factor, the subtrie below a node continues +/// into the following factors, so the same node reached from a different factor is a different subtrie of +/// the product, and a cache keyed by `shared_node_id` would conflate the two. impl ZipperConcrete for ProductZipper<'_, '_, V, A> { fn shared_node_id(&self) -> Option { + if !self.in_last_factor() { + return None + } match self.factor_root() { Some(_) if self.z.is_val() => None, Some(factor) => factor.shared_node_id(), @@ -378,6 +390,9 @@ impl ZipperConcrete for ProductZip } } fn is_shared(&self) -> bool { + if !self.in_last_factor() { + return false + } match self.factor_root() { Some(factor) => factor.is_shared(), None => self.z.is_shared(), @@ -544,6 +559,7 @@ impl<'trie, PrimaryZ, SecondaryZ, V> ZipperAbsolutePath fn root_prefix_path(&self) -> &[u8] { self.primary.root_prefix_path() } } +/// Sharing is only reported in the last factor. See the `ZipperConcrete` impl for [ProductZipper] impl<'trie, PrimaryZ, SecondaryZ, V> ZipperConcrete for ProductZipperG<'trie, PrimaryZ, SecondaryZ, V> where @@ -552,6 +568,9 @@ impl<'trie, PrimaryZ, SecondaryZ, V> ZipperConcrete SecondaryZ: ZipperMoving + ZipperPath + ZipperConcrete, { fn shared_node_id(&self) -> Option { + if self.factor_paths.len() < self.secondary.len() { + return None + } if let Some(idx) = self.factor_idx(true) { self.secondary[idx].shared_node_id() } else { @@ -559,6 +578,9 @@ impl<'trie, PrimaryZ, SecondaryZ, V> ZipperConcrete } } fn is_shared(&self) -> bool { + if self.factor_paths.len() < self.secondary.len() { + return false + } if let Some(idx) = self.factor_idx(true) { self.secondary[idx].is_shared() } else { @@ -2012,6 +2034,31 @@ mod tests { } assert!(factor_roots > 0); } + + /// A node shared between factors must not share a `shared_node_id`, because its subtrie in the product + /// differs by factor. Otherwise a cached cata reuses the result from one factor in another. + #[test] + fn product_zipper_cata_cached_across_factors() { + // `s` is grafted in two places, and `b = a.clone()`, so the same node is in both factors + let mut s = PathMap::::new(); + for p in [&[5u8, 6][..], &[5, 7], &[9]] { s.set_val_at(p, 1); } + let mut a = PathMap::::new(); + a.write_zipper_at_path(&[1u8]).graft_map(s.clone()); + a.write_zipper_at_path(&[2u8]).graft_map(s.clone()); + a.set_val_at(&[3u8], 1); + let b = a.clone(); + + let alg = |_: &ByteMask, children: &mut [usize], val: Option<&u64>| children.iter().sum::() + val.is_some() as usize; + let expected = 7 + 7 * 7; + let pz = ProductZipper::new(a.read_zipper(), [b.read_zipper()]); + assert_eq!(pz.into_cata_cached(alg), expected); + let pzg = ProductZipperG::new(a.read_zipper(), [b.read_zipper()]); + assert_eq!(pzg.into_cata_cached(alg), expected); + + // Sharing is still reported in the last factor + let mut z = ProductZipper::new(a.read_zipper(), [b.read_zipper()]); + assert!(z.descend_to_existing(&[3u8, 1]) == 2 && z.is_shared() && z.shared_node_id().is_some()); + } } //POSSIBLE FUTURE DIRECTION: From 1e4a683e605fc82051bec0933e5ec1dd3e39b45f Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 23 Sep 2026 14:34:00 +0000 Subject: [PATCH 3/4] Report ProductZipper sharing in every factor, keyed by factor Replaces the last-factor-only rule. Sharing in an earlier factor is still useful (a subtrie grafted twice within one factor is the same subtrie of the product), but the same node reached in two different factors is two different subtries, so its id must depend on the factor. A node id is the node's address. In a canonical address the byte below the top byte equals the top byte (0x00 in user space, 0xff in kernel space), so XOR factor + 1 into that byte: the result is never a canonical address, and no two (factor, node) pairs collide. A hash of the pair would not do in 64 bits. An address with no room reports no sharing, and the last factor keeps the node's own id. --- src/product_zipper.rs | 122 ++++++++++++++++++++++++++++++++---------- 1 file changed, 95 insertions(+), 27 deletions(-) diff --git a/src/product_zipper.rs b/src/product_zipper.rs index 10e1b9ea..13a43b6d 100644 --- a/src/product_zipper.rs +++ b/src/product_zipper.rs @@ -156,11 +156,11 @@ impl<'factor_z, 'trie, V: Clone + Send + Sync + Unpin, A: Allocator> ProductZipp _ => None } } - /// `true` once every factor has been entered, i.e. the focus is in the last factor's trie (including - /// its root). See the `ZipperConcrete` impl for why sharing is only reported there. + /// The index of the factor whose trie holds the node at the focus. At a factor root that is the + /// entered factor, not the one whose path ends there. #[inline] - fn in_last_factor(&self) -> bool { - self.factor_paths.len() == self.secondaries.len() + fn focus_node_factor(&self) -> usize { + self.factor_paths.len() } /// Internal method to make sure `self.factor_paths` is correct after an ascend method #[inline] @@ -375,24 +375,42 @@ impl<'trie, V: Clone + Send + Sync + Unpin + 'trie, A: Allocator + 'trie> Zipper } } -/// Sharing is only reported in the last factor. In an earlier factor, the subtrie below a node continues -/// into the following factors, so the same node reached from a different factor is a different subtrie of -/// the product, and a cache keyed by `shared_node_id` would conflate the two. +/// Converts the `shared_node_id` of a node in factor `factor` of a product of `factor_count` factors +/// into the product's `shared_node_id` for it +/// +/// In every factor but the last, the subtrie below a node continues into the following factors, so the +/// same node reached in two different factors is two different subtries of the product. Its id must +/// therefore depend on the factor, while two places that reach it in the same factor still share it. +/// +/// A node id is the node's address. In a canonical address the byte below the top byte equals the top +/// byte (`0x00` in user space, `0xff` in kernel space), so `factor + 1` is XORed into that byte. The +/// result is never a canonical address, so it can't be mistaken for an unencoded id, and it is exact: +/// no two (factor, node) pairs map to the same id. An address with no room for the encoding reports no +/// sharing. The last factor keeps the node's own id, since the product adds nothing below it. +fn factor_shared_node_id(id: u64, factor: usize, factor_count: usize) -> Option { + if factor + 1 >= factor_count { + return Some(id); + } + let tag = u8::try_from(factor + 1).ok()?; + let [.., below_top, top] = id.to_le_bytes(); + // The assumption is that the address space is 48 bit, and the node id was not already tagged + // Skip sharing if that's not the case. + if below_top != top { + return None; + } + Some(id ^ ((tag as u64) << 48)) +} + impl ZipperConcrete for ProductZipper<'_, '_, V, A> { fn shared_node_id(&self) -> Option { - if !self.in_last_factor() { - return None - } - match self.factor_root() { + let id = match self.factor_root() { Some(_) if self.z.is_val() => None, Some(factor) => factor.shared_node_id(), None => self.z.shared_node_id(), - } + }?; + factor_shared_node_id(id, self.focus_node_factor(), self.factor_count()) } fn is_shared(&self) -> bool { - if !self.in_last_factor() { - return false - } match self.factor_root() { Some(factor) => factor.is_shared(), None => self.z.is_shared(), @@ -559,7 +577,7 @@ impl<'trie, PrimaryZ, SecondaryZ, V> ZipperAbsolutePath fn root_prefix_path(&self) -> &[u8] { self.primary.root_prefix_path() } } -/// Sharing is only reported in the last factor. See the `ZipperConcrete` impl for [ProductZipper] +/// See [factor_shared_node_id] for how the factor enters the id impl<'trie, PrimaryZ, SecondaryZ, V> ZipperConcrete for ProductZipperG<'trie, PrimaryZ, SecondaryZ, V> where @@ -568,19 +586,13 @@ impl<'trie, PrimaryZ, SecondaryZ, V> ZipperConcrete SecondaryZ: ZipperMoving + ZipperPath + ZipperConcrete, { fn shared_node_id(&self) -> Option { - if self.factor_paths.len() < self.secondary.len() { - return None - } - if let Some(idx) = self.factor_idx(true) { - self.secondary[idx].shared_node_id() - } else { - self.primary.shared_node_id() - } + let (id, factor) = match self.factor_idx(true) { + Some(idx) => (self.secondary[idx].shared_node_id()?, idx + 1), + None => (self.primary.shared_node_id()?, 0), + }; + factor_shared_node_id(id, factor, self.secondary.len() + 1) } fn is_shared(&self) -> bool { - if self.factor_paths.len() < self.secondary.len() { - return false - } if let Some(idx) = self.factor_idx(true) { self.secondary[idx].is_shared() } else { @@ -2059,6 +2071,62 @@ mod tests { let mut z = ProductZipper::new(a.read_zipper(), [b.read_zipper()]); assert!(z.descend_to_existing(&[3u8, 1]) == 2 && z.is_shared() && z.shared_node_id().is_some()); } + + /// A node keeps one `shared_node_id` within a factor, and gets a different one in each factor + #[test] + fn product_zipper_shared_node_id_by_factor() { + let mut s = PathMap::::new(); + for p in [&[5u8, 6][..], &[5, 7], &[9]] { s.set_val_at(p, 1); } + let mut a = PathMap::::new(); + a.write_zipper_at_path(&[1u8]).graft_map(s.clone()); + a.write_zipper_at_path(&[2u8]).graft_map(s.clone()); + a.set_val_at(&[3u8], 1); + let b = a.clone(); + let c = a.clone(); + + // `s` in factor 0 (twice), factor 1 (twice) and factor 2 + let paths: [&[u8]; 5] = [&[1], &[2], &[3, 1], &[3, 2], &[3, 3, 1]]; + fn ids(mut z: Z, paths: &[&[u8]]) -> Vec { + paths.iter().map(|p| { + z.reset(); + assert_eq!(z.descend_to_existing(p), p.len(), "{p:?}"); + z.shared_node_id().unwrap_or_else(|| panic!("no id at {p:?}")) + }).collect() + } + let s_id = a.read_zipper_at_path(&[1u8]).shared_node_id().unwrap(); + for ids in [ + ids(ProductZipper::new(a.read_zipper(), [b.read_zipper(), c.read_zipper()]), &paths), + ids(ProductZipperG::new(a.read_zipper(), [b.read_zipper(), c.read_zipper()]), &paths), + ] { + assert_eq!(ids[0], ids[1]); + assert_eq!(ids[2], ids[3]); + assert_ne!(ids[0], ids[2]); + assert_ne!(ids[0], ids[4]); + assert_ne!(ids[2], ids[4]); + // The last factor adds nothing below the node, so it keeps the node's own id + assert_eq!(ids[4], s_id); + } + } + + /// The factor is XORed into the byte below the top byte, which leaves a non-canonical address + #[test] + fn factor_shared_node_id_encoding() { + use super::factor_shared_node_id; + let user = 0x0000_7fff_1234_5678u64; + let kernel = 0xffff_8000_0000_1000u64; + assert_eq!(factor_shared_node_id(user, 0, 3), Some(0x0001_7fff_1234_5678)); + assert_eq!(factor_shared_node_id(user, 1, 3), Some(0x0002_7fff_1234_5678)); + assert_eq!(factor_shared_node_id(user, 2, 3), Some(user)); + assert_eq!(factor_shared_node_id(kernel, 0, 3), Some(0xfffe_8000_0000_1000)); + assert_eq!(factor_shared_node_id(kernel, 1, 3), Some(0xfffd_8000_0000_1000)); + assert_eq!(factor_shared_node_id(kernel, 2, 3), Some(kernel)); + // The widest factor that fits, and the first that doesn't + assert_eq!(factor_shared_node_id(user, 254, 300), Some(0x00ff_7fff_1234_5678)); + assert_eq!(factor_shared_node_id(user, 255, 300), None); + // An address using the byte below the top has no room + assert_eq!(factor_shared_node_id(0x0080_0000_0000_1000, 0, 2), None); + assert_eq!(factor_shared_node_id(0x0080_0000_0000_1000, 1, 2), Some(0x0080_0000_0000_1000)); + } } //POSSIBLE FUTURE DIRECTION: From e9d63f3f328b4d7cad9f1a011ec121db58ba6788 Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 23 Sep 2026 14:52:24 +0000 Subject: [PATCH 4/4] Never report sharing from DependentProductZipperG Below a node, the dependent product continues into whatever enroll returns for the path so far and the payload, so the same node reached at two paths can root two different subtries, even within one factor. Tagging the id with the factor, as ProductZipper now does, separates factors but not paths, and the last factor isn't known in advance. The new test grafts one subtrie at [1] and [2] with an enroll that picks the next factor by the first byte; with factor-tagged ids the cached cata reused [1]'s count at [2] and returned 4 instead of 5. Also say at each product zipper's sharing impl why the factor's own id would be inconsistent there. --- src/dependent_zipper.rs | 64 ++++++++++++++++++++++++++++++++--------- src/product_zipper.rs | 13 ++++++++- 2 files changed, 62 insertions(+), 15 deletions(-) diff --git a/src/dependent_zipper.rs b/src/dependent_zipper.rs index 5620e2a4..3970ca03 100644 --- a/src/dependent_zipper.rs +++ b/src/dependent_zipper.rs @@ -189,6 +189,16 @@ impl<'trie, PrimaryZ, SecondaryZ, V, C, F : Clone + for <'a> FnOnce(C, &'a [u8], fn root_prefix_path(&self) -> &[u8] { self.primary.root_prefix_path() } } +/// Sharing is never reported, because any id would be inconsistent. Below a node, the product continues into +/// whatever `enroll` returns for the path so far and the payload, so the same node reached at two paths can +/// root two different subtries of the product, even within one factor. E.g. with a subtrie grafted at `[1]` +/// and `[2]`, and an `enroll` that picks the next factor by the first byte, both paths reach the same node but +/// continue into different tries, and a cache keyed by `shared_node_id`, like `into_cata_cached`, would reuse +/// the result for `[1]` at `[2]` (see `dep_cata_cached_path_dependent_enroll`). Tagging the id with the +/// factor, as [ProductZipper] does, only separates factors, not paths; and the last factor, where the node's +/// own id would be safe, isn't known until `enroll` declines to add another. +/// +/// `is_shared` is `false` to match: each location of the product is reported as reachable by only its path. impl<'trie, PrimaryZ, SecondaryZ, V, C, F : Clone + for <'a> FnOnce(C, &'a [u8], usize) -> (C, Option)> ZipperConcrete for DependentProductZipperG<'trie, PrimaryZ, SecondaryZ, V, C, F> where @@ -196,20 +206,8 @@ impl<'trie, PrimaryZ, SecondaryZ, V, C, F : Clone + for <'a> FnOnce(C, &'a [u8], PrimaryZ: ZipperMoving + ZipperPath + ZipperConcrete, SecondaryZ: ZipperMoving + ZipperConcrete, { - fn shared_node_id(&self) -> Option { - if let Some(idx) = self.factor_idx(true) { - self.secondary[idx].shared_node_id() - } else { - self.primary.shared_node_id() - } - } - fn is_shared(&self) -> bool { - if let Some(idx) = self.factor_idx(true) { - self.secondary[idx].is_shared() - } else { - self.primary.is_shared() - } - } + fn shared_node_id(&self) -> Option { None } + fn is_shared(&self) -> bool { false } } impl<'trie, PrimaryZ, SecondaryZ, V, C, F : Clone + for <'a> FnOnce(C, &'a [u8], usize) -> (C, Option)> ZipperPathBuffer @@ -501,6 +499,8 @@ impl<'trie, PrimaryZ, SecondaryZ, V: Clone + Send + Sync + Unpin, C, F : Clone + #[cfg(test)] mod tests { use crate::zipper::*; + use crate::utils::ByteMask; + use crate::morphisms::Catamorphism; use crate::PathMap; #[test] @@ -568,4 +568,40 @@ rubiconrubicon rubicundusrubicundus ") } + + /// Below a node, the product continues into whatever `enroll` returns for the path, so the same node + /// at two paths can be two different subtries of the product, even within one factor. A cached cata + /// must not reuse the result from one for the other. + #[test] + fn dep_cata_cached_path_dependent_enroll() { + // `s` is grafted at [1] and [2], so both reach the same node in the primary factor + let s = PathMap::single([5u8], 1u64); + let mut a = PathMap::::new(); + a.write_zipper_at_path(&[1u8]).graft_map(s.clone()); + a.write_zipper_at_path(&[2u8]).graft_map(s.clone()); + let x = PathMap::single([7u8], 1u64); + let mut y = PathMap::::new(); + for p in [[8u8], [9]] { y.set_val_at(p, 1); } + + // The next factor after [1] is `x`, and after [2] it is `y` + let dpz = || DependentProductZipperG::new_enroll(a.read_zipper(), (), |_, path: &[u8], idx| { + match idx { + 0 => ((), Some(if path[0] == 1 { x.read_zipper() } else { y.read_zipper() })), + _ => ((), None), + } + }); + + // [1,5] [1,5,7] [2,5] [2,5,8] [2,5,9] + let mut z = dpz(); + let mut vals = 0; + while z.to_next_val() { vals += 1; } + assert_eq!(vals, 5); + + let alg = |_: &ByteMask, children: &mut [usize], val: Option<&u64>| children.iter().sum::() + val.is_some() as usize; + assert_eq!(dpz().into_cata_cached(alg), 5); + + let mut z = dpz(); + assert_eq!(z.descend_to_existing(&[1u8]), 1); + assert_eq!((z.shared_node_id(), z.is_shared()), (None, false)); + } } diff --git a/src/product_zipper.rs b/src/product_zipper.rs index 13a43b6d..14a885ff 100644 --- a/src/product_zipper.rs +++ b/src/product_zipper.rs @@ -401,6 +401,14 @@ fn factor_shared_node_id(id: u64, factor: usize, factor_count: usize) -> Option< Some(id ^ ((tag as u64) << 48)) } +/// The factor's own `shared_node_id` would be inconsistent here. In every factor but the last, the product +/// continues below the node into the following factors, so the same node reached in two different factors +/// (e.g. one trie used as two factors) roots two different subtries of the product, and a cache keyed by the +/// id, like `into_cata_cached`, would reuse the result for one as the result for the other. So the id is +/// tagged with the factor, see `factor_shared_node_id`. Within one factor the tag is enough: the factors +/// below are the same fixed tries wherever the node is reached, so equal ids do mean equal subtries. +/// +/// `is_shared` needs no tag, it only says the focus can be reached by more than one path. impl ZipperConcrete for ProductZipper<'_, '_, V, A> { fn shared_node_id(&self) -> Option { let id = match self.factor_root() { @@ -577,7 +585,10 @@ impl<'trie, PrimaryZ, SecondaryZ, V> ZipperAbsolutePath fn root_prefix_path(&self) -> &[u8] { self.primary.root_prefix_path() } } -/// See [factor_shared_node_id] for how the factor enters the id +/// As for [ProductZipper], the factor's own `shared_node_id` would be inconsistent: the same node reached in +/// two different factors roots two different subtries of the product, since the following factors continue +/// below it, and a cache keyed by the id would conflate them. The id is tagged with the factor, see +/// `factor_shared_node_id`, which is enough because the factors below a node are fixed. impl<'trie, PrimaryZ, SecondaryZ, V> ZipperConcrete for ProductZipperG<'trie, PrimaryZ, SecondaryZ, V> where