-
Notifications
You must be signed in to change notification settings - Fork 64
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
tree: avoid allocs on paths collection #371
tree: avoid allocs on paths collection #371
Conversation
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
childpath := make([]byte, len(path)+1) | ||
copy(childpath, path) | ||
childpath[len(path)] = byte(i) |
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 was unfortunately creating the L1545 slice for every 256 children on every InternalNode
, even if those children's where empty (or any other case not used in the switch below).
paths = append(paths, childpath) | ||
paths = append(paths, childNode.stem[:len(path)+1]) |
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.
In the case of LeafNodes
we can reslice the internal stem
, so directly avoid allocations at this level.
childpath := make([]byte, len(path)+1) | ||
copy(childpath, path) | ||
childpath[len(path)] = byte(i) | ||
list, paths = childNode.collectNonHashedNodes(list, paths, childpath) |
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.
We only do allocations now in confirmed children that are InternalNodes
.
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.
simple but nasty. Good catch.
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
…serialization (#354) * don't rely on the commitment in HashedNode change leaf serialization to add the leaf commitment there fix a few tests fix: incorrect size computation caused commitment overwrite fix some unit tests fix the batch unit test * declare paths for serialization * Add CommitmentBytes back in SerializedNode * fixes from replaying * fix: deepsource warnings * tree: avoid memory allocs while collecting paths (#371) Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * pbss: serialized points in uncompressed form (#370) * config: rename serialized point size constant and value to uncompressed form Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * switch to uncompressed serialization Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> --------- Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * tree: transform first layer to hashed nodes after serializing (#372) * tree: after serializing transform first layer to hashed nodes Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * tree: add comment about temporary code Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> --------- Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * tree: make HashedNode non-pointer and fieldless (#373) * tree: make HashedNode non-pointer and fieldless Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * lints Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * hashednode: add panic comment Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * hashednode: add label in toDot Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> --------- Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> * rebase fix Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> --------- Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com> Co-authored-by: Ignacio Hagopian <jsign.uy@gmail.com>
This PR avoids a big source of allocations detected in our overlay transition benchmark.
I'll explain in the comments.