core/state: trie prefetcher change: calling trie() doesn't stop the associated subfetcher #29035
Conversation
jwasinger
requested review from
karalabe,
holiman and
rjl493456442
as code owners
|
Failed tests... the last commit borks something. |
|
Okay. CI is running successfully locally on two separate machines (mac/linux) for me. Unsure why it fails in github actions. |
jwasinger
force-pushed
the
stateless-witness-prefetcher-changes
branch
from
5ecb17e
to
13196b8
Compare
There was a problem hiding this comment.
Could you please elaborate a bit more about what this PR tries to achieve, and why that is better.
As in, why it's needed for your upcoming work, but also what the effects on the current code is. Does it improve anything? Simplify something? Degrade something?
| // repeated. | ||
| // 2. Finalize of the main account trie. This happens only once per block. | ||
| func (p *triePrefetcher) prefetch(owner common.Hash, root common.Hash, addr common.Address, keys [][]byte) error { | ||
| if p.closed { |
There was a problem hiding this comment.
Is there ever a multi-thread accessing prefetch / close ? If so, might be better to make closed into an atomic.Bool? But if we don't access it that way, it's fine.
There was a problem hiding this comment.
Prefetcher is manipulated in the statedb, which is not regarded as thread-safe. I believe we never do multi-thread accessing prefetch.
|
The biggest difference this pull request introduces is: Originally, when a trie is requested, the prefetching tasks are suspended and resumed once the live trie is copied and returned. This approach could cause problems for witness collection. For instance: If slot A is accessed but not modified, the relevant trie nodes should be wrapped as the witness. Subsequently, slot A will be scheduled for prefetching and queued in the task list. However, if the corresponding storage trie is retrieved without slot A being preloaded at that time, then the relevant trie nodes won't be included in the witness. In this pull request, the prefetching tasks scheduled before retrieving the trie will always be executed. Thus the issue is resolved. Besides, the Copy function is dropped for simplification. And actually the original complexity is not worthwhile anyway. The statedb.Copy is mostly used in miner, but after the change made by marius, we don't copy the statedb in miner anymore. Make no sense to copy the prefetcher for the performance gain. |
…ssociated subfetcher Co-authored-by: Martin HS <martin@swende.se> Co-authored-by: Péter Szilágyi <peterke@gmail.com>
… until pending tasks on the subfetcher have been resolved.
jwasinger
force-pushed
the
stateless-witness-prefetcher-changes
branch
from
453f06d
to
287cead
Compare
|
Unfortunately, the added overhead to block processing and increased memory usage means that this PR is a non-starter. |
|
I think it's maybe a bit premature to close this. Please add the relevant graphs/charts from the benchmark-run, so we can discuss and reason about it |
So apparently
And The prefetchers fetch roughly the same amount of data (bench06 a bit more slots): And the both have same "misses", that is, work performed that was never needed (e.g. fetching slots at the end of transaction, but the next transaction set the slots back to the previous value).
In the "optimistic" prefetcher, if we overload it with requests, it will just fetch what it can, and then we'll say "ok stop now". In the strict one, overloading it with requests means we'll have to wait it out, it will penalize use. So if we do this, maybe we have to be more clever about what to schedule. I'm not really 100% sure what scenarios causes the wasted work, or the delivery misses. Might be worth investigating a bit |
| @@ -237,7 +194,7 @@ func newSubfetcher(db Database, state common.Hash, owner common.Hash, root commo | |||
| owner: owner, | |||
| root: root, | |||
| addr: addr, | |||
| wake: make(chan struct{}, 1), | |||
| wake: make(chan struct{}), | |||
There was a problem hiding this comment.
This change seems problematic. Whereas the original code allowed to schedule the request and then fly off continuing execution, the new code (along with L217 below) will block until a previous request is done (actually, block on the second request).
Any particular reason you made the async notifier synchronous?
|
There's a change in this PR that converted the async wake notifier into a sync one, which can potentially have a really heavy hit on performance as it ill block execution during preloading. Perhaps that's the root of the slowdown. |
|
Closing and reopening my own because OP didn't permit maintainer changes. |
|
:'( I don't know why this is suddenly something I have to explicitly enable. Until recently, PRs I've made in the past have been maintainer-modifiable by default.... |
This pulls trie prefetcher changes from #29029 into a separate PR.
It changes the trie prefetcher s.t. a call to
triereturns a trie (after any pending tasks have been resolved in the subfetcher). After a call totrie, the subfetcher is now available for more work (instead of terminating).