Return ref to replaced value in insert

src/iter/iter.rs

@@ -70,8 +70,9 @@ mod tests {
     fn iter() {
         let map = FlurryHashMap::<usize, usize>::new();
 
-        map.insert(1, 42);
-        map.insert(2, 84);
+        let guard = epoch::pin();
+        map.insert(1, 42, &guard);
+        map.insert(2, 84, &guard);
 
         let guard = epoch::pin();
         assert_eq!(
@@ -84,8 +85,9 @@ mod tests {
     fn keys() {
         let map = FlurryHashMap::<usize, usize>::new();
 
-        map.insert(1, 42);
-        map.insert(2, 84);
+        let guard = epoch::pin();
+        map.insert(1, 42, &guard);
+        map.insert(2, 84, &guard);
 
         let guard = epoch::pin();
         assert_eq!(
@@ -98,10 +100,11 @@ mod tests {
     fn values() {
         let map = FlurryHashMap::<usize, usize>::new();
 
-        map.insert(1, 42);
-        map.insert(2, 84);
+        let mut guard = epoch::pin();
+        map.insert(1, 42, &guard);
+        map.insert(2, 84, &guard);
+        guard.repin();
 
-        let guard = epoch::pin();
         assert_eq!(
             map.values(&guard).collect::<HashSet<&usize>>(),
             HashSet::from_iter(vec![&42, &84])

src/lib.rs

@@ -6,6 +6,25 @@
 //! on shared references, retrieval operations do *not* entail locking, and there is *not* any
 //! support for locking the entire table in a way that prevents all access.
 //!
+//! # A note on `Guard` and memory use
+//!
+//! You may have noticed that many of the access methods on this map take a reference to an
+//! [`epoch::Guard`]. The exact details of this are beyond the scope of this documentation (for
+//! that, see [`crossbeam::epoch`]), but some of the implications bear repeating here. You obtain a
+//! `Guard` using [`epoch::pin`], and you can use references to the same guard to make multiple API
+//! calls if you wish. Whenever you get a reference to something stored in the map, that reference
+//! is tied to the lifetime of the `Guard` that you provided. This is because each `Guard` prevents
+//! the destruction of any item associated with it. Whenever something is read under a `Guard`,
+//! that something stays around for _at least_ as long as the `Guard` does. The map delays
+//! deallocating values until it safe to do so, and in order to amortize the cost of the necessary
+//! bookkeeping it may delay even further until there's a _batch_ of items that need to be
+//! deallocated.
+//!
+//! Notice that there is a trade-off here. Creating and dropping a `Guard` is not free, since it
+//! also needs to interact with said bookkeeping. But if you keep one around for a long time, you
+//! may accumulate much garbage which will take up valuable free memory on your system. Use your
+//! best judgement in deciding whether or not to re-use a `Guard`.
+//!
 //! # Consistency
 //!
 //! Retrieval operations (including [`get`](FlurryHashMap::get)) generally do not block, so may
@@ -428,14 +447,13 @@ where
     /// Maps `key` to `value` in this table.
     ///
     /// The value can be retrieved by calling [`get`] with a key that is equal to the original key.
-    pub fn insert(&self, key: K, value: V) -> Option<()> {
-        self.put(key, value, false)
+    pub fn insert<'g>(&self, key: K, value: V, guard: &'g Guard) -> Option<&'g V> {
+        self.put(key, value, false, guard)
     }
 
-    fn put(&self, key: K, value: V, no_replacement: bool) -> Option<()> {
+    fn put<'g>(&self, key: K, value: V, no_replacement: bool, guard: &'g Guard) -> Option<&'g V> {
         let h = self.hash(&key);
 
-        let guard = &crossbeam::epoch::pin();
         let mut table = self.table.load(Ordering::SeqCst, guard);
 
         let mut node = Owned::new(BinEntry::Node(Node {
@@ -516,7 +534,11 @@ where
                     if no_replacement && head.hash == h && &head.key == key =>
                 {
                     // fast path if replacement is disallowed and first bin matches
-                    return Some(());
+                    let v = head.value.load(Ordering::SeqCst, guard);
+                    // safety: since the value is present now, and we've held a guard from the
+                    // beginning of the search, the value cannot be dropped until the next epoch,
+                    // which won't arrive until after we drop our guard.
+                    return Some(unsafe { v.deref() });
                 }
                 BinEntry::Node(ref head) => {
                     // bin is non-empty, need to link into it, so we must take the lock
@@ -545,6 +567,13 @@ where
                         let n = unsafe { p.deref() }.as_node().unwrap();
                         if n.hash == h && &n.key == key {
                             // the key already exists in the map!
+                            let current_value = head.value.load(Ordering::SeqCst, guard);
+
+                            // safety: since the value is present now, and we've held a guard from
+                            // the beginning of the search, the value cannot be dropped until the
+                            // next epoch, which won't arrive until after we drop our guard.
+                            let current_value = unsafe { current_value.deref() };
+
                             if no_replacement {
                                 // the key is not absent, so don't update
                             } else if let BinEntry::Node(Node { value, .. }) = *node.into_box() {
@@ -554,6 +583,8 @@ where
                                     Ordering::SeqCst,
                                     guard,
                                 );
+                                // NOTE: now_garbage == current_value
+
                                 // safety: need to guarantee that now_garbage is no longer
                                 // reachable. more specifically, no thread that executes _after_
                                 // this line can ever get a reference to now_garbage.
@@ -577,7 +608,7 @@ where
                             } else {
                                 unreachable!();
                             }
-                            break Some(());
+                            break Some(current_value);
                         }
 
                         // TODO: This Ordering can probably be relaxed due to the Mutex

tests/basic.rs

@@ -10,7 +10,8 @@ fn new() {
 #[test]
 fn insert() {
     let map = FlurryHashMap::<usize, usize>::new();
-    let old = map.insert(42, 0);
+    let guard = epoch::pin();
+    let old = map.insert(42, 0, &guard);
     assert!(old.is_none());
 }
 
@@ -29,7 +30,7 @@ fn get_empty() {
 fn insert_and_get() {
     let map = FlurryHashMap::<usize, usize>::new();
 
-    map.insert(42, 0);
+    map.insert(42, 0, &epoch::pin());
     {
         let guard = epoch::pin();
         let e = map.get(&42, &guard).unwrap();
@@ -41,9 +42,10 @@ fn insert_and_get() {
 fn update() {
     let map = FlurryHashMap::<usize, usize>::new();
 
-    map.insert(42, 0);
-    let old = map.insert(42, 1);
-    assert!(old.is_some());
+    let guard = epoch::pin();
+    map.insert(42, 0, &guard);
+    let old = map.insert(42, 1, &guard);
+    assert_eq!(old, Some(&0));
     {
         let guard = epoch::pin();
         let e = map.get(&42, &guard).unwrap();
@@ -58,13 +60,13 @@ fn concurrent_insert() {
     let map1 = map.clone();
     let t1 = std::thread::spawn(move || {
         for i in 0..64 {
-            map1.insert(i, 0);
+            map1.insert(i, 0, &epoch::pin());
         }
     });
     let map2 = map.clone();
     let t2 = std::thread::spawn(move || {
         for i in 0..64 {
-            map2.insert(i, 1);
+            map2.insert(i, 1, &epoch::pin());
         }
     });
 
@@ -85,7 +87,7 @@ fn current_kv_dropped() {
 
     let map = FlurryHashMap::<Arc<usize>, Arc<usize>>::new();
 
-    map.insert(dropped1.clone(), dropped2.clone());
+    map.insert(dropped1.clone(), dropped2.clone(), &epoch::pin());
     assert_eq!(Arc::strong_count(&dropped1), 2);
     assert_eq!(Arc::strong_count(&dropped2), 2);
 
@@ -105,11 +107,11 @@ fn drop_value() {
 
     let map = FlurryHashMap::<usize, Arc<usize>>::new();
 
-    map.insert(42, dropped1.clone());
+    map.insert(42, dropped1.clone(), &epoch::pin());
     assert_eq!(Arc::strong_count(&dropped1), 2);
     assert_eq!(Arc::strong_count(&dropped2), 1);
 
-    map.insert(42, dropped2.clone());
+    map.insert(42, dropped2.clone(), &epoch::pin());
     assert_eq!(Arc::strong_count(&dropped2), 2);
 
     drop(map);

tests/jdk/map_check.rs

@@ -29,8 +29,9 @@ where
     K: Sync + Send + Copy + Hash + Eq,
 {
     let mut sum = 0;
+    let guard = epoch::pin();
     for i in 0..keys.len() {
-        if map.insert(keys[i], 0).is_none() {
+        if map.insert(keys[i], 0, &guard).is_none() {
             sum += 1;
         }
     }