Avoid unnecessary allocation in delete-miss

## This Commit

Uses `bangsafe`'s `DeleteResult` construct to avoid unnecessary
`Rc::new`ing when trying to delete a node that isn't there.

## Why?

Previously, `Child::Delete` would call `Rc::new` on the result of
`n.delete(k)` regardless of what happened. This means that, if
`n.delete(k)` did nothing, we'd allocate a new `Rc` instead of cloning
an existing one. See [this comment][0] for more details.

## Benchmarks

Ignoring `find`, `insert`, and `find-miss` since those codepaths weren't
touched, we see:

```
delete/immutable/6          time:   [58.097 ns 58.167 ns 58.250 ns]
                            change: [+12.429% +12.732% +13.025%]
                            Performance has regressed.
delete/immutable/126        time:   [161.59 ns 161.74 ns 161.91 ns]
                            change: [+11.545% +11.783% +12.023%]
                            Performance has regressed.
delete/immutable/2046       time:   [306.21 ns 306.81 ns 307.88 ns]
                            change: [+1.7977% +2.2199% +2.5886%]
                            Performance has regressed.
delete/immutable/32766      time:   [494.58 ns 495.30 ns 496.57 ns]
                            change: [+6.8019% +7.0829% +7.3743%]
                            Performance has regressed.
delete-miss/immutable/6     time:   [34.370 ns 34.401 ns 34.438 ns]
                            change: [-51.402% -51.225% -51.080%]
                            Performance has improved.
delete-miss/immutable/126   time:   [45.142 ns 45.171 ns 45.200 ns]
                            change: [-73.200% -73.095% -73.020%]
                            Performance has improved.
delete-miss/immutable/2046  time:   [57.241 ns 57.282 ns 57.327 ns]
                            change: [-82.457% -82.430% -82.402%]
                            Performance has improved.
delete-miss/immutable/32766 time:   [67.454 ns 67.489 ns 67.527 ns]
                            change: [-86.728% -86.697% -86.672%]
                            Performance has improved.
```

which is expected. `delete` is now doing more work because it's tracking
whether or not the value was found. So when the value _is_ found, we're
doing extra work for "nothing"*. However, in `delete-miss`, we now avoid
unnecessary allocations of new `Rc`s so there are huge improvements.

\* I say this is for "nothing" but I don't really mean it. Realistically,
`Tree::delete` should return the value of the deleted node and
`DeleteResult` is a reasonable place to store that because the caller
needs both the new tree and the `Option<Rc<V>>`. Because of this, I'm
okay with the hit to `delete` and a future PR should add the deleted
value to the return value of `delete`.

[0]: #17 (comment)

src/bangsafe.rs

@@ -30,6 +30,8 @@ use std::fmt;
 use std::mem::ManuallyDrop;
 use std::ptr::NonNull;
 
+use crate::util::DeleteResult;
+
 /// Marker value for a `Node`'s height when its value has been removed during deletion. This is
 /// used to ensure that the value isn't re-dropped in `Node::drop`.
 const VALUE_REMOVED: usize = 0;
@@ -202,17 +204,6 @@ impl<K, V> Tree<K, V> {
     }
 }
 
-enum DeleteResult<V> {
-    /// The key wasn't found so nothing was deleted.
-    NotFound,
-    /// The Node returning this is being deleted and has no children to replace itself with. The
-    /// parent who receives this should drop the child and remove its value using
-    /// [`Node::take_value`].
-    DeleteSelf,
-    /// A child node was deleted yielding the value `V` which can be returned to the parent.
-    DeletedChild(V),
-}
-
 struct Node<K, V> {
     key: K,
     value: ManuallyDrop<V>,

src/immutable.rs

@@ -36,6 +36,8 @@
 use std::cmp;
 use std::rc::Rc;
 
+use crate::util::DeleteResult;
+
 /// A Binary Search Tree. This can be used for inserting, finding,
 /// and deleting keys and values. Note that this data structure is
 /// immutable - operations that would modify the tree instead
@@ -142,9 +144,10 @@ impl<K, V> Tree<K, V> {
     where
         K: cmp::Ord,
     {
-        match &self.root {
-            None => Self::new(),
-            Some(n) => Self { root: n.delete(k) },
+        match self.root.as_ref().map(|root| root.delete(k)) {
+            Some(DeleteResult::DeleteSelf) => Self { root: None },
+            Some(DeleteResult::DeletedChild(child)) => Self { root: Some(child) },
+            None | Some(DeleteResult::NotFound) => self.clone(),
         }
     }
 
@@ -263,15 +266,17 @@ impl<K, V> Child<K, V> {
         }
     }
 
-    fn delete(&self, k: &K) -> Self
+    fn delete(&self, k: &K) -> DeleteResult<Self>
     where
         K: cmp::Ord,
     {
-        let new_node = match &self.0 {
-            Some(n) => n.delete(k).map(Rc::new),
-            None => None,
-        };
-        Self(new_node)
+        match self.0.as_ref().map(|n| n.delete(k)) {
+            Some(DeleteResult::DeleteSelf) => DeleteResult::DeletedChild(Self(None)),
+            Some(DeleteResult::DeletedChild(child)) => {
+                DeleteResult::DeletedChild(Self(Some(Rc::new(child))))
+            }
+            None | Some(DeleteResult::NotFound) => DeleteResult::NotFound,
+        }
     }
 }
 
@@ -366,43 +371,55 @@ impl<K, V> Node<K, V> {
         }
     }
 
-    fn delete(&self, k: &K) -> Option<Self>
+    fn delete(&self, k: &K) -> DeleteResult<Self>
     where
         K: cmp::Ord,
     {
         match k.cmp(&self.key) {
-            cmp::Ordering::Less => {
-                let new_left = self.left.delete(k);
-                Some(self.clone_with_children(new_left, self.right.clone()))
-            }
-            cmp::Ordering::Equal => match (&self.left.0, &self.right.0) {
-                (None, None) => None,
-                (None, Some(right)) => Some(Node::clone(right)),
-                (Some(left), None) => Some(Node::clone(left)),
-
-                // If we have two children we have to figure out
-                // which node to promote. We choose here this node's
-                // predecessor. That is, the largest node in this node's
-                // left subtree.
-                (Some(left_child), _) => {
-                    let (pred_key, pred_val, new_left) = left_child.delete_largest();
-                    let height = new_left.height().max(self.right.height()) + 1;
-                    Some(
-                        Node {
-                            height,
-                            key: pred_key,
-                            left: new_left,
-                            right: self.right.clone(),
-                            value: pred_val,
-                        }
-                        .balance(),
-                    )
+            cmp::Ordering::Less => match self.left.delete(k) {
+                DeleteResult::DeleteSelf => {
+                    unreachable!("`Child::delete` doesn't return `DeleteSelf`")
                 }
+                DeleteResult::DeletedChild(new_left) => DeleteResult::DeletedChild(
+                    self.clone_with_children(new_left, self.right.clone()),
+                ),
+                DeleteResult::NotFound => DeleteResult::NotFound,
             },
-            cmp::Ordering::Greater => {
-                let new_right = self.right.delete(k);
-                Some(self.clone_with_children(self.left.clone(), new_right))
+            cmp::Ordering::Equal => {
+                match (&self.left.0, &self.right.0) {
+                    (None, None) => DeleteResult::DeleteSelf,
+                    (None, Some(right)) => DeleteResult::DeletedChild(Node::clone(right)),
+                    (Some(left), None) => DeleteResult::DeletedChild(Node::clone(left)),
+
+                    // If we have two children we have to figure out
+                    // which node to promote. We choose here this node's
+                    // predecessor. That is, the largest node in this node's
+                    // left subtree.
+                    (Some(left_child), _) => {
+                        let (pred_key, pred_val, new_left) = left_child.delete_largest();
+                        let height = new_left.height().max(self.right.height()) + 1;
+                        DeleteResult::DeletedChild(
+                            Node {
+                                height,
+                                key: pred_key,
+                                left: new_left,
+                                right: self.right.clone(),
+                                value: pred_val,
+                            }
+                            .balance(),
+                        )
+                    }
+                }
             }
+            cmp::Ordering::Greater => match self.right.delete(k) {
+                DeleteResult::DeleteSelf => {
+                    unreachable!("`Child::delete` doesn't return `DeleteSelf`")
+                }
+                DeleteResult::DeletedChild(new_right) => DeleteResult::DeletedChild(
+                    self.clone_with_children(self.left.clone(), new_right),
+                ),
+                DeleteResult::NotFound => DeleteResult::NotFound,
+            },
         }
     }
 
@@ -648,6 +665,21 @@ mod tests {
         assert_eq!(tree.find(&3), Some(&4));
     }
 
+    #[test]
+    fn delete_miss_no_alloc() {
+        let mut tree = Tree::new();
+        tree = tree.insert(2, 3);
+        tree = tree.insert(1, 2);
+        tree = tree.insert(3, 4);
+        let tree = tree.delete(&100);
+
+        // We deleted 100 which is greater than the root, 2, so we should've gone right.
+        // Make sure we didn't allocate a new right child and only incremented the count.
+        let root = tree.root.as_ref().unwrap();
+        let right = root.right.0.as_ref().unwrap();
+        assert_eq!(Rc::strong_count(right), 2);
+    }
+
     /// Assert the heights of the root, left child, and right child of a tree.
     macro_rules! assert_heights {
         ($tree:ident, $height:expr, $left_height:expr, $right_height:expr) => {{

src/lib.rs

@@ -28,5 +28,7 @@
 pub mod bangsafe;
 pub mod immutable;
 
+pub(crate) mod util;
+
 #[cfg(test)]
 mod test;

src/util.rs

@@ -0,0 +1,9 @@
+pub(crate) enum DeleteResult<V> {
+    /// The key wasn't found so nothing was deleted.
+    NotFound,
+    /// The Node returning this is being deleted and has no children. Its parent can take its `V`
+    /// before dropping it.
+    DeleteSelf,
+    /// A child node was deleted yielding the value `V`.
+    DeletedChild(V),
+}