Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

In-memory trie operations #1408

Merged
merged 41 commits into from
Jul 14, 2016
Merged

In-memory trie operations #1408

merged 41 commits into from
Jul 14, 2016

Conversation

rphmeier
Copy link
Contributor

@rphmeier rphmeier commented Jun 23, 2016

This is an optimization PR introducing an alternative implementation of TrieDBMut.
While the old TrieDBMut recalculates all affected node RLPs and commits to the DB at the end of each insert or remove operation, the new one does only when it is dropped or root or commit is explicitly called.

@rphmeier rphmeier added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Jun 23, 2016
// encode them, and write them to the DB.
//
// TODO: parallelize
fn to_rlp(self, db: &mut HashDB, storage: &mut NodeStorage) -> Bytes {
Copy link
Collaborator

@arkpar arkpar Jun 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returning ElasticArray1024 here would save lots of memory allocations
stream.drain()

@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jun 23, 2016
@rphmeier
Copy link
Contributor Author

So this implementation has a flaw where it never deletes nodes from the DB and may re-insert nodes that already exist in it.

@rphmeier rphmeier added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 24, 2016
@rphmeier
Copy link
Contributor Author

gives ~5% speedup importing the first 250k blocks for me

@coveralls
Copy link

coveralls commented Jul 13, 2016

Coverage Status

Coverage decreased (-0.06%) to 76.424% when pulling e64d6a7 on rphmeier:insertion_trie into 4226c0f on ethcore:master.

@gavofyork
Copy link
Contributor

coverage decreased?

Some((new, changed)) => {
children[idx] = Some(new.into());
let branch = Node::Branch(children, value);
if changed {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

} else { line is a bit wasteful for these trivial ternary-like conditionals. typically use match instead.

@gavofyork
Copy link
Contributor

a few points where style could better follow the "house rules". otherwise looks like quality code. didn't do a deep inspection of the logic, which is likely unnecessary assuming all tests pass and unlikely to uncover any deep-seated bugs.

probably good to get another pair of eyes over it though.

@gavofyork gavofyork added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 14, 2016
@coveralls
Copy link

coveralls commented Jul 14, 2016

Coverage Status

Coverage decreased (-0.02%) to 76.461% when pulling a9de714 on rphmeier:insertion_trie into 4226c0f on ethcore:master.

@rphmeier
Copy link
Contributor Author

rphmeier commented Jul 14, 2016

coverage decreased as a percentage because the code itself is larger. i have also added an insert_empty test because this is where consensus failed previously. this reduced the amount that coverage decreased by.

@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Jul 14, 2016
@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 14, 2016
@coveralls
Copy link

coveralls commented Jul 14, 2016

Coverage Status

Changes Unknown when pulling 9c36b5f on rphmeier:insertion_trie into * on ethcore:master*.

@rphmeier
Copy link
Contributor Author

travis failed on an unrelated network test, but not before all trie json tests and util tests passed.

@arkpar arkpar closed this Jul 14, 2016
@arkpar arkpar reopened this Jul 14, 2016
@arkpar arkpar merged commit 517c705 into openethereum:master Jul 14, 2016
@rphmeier rphmeier deleted the insertion_trie branch July 14, 2016 16:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants