From e60084196d01f6468614c9b008afa04c1d47a2c0 Mon Sep 17 00:00:00 2001 From: Arthur Silva Date: Thu, 21 Jan 2021 12:39:28 +0100 Subject: [PATCH] Fix RemoveAction::PullUp semantics It must pull the highest or lowest item in the subtree and not just from the next level --- benches/native.rs | 2 +- src/hash/set.rs | 2 +- src/nodes/btree.rs | 116 ++++++++++++++++++++++----------------------- 3 files changed, 60 insertions(+), 60 deletions(-) diff --git a/benches/native.rs b/benches/native.rs index 36db8f1..dcd3f5d 100644 --- a/benches/native.rs +++ b/benches/native.rs @@ -8,7 +8,7 @@ extern crate im; extern crate rand; extern crate test; -use rand::{rngs::SmallRng, SeedableRng, Rng}; +use rand::{rngs::SmallRng, Rng, SeedableRng}; use std::collections::{BTreeMap, HashMap, VecDeque}; use std::iter::FromIterator; use test::Bencher; diff --git a/src/hash/set.rs b/src/hash/set.rs index 4f1dfee..53d3d6b 100644 --- a/src/hash/set.rs +++ b/src/hash/set.rs @@ -33,8 +33,8 @@ use std::ops::{Add, Deref, Mul}; use crate::nodes::hamt::{hash_key, Drain as NodeDrain, HashValue, Iter as NodeIter, Node}; use crate::ordset::OrdSet; -use crate::Vector; use crate::util::{Pool, PoolRef, Ref}; +use crate::Vector; /// Construct a set from a sequence of values. /// diff --git a/src/nodes/btree.rs b/src/nodes/btree.rs index da74b6f..3eeb7fb 100644 --- a/src/nodes/btree.rs +++ b/src/nodes/btree.rs @@ -92,9 +92,14 @@ pub(crate) enum Remove { Update(A, Node), } +enum Boundary { + Lowest, + Highest, +} + enum RemoveAction { DeleteAt(usize), - PullUp(usize, usize, usize), + PullUp(Boundary, usize, usize), Merge(usize), StealFromLeft(usize), StealFromRight(usize), @@ -636,14 +641,32 @@ impl Node { A::Key: Borrow, { let index = A::search_key(&self.keys, key); - self.remove_index(pool, index, key) + self.remove_index(pool, index, Ok(key)) + } + + fn remove_target( + &mut self, + pool: &Pool>, + target: Result<&BK, Boundary>, + ) -> Remove + where + A: Clone, + BK: Ord + ?Sized, + A::Key: Borrow, + { + let index = match target { + Ok(key) => A::search_key(&self.keys, key), + Err(Boundary::Lowest) => Err(0), + Err(Boundary::Highest) => Err(self.keys.len()), + }; + self.remove_index(pool, index, target) } fn remove_index( &mut self, pool: &Pool>, index: Result, - key: &BK, + target: Result<&BK, Boundary>, ) -> Remove where A: Clone, @@ -656,47 +679,30 @@ impl Node { match (&self.children[index], &self.children[index + 1]) { // If we're a leaf, just delete the entry. (&None, &None) => RemoveAction::DeleteAt(index), - // Right is empty. Attempt to steal from left if enough capacity, - // otherwise pull the predecessor up. - (&Some(ref left), &None) => { - if !left.too_small() { - RemoveAction::StealFromLeft(index + 1) - } else { - RemoveAction::PullUp(left.keys.len() - 1, index, index) - } - } - // Left is empty. Attempt to steal from right if enough capacity, - // otherwise pull the successor up. - (&None, &Some(ref right)) => { - if !right.too_small() { - RemoveAction::StealFromRight(index) - } else { - RemoveAction::PullUp(0, index, index + 1) - } - } - // Both left and right are non empty. // First consider pulling either predecessor (from left) or successor (from right). - // Then attempt to steal from left or right if enough capacity, // otherwise just merge the two small children. (&Some(ref left), &Some(ref right)) => { if !left.too_small() { - RemoveAction::PullUp(left.keys.len() - 1, index, index) + RemoveAction::PullUp(Boundary::Highest, index, index) } else if !right.too_small() { - RemoveAction::PullUp(0, index, index + 1) - } else if left.has_room() && !right.too_small() { - RemoveAction::StealFromRight(index) - } else if right.has_room() && !left.too_small() { - RemoveAction::StealFromLeft(index + 1) + RemoveAction::PullUp(Boundary::Lowest, index, index + 1) } else { RemoveAction::Merge(index) } } + _ => unreachable!("Branch missing children"), } } - // Key is adjacent to some key in node + // Target is adjacent to some key in node Err(index) => match self.children[index] { - // No child at location means key isn't in map. - None => return Remove::NoChange, + // We're deading with a leaf node + None => match target { + // No child at location means key isn't in map. + Ok(_key) => return Remove::NoChange, + // Looking for the lowest or highest key + Err(Boundary::Lowest) => RemoveAction::DeleteAt(0), + Err(Boundary::Highest) => RemoveAction::DeleteAt(self.keys.len() - 1), + }, // Child at location, but it's at minimum capacity. Some(ref child) if child.too_small() => { let left = if index > 0 { @@ -732,13 +738,13 @@ impl Node { self.children.remove(index); Remove::Removed(pair) } - RemoveAction::PullUp(target_index, pull_to, child_index) => { + RemoveAction::PullUp(boundary, pull_to, child_index) => { let children = &mut self.children; let mut update = None; let value; if let Some(&mut Some(ref mut child_ref)) = children.get_mut(child_index) { let child = PoolRef::make_mut(pool, child_ref); - match child.remove_index(pool, Ok(target_index), key) { + match child.remove_target(pool, Err(boundary)) { Remove::NoChange => unreachable!(), Remove::Removed(pulled_value) => { value = self.keys.set(pull_to, pulled_value); @@ -765,7 +771,7 @@ impl Node { PoolRef::unwrap_or_clone(left), PoolRef::unwrap_or_clone(right), ); - let (removed, new_child) = match merged_child.remove(pool, key) { + let (removed, new_child) = match merged_child.remove_target(pool, target) { Remove::NoChange => unreachable!(), Remove::Removed(removed) => (removed, merged_child), Remove::Update(removed, updated_child) => (removed, updated_child), @@ -784,13 +790,7 @@ impl Node { { let mut children = self.children.as_mut_slice()[index - 1..=index] .iter_mut() - .map(|n| { - if let Some(ref mut o) = *n { - o - } else { - unreachable!() - } - }); + .map(|n| n.as_mut().unwrap()); let left = PoolRef::make_mut(pool, children.next().unwrap()); let child = PoolRef::make_mut(pool, children.next().unwrap()); // Prepare the rebalanced node. @@ -798,7 +798,7 @@ impl Node { left.children.last().unwrap().clone(), self.keys[index - 1].clone(), ); - match child.remove(pool, key) { + match child.remove_target(pool, target) { Remove::NoChange => { // Key wasn't there, we need to revert the steal. child.pop_min(); @@ -830,18 +830,12 @@ impl Node { { let mut children = self.children.as_mut_slice()[index..index + 2] .iter_mut() - .map(|n| { - if let Some(ref mut o) = *n { - o - } else { - unreachable!() - } - }); + .map(|n| n.as_mut().unwrap()); let child = PoolRef::make_mut(pool, children.next().unwrap()); let right = PoolRef::make_mut(pool, children.next().unwrap()); // Prepare the rebalanced node. child.push_max(right.children[0].clone(), self.keys[index].clone()); - match child.remove(pool, key) { + match child.remove_target(pool, target) { Remove::NoChange => { // Key wasn't there, we need to revert the steal. child.pop_max(); @@ -868,11 +862,17 @@ impl Node { Remove::Removed(out_value) } RemoveAction::MergeFirst(index) => { - if self.keys[index].cmp_keys(key) != Ordering::Equal - && !self.child_contains(index, key) - && !self.child_contains(index + 1, key) - { - return Remove::NoChange; + if let Ok(key) = target { + // Bail early if we're looking for a not existing key + match self.keys[index].cmp_keys(key) { + Ordering::Less if !self.child_contains(index + 1, key) => { + return Remove::NoChange + } + Ordering::Greater if !self.child_contains(index, key) => { + return Remove::NoChange + } + _ => (), + } } let left = self.children.remove(index).unwrap(); let right = mem::replace(&mut self.children[index], None).unwrap(); @@ -884,7 +884,7 @@ impl Node { ); let update; let out_value; - match merged.remove(pool, key) { + match merged.remove_target(pool, target) { Remove::NoChange => { panic!("nodes::btree::Node::remove: caught an absent key too late while merging"); } @@ -911,7 +911,7 @@ impl Node { let out_value; if let Some(&mut Some(ref mut child_ref)) = self.children.get_mut(index) { let child = PoolRef::make_mut(pool, child_ref); - match child.remove(pool, key) { + match child.remove_target(pool, target) { Remove::NoChange => return Remove::NoChange, Remove::Removed(value) => { out_value = value;