-
Notifications
You must be signed in to change notification settings - Fork 19.6k
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, cmd/utils: when configured via flag, in block execution: prefetch all reads from account/storage tries #29807
Conversation
jiejuelema |
Okay |
Oaky |
|
Full sync benchmark nodes are running around block 12.2 million (bench7 is running this PR with the witness collection flag enabled, bench8 is master) Witness collection mode is causing additional prefetch reads which all hit the db cache. There appears to be no performance impact. Prefetcher duplicate/waste metrics cease to be meaningful if |
cmd/utils/flags.go
Outdated
@@ -954,6 +954,12 @@ Please note that --` + MetricsHTTPFlag.Name + ` must be set to start the server. | |||
Value: metrics.DefaultConfig.InfluxDBOrganization, | |||
Category: flags.MetricsCategory, | |||
} | |||
|
|||
CollectWitnessesFlag = &cli.BoolFlag{ | |||
Name: "collectwitnesses", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please name this debug.collectwitness
and move it to the debug namespace in the usage section so we know it's not something to rely on.
As for the description, perhaps: Enable state witness generation during block execution. Work in progress flag, don't use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also Pls rename the flag to DebugCollectWitness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please name this debug.collectwitness and move it to the debug namespace in the usage section so we know it's not something to rely on.
core/state/statedb.go
Outdated
@@ -604,6 +610,9 @@ func (s *StateDB) getStateObject(addr common.Address) *stateObject { | |||
if acc == nil { | |||
return nil | |||
} | |||
if s.collectWitness && s.prefetcher != nil { | |||
s.prefetcher.prefetch(common.Hash{}, s.originalRoot, common.Address{}, [][]byte{addr[:]}) | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to add the read to the prefetcher if you load from the trie too. Since the prefetcher's trie and the statedb trie are different. Yes, it's a bit of double work, but we only do that while generating the snapshot, so it's fine really.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow.
@karalabe I think I've mostly addressed your feedback and simplified the PR. |
…g: prefetch all reads from account/storage tries, terminate prefetcher synchronously.
d9450f0
to
1be108b
Compare
1be108b
to
e6e034c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
Pulled out from #29719