-
Notifications
You must be signed in to change notification settings - Fork 10
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
trie/utils: simplify GetTreeKeyWithEvaluatedAddress #421
Conversation
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
0942032
to
fc7e8ef
Compare
// little-endian, 32-byte aligned treeIndex | ||
var index [32]byte | ||
for i := 0; i < len(treeIndex); i++ { | ||
binary.LittleEndian.PutUint64(index[i*8:(i+1)*8], treeIndex[i]) | ||
} | ||
verkle.FromLEBytes(&poly[3], index[:16]) | ||
verkle.FromLEBytes(&poly[4], index[16:]) | ||
trieIndexBytes := treeIndex.Bytes32() | ||
verkle.FromBytes(&poly[3], trieIndexBytes[16:]) | ||
verkle.FromBytes(&poly[4], trieIndexBytes[:16]) |
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.
This changes avoid this loop and copying stuff to that auxiliary index
.
Note this is the same trick we did in GetTreeKey(...)
(L96 in this file).
func BenchmarkGetTreeKeyWithEvaluatedAddress(b *testing.B) { | ||
var buf [32]byte | ||
rand.Read(buf[:]) | ||
addrpoint := EvaluateAddressPoint(buf[:]) | ||
|
||
rand.Read(buf[:]) | ||
n := uint256.NewInt(0).SetBytes32(buf[:]) | ||
|
||
_ = verkle.GetConfig() | ||
|
||
b.ReportAllocs() | ||
b.ResetTimer() | ||
for i := 0; i < b.N; i++ { | ||
_ = GetTreeKeyWithEvaluatedAddess(addrpoint, n, 0) | ||
} | ||
} |
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.
Out of curiosity I added this benchmark to compare the pre and post change, and didn't notice any significant speedup.
In any case, avoiding an auxiliary array and copying is good both for doing less work, and also a more compact implementation. (and, if you mind, a closer match to GetTreeKey(...)
since there's no reason why doing the same thing in different ways).
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.
LGTM
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>
This PR makes a slight change in
GetTreeKeyWithEvaluatedAddress
which allows to avoid copying the tree index to a separate array to accommodate for endianness.This is a trick I introduced a long ago in
GetTreeKey(...)
but was just missing in this other variant.