Remove RandomMultiPair and make (key, value) the only entry identity

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"

src/cdc/change.rs

@@ -1,8 +1,5 @@
 #[cfg(feature = "multimap")]
-use {
-    crate::core::multipair::{OrdMultiPair, RandomMultiPair},
-    crate::core::pair::Pair,
-};
+use {crate::core::multipair::OrdMultiPair, crate::core::pair::Pair};
 
 /// Unique event identifier.
 ///
@@ -190,13 +187,6 @@ where
     }
 }
 
-#[cfg(feature = "multimap")]
-impl<K, V> From<ChangeEvent<RandomMultiPair<K, V>>> for ChangeEvent<Pair<K, V>> {
-    fn from(ev: ChangeEvent<RandomMultiPair<K, V>>) -> Self {
-        multipair_change_event_into_pair(ev)
-    }
-}
-
 #[cfg(feature = "multimap")]
 impl<K, V> From<ChangeEvent<OrdMultiPair<K, V>>> for ChangeEvent<Pair<K, V>> {
     fn from(ev: ChangeEvent<OrdMultiPair<K, V>>) -> Self {

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::<RandomMultiPair<usize, &'static str>>();
         assert_range_works_as_expected::<OrdMultiPair<usize, &'static str>>();
     }
 
@@ -819,7 +818,6 @@ mod tests {
 
     #[test]
     fn test_range_excludes_all_values_at_bounds() {
-        assert_range_excludes_values_at_bounds::<RandomMultiPair<usize, &'static str>>();
         assert_range_excludes_values_at_bounds::<OrdMultiPair<usize, &'static str>>();
     }
 
@@ -860,7 +858,6 @@ mod tests {
 
     #[test]
     fn test_get_works_as_expected() {
-        assert_get_works_as_expected::<RandomMultiPair<usize, &'static str>>();
         assert_get_works_as_expected::<OrdMultiPair<usize, &'static str>>();
     }
 

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<const EMIT_CDC: bool, F>(&self, predicate: F) -> (Option<T>, Vec<ChangeEvent<T>>)
-    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<F>(&self, predicate: F) -> Option<T>
-    where
-        F: Fn(&T) -> bool,
-    {
-        self.remove_where_inner::<false, F>(predicate).0
-    }
-
-    #[cfg(feature = "multimap")]
-    pub(crate) fn remove_where_cdc<F>(&self, predicate: F) -> (Option<T>, Vec<ChangeEvent<T>>)
-    where
-        F: Fn(&T) -> bool,
-    {
-        self.remove_where_inner::<true, F>(predicate)
-    }
-
     #[inline(always)]
     fn lock_node_for_value_optimistic<Q>(&self, value: &Q) -> Option<ArcMutexGuard<RawMutex, Node>>
     where
@@ -1440,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::RandomMultiPair;
     use crate::core::node::NodeLike;
     use rand::Rng;
     use std::collections::HashSet;
@@ -2203,137 +2131,6 @@ mod tests {
         assert!(!set.remove(&5).is_some());
     }
 
-    #[cfg(feature = "multimap")]
-    #[test]
-    fn replace_of_logically_equal_pair_preserves_discriminator_and_position() {
-        use crate::core::multipair::MultiPairInsertHelper;
-
-        let set = BTreeSet::<RandomMultiPair<usize, &'static str>>::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() {
-        let set = BTreeSet::<usize>::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<_>>(), vec![0, 1, 2]);
-
-        assert!(set.insert(4));
-        assert_eq!(set.iter().collect::<Vec<_>>(), vec![0, 1, 2, 4]);
-    }
-
-    #[cfg(feature = "multimap")]
-    #[test]
-    fn test_remove_where_deletes_the_position_found_under_the_node_lock() {
-        let set = BTreeSet::<RandomMultiPair<usize, &'static str>>::new();
-        set.attach_node(vec![
-            RandomMultiPair {
-                key: 1,
-                value: "target",
-                discriminator: 5,
-            },
-            RandomMultiPair {
-                key: 1,
-                value: "middle",
-                discriminator: 20,
-            },
-            RandomMultiPair {
-                key: 1,
-                value: "last",
-                discriminator: 30,
-            },
-        ]);
-
-        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<_>>(),
-            vec!["middle", "last"]
-        );
-    }
-
-    #[cfg(all(feature = "cdc", feature = "multimap"))]
-    #[test]
-    fn test_remove_where_cdc_removes_empty_node() {
-        let set = BTreeSet::<usize>::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::<i32>::new();

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<K, V> = RandomMultiPair<K, V>;
+/// 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<K, V> = OrdMultiPair<K, V>;
 
 /// Common contract for key-value pairs stored by `BTreeMultiMap`.
 ///