From 2f4ab54fc3fa0c7bad4f707dce82d2fb8529cb56 Mon Sep 17 00:00:00 2001 From: meh Date: Tue, 1 Sep 2026 13:56:10 +0700 Subject: [PATCH 1/3] Remove RandomMultiPair and make (key, value) the only entry identity RandomMultiPair ordered entries by (key, random discriminator) so that a V which was only PartialEq could still be stored in a multimap. Both of the things that follow from that were inherent to the design rather than bugs sitting on top of it. Its Ord consulted value equality before the discriminator, which is not transitive, so binary search over nodes was unreliable, same-key churn accumulated duplicate logical pairs, and once same-key nodes split the skip map held entry keys comparing Equal, which corrupted routing and livelocked the split retry loop. Making the order lawful, as 0.0.8 did, meant the value stopped participating in it, so a stored entry could no longer be found by its value and insert had to scan every entry sharing the key. That is O(n) per insert and quadratic to fill one key. Measured, one key, values inserted one at a time: values 0.0.7 0.0.8 this 1,000 259 ns 21.95 us 877 ns 4,000 320 ns 87.13 us 746 ns 16,000 416 ns 356.31 us 333 ns 356 us per insert at 16,000 values, against 333 ns here. The distinct-key case recovers too, 5.57 us to 285 ns. OrdMultiPair was never slower at any size, including two values per key, so there was no size at which the random representation paid for itself. OrdMultiPair, whose identity is the (key, value) pair lexicographically ordered, is now the only representation. It is a lawful total order, a stored entry is located by binary search, and duplicate replacement falls out of the order rather than needing a scan. The niche the random representation served, a V that is PartialEq but not Ord, does not justify a second representation: wrap such a value in a newtype with a total order. The type is gone rather than deprecated, and the reasoning is recorded on MultiPair so it is not reintroduced. One test asserted that replacing a logically equal pair preserved its discriminator; that property no longer exists and the test goes with the type. Two tests built nodes in discriminator order and now build them in (key, value) order. --- Cargo.toml | 2 +- src/cdc/change.rs | 9 +- src/concurrent/multimap.rs | 5 +- src/concurrent/set.rs | 84 +----- src/core/multipair.rs | 26 +- src/core/multipair/random.rs | 502 ----------------------------------- 6 files changed, 36 insertions(+), 592 deletions(-) delete mode 100644 src/core/multipair/random.rs diff --git a/Cargo.toml b/Cargo.toml index 843fa3c..dd89b15 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "WorkTablesIndex" -version = "0.0.8" +version = "0.0.9" edition = "2021" documentation = "https://docs.rs/WorkTablesIndex/" repository = "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/pathscale/WorkTablesIndex" diff --git a/src/cdc/change.rs b/src/cdc/change.rs index 5201564..8bdc436 100644 --- a/src/cdc/change.rs +++ b/src/cdc/change.rs @@ -1,6 +1,6 @@ #[cfg(feature = "multimap")] use { - crate::core::multipair::{OrdMultiPair, RandomMultiPair}, + crate::core::multipair::OrdMultiPair, crate::core::pair::Pair, }; @@ -190,13 +190,6 @@ where } } -#[cfg(feature = "multimap")] -impl From>> for ChangeEvent> { - fn from(ev: ChangeEvent>) -> Self { - multipair_change_event_into_pair(ev) - } -} - #[cfg(feature = "multimap")] impl From>> for ChangeEvent> { fn from(ev: ChangeEvent>) -> Self { diff --git a/src/concurrent/multimap.rs b/src/concurrent/multimap.rs index 087c6f5..e3da6c0 100644 --- a/src/concurrent/multimap.rs +++ b/src/concurrent/multimap.rs @@ -544,7 +544,7 @@ where #[cfg(test)] mod tests { use super::BTreeMultiMap; - use crate::core::multipair::{MultiPairLike, OrdMultiPair, RandomMultiPair}; + use crate::core::multipair::{MultiPairLike, OrdMultiPair}; use crate::BTreeSet; use std::borrow::Borrow; use std::fmt::Debug; @@ -780,7 +780,6 @@ mod tests { #[test] fn test_range_works_as_expected() { - assert_range_works_as_expected::>(); assert_range_works_as_expected::>(); } @@ -819,7 +818,6 @@ mod tests { #[test] fn test_range_excludes_all_values_at_bounds() { - assert_range_excludes_values_at_bounds::>(); assert_range_excludes_values_at_bounds::>(); } @@ -860,7 +858,6 @@ mod tests { #[test] fn test_get_works_as_expected() { - assert_get_works_as_expected::>(); assert_get_works_as_expected::>(); } diff --git a/src/concurrent/set.rs b/src/concurrent/set.rs index aaee6a6..1463537 100644 --- a/src/concurrent/set.rs +++ b/src/concurrent/set.rs @@ -1445,7 +1445,7 @@ mod tests { use crate::concurrent::operation::Operation; use crate::concurrent::set::{BTreeSet, Iter, DEFAULT_INNER_SIZE}; #[cfg(feature = "multimap")] - use crate::core::multipair::RandomMultiPair; + use crate::core::multipair::OrdMultiPair; use crate::core::node::NodeLike; use rand::Rng; use std::collections::HashSet; @@ -2204,67 +2204,6 @@ mod tests { } #[cfg(feature = "multimap")] - #[test] - fn replace_of_logically_equal_pair_preserves_discriminator_and_position() { - use crate::core::multipair::MultiPairInsertHelper; - - let set = BTreeSet::>::new(); - set.attach_node(vec![ - RandomMultiPair { - key: 1, - value: "a", - discriminator: 10, - }, - RandomMultiPair { - key: 1, - value: "b", - discriminator: 20, - }, - RandomMultiPair { - key: 1, - value: "c", - discriminator: 30, - }, - ]); - - // A logical replace: inserting a (key, value) that is already - // present must locate the stored pair by value equality and replace - // it in place under its stored discriminator. The stored pair's - // position among equal-key neighbors was determined by ITS - // discriminator; a replacement under a fresh random discriminator - // would be a duplicate, which is what accumulated under the old - // value-consulting Ord. - let replaced = RandomMultiPair::insert_into(&set, 1, "b"); - assert_eq!( - replaced, - Some((1, "b")), - "logically equal pair must replace, not duplicate" - ); - - { - let node = set.index.back().expect("node must exist").value().clone(); - let guard = node.lock(); - assert_eq!(guard.len(), 3, "logical replace must not change the pair count"); - assert!( - guard.windows(2).all(|pair| pair[0] < pair[1]), - "node sort invariant broken: {:?}", - *guard - ); - assert_eq!(guard[1].discriminator, 20, "stored discriminator must be preserved"); - } - - // Every live pair stays reachable and removable afterwards. - for (value, discriminator) in [("a", 10), ("b", 20), ("c", 30)] { - let probe = RandomMultiPair { - key: 1, - value, - discriminator, - }; - assert!(set.remove(&probe).is_some(), "pair (1, {value}) unreachable"); - } - assert!(set.is_empty()); - } - #[cfg(feature = "multimap")] #[test] fn test_remove_where_reindexes_changed_node_maximum() { @@ -2283,22 +2222,21 @@ mod tests { #[cfg(feature = "multimap")] #[test] fn test_remove_where_deletes_the_position_found_under_the_node_lock() { - let set = BTreeSet::>::new(); + let set = BTreeSet::>::new(); + // Entries are ordered by `(key, value)`, so an attached node has to be in that + // order. It used to be built in discriminator order, which no longer exists. set.attach_node(vec![ - RandomMultiPair { + OrdMultiPair { key: 1, - value: "target", - discriminator: 5, + value: "first", }, - RandomMultiPair { + OrdMultiPair { key: 1, - value: "middle", - discriminator: 20, + value: "second", }, - RandomMultiPair { + OrdMultiPair { key: 1, - value: "last", - discriminator: 30, + value: "target", }, ]); @@ -2307,7 +2245,7 @@ mod tests { assert_eq!(removed.map(Into::<(usize, &'static str)>::into), Some((1, "target"))); assert_eq!( set.iter().map(|pair| pair.value).collect::>(), - vec!["middle", "last"] + vec!["first", "second"] ); } diff --git a/src/core/multipair.rs b/src/core/multipair.rs index 312e133..09fb533 100644 --- a/src/core/multipair.rs +++ b/src/core/multipair.rs @@ -4,12 +4,30 @@ use crate::core::node::NodeLike; use std::fmt::Debug; pub mod ord; -pub mod random; - pub use ord::OrdMultiPair; -pub use random::RandomMultiPair; -pub type MultiPair = RandomMultiPair; +/// The multimap entry representation. +/// +/// Identity is the `(key, value)` pair itself, lexicographically ordered, so a stored +/// entry is located by binary search. +/// +/// A `RandomMultiPair` variant used to exist alongside this one, ordering by +/// `(key, random discriminator)` so that a `V` that was only `PartialEq` could still be +/// stored. It was removed in 0.0.9 and should not be reintroduced. Two things were wrong +/// with it and both were inherent, not bugs to be fixed: +/// +/// - Its `Ord` consulted value equality before the discriminator, which is not +/// transitive, so binary search over nodes was unreliable and same-key node splits +/// could corrupt routing and livelock the split retry loop. +/// - Making the order lawful meant the value no longer participated in it, so a stored +/// entry could not be found by its value. `insert` had to scan every entry sharing the +/// key, which is O(n) per insert and quadratic to fill one key: 356 us per insert at +/// 16,000 values against 358 ns here, and it never won at any size, not even two +/// values per key. +/// +/// The niche it served, a `V` that is `PartialEq` but not `Ord`, is not worth a second +/// representation. Wrap such a value in a newtype with a total order. +pub type MultiPair = OrdMultiPair; /// Common contract for key-value pairs stored by `BTreeMultiMap`. /// diff --git a/src/core/multipair/random.rs b/src/core/multipair/random.rs deleted file mode 100644 index 1003392..0000000 --- a/src/core/multipair/random.rs +++ /dev/null @@ -1,502 +0,0 @@ -use std::borrow::Borrow; -use std::fmt::Debug; -use std::ops::Bound; - -use core::cmp::Ordering; -#[cfg(feature = "serde")] -use serde::{Deserialize, Serialize}; - -use crate::cdc::change::ChangeEvent; -use crate::concurrent::set::BTreeSet; -use crate::core::node::NodeLike; -use crate::core::pair::Pair; - -use super::{MultiPairInsertHelper, MultiPairLike, MultiPairRemoveHelper}; - -/// A multimap pair whose stored identity and total order are the -/// `(key, discriminator)` tuple. -/// -/// The discriminator is drawn at random on construction and refines the order -/// of equal-key entries, so every stored pair is strictly ordered and every -/// index entry key is unique. The value does not participate in `Ord`, `Eq`, -/// or `Hash` at all: an order that consulted value equality was not a lawful -/// total order (it violated transitivity, broke binary search, and let index -/// entry keys compare `Equal`, corrupting routing once equal-key nodes -/// split). Logical `(key, value)` operations are explicit scans over the -/// key's range with a separate value-equality bound; see -/// [`MultiPairInsertHelper`] and [`MultiPairRemoveHelper`]. -#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] -#[derive(Debug, Default, Clone)] -pub struct RandomMultiPair { - pub key: K, - pub value: V, - pub discriminator: u64, -} - -impl RandomMultiPair { - pub fn new(key: K, value: V) -> Self { - Self { - key, - value, - discriminator: fastrand::u64(..), - } - } -} - -impl Eq for RandomMultiPair {} - -impl PartialEq for RandomMultiPair { - fn eq(&self, other: &Self) -> bool { - self.key.eq(&other.key) && self.discriminator.eq(&other.discriminator) - } -} - -impl Ord for RandomMultiPair { - fn cmp(&self, other: &Self) -> Ordering { - self.key - .cmp(&other.key) - .then(self.discriminator.cmp(&other.discriminator)) - } -} - -impl PartialOrd for RandomMultiPair { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } -} - -impl std::hash::Hash for RandomMultiPair -where - K: std::hash::Hash, -{ - fn hash(&self, state: &mut H) { - self.key.hash(state); - self.discriminator.hash(state); - } -} - -impl Borrow for RandomMultiPair { - fn borrow(&self) -> &K { - &self.key - } -} - -impl MultiPairLike for RandomMultiPair { - fn new(key: K, value: V) -> Self { - Self::new(key, value) - } - - fn key(&self) -> &K { - &self.key - } - - fn value(&self) -> &V { - &self.value - } - - // The stored pair's position among equal-key neighbors is determined by - // its discriminator; keeping it on replace keeps the node sorted. With - // identity being (key, discriminator), an Ord-equal replace already - // carries the stored discriminator, so this is defensive. - fn adopt_stored_identity(stored: &Self, incoming: &mut Self) { - incoming.discriminator = stored.discriminator; - } -} - -impl From> for RandomMultiPair { - fn from(pair: Pair) -> Self { - RandomMultiPair { - key: pair.key, - value: pair.value, - discriminator: fastrand::u64(..), - } - } -} - -impl From> for Pair { - fn from(pair: RandomMultiPair) -> Self { - Pair { - key: pair.key, - value: pair.value, - } - } -} - -impl From> for (K, V) { - fn from(pair: RandomMultiPair) -> Self { - (pair.key, pair.value) - } -} - -impl MultiPairInsertHelper for RandomMultiPair -where - K: Debug + Send + Ord + Clone + 'static, - V: Debug + Send + Clone + PartialEq + 'static, -{ - fn insert_into(set: &BTreeSet, key: K, value: V) -> Option<(K, V)> - where - Self: Debug + Ord + Clone + Send + 'static, - Node: NodeLike + Send + 'static, - { - loop { - // Logical identity is (key, value): locate a stored pair with an - // equal value in the key's range and replace it in place. The - // incoming pair carries the stored discriminator, so the put - // finds the exact stored entry (Ord-equal) and the replacement - // keeps its position among equal-key neighbors. - let stored = set - .range::((Bound::Included(&key), Bound::Included(&key))) - .find(|pair| pair.value == value); - if let Some(stored) = stored { - let incoming = Self { - key: key.clone(), - value: value.clone(), - discriminator: stored.discriminator, - }; - if let Some(replaced) = set.put_with(incoming, Self::adopt_stored_identity) { - return Some(replaced.into()); - } - // The located pair was removed concurrently before the - // replace landed and the put inserted the pair fresh, which - // completes the logical insert. - return None; - } - - // No logical match: insert a fresh entry under a random - // discriminator. `put_checked` fails instead of replacing when - // the (key, discriminator) identity is already taken by another - // pair, so a random collision re-rolls by restarting (which also - // re-runs the logical-match scan). A collision that lands inside - // a concurrent split commit is replaced there instead of failing; - // surface it as the replace it was. - let candidate = Self::new(key.clone(), value.clone()); - match set.put_checked(candidate) { - Ok((replaced, _)) => return replaced.map(Into::into), - Err((node_guard, _, _)) => { - drop(node_guard); - continue; - } - } - } - } - - #[cfg(feature = "cdc")] - fn insert_cdc_into(set: &BTreeSet, key: K, value: V) -> (Option<(K, V)>, Vec>) - where - Self: Debug + Ord + Clone + Send + 'static, - Node: NodeLike + Send + 'static, - { - // See `insert_into`; this is the event-emitting twin. - loop { - let stored = set - .range::((Bound::Included(&key), Bound::Included(&key))) - .find(|pair| pair.value == value); - if let Some(stored) = stored { - let incoming = Self { - key: key.clone(), - value: value.clone(), - discriminator: stored.discriminator, - }; - let (replaced, events) = set.put_cdc_with(incoming, Self::adopt_stored_identity); - return (replaced.map(Into::into), events); - } - - let candidate = Self::new(key.clone(), value.clone()); - match set.put_cdc_checked(candidate) { - Ok((replaced, events)) => return (replaced.map(Into::into), events), - Err((node_guard, _, _)) => { - drop(node_guard); - continue; - } - } - } - } -} - -impl MultiPairRemoveHelper for RandomMultiPair -where - K: Debug + Send + Ord + Clone + 'static, - V: Debug + Send + Clone + PartialEq + 'static, -{ - fn remove_from(set: &BTreeSet, key: &K, value: &V) -> Option<(K, V)> - where - Self: Ord + Clone + 'static, - Node: NodeLike + Send + 'static, - { - let pair_to_remove = set - .range::((Bound::Included(key), Bound::Included(key))) - .find(|pair| pair.key == *key && pair.value == *value); - - if let Some(pair_to_remove) = pair_to_remove { - if let Some(removed) = set.remove(&pair_to_remove) { - return Some(removed.into()); - } - - // A concurrent remove/reinsert can replace the located pair with a - // logically equal pair that has a new discriminator. Revalidate - // the logical pair under the structural write lock in that case. - return set - .remove_where(|pair| pair.key == *key && pair.value == *value) - .map(Into::into); - } - - None - } - - fn remove_cdc_from(set: &BTreeSet, key: &K, value: &V) -> (Option<(K, V)>, Vec>) - where - Self: Ord + Clone + 'static, - Node: NodeLike + Send + 'static, - { - let pair_to_remove = set - .range::((Bound::Included(key), Bound::Included(key))) - .find(|pair| pair.key == *key && pair.value == *value); - - if let Some(pair_to_remove) = pair_to_remove { - let (res, evs) = set.remove_cdc(&pair_to_remove); - if res.is_some() { - return (res.map(Into::into), evs); - } - - // See `remove_from`: the locator can become stale when an equal - // logical pair is concurrently removed and reinserted. - let (res, evs) = set.remove_where_cdc(|pair| pair.key == *key && pair.value == *value); - return (res.map(Into::into), evs); - } - - (None, vec![]) - } -} - -#[cfg(test)] -mod test { - use super::*; - use crate::core::node::NodeLike; - use std::ops::Bound::*; - - #[test] - fn borrow_test() { - let pair = RandomMultiPair::new(1usize, 2usize); - assert_eq!( as Borrow>::borrow(&pair), &1usize); - } - - #[test] - fn eq_test() { - // Identity is (key, discriminator); the value does not participate. - let pair_one = RandomMultiPair { - key: 1usize, - value: 2usize, - discriminator: 7, - }; - let pair_two = RandomMultiPair { - key: 1usize, - value: 3usize, - discriminator: 8, - }; - assert_ne!(pair_one, pair_two); - - let same_slot = RandomMultiPair { - key: 1usize, - value: 3usize, - discriminator: 7, - }; - assert_eq!(pair_one, same_slot); - } - - #[test] - fn equal_pairs_have_equal_hashes() { - use std::hash::{DefaultHasher, Hash, Hasher}; - - // Hash follows the (key, discriminator) identity, so Eq-equal pairs - // hash equal even when their values differ. - let pair_one = RandomMultiPair { - key: 1usize, - value: 2usize, - discriminator: 3, - }; - let pair_two = RandomMultiPair { - key: 1usize, - value: 9usize, - discriminator: 3, - }; - - assert_eq!(pair_one, pair_two); - - let mut hash_one = DefaultHasher::new(); - pair_one.hash(&mut hash_one); - let mut hash_two = DefaultHasher::new(); - pair_two.hash(&mut hash_two); - - assert_eq!(hash_one.finish(), hash_two.finish()); - } - - // Exhaustive Ord-laws check over a small domain dense in collisions on - // every axis. The previous Ord consulted value equality before the - // discriminator, which made it non-transitive (a == b and b == c with - // a < c was reachable), broke binary search, and let index entry keys - // compare Equal. - #[test] - fn ord_is_a_lawful_total_order() { - use core::cmp::Ordering; - use std::hash::{DefaultHasher, Hash, Hasher}; - - let mut pairs = Vec::new(); - for key in 0usize..4 { - for value in 0usize..4 { - for discriminator in 0u64..4 { - pairs.push(RandomMultiPair { - key, - value, - discriminator, - }); - } - } - } - - let hash_of = |pair: &RandomMultiPair| { - let mut hasher = DefaultHasher::new(); - pair.hash(&mut hasher); - hasher.finish() - }; - - for a in &pairs { - assert_eq!(a.cmp(a), Ordering::Equal, "reflexivity: {a:?}"); - for b in &pairs { - let ab = a.cmp(b); - let ba = b.cmp(a); - // Antisymmetry / duality. - assert_eq!(ab, ba.reverse(), "antisymmetry: {a:?} vs {b:?}"); - // Eq consistency with Ord, and Hash consistency with Eq. - assert_eq!(ab == Ordering::Equal, a == b, "Eq/Ord consistency: {a:?} vs {b:?}"); - assert_eq!(a.partial_cmp(b), Some(ab), "PartialOrd/Ord consistency"); - if a == b { - assert_eq!(hash_of(a), hash_of(b), "Hash/Eq consistency: {a:?} vs {b:?}"); - } - for c in &pairs { - let bc = b.cmp(c); - if ab == bc { - assert_eq!(a.cmp(c), ab, "transitivity: {a:?}, {b:?}, {c:?}"); - } - if ab != Ordering::Greater && bc != Ordering::Greater { - assert_ne!(a.cmp(c), Ordering::Greater, "transitivity of <=: {a:?}, {b:?}, {c:?}"); - } - } - } - } - } - - #[test] - fn replace_of_logically_equal_pair_preserves_discriminator_and_position() { - let set = BTreeSet::>::new(); - set.attach_node(vec![ - RandomMultiPair { - key: 1, - value: "a", - discriminator: 10, - }, - RandomMultiPair { - key: 1, - value: "b", - discriminator: 20, - }, - RandomMultiPair { - key: 1, - value: "c", - discriminator: 30, - }, - ]); - - let replaced = RandomMultiPair::insert_into(&set, 1, "b"); - assert_eq!(replaced, Some((1, "b")), "logical duplicate must replace in place"); - - let stored = set - .range::((Bound::Included(&1), Bound::Included(&1))) - .collect::>(); - assert_eq!(stored.len(), 3); - assert_eq!( - stored.iter().map(|pair| pair.discriminator).collect::>(), - vec![10, 20, 30], - "replace must preserve the stored discriminator and position" - ); - assert_eq!(stored[1].value, "b"); - } - - #[test] - fn node_like() { - let mut vec = Vec::new(); - let p1 = RandomMultiPair::new(1, "a"); - let p2 = RandomMultiPair::new(1, "b"); - let p3 = RandomMultiPair::new(2, "c"); - - NodeLike::insert(&mut vec, p1.clone()); - NodeLike::insert(&mut vec, p2.clone()); - assert_eq!(vec.len(), 2); - - NodeLike::insert(&mut vec, p1.clone()); - assert_eq!(vec.len(), 2); - - NodeLike::insert(&mut vec, p3.clone()); - assert_eq!(vec.len(), 3); - } - - #[test] - fn range_bounds() { - let mut vec = Vec::new(); - - let p1a = RandomMultiPair::new(1, "a"); - let p1b = RandomMultiPair::new(1, "b"); - let p1c = RandomMultiPair::new(1, "c"); - let p2a = RandomMultiPair::new(2, "a"); - let p2b = RandomMultiPair::new(2, "b"); - let p3a = RandomMultiPair::new(3, "a"); - let p3b = RandomMultiPair::new(3, "b"); - let p3c = RandomMultiPair::new(3, "c"); - let p3d = RandomMultiPair::new(3, "d"); - let p4a = RandomMultiPair::new(4, "a"); - - NodeLike::insert(&mut vec, p4a.clone()); - NodeLike::insert(&mut vec, p1a.clone()); - NodeLike::insert(&mut vec, p1c.clone()); - NodeLike::insert(&mut vec, p1b.clone()); - NodeLike::insert(&mut vec, p2b.clone()); - NodeLike::insert(&mut vec, p2a.clone()); - NodeLike::insert(&mut vec, p3a.clone()); - NodeLike::insert(&mut vec, p3b.clone()); - NodeLike::insert(&mut vec, p3d.clone()); - NodeLike::insert(&mut vec, p3c.clone()); - assert_eq!(vec.len(), 10); - - let start_1 = vec.rank(Included(&1), true).map_or(0, |rank| rank + 1); - let end_1 = vec.rank(Excluded(&1), true).unwrap(); - let range_1 = &vec[start_1..=end_1]; - assert_eq!(range_1.len(), 3); - assert!(range_1.contains(&p1a)); - assert!(range_1.contains(&p1b)); - assert!(range_1.contains(&p1c)); - - let end_2 = vec.rank(Excluded(&2), true).unwrap(); - let range_2 = &vec[start_1..=end_2]; - assert_eq!(range_2.len(), 5); - assert!(range_2.contains(&p1a)); - assert!(range_2.contains(&p1b)); - assert!(range_2.contains(&p1c)); - assert!(range_2.contains(&p2a)); - assert!(range_2.contains(&p2b)); - assert_ne!(range_2.contains(&p3a), true); - - let start_3 = vec.rank(Included(&3), true).unwrap() + 1; - let end_3 = vec.rank(Excluded(&3), true).unwrap(); - let range_3 = &vec[start_3..=end_3]; - assert_eq!(range_3.len(), 4); - assert!(range_3.contains(&p3a)); - assert!(range_3.contains(&p3b)); - assert!(range_3.contains(&p3c)); - assert!(range_3.contains(&p3d)); - - let start_4 = vec.rank(Included(&4), true).unwrap() + 1; - let end_4 = vec.rank(Excluded(&4), true).unwrap(); - let range_4 = &vec[start_4..=end_4]; - assert_eq!(range_4.len(), 1); - assert!(range_4.contains(&p4a)); - } -} From b530331215a3889ebf87a9c713c74623f54717c0 Mon Sep 17 00:00:00 2001 From: meh Date: Tue, 1 Sep 2026 14:38:22 +0700 Subject: [PATCH 2/3] Delete the scan-based removal path with the representation that needed it remove_where, remove_where_inner and remove_where_cdc walked a node looking for the first element satisfying a predicate. Only RandomMultiPair used them: its Ord did not include the value, so removing a logical (key, value) pair meant finding it by scanning. Ord identity removes by binary search on the pair itself, so nothing reaches them now. That is one more unbounded scan gone from the set alongside the one in insert, and the three tests that exercised it go with it. --- src/concurrent/set.rs | 137 ------------------------------------------ 1 file changed, 137 deletions(-) diff --git a/src/concurrent/set.rs b/src/concurrent/set.rs index 1463537..3118fe0 100644 --- a/src/concurrent/set.rs +++ b/src/concurrent/set.rs @@ -568,74 +568,6 @@ where // critical section after the ordinary point-removal path has missed. Only // the multimap paths use this, and it relies on NodeLike::delete_at (also // multimap-gated), so gate the whole family to avoid an unconditional break. - #[cfg(feature = "multimap")] - fn remove_where_inner(&self, predicate: F) -> (Option, Vec>) - where - F: Fn(&T) -> bool, - { - let mut cdc = vec![]; - let _global_guard = self.index_lock.write(); - - for target_node_entry in self.index.iter() { - let mut node_guard = target_node_entry.value().lock_arc(); - let Some(target_index) = node_guard.iter().position(&predicate) else { - continue; - }; - let old_max = node_guard.max().cloned().expect("target node must have a maximum"); - let deleted = NodeLike::delete_at(&mut *node_guard, target_index) - .expect("target position was found while the node was locked"); - - #[cfg(feature = "cdc")] - if EMIT_CDC { - let node_element_removal = ChangeEvent::RemoveAt { - event_id: self.event_id.fetch_add(1, Ordering::Relaxed).into(), - max_value: old_max.clone(), - value: deleted.clone(), - index: target_index, - }; - cdc.push(node_element_removal); - } - - if node_guard.max() != Some(&old_max) { - let node = target_node_entry.value().clone(); - target_node_entry.remove(); - - if let Some(new_max) = node_guard.max().cloned() { - self.index.insert(new_max, node); - } else { - #[cfg(feature = "cdc")] - if EMIT_CDC { - let node_removal = ChangeEvent::RemoveNode { - event_id: self.event_id.fetch_add(1, Ordering::Relaxed).into(), - max_value: old_max, - }; - cdc.push(node_removal); - } - } - } - - return (Some(deleted), cdc); - } - - (None, cdc) - } - - #[cfg(feature = "multimap")] - pub(crate) fn remove_where(&self, predicate: F) -> Option - where - F: Fn(&T) -> bool, - { - self.remove_where_inner::(predicate).0 - } - - #[cfg(feature = "multimap")] - pub(crate) fn remove_where_cdc(&self, predicate: F) -> (Option, Vec>) - where - F: Fn(&T) -> bool, - { - self.remove_where_inner::(predicate) - } - #[inline(always)] fn lock_node_for_value_optimistic(&self, value: &Q) -> Option> where @@ -2203,75 +2135,6 @@ mod tests { assert!(!set.remove(&5).is_some()); } - #[cfg(feature = "multimap")] - #[cfg(feature = "multimap")] - #[test] - fn test_remove_where_reindexes_changed_node_maximum() { - let set = BTreeSet::::with_maximum_node_size(4); - for value in 0..4 { - set.insert(value); - } - - assert_eq!(set.remove_where(|value| *value == 3), Some(3)); - assert_eq!(set.iter().collect::>(), vec![0, 1, 2]); - - assert!(set.insert(4)); - assert_eq!(set.iter().collect::>(), vec![0, 1, 2, 4]); - } - - #[cfg(feature = "multimap")] - #[test] - fn test_remove_where_deletes_the_position_found_under_the_node_lock() { - let set = BTreeSet::>::new(); - // Entries are ordered by `(key, value)`, so an attached node has to be in that - // order. It used to be built in discriminator order, which no longer exists. - set.attach_node(vec![ - OrdMultiPair { - key: 1, - value: "first", - }, - OrdMultiPair { - key: 1, - value: "second", - }, - OrdMultiPair { - key: 1, - value: "target", - }, - ]); - - let removed = set.remove_where(|pair| pair.value == "target"); - - assert_eq!(removed.map(Into::<(usize, &'static str)>::into), Some((1, "target"))); - assert_eq!( - set.iter().map(|pair| pair.value).collect::>(), - vec!["first", "second"] - ); - } - - #[cfg(all(feature = "cdc", feature = "multimap"))] - #[test] - fn test_remove_where_cdc_removes_empty_node() { - let set = BTreeSet::::new(); - set.insert(7); - - let (removed, events) = set.remove_where_cdc(|value| *value == 7); - - assert_eq!(removed, Some(7)); - assert!(set.is_empty()); - assert!(matches!( - events.as_slice(), - [ - ChangeEvent::RemoveAt { - max_value: 7, - value: 7, - .. - }, - ChangeEvent::RemoveNode { max_value: 7, .. } - ] - )); - } - #[test] fn test_remove_multiple_elements() { let set = BTreeSet::::new(); From 05a74bdc8de2b455cc7b5676aa22afe912f4ea34 Mon Sep 17 00:00:00 2001 From: meh Date: Tue, 1 Sep 2026 14:59:18 +0700 Subject: [PATCH 3/3] Drop the imports the deleted tests were using The tests that exercised the random representation are gone, and with them the only uses of ChangeEvent and OrdMultiPair inside the set test module. --- src/cdc/change.rs | 5 +---- src/concurrent/set.rs | 4 ---- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/src/cdc/change.rs b/src/cdc/change.rs index 8bdc436..49f7ba6 100644 --- a/src/cdc/change.rs +++ b/src/cdc/change.rs @@ -1,8 +1,5 @@ #[cfg(feature = "multimap")] -use { - crate::core::multipair::OrdMultiPair, - crate::core::pair::Pair, -}; +use {crate::core::multipair::OrdMultiPair, crate::core::pair::Pair}; /// Unique event identifier. /// diff --git a/src/concurrent/set.rs b/src/concurrent/set.rs index 3118fe0..4a04513 100644 --- a/src/concurrent/set.rs +++ b/src/concurrent/set.rs @@ -1372,12 +1372,8 @@ where #[cfg(test)] mod tests { - #[cfg(feature = "cdc")] - use crate::cdc::change::ChangeEvent; use crate::concurrent::operation::Operation; use crate::concurrent::set::{BTreeSet, Iter, DEFAULT_INNER_SIZE}; - #[cfg(feature = "multimap")] - use crate::core::multipair::OrdMultiPair; use crate::core::node::NodeLike; use rand::Rng; use std::collections::HashSet;