-
Notifications
You must be signed in to change notification settings - Fork 21k
trie: reduce the memory allocation in trie hashing #31902
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
base: master
Are you sure you want to change the base?
Conversation
@@ -176,8 +182,8 @@ func (h *hasher) encodedBytes() []byte { | |||
} | |||
|
|||
// hashData hashes the provided data | |||
func (h *hasher) hashData(data []byte) hashNode { | |||
n := make(hashNode, 32) | |||
func (h *hasher) hashData(data []byte) []byte { |
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.
Could we have made the return type here and hashNode
a [32]byte
? It probably wouldn't save much on the total size of allocations, but it will reduce the number of individual allocations.
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.
Sounds reasonable to me. But I don't want to do it in this pr, otherwise it will end up as a giant PR I think.
func (h *hasher) hashFullNodeChildren(n *fullNode) *fullNode { | ||
var children [17]node | ||
func (h *hasher) encodeFullNode(n *fullNode) []byte { | ||
fn := fnEncoderPool.Get().(*fullnodeEncoder) |
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 introduces a synchronization point for all parallel hashers, which is probably why the performance degraded slightly. I think we can do better.
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.
Really? I think sync.Pool should be efficient for concurrent usage.
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.
Well, it might be efficient in what it's doing but it is certainly not free.
6b5ccf9
to
17b0a10
Compare
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
My 2c is, optimizing to reduce GC churn at the cost of increased runtime is not really an optimization. We just moved the cost of GC churn to somewhere else (the sync pool in this case). |
79a934b
to
b7aa034
Compare
This pull request optimizes trie hashing by reducing memory allocation overhead. Specifically:
Benchmark results
Memory profile