VKT: Optimizations #147
VKT: Optimizations #147
Conversation
Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
…ory usage Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
jsign
force-pushed
the
jsign/benchktries
branch
from
13a685b to
e889d40
Compare
| @@ -0,0 +1,76 @@ | |||
| package tests | |||
There was a problem hiding this comment.
Here we have the benchmarks from #146.
I only removed the statelessness one as we discussed, so we avoid test flags.
| trieIndexBytes := treeIndex.Bytes32() | ||
| verkle.FromBytes(&poly[3], trieIndexBytes[16:]) | ||
| verkle.FromBytes(&poly[4], trieIndexBytes[:16]) | ||
| if !treeIndex.IsZero() { | ||
| trieIndexBytes := treeIndex.Bytes32() | ||
| verkle.FromBytes(&poly[3], trieIndexBytes[16:]) | ||
| verkle.FromBytes(&poly[4], trieIndexBytes[:16]) | ||
| } |
There was a problem hiding this comment.
First optimization is avoiding work if treeIndex is zero, which makes sense and is a pretty normal case.
| resolver := | ||
| func(h []byte) ([]byte, error) { | ||
| return trie.db.diskdb.Get(h) | ||
| } | ||
| resolver := func(h []byte) ([]byte, error) { | ||
| return trie.db.diskdb.Get(h) | ||
| } |
| flush := make(chan verkle.VerkleNode) | ||
| type vnflush struct { | ||
| n verkle.VerkleNode | ||
| value []byte | ||
| dbKey []byte | ||
| } | ||
| flush := make(chan vnflush, 1024) | ||
| resolver := func(n verkle.VerkleNode) { | ||
| flush <- n | ||
| value, err := n.Serialize() | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| dbKey := nodeToDBKey(n) | ||
| flush <- vnflush{ | ||
| n: n, | ||
| value: value, | ||
| dbKey: dbKey, | ||
| } |
There was a problem hiding this comment.
Looking further at where the wall-clock time was going, I realized that after Commit()ing the underlying go-verkle trie, a significant amount of time was spent flushing the result.
What I did in ethereum/go-verkle#314, is making Flush() for the root node to do the work in parallel. This means that resolver will be called by multiple goroutines balancing more CPU work in all available cores.
To squeeze things more, the Serialize() part of the process was now moved from L258 to the resolver. This means that the Serialize() work will also be done in the goroutines that are flushing the result, and not in a single core.
In a nutshell, since the resolver execution is exploiting all cores, we want to do as much work as possible there, so the main goroutine ranging L269 is receiving plain results to be stored in the diskdb. This main goroutine isn't doing heavy CPU work, just receiving exactly what needs to be stored.
This is important, since if the range in L269 is slow, it will slow down everything. Note how I also made a buffered channel in L246. We need to avoid as much as possible to be blocking goroutines, so having extra breathing room also improved performance.
There was a problem hiding this comment.
So if #295 gets merged, Serialize will become a tad more expensive - at the cost of a cheaper deserialization (hence not producing a tree that is crash-prone because of potentially invalid commitments, as in your current approach). Your change, I believe, will mitigate this problem and make it cheaper to follow this approach. That's very nice.
Signed-off-by: Ignacio Hagopian <[email protected]>
…umption Signed-off-by: Ignacio Hagopian <[email protected]>
Signed-off-by: Ignacio Hagopian <[email protected]>
|
Should be interesting if we consider ethereum/go-verkle#314, but for now I'll close this PR. |
This PR contains more optimizations apart from the ones in:
The
go.modin this PR uses those branches; we should update the official commits whenever those PRs are merged.Apart from that, we're doing other optimizations that will be explained in PR comments.
The results are significant comparing
beverly-hillsvs this PR: