-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Rc::try_unwrap
for (possibly?) better performance
#85
Conversation
And in `Node::remove_value`
Codecov Report
@@ Coverage Diff @@
## main #85 +/- ##
==========================================
- Coverage 66.05% 65.75% -0.30%
==========================================
Files 26 26
Lines 1903 1907 +4
Branches 486 489 +3
==========================================
- Hits 1257 1254 -3
- Misses 248 253 +5
- Partials 398 400 +2
|
@appcypher and I figured out that adding It's for sure safe to annotate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting this up here for discussion.
We can make use of mutability in
Node::set_value
andNode::remove_value
, by turning values of typeRc<Node<...>>
intomut Node<...>
usingRc::try_unwrap(...).unwrap_or_else(|rc| (*rc).clone())
.Essentially that'll just give us an owned
Node
if we hold the only reference to it, or it'll clone the innerRc
.Theory is that if the calling code is basically doing lots of changes on the HAMT in a tight loop, that's going to get sped up, since that'll end up being lots of in-place mutation.
The benchmarks don't look too promising though. I got ~10% improvements.
To test this, just compare the two commits in the PR against each other. Specifically their
cargo bench -p wnfs-bench node
.Also, I had to hand-expand the
async_recursion
macro forset_value
andremove_value
, because otherwise there would've been lifetime issues with the original versions. We should probably figure out another way, but only if we really want to keep this PR.