Skip to content

Implement OwnedNode using a node decode plan.#34

Merged
arkpar merged 7 commits into
masterfrom
jimpo/decode-plan
Nov 18, 2019
Merged

Implement OwnedNode using a node decode plan.#34
arkpar merged 7 commits into
masterfrom
jimpo/decode-plan

Conversation

@jimpo
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread test-support/reference-trie/src/lib.rs
Comment thread trie-db/src/iterator.rs
Comment thread trie-db/src/iterator.rs Outdated
Comment thread trie-db/src/triedb.rs
Comment thread trie-db/src/node.rs
Comment thread trie-db/src/node.rs Outdated
@jimpo
Copy link
Copy Markdown
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.

Comment thread trie-db/benches/bench.rs Outdated
Comment thread trie-db/src/iterator.rs Outdated
// unnecessarily.
IterStep::YieldNode
} else {
let node = b.node.node();
Copy link
Copy Markdown
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
Copy Markdown
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