Skip to content
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

Merged
merged 7 commits into from
Nov 18, 2019
Merged

Conversation

jimpo
Copy link
Contributor

@jimpo jimpo commented Nov 9, 2019

The goal of this change is to remove some of the duplication and inefficiency of OwnedNode. OwnedNode is a parallel data structure to Node that is constructed by copying a lot of data into new allocations. Notably, an OwnedNode cannot easily be converted to a Node because of alignment issues between NibbleVec, which OwnedNode uses to represent partial keys, and NibbleSlice, which Node 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 a Node directly, which has a lifetime, one can construct a NodePlan which is an owned structure and can be used to quickly create Nodes thereafter. The OwnedNode can be cheaply converted into a Node by using the NodePlan 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.

Copy link
Contributor

@cheme cheme left a 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.

@jimpo
Copy link
Contributor Author

jimpo commented Nov 13, 2019

@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.

// unnecessarily.
IterStep::YieldNode
} else {
let node = b.node.node();
Copy link
Contributor

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.

Copy link
Contributor Author

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!

@jimpo jimpo force-pushed the jimpo/decode-plan branch from 1310c84 to 95075cc Compare November 14, 2019 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants