-
Notifications
You must be signed in to change notification settings - Fork 380
refactor: sampling optimalizations #5167
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
This reverts commit 8c62607.
| }() | ||
|
|
||
| sampleItems := make([]SampleItem, 0, SampleSize) | ||
| sampleItems := make([]SampleItem, 0, SampleSize+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.
why +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.
lower there is a condition when if len(sampleItems) > SampleSize it cuts from the end. so sampleItems used length is SampleSize+1.
|
|
||
| // NewPrefixHasher returns new hasher which is Keccak-256 hasher | ||
| // with prefix value added as initial data. | ||
| func NewPrefixHasher(prefix []byte) hash.Hash { |
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 also called in proof.go. Do we want that one to use the stateful hasher?
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.
yeah, it is used in the sample creation as well and both should do the same calculation.
| } | ||
|
|
||
| // NewHasher returns new Keccak-256 hasher. | ||
| func NewStatefulHasher(state []byte) (sha3.StatefulHash, error) { |
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.
Should new tests be added in hasher_test for this new stateful hasher?
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.
I made some test in its repo
This PR optimizes the
ReserveSamplefunction to improve performance during sampling operations. The key optimizations include addingChunkTypeto theSampleItemstruct to avoid redundant parsing, reusing hashers in worker goroutines instead of creating new ones for each SOC chunk and using new optimized hasher function for transformed address calculations. These changes reduce memory allocations, GC pressure, and CPU usage without altering the functional behavior of the sampling process.Checklist
Description
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
Screenshots (if appropriate):