-
Notifications
You must be signed in to change notification settings - Fork 699
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
Optimize bloom filter #2588
Optimize bloom filter #2588
Conversation
// positive probability of a bloom filter with [numEntries] after [count] | ||
// additions. | ||
// | ||
// It is guaranteed to return a value in the range [minHashes, maxHashes]. |
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.
❤️
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.
Addressing my nits in #2591
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 🏆 - left small comments
Why this should be merged
This PR improves the performance of handling bloom filters over the
p2p
network.Old
Marshal
New
Marshal
Old
Parse
New
Parse
Old
Contains
New
Contains
How this works
Note: This PR is not backwards compatible. Updated nodes will drop requested from un-updated nodes (and visa versa).
The major improvements from this PR come from:
Marshal
entries
from[]uint64
to[]byte
to avoid needing to allocate a potentially large entries array and then individually pack entries into the array inParse
.Contains
How this was tested