From aac043334e1b2d473c65cba098d805e88bac17fa Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Thu, 29 Oct 2026 18:42:57 +0000 Subject: [PATCH 1/2] Fix graft_child_maps and graft_masked_branches grafting nothing Grafting nothing at a branch must neither create nor destroy a location: an existing branch is emptied and survives as a dangling path, an absent one stays absent, as after graft of an empty source. graft_child_maps' slow path left the old branch in place for an empty map or one without a root value, its fast path grafted an empty node (tripping a debug assertion) and created the focus, and graft_masked_branches created the focus when every masked branch was absent from the source. --- Cargo.toml | 4 + benches/graft_nothing.rs | 220 ++++++++++++++++++++++++++ src/dense_byte_node.rs | 37 ++++- src/write_zipper.rs | 333 +++++++++++++++++++++++++++++---------- 4 files changed, 503 insertions(+), 91 deletions(-) create mode 100644 benches/graft_nothing.rs diff --git a/Cargo.toml b/Cargo.toml index 8b9d32cf..ff305285 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -130,6 +130,10 @@ harness = false name = "graft_masked_branches" harness = false +[[bench]] +name = "graft_nothing" +harness = false + [[bench]] name = "byte_mask" harness = false diff --git a/benches/graft_nothing.rs b/benches/graft_nothing.rs new file mode 100644 index 00000000..38e0a520 --- /dev/null +++ b/benches/graft_nothing.rs @@ -0,0 +1,220 @@ +//! Grafting when the source has nothing to give. +//! +//! `graft_masked_branches` and `graft_child_maps` replace each masked branch of the destination +//! with the source's. `benches/graft_masked_branches.rs` covers sources that have every masked +//! branch. This covers the rest: masked branches the source lacks or holds only as dangling +//! paths, a source with no node at all, a destination focus that does not exist, and empty maps. +//! Those are where grafting nothing has to empty a branch while keeping its location, so they +//! are where the cost of doing that shows. +//! +//! ```sh +//! cargo bench --bench graft_nothing +//! ``` + +use divan::{Bencher, Divan}; +use rand::seq::SliceRandom; +use rand::{SeedableRng, rngs::StdRng}; + +use pathmap::PathMap; +use pathmap::utils::ByteMask; +use pathmap::zipper::*; + +fn main() { + Divan::from_args().sample_count(1000).main(); +} + +/// Masked branch counts. 1 and 2 take the per-branch path in `graft_masked_branches`, 3 and up +/// the byte-node merge; `graft_child_maps` switches to its fast path above 2 with `remove_unset`. +const BITS: [usize; 7] = [1, 2, 3, 8, 30, 100, 200]; + +/// Branches the destination holds besides the masked ones +const DST_EXTRA: usize = 50; + +/// The first `n` bytes of a fixed shuffle, so masks are spread out and runs are reproducible +fn bytes(n: usize) -> Vec { + let mut all: Vec = (0u8..=255).collect(); + all.shuffle(&mut StdRng::seed_from_u64(0x9a4f_7_u64)); + all.truncate(n); + all +} + +fn masked(bits: usize) -> Vec { + bytes(bits) +} + +/// The masked bytes, then `DST_EXTRA` more that are not masked +fn dst_bytes(bits: usize) -> Vec { + bytes((bits + DST_EXTRA).min(256)) +} + +/// A destination with a small subtrie and a value at each of `branches` +fn dst_with(branches: &[u8]) -> PathMap { + let mut m = PathMap::new(); + m.set_val_at(b"", 1); + for (i, b) in branches.iter().copied().enumerate() { + let i = i as u32; + m.set_val_at([b], 10_000 + i); + m.set_val_at([b, 1], 10_001 + i); + m.set_val_at([b, 2, 3], 10_002 + i); + } + m +} + +/// What the source holds at each masked byte +#[derive(Clone, Copy)] +enum Src { + /// A value and a subtrie at every masked byte: nothing is empty + Full, + /// Every other masked byte; the rest are absent + Half, + /// No masked byte, but branches elsewhere, so the source still has a node to merge from + NoneMasked, + /// A dangling path at every masked byte + Dangling, + /// Only a root value: no node at all + BareValue, +} + +fn src(kind: Src, masked: &[u8]) -> PathMap { + let mut m = PathMap::new(); + m.set_val_at(b"", 0); + let put = |m: &mut PathMap, b: u8, i: u32| { + m.set_val_at([b], 20_000 + i); + m.set_val_at([b, 4], 20_001 + i); + m.set_val_at([b, 5, 6], 20_002 + i); + }; + match kind { + Src::Full => for (i, b) in masked.iter().copied().enumerate() { put(&mut m, b, i as u32) }, + Src::Half => for (i, b) in masked.iter().copied().enumerate().step_by(2) { put(&mut m, b, i as u32) }, + Src::NoneMasked => { + let unmasked: Vec = (0u8..=255).filter(|b| !masked.contains(b)).take(3).collect(); + for (i, b) in unmasked.into_iter().enumerate() { put(&mut m, b, i as u32) } + } + Src::Dangling => for b in masked.iter().copied() { m.create_path([b]); }, + Src::BareValue => {} + } + m +} + +fn run_masked(bencher: Bencher, src: PathMap, dst: PathMap, focus: &[u8], mask: ByteMask, remove_unset: bool) { + let focus = focus.to_vec(); + bencher + .with_inputs(|| dst.clone()) + .bench_local_values(|mut dst| { + { + let mut wz = dst.write_zipper_at_path(&focus); + wz.graft_masked_branches(&src.read_zipper(), mask, remove_unset); + } + dst + }); +} + +fn masked_case(bencher: Bencher, bits: usize, kind: Src, remove_unset: bool) { + let m = masked(bits); + run_masked(bencher, src(kind, &m), dst_with(&dst_bytes(bits)), b"", ByteMask::from_iter(m.iter().copied()), remove_unset); +} + +// ------------------------------------------------------------ graft_masked_branches + +/// Baseline: the source has every masked branch, so nothing is grafted from nothing +#[divan::bench(args = BITS)] +fn masked_src_full(bencher: Bencher, bits: usize) { + masked_case(bencher, bits, Src::Full, false); +} + +/// Half the masked branches are absent from the source and are emptied in the destination +#[divan::bench(args = BITS)] +fn masked_src_half(bencher: Bencher, bits: usize) { + masked_case(bencher, bits, Src::Half, false); +} + +/// Every masked branch is absent from the source, which still has a node +#[divan::bench(args = BITS)] +fn masked_src_none_masked(bencher: Bencher, bits: usize) { + masked_case(bencher, bits, Src::NoneMasked, false); +} + +/// Every masked branch is a dangling path in the source, which counts as nothing +#[divan::bench(args = BITS)] +fn masked_src_dangling(bencher: Bencher, bits: usize) { + masked_case(bencher, bits, Src::Dangling, false); +} + +/// The source has no node at all, so every masked branch of the destination is emptied +#[divan::bench(args = BITS)] +fn masked_src_bare_value(bencher: Bencher, bits: usize) { + masked_case(bencher, bits, Src::BareValue, false); +} + +/// As [masked_src_half], with `remove_unset`: the unmasked branches go outright +#[divan::bench(args = BITS)] +fn masked_src_half_remove_unset(bencher: Bencher, bits: usize) { + masked_case(bencher, bits, Src::Half, true); +} + +/// The destination focus does not exist and the source lacks every masked branch: nothing to do, +/// and nothing may be created +#[divan::bench(args = BITS)] +fn masked_missing_focus_src_none_masked(bencher: Bencher, bits: usize) { + let m = masked(bits); + run_masked(bencher, src(Src::NoneMasked, &m), dst_with(&dst_bytes(bits)), &[255, 254, 253], ByteMask::from_iter(m.iter().copied()), false); +} + +/// The destination focus does not exist and the source has every masked branch: the focus is created +#[divan::bench(args = BITS)] +fn masked_missing_focus_src_full(bencher: Bencher, bits: usize) { + let m = masked(bits); + run_masked(bencher, src(Src::Full, &m), dst_with(&dst_bytes(bits)), &[255, 254, 253], ByteMask::from_iter(m.iter().copied()), false); +} + +// ------------------------------------------------------------------ graft_child_maps + +/// One map per masked byte: full, or empty for every other byte when `half_empty` +fn child_maps(masked: &[u8], half_empty: bool) -> Vec> { + masked.iter().enumerate().map(|(i, _)| { + let mut m = PathMap::new(); + if !(half_empty && i % 2 == 1) { + m.set_val_at(b"", 20_000 + i as u32); + m.set_val_at([4u8], 20_001 + i as u32); + m.set_val_at([5u8, 6], 20_002 + i as u32); + } + m + }).collect() +} + +fn run_child_maps(bencher: Bencher, bits: usize, half_empty: bool, remove_unset: bool) { + let m = masked(bits); + let maps = child_maps(&m, half_empty); + let mask = ByteMask::from_iter(m.iter().copied()); + let dst = dst_with(&dst_bytes(bits)); + bencher + .with_inputs(|| (dst.clone(), maps.clone())) + .bench_local_values(|(mut dst, maps)| { + dst.write_zipper().graft_child_maps(mask, maps, remove_unset); + dst + }); +} + +/// Slow path, every map non-empty: each branch is emptied before its map goes in +#[divan::bench(args = BITS)] +fn child_maps_keep_full(bencher: Bencher, bits: usize) { + run_child_maps(bencher, bits, false, false); +} + +/// Slow path, every other map empty: those branches are emptied and kept as dangling paths +#[divan::bench(args = BITS)] +fn child_maps_keep_half_empty(bencher: Bencher, bits: usize) { + run_child_maps(bencher, bits, true, false); +} + +/// Fast path (`remove_unset`, more than two maps), every map non-empty +#[divan::bench(args = BITS)] +fn child_maps_remove_unset_full(bencher: Bencher, bits: usize) { + run_child_maps(bencher, bits, false, true); +} + +/// Fast path, every other map empty: the empty ones are left out of the new node +#[divan::bench(args = BITS)] +fn child_maps_remove_unset_half_empty(bencher: Bencher, bits: usize) { + run_child_maps(bencher, bits, true, true); +} diff --git a/src/dense_byte_node.rs b/src/dense_byte_node.rs index 0b7602c5..6a4717bd 100644 --- a/src/dense_byte_node.rs +++ b/src/dense_byte_node.rs @@ -1610,14 +1610,37 @@ where } } - let src_range_mask = src_node.mask & ByteMask::from_range(range_start..=range_end); + let range_mask = ByteMask::from_range(range_start..=range_end); + let src_range_mask = src_node.mask & range_mask; let mut src_ix = src_node.mask.index_of(range_start) as usize; - for child_byte in src_range_mask.iter() { - let cf = unsafe { src_node.values.get_unchecked(src_ix) }; - src_ix += 1; - - if cf.has_rec() || cf.has_val() { - new_values.v.push(CfDst::from_cf(cf.clone())); + if REMOVE_UNSET { + //Only the source's branches can survive + for child_byte in src_range_mask.iter() { + let cf = unsafe { src_node.values.get_unchecked(src_ix) }; + src_ix += 1; + if cf.has_val() || cf.rec().is_some_and(|rec| !rec.is_empty() && !rec.as_tagged().node_is_empty()) { + new_values.v.push(CfDst::from_cf(cf.clone())); + new_mask.set_bit(child_byte); + } + } + prev_end = range_end as usize + 1; + continue + } + for child_byte in (src_range_mask | (old_mask & range_mask)).iter() { + if src_range_mask.test_bit(child_byte) { + let cf = unsafe { src_node.values.get_unchecked(src_ix) }; + src_ix += 1; + // A dangling source branch is nothing to graft, like an absent one + if cf.has_val() || cf.rec().is_some_and(|rec| !rec.is_empty() && !rec.as_tagged().node_is_empty()) { + new_values.v.push(CfDst::from_cf(cf.clone())); + new_mask.set_bit(child_byte); + continue + } + } + if old_mask.test_bit(child_byte) { + // Grafting nothing over an existing branch empties it, but the location survives + // as a dangling path, as after `graft` of an empty source + new_values.v.push(CfDst::new(Some(TrieNodeODRc::new_empty()), None)); new_mask.set_bit(child_byte); } } diff --git a/src/write_zipper.rs b/src/write_zipper.rs index bde799aa..ac92a30f 100644 --- a/src/write_zipper.rs +++ b/src/write_zipper.rs @@ -1672,6 +1672,35 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC ; *focus_node = replacement; } + /// One masked branch of [Self::graft_masked_branches]: `self`'s branch at `byte` becomes the + /// source's, value included. A source with nothing there (a dangling branch included) is + /// grafting nothing; see [Self::empty_branch] + fn graft_masked_branch>(&mut self, src: &Z, byte: u8) { + let src_node = src.get_focus_at([byte]).into_option().filter(|node| !node.as_tagged().node_is_empty()); + let src_val = src.val_at([byte]); + if src_node.is_none() && src_val.is_none() { + return self.empty_branch(byte) + } + //`graft_src_at`, without looking the source up a second time + self.descend_to_byte(byte); + self.graft_internal(src_node); + match src_val { + Some(val) => { self.set_val(val.clone()); }, + None => { self.remove_val(false); }, + } + self.ascend_byte(); + } + /// Grafting nothing at the branch `byte`: its contents and value go, but the location is + /// neither created nor destroyed. An existing branch survives as a dangling path, as after + /// `graft` of an empty source, and an absent one stays absent + fn empty_branch(&mut self, byte: u8) { + self.descend_to_byte(byte); + if self.path_exists() { + self.remove_branches(false); + self.remove_val(false); + } + self.ascend_byte(); + } /// See [ZipperWriting::graft_masked_branches] pub fn graft_masked_branches>(&mut self, src: &Z, child_mask: ByteMask, remove_unset: bool) { // The dense-node merge handles both pieces of the contract directly: it removes @@ -1690,9 +1719,7 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC } // SAFETY: this arm is selected only when `child_mask` has one bit. let byte = unsafe { child_mask.indexed_bit::(0).unwrap_unchecked() }; - self.descend_to_byte(byte); - self.graft_src_at(src, &[byte]); - self.ascend_byte(); + self.graft_masked_branch(src, byte); } 2 => { if remove_unset { @@ -1700,95 +1727,95 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC } // SAFETY: this arm is selected only when `child_mask` has two bits. let first_byte = unsafe { child_mask.indexed_bit::(0).unwrap_unchecked() }; - self.descend_to_byte(first_byte); - self.graft_src_at(src, &[first_byte]); - self.ascend_byte(); + self.graft_masked_branch(src, first_byte); // SAFETY: `first_byte` is one of the two set bits, so it has a successor. let second_byte = unsafe { child_mask.next_bit(first_byte).unwrap_unchecked() }; - self.descend_to_byte(second_byte); - self.graft_src_at(src, &[second_byte]); - self.ascend_byte(); + self.graft_masked_branch(src, second_byte); } _ => { let src_focus = src.get_focus(); - match src_focus.try_as_tagged() { - Some(src_tagged) => { - // Split the focus if we're in the middle of another node. A focus that - // is a dangling stub (an empty child node, which `get_child_mut` declines - // to hand out) has no node to merge into even after the split, so the - // branches are merged into a fresh node that is grafted in afterwards. - // This used to unwrap the missing node. - let mut fresh_node: Option> = None; - let self_focus_node = match self.try_borrow_focus_mut() { + //A source with nothing below its focus empties the masked branches; that is the + // merge below with an empty byte node as the source + let src_tagged = src_focus.try_as_tagged().filter(|node| !node.node_is_empty()); + if src_tagged.is_none() && (remove_unset || child_mask & self.child_mask() == ByteMask::EMPTY) { + if remove_unset { + self.remove_branches(false); + } + return + } + let empty_src; + let src_tagged = match src_tagged { + Some(src_tagged) => src_tagged, + None => { + empty_src = TrieNodeODRc::new_in(crate::dense_byte_node::DenseByteNode::::new_in(self.alloc.clone()), self.alloc.clone()); + empty_src.as_tagged() + } + }; + // Split the focus if we're in the middle of another node. A focus that + // is a dangling stub (an empty child node, which `get_child_mut` declines + // to hand out) has no node to merge into even after the split, so the + // branches are merged into a fresh node that is grafted in afterwards. + // This used to unwrap the missing node. + // A focus that does not exist is not split either, since the split would + // create it: the fresh node is grafted only if the merge produced something. + let mut fresh_node: Option> = None; + let self_focus_node = match self.try_borrow_focus_mut() { + Some(node) => node, + None => { + if self.path_exists() { + self.split_at_focus(); + } + match self.try_borrow_focus_mut() { Some(node) => node, None => { - self.split_at_focus(); - match self.try_borrow_focus_mut() { - Some(node) => node, - None => { - let alloc = self.alloc.clone(); - fresh_node.insert(TrieNodeODRc::new_in( - crate::dense_byte_node::DenseByteNode::::new_in(alloc.clone()), alloc)) - } - } + let alloc = self.alloc.clone(); + fresh_node.insert(TrieNodeODRc::new_in( + crate::dense_byte_node::DenseByteNode::::new_in(alloc.clone()), alloc)) } - }; - match src_tagged { - TaggedNodeRef::DenseByteNode(src_node) => { - if remove_unset { - Self::merge_branches_into_focus::, true>(self_focus_node, src_node, child_mask); - } else { - Self::merge_branches_into_focus::, false>(self_focus_node, src_node, child_mask); - } - }, - TaggedNodeRef::CellByteNode(src_node) => { - if remove_unset { - Self::merge_branches_into_focus::, true>(self_focus_node, src_node, child_mask); - } else { - Self::merge_branches_into_focus::, false>(self_focus_node, src_node, child_mask); - } - }, - TaggedNodeRef::LineListNode(src_node) => { - let mut src_node = src_node.clone(); - let src_dense = src_node.convert_to_dense::>(3); - let src_dense = src_dense.as_tagged().as_dense().unwrap(); - if remove_unset { - Self::merge_branches_into_focus::, true>(self_focus_node, src_dense, child_mask); - } else { - Self::merge_branches_into_focus::, false>(self_focus_node, src_dense, child_mask); - } - }, - TaggedNodeRef::TinyRefNode(src_node) => { - let mut src_node = src_node.into_full().unwrap(); - let src_dense = src_node.convert_to_dense::>(3); - let src_dense = src_dense.as_tagged().as_dense().unwrap(); - if remove_unset { - Self::merge_branches_into_focus::, true>(self_focus_node, src_dense, child_mask); - } else { - Self::merge_branches_into_focus::, false>(self_focus_node, src_dense, child_mask); - } - }, - TaggedNodeRef::EmptyNode => { - if remove_unset { - self.remove_branches(false); - } else { - self.remove_unmasked_branches(child_mask.not(), false); - } - }, } - if let Some(node) = fresh_node { - if !node.as_tagged().node_is_empty() { - self.graft_internal(Some(node)); - } + } + }; + match src_tagged { + TaggedNodeRef::DenseByteNode(src_node) => { + if remove_unset { + Self::merge_branches_into_focus::, true>(self_focus_node, src_node, child_mask); + } else { + Self::merge_branches_into_focus::, false>(self_focus_node, src_node, child_mask); } }, - None => { + TaggedNodeRef::CellByteNode(src_node) => { + if remove_unset { + Self::merge_branches_into_focus::, true>(self_focus_node, src_node, child_mask); + } else { + Self::merge_branches_into_focus::, false>(self_focus_node, src_node, child_mask); + } + }, + TaggedNodeRef::LineListNode(src_node) => { + let mut src_node = src_node.clone(); + let src_dense = src_node.convert_to_dense::>(3); + let src_dense = src_dense.as_tagged().as_dense().unwrap(); + if remove_unset { + Self::merge_branches_into_focus::, true>(self_focus_node, src_dense, child_mask); + } else { + Self::merge_branches_into_focus::, false>(self_focus_node, src_dense, child_mask); + } + }, + TaggedNodeRef::TinyRefNode(src_node) => { + let mut src_node = src_node.into_full().unwrap(); + let src_dense = src_node.convert_to_dense::>(3); + let src_dense = src_dense.as_tagged().as_dense().unwrap(); if remove_unset { - self.remove_branches(false); + Self::merge_branches_into_focus::, true>(self_focus_node, src_dense, child_mask); } else { - self.remove_unmasked_branches(child_mask.not(), false); + Self::merge_branches_into_focus::, false>(self_focus_node, src_dense, child_mask); } + }, + TaggedNodeRef::EmptyNode => unreachable!(), + } + if let Some(node) = fresh_node { + if !node.as_tagged().node_is_empty() { + self.graft_internal(Some(node)); } } } @@ -1813,7 +1840,7 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC for child_byte in child_mask.iter() { let map = maps_iter.next().expect("maps iterator returned fewer items than the number of set bits in child_mask"); let (src_root_node, src_root_val) = map.into_root(); - if let Some(node) = src_root_node { + if let Some(node) = src_root_node.filter(|n| !n.as_tagged().node_is_empty()) { new_node.set_child(child_byte, node); } if let Some(val) = src_root_val { @@ -1821,7 +1848,12 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC } } let new_node_odrc = TrieNodeODRc::new_in(new_node, self.alloc.clone()); - self.graft_internal(Some(new_node_odrc)); + if new_node_odrc.as_tagged().node_is_empty() { + // Every map was empty. Grafting the empty node would create the focus out of nothing + self.remove_branches(false); + } else { + self.graft_internal(Some(new_node_odrc)); + } } else { // If we don't have enough children to justify forcing a new ByteNode, just set the nodes if remove_unset { @@ -1832,11 +1864,28 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC let map = maps_iter.next().expect("maps iterator returned fewer items than the number of set bits in child_mask"); let (src_root_node, src_root_val) = map.into_root(); - if let Some(node) = src_root_node { - self.set_node_at_child_path(&[child_byte], node) - } - if let Some(val) = src_root_val { - let _ = self.set_val_at(&[child_byte], val); + let src_root_node = src_root_node.filter(|n| !n.as_tagged().node_is_empty()); + if remove_unset { + //Everything below the focus is already gone + if let Some(node) = src_root_node { + self.set_node_at_child_path(&[child_byte], node) + } + if let Some(val) = src_root_val { + let _ = self.set_val_at(&[child_byte], val); + } + } else { + //Each branch is *replaced* by its map, the way `graft` replaces a subtrie. An + // empty map empties the branch, whose location survives as a dangling path + self.descend_to_byte(child_byte); + match src_root_node { + Some(node) => self.graft_internal(Some(node)), + None => { self.remove_branches(false); }, + } + match src_root_val { + Some(val) => { self.set_val(val); }, + None => { self.remove_val(false); }, + } + self.ascend_byte(); } } } @@ -7141,6 +7190,122 @@ mod tests { assert_eq!(map.val_at(b"focus"), Some(&8)); } + /// lean/FINDINGS.md #15, `graft_child_maps_dense`: grafting nothing must neither create nor + /// destroy a location, and `graft_child_maps` must not abort on a dense destination. + /// + /// The contract for one masked branch is that of `graft` at that branch: the source's + /// contents and value replace whatever was there. A source with nothing at the branch + /// (absent, or a dangling path) therefore empties an existing branch -- which then survives + /// as a dangling path, exactly as after `graft` of an empty source -- and leaves an absent + /// branch absent. `remove_unset` removes the unset branches outright first. + #[test] + fn graft_child_maps_dense() { + use crate::utils::BitMask; + fn mask(bytes: &[u8]) -> ByteMask { + let mut m = ByteMask::EMPTY; + for b in bytes { m.set_bit(*b); } + m + } + fn dump(m: &PathMap) -> Vec<(Vec, Option)> { + // Every location, dangling ones included, with its value. + let mut z = m.read_zipper(); + let mut out = Vec::new(); + while z.to_next_step() { out.push((z.path().to_vec(), z.val().copied())); } + out + } + fn mk(kvs: &[(&[u8], u64)]) -> PathMap { + let mut m = PathMap::new(); + for (k, v) in kvs { m.set_val_at(k, *v); } + m + } + + // (a) A dense destination (>= 3 branches) used to reach `node_get_child_mut` with an + // empty key and abort. + let mut dst = mk(&[(&[0, 0], 1), (&[1, 0], 1), (&[2, 0], 1), (&[3, 0], 1)]); + let child = mk(&[(&[], 7), (&[3], 8)]); + dst.write_zipper().graft_child_maps(mask(&[0]), vec![child], false); + assert_eq!(dump(&dst), vec![ + (vec![0], Some(7)), (vec![0, 3], Some(8)), + (vec![1], None), (vec![1, 0], Some(1)), + (vec![2], None), (vec![2, 0], Some(1)), + (vec![3], None), (vec![3, 0], Some(1)), + ]); + + // (b) Empty maps at a non-existent focus: nothing is created, both with and without + // `remove_unset`, and the same for `graft_masked_branches` from an empty source. + for remove_unset in [true, false] { + let mut m = mk(&[(&[1], 1)]); + { + let mut wz = m.write_zipper(); + wz.descend_to(&[0u8, 0]); + wz.graft_child_maps(mask(&[0, 1, 2]), vec![PathMap::new(); 3], remove_unset); + assert!(!wz.path_exists()); + } + assert_eq!(dump(&m), vec![(vec![1], Some(1))]); + } + for bits in [&[0u8][..], &[0, 2], &[0, 2, 3]] { + let mut m = mk(&[(&[1], 1)]); + let src = mk(&[(&[1, 0], 5)]); + { + let mut wz = m.write_zipper(); + wz.descend_to(&[2u8, 2]); + wz.graft_masked_branches(&src.read_zipper(), mask(bits), false); + assert!(!wz.path_exists(), "{bits:?}"); + } + assert_eq!(dump(&m), vec![(vec![1], Some(1))], "{bits:?}"); + } + + // (c) A masked branch the source lacks is emptied but its location survives; a branch + // the source has is replaced, value included (and a missing source value removes the + // old one). One-, two- and three-bit masks take different code paths. + let src = mk(&[(&[1, 0], 5)]); + for bits in [&[0u8][..], &[0, 1], &[0, 1, 2]] { + let mut m = mk(&[(&[0], 9), (&[0, 7], 9), (&[1], 1), (&[1, 1], 1), (&[3], 3)]); + m.write_zipper().graft_masked_branches(&src.read_zipper(), mask(bits), false); + let mut expected = vec![(vec![0], None)]; + if bits.contains(&1) { + expected.extend([(vec![1], None), (vec![1, 0], Some(5))]); + } else { + expected.extend([(vec![1], Some(1)), (vec![1, 1], Some(1))]); + } + expected.push((vec![3], Some(3))); + assert_eq!(dump(&m), expected, "{bits:?}"); + } + // A source whose focus is a bare value (no node) or a dangling path takes yet another + // path, and must behave as an empty source. + for src in [mk(&[(&[], 4)]), { let mut s = PathMap::::new(); s.write_zipper().create_path(); s }] { + let mut m = mk(&[(&[0], 9), (&[0, 7], 9), (&[1], 1), (&[3], 3)]); + m.write_zipper().graft_masked_branches(&src.read_zipper(), mask(&[0, 1, 2]), false); + assert_eq!(dump(&m), vec![(vec![0], None), (vec![1], None), (vec![3], Some(3))]); + } + // With `remove_unset` the branches go outright. + let mut m = mk(&[(&[0], 9), (&[0, 7], 9), (&[1], 1), (&[3], 3)]); + m.write_zipper().graft_masked_branches(&src.read_zipper(), mask(&[0, 1, 2]), true); + assert_eq!(dump(&m), vec![(vec![1], None), (vec![1, 0], Some(5))]); + + // (d) `graft_child_maps` fed the source's own child subtries agrees with (c). + for bits in [&[0u8][..], &[0, 1], &[0, 1, 2]] { + let maps: Vec> = bits.iter().map(|b| { + let mut z = src.read_zipper(); + z.descend_to_byte(*b); + z.make_map() + }).collect(); + let mut m = mk(&[(&[0], 9), (&[0, 7], 9), (&[1], 1), (&[1, 1], 1), (&[3], 3)]); + let mut reference = m.clone(); + m.write_zipper().graft_child_maps(mask(bits), maps, false); + reference.write_zipper().graft_masked_branches(&src.read_zipper(), mask(bits), false); + assert_eq!(dump(&m), dump(&reference), "{bits:?}"); + } + // A map with contents but no root value removes the old value; an empty map empties + // the branch and keeps the location. + let mut m = mk(&[(&[0], 9), (&[0, 7], 9), (&[1], 1)]); + m.write_zipper().graft_child_maps(mask(&[0]), vec![mk(&[(&[3], 3)])], false); + assert_eq!(dump(&m), vec![(vec![0], None), (vec![0, 3], Some(3)), (vec![1], Some(1))]); + let mut m = mk(&[(&[0], 9), (&[0, 7], 9), (&[1], 1)]); + m.write_zipper().graft_child_maps(mask(&[0]), vec![PathMap::new()], false); + assert_eq!(dump(&m), vec![(vec![0], None), (vec![1], Some(1))]); + } + #[test] fn write_zipper_graft_masked_branches_test4() { // Upper bound 0: remove_unset=true with an empty mask. From 86436d9140009311185cdf4363169badcf65431c Mon Sep 17 00:00:00 2001 From: Igor Malovitsa Date: Wed, 4 Nov 2026 18:10:01 +0000 Subject: [PATCH 2/2] Merge graft_child_maps into the focus in one pass Without remove_unset, more than two maps were grafted one branch at a time, each a descend, graft, set or remove value, and ascend. The maps are now gathered into one byte node and merged into the focus the way graft_masked_branches merges a source node, which that merge is factored out for. An absent source node is now an empty byte node on the stack rather than an allocated one. --- src/write_zipper.rs | 173 +++++++++++++++++++++++--------------------- 1 file changed, 90 insertions(+), 83 deletions(-) diff --git a/src/write_zipper.rs b/src/write_zipper.rs index ac92a30f..10d4eb7c 100644 --- a/src/write_zipper.rs +++ b/src/write_zipper.rs @@ -1735,89 +1735,94 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC } _ => { let src_focus = src.get_focus(); - //A source with nothing below its focus empties the masked branches; that is the - // merge below with an empty byte node as the source - let src_tagged = src_focus.try_as_tagged().filter(|node| !node.node_is_empty()); - if src_tagged.is_none() && (remove_unset || child_mask & self.child_mask() == ByteMask::EMPTY) { - if remove_unset { - self.remove_branches(false); - } - return + self.graft_masked_from_node(src_focus.try_as_tagged(), child_mask, remove_unset); + } + } + } + + /// The byte-node merge behind [Self::graft_masked_branches] (more than two masked branches) + /// and [Self::graft_child_maps]: each masked branch of the focus becomes `src`'s branch, and a + /// masked branch `src` lacks is emptied but kept as a dangling path, as by `graft` of nothing + fn graft_masked_from_node(&mut self, src: Option>, child_mask: ByteMask, remove_unset: bool) { + //A source with nothing below its focus empties the masked branches; that is the merge + // below with an empty byte node as the source + let src = src.filter(|node| !node.node_is_empty()); + if src.is_none() && (remove_unset || child_mask & self.child_mask() == ByteMask::EMPTY) { + if remove_unset { + self.remove_branches(false); + } + return + } + //An empty node holds no allocation, so it can live on the stack + let empty_src = crate::dense_byte_node::DenseByteNode::::new_in(self.alloc.clone()); + let src_tagged = match src { + Some(src) => src, + None => empty_src.as_tagged(), + }; + // Split the focus if we're in the middle of another node. A focus that + // is a dangling stub (an empty child node, which `get_child_mut` declines + // to hand out) has no node to merge into even after the split, so the + // branches are merged into a fresh node that is grafted in afterwards. + // This used to unwrap the missing node. + // A focus that does not exist is not split either, since the split would + // create it: the fresh node is grafted only if the merge produced something. + let mut fresh_node: Option> = None; + let self_focus_node = match self.try_borrow_focus_mut() { + Some(node) => node, + None => { + if self.path_exists() { + self.split_at_focus(); } - let empty_src; - let src_tagged = match src_tagged { - Some(src_tagged) => src_tagged, - None => { - empty_src = TrieNodeODRc::new_in(crate::dense_byte_node::DenseByteNode::::new_in(self.alloc.clone()), self.alloc.clone()); - empty_src.as_tagged() - } - }; - // Split the focus if we're in the middle of another node. A focus that - // is a dangling stub (an empty child node, which `get_child_mut` declines - // to hand out) has no node to merge into even after the split, so the - // branches are merged into a fresh node that is grafted in afterwards. - // This used to unwrap the missing node. - // A focus that does not exist is not split either, since the split would - // create it: the fresh node is grafted only if the merge produced something. - let mut fresh_node: Option> = None; - let self_focus_node = match self.try_borrow_focus_mut() { + match self.try_borrow_focus_mut() { Some(node) => node, None => { - if self.path_exists() { - self.split_at_focus(); - } - match self.try_borrow_focus_mut() { - Some(node) => node, - None => { - let alloc = self.alloc.clone(); - fresh_node.insert(TrieNodeODRc::new_in( - crate::dense_byte_node::DenseByteNode::::new_in(alloc.clone()), alloc)) - } - } + let alloc = self.alloc.clone(); + fresh_node.insert(TrieNodeODRc::new_in( + crate::dense_byte_node::DenseByteNode::::new_in(alloc.clone()), alloc)) } - }; - match src_tagged { - TaggedNodeRef::DenseByteNode(src_node) => { - if remove_unset { - Self::merge_branches_into_focus::, true>(self_focus_node, src_node, child_mask); - } else { - Self::merge_branches_into_focus::, false>(self_focus_node, src_node, child_mask); - } - }, - TaggedNodeRef::CellByteNode(src_node) => { - if remove_unset { - Self::merge_branches_into_focus::, true>(self_focus_node, src_node, child_mask); - } else { - Self::merge_branches_into_focus::, false>(self_focus_node, src_node, child_mask); - } - }, - TaggedNodeRef::LineListNode(src_node) => { - let mut src_node = src_node.clone(); - let src_dense = src_node.convert_to_dense::>(3); - let src_dense = src_dense.as_tagged().as_dense().unwrap(); - if remove_unset { - Self::merge_branches_into_focus::, true>(self_focus_node, src_dense, child_mask); - } else { - Self::merge_branches_into_focus::, false>(self_focus_node, src_dense, child_mask); - } - }, - TaggedNodeRef::TinyRefNode(src_node) => { - let mut src_node = src_node.into_full().unwrap(); - let src_dense = src_node.convert_to_dense::>(3); - let src_dense = src_dense.as_tagged().as_dense().unwrap(); - if remove_unset { - Self::merge_branches_into_focus::, true>(self_focus_node, src_dense, child_mask); - } else { - Self::merge_branches_into_focus::, false>(self_focus_node, src_dense, child_mask); - } - }, - TaggedNodeRef::EmptyNode => unreachable!(), } - if let Some(node) = fresh_node { - if !node.as_tagged().node_is_empty() { - self.graft_internal(Some(node)); - } + } + }; + match src_tagged { + TaggedNodeRef::DenseByteNode(src_node) => { + if remove_unset { + Self::merge_branches_into_focus::, true>(self_focus_node, src_node, child_mask); + } else { + Self::merge_branches_into_focus::, false>(self_focus_node, src_node, child_mask); + } + }, + TaggedNodeRef::CellByteNode(src_node) => { + if remove_unset { + Self::merge_branches_into_focus::, true>(self_focus_node, src_node, child_mask); + } else { + Self::merge_branches_into_focus::, false>(self_focus_node, src_node, child_mask); } + }, + TaggedNodeRef::LineListNode(src_node) => { + let mut src_node = src_node.clone(); + let src_dense = src_node.convert_to_dense::>(3); + let src_dense = src_dense.as_tagged().as_dense().unwrap(); + if remove_unset { + Self::merge_branches_into_focus::, true>(self_focus_node, src_dense, child_mask); + } else { + Self::merge_branches_into_focus::, false>(self_focus_node, src_dense, child_mask); + } + }, + TaggedNodeRef::TinyRefNode(src_node) => { + let mut src_node = src_node.into_full().unwrap(); + let src_dense = src_node.convert_to_dense::>(3); + let src_dense = src_dense.as_tagged().as_dense().unwrap(); + if remove_unset { + Self::merge_branches_into_focus::, true>(self_focus_node, src_dense, child_mask); + } else { + Self::merge_branches_into_focus::, false>(self_focus_node, src_dense, child_mask); + } + }, + TaggedNodeRef::EmptyNode => unreachable!(), + } + if let Some(node) = fresh_node { + if !node.as_tagged().node_is_empty() { + self.graft_internal(Some(node)); } } } @@ -1829,12 +1834,8 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC pub fn graft_child_maps>>(&mut self, child_mask: ByteMask, maps: I, remove_unset: bool) { let map_count = child_mask.count_bits(); - // If we're replacing all children, and we have enough children to justify a ByteNode, - // then we want to build a new node with the children from the maps - if map_count > 2 && remove_unset { - //GOAT, we could add a fast-path for `map_count > 2 && !remove_unset`, but for now - // we'll let the slow-path handle it, since it requires logic to upgrade the focus_node to ByteNode - + // With enough children to justify a ByteNode, gather the maps into one + if map_count > 2 { let mut new_node = crate::dense_byte_node::DenseByteNode::with_capacity_in(map_count, self.alloc.clone()); let mut maps_iter = maps.into_iter(); for child_byte in child_mask.iter() { @@ -1847,6 +1848,12 @@ impl <'a, 'path, V: Clone + Send + Sync + Unpin, A: Allocator + 'a> WriteZipperC new_node.set_val(child_byte, val); } } + if !remove_unset { + //Each branch is *replaced* by its map, exactly as `graft_masked_branches` replaces + // it with the source's, so merge the gathered node in the same way + return self.graft_masked_from_node(Some(new_node.as_tagged()), child_mask, false) + } + //Replacing all children: the gathered node is the new focus let new_node_odrc = TrieNodeODRc::new_in(new_node, self.alloc.clone()); if new_node_odrc.as_tagged().node_is_empty() { // Every map was empty. Grafting the empty node would create the focus out of nothing