core: improve trie updates (part 2) #21047
Conversation
|
I'm going to restart the benchmark from block 5M now, due to the problems with
So far, this PR only exports the account trie, not yet storage tries. |
|
On the new benchmark run, both finished generating the snapshot after about |
|
Block around So after ~24h, Here are account and storage update charts: Start block after snapshot generation was around
I have some new modifications which I will push soon, to deliver storage tries in object form too. |
|
About to deploy a new update, here's the result chart for the first run (account tries delivered as objects, storage tries warmed up via prefetching but not delivered as objects) |
|
New benchmark started. Started at 5M, this is the chart from where both of them were done generating the snapshot: https://geth-bench.ethdevops.io/d/Jpk-Be5Wk/dual-geth?orgId=1&var-exp=bench01&var-master=bench02&var-percentile=50&from=1589212697550&to=now |
|
After ~12 hours, here's a chart showing . |
First runThe first run was based on the version which delivers account tries, but not storage tries. This PR: Start block after snapshot generation:
Start block after snapshot generation: Second runThe second run is this PR with storage-trie-delivery added in Start block after snapshot generation:
Start block after snapshot generation: |
|
Interestingly, the removal of the regular prefetcher causes the loading-from-snapshot to take a hit. So the gains that are won on account_update and storage_update are somewhat eaten into by speed-loss in |
|
The last couple of days: The block heights are |
|
Preparing a third run now, where I've re-enabled the old prefetcher, which executes the next block on current state. The hope being that this should address the regression on |
|
Third run in progress |
Looks like adding back the original prefetcher adds a regression. Although the snapshot reads are now in league with master: I can't really explain it -- my best guess would be that we're hitting some IO bottleneck, and although we improve in some parts, there's some overall higher sluggishness in all IO. (there's no obvious extra egress from experimental on the last run either) |
|
Comparing run 2 with run 3: Run 2, blocks 5050253 - 6001565 : https://geth-bench.ethdevops.io/d/Jpk-Be5Wk/dual-geth?orgId=1&from=1589212470448&to=1589312346463
Run 3, blocks 5056095 - 6000581: https://geth-bench.ethdevops.io/d/Jpk-Be5Wk/dual-geth?orgId=1&from=1589548255204&to=1589652724888
|
|
Looking at the efficiency of the trie prefetcher: We can see that the second run has 5.18K fetches on aveage, whereas the third run has 4.94K fetches on average. Which is the reason why the |
holiman
force-pushed
the
improve_updates_2
branch
from
d62220c
to
6385d2d
Compare
|
ready for review. Should I disable to 'regular' prefetcher for good, or leave that as a separate thing later? |
|
Here are the last 6 hours of them both being up to head. I'll swap the machines around for a sanity check now |
Fourth run
and
Q.E.D |
holiman
force-pushed
the
improve_updates_2
branch
from
6385d2d
to
96b7e15
Compare
|
rebased |
holiman
force-pushed
the
improve_updates_2
branch
2 times, most recently
from
3bd0b1a
to
e9edf05
Compare
core/blockchain.go
Outdated
| @@ -1769,6 +1782,7 @@ func (bc *BlockChain) insertChain(chain types.Blocks, verifySeals bool) (int, er | |||
| parent = bc.GetHeader(block.ParentHash(), block.NumberU64()-1) | |||
| } | |||
| statedb, err := state.New(parent.Root, bc.stateCache, bc.snaps) | |||
| statedb.UsePrefetcher(bc.triePrefetcher) | |||
There was a problem hiding this comment.
Can we pass the prefetcher pointer to the statedb directly?
And also I think the triePrefetcher only make sense if we use snapshot. If snapshot is disabled, then we should disable this too.
There was a problem hiding this comment.
The bc.triePrefetcher will be nil if it's disabled. The rest of the snapshot-stuff is handled via bc, so it's a bit cumbersome to make the connection directly to statedb.
| // prefetcher | ||
| s.trie = s.db.prefetcher.GetTrie(s.data.Root) | ||
| } | ||
| if s.trie == nil { |
There was a problem hiding this comment.
If we load the data from the trie directly(without hitting in the snapshot for some reasons), is it ok to reload the path again by triePrefetcher?
I think it's fine.
(a) Reload will be super cheap if it's in the memory.
(b) Usually we can hit all slots in the snapshot
There was a problem hiding this comment.
I think we would only not use the snapshot if we're in the process of creating it, which is a temporary headache, and it's ok to load it twice in that case, since it would also have been warmed up in the cache.
Another similar thing is if we have two identical storage tries, in two separate contracts. We can only "hand out' the trie to one of them, and the second one will have to load from disk/cache. Otherwise, the first one would modify the trie, and the second one would get the modified version.
core/state/state_object.go
Outdated
| trieChanges = append(trieChanges, key) | ||
| } | ||
| if len(trieChanges) > 0 && s.db.prefetcher != nil && s.data.Root != emptyRoot { | ||
| s.db.prefetcher.PrefetchStorage(s.data.Root, trieChanges) |
There was a problem hiding this comment.
Prehaps we can cut down the number of slots to be resolved.
If the original value in storage is empty, then we don't need to resolve the path of slots.
But no idea how many this kind of slots we will have(create new slots in the contract).
There was a problem hiding this comment.
Why don't we need to resolve the path is the original value is empty? if we're going to write to it, we'll still need to resolve the path.
| var storage map[common.Hash][]byte | ||
| if s.db.snap != nil { |
There was a problem hiding this comment.
Any particular reason to move this storage initialization?
There was a problem hiding this comment.
Yes, we might not need to do it at all, if there are no pending storage changes
There was a problem hiding this comment.
That is, if the value == originStorage
core/state/statedb.go
Outdated
| s.prefetcher.Pause() | ||
| // We only want to do this _once_, if someone calls IntermediateRoot again, | ||
| // we shouldn't fetch the trie again | ||
| if s.originalRoot != (common.Hash{}) { |
There was a problem hiding this comment.
Perhaps we can do the similar thing like storage trie, whenever we retrieve the resolved trie from the prefetcher, mark it as nil.
There was a problem hiding this comment.
You mean to just release the reference for GC, or do you mean for correctness?
There was a problem hiding this comment.
The cached tries should only be accessed for one time and then throw it away. E.g. storage trie.
So I mean for state trie it's same. If we fetch the state trie from prefetcher then mark it as nil.
(although the current code does the same logic)
|
I addressed some of the concerns, I need to think a bit more about the remaining ones + do a rebase. |
|
Wow, this PR is really shining after a few days |
|
Here's a later segment, when none of the nodes were serving a lot of data to other peers Note: the scales are different, this PR is ~50ms faster per block |
|
Running a new benchmark now;
Both were just stopped from an earlier run, and not wiped, so should continue where they left off. |
|
I made an alternative version here:https://github.com/holiman/go-ethereum/tree/improve_updates_3 -- it splits up the account-fetching and storage-fetching in two separate goroutines. This PR and that branch is running on the benchmarkers now, and they're getting about the same number of performance-stats -- same number of deliveries. Both work fine, it's rather a question of which implementation is 'nicest'. The latter one is perhaps less hacky, but has a bit more code. |
core/blockchain.go
Outdated
| tp := state.NewTriePrefetcher(bc.stateCache) | ||
|
|
||
| go func() { | ||
| bc.wg.Add(1) |
There was a problem hiding this comment.
Please call Add in the outer goroutine instead of the one that's being launched.
core/state/trie_prefetcher.go
Outdated
| root: common.Hash{}, | ||
| } | ||
| // Wait for it | ||
| <-p.deliveryCh |
There was a problem hiding this comment.
Pass in a chan with the cmd instead
karalabe
force-pushed
the
improve_updates_2
branch
from
d4ee1ec
to
df8db52
Compare
core/state/trie_prefetcher.go
Outdated
| if storageTrie, ok := p.storageTries[root]; ok { | ||
| // Two accounts may well have the same storage root, but we cannot allow | ||
| // them both to make updates to the same trie instance. Therefore, | ||
| // we need to either delete the trie now, or deliver a copy of the trie. | ||
| delete(p.storageTries, root) | ||
| return storageTrie | ||
| } |
There was a problem hiding this comment.
@karalabe I've been thinking about this. The current Trie implementation already copies on modifications, so if we just copy the trie root node before delivering it, I think it would be fine if two accounts have the same storage root and obtain the "same" trie.
If we were to use a less copy-happy trie implementation, we'd have to do the deletion here, but we're not.
There was a problem hiding this comment.
True, though I don't expect to see a gain (i.e. the chance of two tries being the same is more of a deliberate attack than a thing that would normally happen). Still, the code would get cleaner.
| @@ -808,6 +846,10 @@ func (s *StateDB) IntermediateRoot(deleteEmptyObjects bool) common.Hash { | |||
| obj.updateRoot(s.db) | |||
| s.updateStateObject(obj) | |||
| } | |||
| usedAddresses = append(usedAddresses, addr) | |||
| } | |||
There was a problem hiding this comment.
If we were to use two separate routines for accounts/storage respectively, and had two separate Pause functions, we could do it in separate phases:
- Pause the storage worker,
- Do the storage hashing
- Pause the account worker
- Do the account hashing
It would look something like this:
// Update the root of all stateobjects in pending
for addr := range s.stateObjectsPending {
obj := s.stateObjects[addr]
if !obj.deleted {
obj.updateRoot(s.db)
}
}
if s.prefetcher != nil{
s.prefetcher.PauseAccounts()
if trie := s.prefetcher.GetTrie(s.originalRoot); trie != nil {
s.trie = trie
}
}
// Now update the account trie
for addr := range s.stateObjectsPending {
obj := s.stateObjects[addr]
if obj.deleted {
s.deleteStateObject(obj)
} else {
s.updateStateObject(obj)
}
usedAddresses = append(usedAddresses, addr)
}There was a problem hiding this comment.
Here's a working implementation, perhaps still with some rough edges: https://github.com/holiman/go-ethereum/pull/23/files
There was a problem hiding this comment.
If the storage hashing takes ~5-10ms of mostly-non-disk-accessing processing, then an additional 5-10ms given to the prefetcher for the remainder accounts might improve the account trie performance
EDIT: on more recent blocks, storage hash is closer to 15ms
There was a problem hiding this comment.
I'm wondering whether we should go all in and have 1 prefetcher per trie (root). That would maximize concurrency and make it more generic in that there's no account/storage distinction. Will try to have a stab at it. Maybe we should prep a new database with the snapshots generated.
|
The last run with a bit of extended (expensive stats):
|
|
The account and storage update indeed drops as expected.
However, important to keep in mind that account and storage reads from the snapshot bump upwards, since it is racing for resources vs. the prefetcher. Not sure if we can somehow prioritize the snapshot or if it's even worth it. This might be a counterargument against preloading on multiple threads, since each would be a hit vs. execution.
The account and storage commits also increased in this last run. This might however be due to the extra overhead in maintaining and calculating the "wasted" slots. Would be interesting to see a run with this feature disabled eventually, just to put a number on it.
|
karalabe
force-pushed
the
improve_updates_2
branch
from
c262b76
to
97e0aff
Compare
core/state/statedb.go
Outdated
|
|
||
| // If there's a prefetcher running, make an inactive copy of it that can | ||
| // only access data but does not actively preload (since the user will not | ||
| // know that they need to explicitly terminate an acive copy). |
core/state/trie_prefetcher.go
Outdated
| storageSkipMeter: p.storageSkipMeter, | ||
| storageWasteMeter: p.storageWasteMeter, | ||
| } | ||
| // If the prefetcher is alreacy a copy, duplicate the data |
| @@ -49,49 +37,114 @@ var ( | |||
| type triePrefetcher struct { | |||
| db Database // Database to fetch trie nodes through | |||
| root common.Hash // Root hash of theaccount trie for metrics | |||
| fetches map[common.Hash]Trie // Partially or fully fetcher tries | |||
There was a problem hiding this comment.
it's quite easy to misread fetches and fetchers, when skimming through the code
| trie, err := sf.db.OpenTrie(sf.root) | ||
| if err != nil { | ||
| log.Warn("Trie prefetcher failed opening trie", "root", sf.root, "err", err) | ||
| return | ||
| } |
There was a problem hiding this comment.
If, for some reason, we are missing a root, I think the effect will be that the subfetcher fails to start (and logs a warning), and later on, when we try to peek, nothing is listening on the copy chan, and we'll simply block forever.
I don't have a good idea on how to handle it differently, just making a note of it now...
Well, I guess one idea would be to, instead of returning here, have a loop which justs listens to the channels, doesn't actually do any work, and always deliver nil if data is requested.
| case sf.copy <- ch: | ||
| // Subfetcher still alive, return copy from it | ||
| return <-ch |
There was a problem hiding this comment.
My previous comment, about being blocked on the peek -- it might be possible to handle that by doing the same check here as done a few lines below:
if sf.trie == nil {
return nil
}
Becuse if the root could not be opened, then the sf.trie will be nil.
| func (sf *subfetcher) peek() Trie { | ||
| ch := make(chan Trie) | ||
| select { | ||
| case sf.copy <- ch: | ||
| // Subfetcher still alive, return copy from it | ||
| return <-ch | ||
|
|
||
| case <-sf.term: | ||
| // Subfetcher already terminated, return a copy directly | ||
| if sf.trie == nil { | ||
| return nil | ||
| } | ||
| return sf.db.CopyTrie(sf.trie) | ||
| } | ||
| } |
There was a problem hiding this comment.
So maybe this would be more 'safe'
| func (sf *subfetcher) peek() Trie { | |
| ch := make(chan Trie) | |
| select { | |
| case sf.copy <- ch: | |
| // Subfetcher still alive, return copy from it | |
| return <-ch | |
| case <-sf.term: | |
| // Subfetcher already terminated, return a copy directly | |
| if sf.trie == nil { | |
| return nil | |
| } | |
| return sf.db.CopyTrie(sf.trie) | |
| } | |
| } | |
| func (sf *subfetcher) peek() Trie { | |
| if sf.trie == nil { | |
| return nil | |
| } | |
| ch := make(chan Trie) | |
| select { | |
| case sf.copy <- ch: | |
| // Subfetcher still alive, return copy from it | |
| return <-ch | |
| case <-sf.term: | |
| // Subfetcher already terminated, return a copy directly | |
| return sf.db.CopyTrie(sf.trie) | |
| } | |
| } |
There was a problem hiding this comment.
Ah wait, you always close the term channel when exiting the loop, so it won't actually block here. Nevermind the noise then!
|
LGTM!! |
Squashed from the following commits: core/state: lazily init snapshot storage map core/state: fix flawed meter on storage reads core/state: make statedb/stateobjects reuse a hasher core/blockchain, core/state: implement new trie prefetcher core: make trie prefetcher deliver tries to statedb core/state: refactor trie_prefetcher, export storage tries blockchain: re-enable the next-block-prefetcher state: remove panics in trie prefetcher core/state/trie_prefetcher: address some review concerns sq
karalabe
force-pushed
the
improve_updates_2
branch
from
de8a609
to
42f9f1f
Compare
This is part 2 of #20796. It's very much work in progress, and a bit hacky.
Feedback regarding the design is appreciated