-
Notifications
You must be signed in to change notification settings - Fork 69
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
Implement OwnedNode using a node decode plan. #34
Conversation
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.
I like this node plan change, still it would make more sense if it would also use some OwnedNode<&'a[u8]> at some place in the code.
It seems the change in perf is a bit surprising, but my bench is done without much thinking, still got a 10 to 20 % reg on it (iteration on a inmemory trie).
https://gist.github.com/cheme/b5943e53ecce6aa54ded8eb0d128eef5
That is a bit problematic, but this is very naive bench, so having bench in the pr could help asserting the change impact at this point.
@cheme Thanks for pointing out the regression. I added a benchmark and some performance optimizations. I am now seeing at 18.8% perf improvement in this PR over master for trie iteration. |
trie-db/src/iterator.rs
Outdated
// unnecessarily. | ||
IterStep::YieldNode | ||
} else { | ||
let node = b.node.node(); |
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.
naïve question, can't we just match on NodePlan instead, and only access the fields if they are needed.
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.
Nice, didn't think of that! With this, it's a 43% speedup over master!
1310c84
to
95075cc
Compare
The goal of this change is to remove some of the duplication and inefficiency of
OwnedNode
.OwnedNode
is a parallel data structure toNode
that is constructed by copying a lot of data into new allocations. Notably, anOwnedNode
cannot easily be converted to aNode
because of alignment issues betweenNibbleVec
, whichOwnedNode
uses to represent partial keys, andNibbleSlice
, whichNode
uses to represent partial keys.The idea head is for
OwnedNode
to just store an encoded trie node along with a blueprint for quickly decoding into a node. When an encoded node is first decoded, instead of creating aNode
directly, which has a lifetime, one can construct aNodePlan
which is an owned structure and can be used to quickly create Nodes thereafter. TheOwnedNode
can be cheaply converted into aNode
by using theNodePlan
on its owned data.This is a breaking change as it modifies the
NodeCodec
trait and adds a new required method. This probably needs a minor version bump at a minimum. The actual code changes required to upgrade are fairly minimal though.Using the newly added benchmark, trie traversal on an in-memory trie is 42% faster.