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

core/state/snapshot: less disk reads #6

Closed

Conversation

holiman
Copy link

@holiman holiman commented Mar 30, 2021

This PR attempts to minimize the number of disk reads. When we have a slice of snapshot values, which do not match the trie data, we currently iterates the disk to retrieve the canonical data.
What this PR does, is commit the (incorrect) snapshot data to a trie, which will be 99% correct. When iterating the trie, we then use the snapshot-trie-database for resolving hashes.
When doing so, we can read 99% of the leaves the from the memory db instead of resolving from disk.

This is still work in progress, needs to be tidied up. It can also be implemented differently. The upside of this particular way to implement it is that most of the modifications are performed in the node iterator, and does not touch the trie database.

rjl493456442 and others added 30 commits March 24, 2021 18:44
* core/state/snapshot: refactor

* core/state/snapshot: tiny fix and polish

Co-authored-by: rjl493456442 <garyrong0905@gmail.com>
core/state/snapshot: less copy

core/state/snapshot: revert split loop

core/state/snapshot: handle storage becoming empty, improve test robustness

core/state: test modified codehash

core/state/snapshot: polish
@holiman holiman requested a review from rjl493456442 as a code owner March 30, 2021 06:29
@holiman
Copy link
Author

holiman commented Mar 30, 2021

Generating without this PR:
Screenshot_2021-03-30 Dual Geth - Grafana(1)
Generating with this PR:
Screenshot_2021-03-30 Dual Geth - Grafana

^ Note that the two runs are on the same machine at different times, and the latter image shows a much shorter time segment

Copy link
Owner

@rjl493456442 rjl493456442 left a comment

Choose a reason for hiding this comment

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

Code wise I think it's OK, just some nitpicks

core/state/snapshot/generate.go Outdated Show resolved Hide resolved
core/state/snapshot/generate.go Outdated Show resolved Hide resolved
var (
trieMore bool
iter = trie.NewIterator(tr.NodeIterator(origin))
nodeIt = tr.NodeIterator(origin)
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can pass the auxiliary database here. iter = trie.NewIterator(tr.NodeIteratorWithDatabase(origin))

@@ -428,6 +445,7 @@ func (dl *diskLayer) generateRange(root common.Hash, prefix []byte, kind string,
istart time.Time
internal time.Duration
)
nodeIt.AddResolver(snapTrieDb)
Copy link
Owner

Choose a reason for hiding this comment

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

We can pass the db directly when we construct the iterator

@holiman
Copy link
Author

holiman commented Mar 30, 2021

Code wise I think it's OK, just some nitpicks

Yeah, well, one thing that would be kind of neat, or at least interesting to check, is to

  • not do a second round of trie:ing after the proof has failed, but rather use the db from the proof-phase. Right now I wasn't sue what trie to use where, so I just created a new one for the proof-of-concept,
  • and if that works, replace the stacktrie with a regular empty trie, so we can use it if the proof is unsuccessful.

@rjl493456442
Copy link
Owner

Code wise I think it's OK, just some nitpicks

Yeah, well, one thing that would be kind of neat, or at least interesting to check, is to

  • not do a second round of trie:ing after the proof has failed, but rather use the db from the proof-phase. Right now I wasn't sue what trie to use where, so I just created a new one for the proof-of-concept,
  • and if that works, replace the stacktrie with a regular empty trie, so we can use it if the proof is unsuccessful.

Yes sure, I can do it. We need to modify the range prover a bit, let me see how to fix it.

@holiman
Copy link
Author

holiman commented Apr 14, 2021

Closing this in favour of ethereum#22667

@holiman holiman closed this Apr 14, 2021
rjl493456442 pushed a commit that referenced this pull request May 28, 2021
[R4R] modify params for Parlia consensus with 21 validators
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.

2 participants