VKT state migration optimized methods & lower LeafNode memory pressure #338
Conversation
jsign
changed the title
| if i >= NodeWidth-1 { | ||
| if i >= NodeWidth { |
There was a problem hiding this comment.
I forgot to mention: I found this bug the hard way while doing the helpers and running stuff converting trees...
I think we got lucky to not see this bug happening before.
jsign
force-pushed
the
jsign/newconversion
branch
from
a32ba28 to
5126adb
Compare
| ret[i] = LeafNode{ | ||
| values: nv.Values, | ||
| stem: nv.Stem, | ||
| c1: Generator(), | ||
| c2: Generator(), | ||
| } | ||
|
|
||
| var c1poly, c2poly [NodeWidth]Fr | ||
|
|
||
| valsslice := make([][]byte, NodeWidth) | ||
| for idx := range nv.Values { | ||
| valsslice[idx] = nv.Values[idx] | ||
| } | ||
|
|
||
| fillSuffixTreePoly(c1poly[:], valsslice[:NodeWidth/2]) | ||
| ret[i].c1 = cfg.CommitToPoly(c1poly[:], 0) | ||
| fillSuffixTreePoly(c2poly[:], valsslice[NodeWidth/2:]) | ||
| ret[i].c2 = cfg.CommitToPoly(c2poly[:], 0) |
There was a problem hiding this comment.
Now that we merged the other emptyCode cached PR, we can exploit the same here. Maybe I can extract this code section to be shared between both.
For now, it won't make a big difference in the conversion since we're bottleneck by compactions.
I prefer to merge this PR as is with our correctness guarantees, and I'll do a PR right after with only that refactor.
This change was optimized for the tree conversion. In that case, we have a lot of memory pressure by having 256-slices where, most of the nodes, only use a few slots. The more efficient the RAM use, the more batching we can do. For replay benchmark or "usual" run of block execution, that memory waste isn't that big since we don't have that many nodes in memory. In that case, using a map is paying some CPU cost for the cryptographic hash function maybe unnecessarily. I'd say is I change I'd ideally want to keep for the conversion code runs, and maybe not for master. To mention some numbers, the |
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
… tree building Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
jsign
force-pushed
the
jsign/newconversion
branch
from
0ebfd85 to
1604357
Compare
|
Marking this PR This PR is complete and works for a full tree conversion, but now we're also exploring the overlay tree conversion in parallel. |
This PR contains the following:
conversion.gofile with specifically crafted methods to supportgethVKT state migration subcommand.LeafNodesoptimized for the conversion case where spare 256 slices create a lot of memory pressure for the VKT conversion. This might be beneficial too for "daily" usage, but I doubt it might be entirely useful for that case.I preferred to do some heavy commenting in the code for future readers, but it will also serve as PR comments.
For context, see gballet/go-ethereum#196.
This PR is reviewable and mergable, but let's hold it open for a bit until we fully understand implications/results.