Replace broken empty_at_index with empty_random

`empty_at_index` was broken in many ways. The primary one was that it
would give entirely incorrect results if called with the same index
twice without calling `refresh` in between.

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "evmap"
-version = "10.0.2"
+version = "11.0.0-alpha.1"
 authors = ["Jon Gjengset <[email protected]>"]
 edition = "2018"
 license = "MIT OR Apache-2.0"
@@ -20,10 +20,15 @@ maintenance = { status = "passively-maintained" }
 [features]
 default = []
 indexed = ["indexmap"]
+eviction = ["indexed", "rand"]
 
 [dependencies]
 indexmap = { version = "1.1.0", optional = true }
 smallvec = "1.0.0"
 hashbag = "0.1.2"
 bytes = { version = "0.5", optional = true }
+rand = { version = "0.7", default-features = false, features = ["alloc"], optional = true }
 slab = "0.4"
+
+[dev-dependencies]
+rand = "0.7"

src/lib.rs

@@ -294,9 +294,11 @@ pub enum Operation<K, V> {
  Remove(K, V),
  /// Remove the value set for this key.
  Empty(K),
- #[cfg(feature = "indexed")]
- /// Drop a key at a random index
- EmptyRandom(usize),
+ #[cfg(feature = "eviction")]
+ /// Drop keys at the given indices.
+ ///
+ /// The list of indices must be sorted in ascending order.
+ EmptyAt(Vec<usize>),
  /// Remove all values in the value set for this key.
  Clear(K),
  /// Remove all values for all keys.

src/write.rs

@@ -506,25 +506,42 @@ where
  self.add_op(Operation::Reserve(k, additional))
  }
 
- #[cfg(feature = "indexed")]
- /// Remove the value-bag for a key at a specified index.
+ #[cfg(feature = "eviction")]
+ /// Remove the value-bag for `n` randomly chosen keys.
  ///
- /// This is effectively random removal.
- /// The value-bag will only disappear from readers after the next call to `refresh()`.
- ///
- /// Note that this does a _swap-remove_, so use it carefully.
- pub fn empty_at_index(&mut self, index: usize) -> Option<(&K, &Values<V, S>)> {
- self.add_op(Operation::EmptyRandom(index));
- // the actual emptying won't happen until refresh(), which needs &mut self
- // so it's okay for us to return the references here
-
- // NOTE to future zealots intent on removing the unsafe code: it's **NOT** okay to use
- // self.w_handle here, since that does not have all pending operations applied to it yet.
- // Specifically, there may be an *eviction* pending that already has been applied to the
- // r_handle side, but not to the w_handle (will be applied on the next swap). We must make
- // sure that we read the most up to date map here.
+ /// This method immediately calls `refresh()` to ensure that the keys and values it returns
+ /// match the elements that will be emptied on the next call to `refresh()`. The value-bags
+ /// will only disappear from readers after the next call to `refresh()`.
+ pub fn empty_random<'a>(
+ &'a mut self,
+ rng: &mut impl rand::Rng,
+ n: usize,
+ ) -> impl ExactSizeIterator<Item = (&'a K, &'a Values<V, S>)> {
+ // force a refresh so that our view into self.r_handle matches the indices we choose.
+ // if we didn't do this, the `i`th element of r_handle may be a completely different
+ // element than the one that _will_ be evicted when `EmptyAt([.. i ..])` is applied.
+ // this would be bad since we are telling the caller which elements we are evicting!
+ // note also that we _must_ use `r_handle`, not `w_handle`, since `w_handle` may have
+ // pending operations even after a refresh!
+ self.refresh();
+
  let inner = self.r_handle.inner.load(atomic::Ordering::SeqCst);
- unsafe { (*inner).data.get_index(index) }.map(|(k, vs)| (k, vs.user_friendly()))
+ let inner: &'a _ = unsafe { &(*inner).data };
+
+ // let's pick some (distinct) indices to evict!
+ let n = n.min(inner.len());
+ let indices = rand::seq::index::sample(rng, inner.len(), n);
+
+ // we need to sort the indices so that, later, we can make sure to swap remove from last to
+ // first (and so not accidentally remove the wrong index).
+ let mut to_remove = indices.clone().into_vec();
+ to_remove.sort();
+ self.add_op(Operation::EmptyAt(to_remove));
+
+ indices.into_iter().map(move |i| {
+ let (k, vs) = inner.get_index(i).expect("in-range");
+ (k, vs.user_friendly())
+ })
  }
 
  /// Apply ops in such a way that no values are dropped, only forgotten
@@ -569,9 +586,11 @@ where
  Operation::Purge => {
  inner.data.clear();
  }
- #[cfg(feature = "indexed")]
- Operation::EmptyRandom(index) => {
- inner.data.swap_remove_index(index);
+ #[cfg(feature = "eviction")]
+ Operation::EmptyAt(ref indices) => {
+ for &index in indices.iter().rev() {
+ inner.data.swap_remove_index(index);
+ }
  }
  Operation::Remove(ref key, ref value) => {
  if let Some(e) = inner.data.get_mut(key) {
@@ -645,9 +664,11 @@ where
  Operation::Purge => {
  inner.data.clear();
  }
- #[cfg(feature = "indexed")]
- Operation::EmptyRandom(index) => {
- inner.data.swap_remove_index(index);
+ #[cfg(feature = "eviction")]
+ Operation::EmptyAt(indices) => {
+ for &index in indices.iter().rev() {
+ inner.data.swap_remove_index(index);
+ }
  }
  Operation::Remove(key, value) => {
  if let Some(e) = inner.data.get_mut(&key) {

tests/lib.rs

@@ -445,21 +445,51 @@ fn empty() {
 }
 
 #[test]
-#[cfg(feature = "indexed")]
+#[cfg(feature = "eviction")]
 fn empty_random() {
  let (r, mut w) = evmap::new();
  w.insert(1, "a");
  w.insert(1, "b");
  w.insert(2, "c");
- w.empty_at_index(0);
+
+ let mut rng = rand::thread_rng();
+ let removed: Vec<_> = w.empty_random(&mut rng, 1).map(|(&k, _)| k).collect();
  w.refresh();
 
  // should only have one value set left
- assert_eq!(r.len(), 1);
+ assert_eq!(removed.len(), 1);
+ let kept = 3 - removed[0];
  // one of them must have gone away
- assert!(
- (!r.contains_key(&1) && r.contains_key(&2)) || (r.contains_key(&1) && !r.contains_key(&2))
- );
+ assert_eq!(r.len(), 1);
+ assert!(!r.contains_key(&removed[0]));
+ assert!(r.contains_key(&kept));
+
+ // remove the other one
+ let removed: Vec<_> = w.empty_random(&mut rng, 100).map(|(&k, _)| k).collect();
+ w.refresh();
+
+ assert_eq!(removed.len(), 1);
+ assert_eq!(removed[0], kept);
+ assert!(r.is_empty());
+}
+
+#[test]
+#[cfg(feature = "eviction")]
+fn empty_random_multiple() {
+ let (r, mut w) = evmap::new();
+ w.insert(1, "a");
+ w.insert(1, "b");
+ w.insert(2, "c");
+
+ let mut rng = rand::thread_rng();
+ let removed1: Vec<_> = w.empty_random(&mut rng, 1).map(|(&k, _)| k).collect();
+ let removed2: Vec<_> = w.empty_random(&mut rng, 1).map(|(&k, _)| k).collect();
+ w.refresh();
+
+ assert_eq!(removed1.len(), 1);
+ assert_eq!(removed2.len(), 1);
+ assert_ne!(removed1[0], removed2[0]);
+ assert!(r.is_empty());
 }
 
 #[test]