improve performance for BloomFilter #782
Conversation
|
@valyala Could you review this PR ? thank you. |
|
@ahfuzhang , could you provide benchmark results for these performance improvements? Please add missing benchmarks in a separate pull request if needed, so software engineers could validate performance improvements by running |
|
|
||
| import ( | ||
| "fmt" | ||
| bitutil "math/bits" |
There was a problem hiding this comment.
It isn't a good idea to use aliases for packages, since it complicates investigating and reading the code. It is better to rename local variables, which clash with the package names instead.
| bf.bits = bits | ||
| } | ||
|
|
||
| func roundUpPowerOf2(x int64) int64 { |
There was a problem hiding this comment.
This function isn't used, so it must be removed. Also, it might be better from the readability and performance PoV to use bits.Len64() instead of the manual calculations (if this function will be used in the future):
func roundUpPowerOf(x int64) int64 {
return int64(1 << bits.Len(uint64(x)))
}
|
|
||
| func initBloomFilter(bits, hashes []uint64) { | ||
| maxBits := uint64(len(bits)) * 64 | ||
| maxBits := uint64(len(bits)) << 6 // x<<6 means x*64 |
There was a problem hiding this comment.
This optimization is performed by Go compiler, so it isn't needed - it just complicates reading and maintaining the code.
| maxBits := uint64(len(bits)) * 64 | ||
| maxBits := uint64(len(bits)) << 6 // x<<6 means x*64 | ||
| ptr := unsafe.Pointer(unsafe.SliceData(bits)) | ||
| if bitutil.OnesCount64(maxBits) == 1 { // Determine whether it is a power of 2 |
There was a problem hiding this comment.
This fast path will be triggered in very rare cases, so its' usefulness is questionable. Suppose the average size of the bloom filter is 32*128 bits. Then this optimization will be triggered only 6 out of 31 times - for (64*1, 64*2, 64*4, 64*8, 64*16, 64*32) bit lengths, or in 6/31=19% of cases. It isn't worth complicating the code for such cases. Additionally, more code in hot paths means the CPU must fit more code into instruction cache. This may lead to slowdowns if the needed instructions cannot fit the instruction cache.
| i := idx >> 6 | ||
| j := idx & 63 |
There was a problem hiding this comment.
These optimizations aren't needed - Go compiler performs them on itself.
|
Thank you very much. My optimization wasn't rigorous enough, so I'll cancel it for now. Thank you for your time. |
2 participants
Describe Your Changes
Checklist
The following checks are mandatory: