Avoid unnecessary allocation in delete-miss #28
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mlodato517
force-pushed
the
avoid-allocation-in-delete-miss
branch
from
0ab9ade to
286c94b
Compare
mlodato517
commented
Mar 16, 2023
| 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) { |
There was a problem hiding this comment.
Best viewed hiding whitespace
| @@ -28,5 +28,7 @@ | |||
| pub mod bangsafe; | |||
| pub mod immutable; | |||
|
|
|||
| pub(crate) mod util; | |||
There was a problem hiding this comment.
Ah the obligatory util module - now it's a real crate
## 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)
mlodato517
force-pushed
the
avoid-allocation-in-delete-miss
branch
from
286c94b to
3447a55
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This Commit
Uses
bangsafe'sDeleteResultconstruct to avoid unnecessaryRc::newing when trying to delete a node that isn't there.Why?
Previously,
Child::Deletewould callRc::newon the result ofn.delete(k)regardless of what happened. This means that, ifn.delete(k)did nothing, we'd allocate a newRcinstead of cloning an existing one. See this comment for more details.Benchmarks
Ignoring
find,insert, andfind-misssince those codepaths weren't touched, we see:which is expected.
deleteis 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, indelete-miss, we now avoid unnecessary allocations of newRcs so there are huge improvements.* I say this is for "nothing" but I don't really mean it. Realistically,
Tree::deleteshould return the value of the deleted node andDeleteResultis a reasonable place to store that because the caller needs both the new tree and theOption<Rc<V>>. Because of this, I'm okay with the hit todeleteand a future PR should add the deleted value to the return value ofdelete.