Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8775 +/- ##
==========================================
+ Coverage 83.22% 83.25% +0.02%
==========================================
Files 418 414 -4
Lines 32385 32801 +416
==========================================
+ Hits 26952 27308 +356
- Misses 4050 4077 +27
- Partials 1383 1416 +33
🚀 New features to boost your workflow:
|
bdccc9b to
1f26e66
Compare
easwars
left a comment
There was a problem hiding this comment.
I thought we decided to make it possible for the user to set the default buffer pool for the whole process through an experimental API, and get rid of the existing dial option and the server option. Was your plan to do that in a follow-up?
mem/buffer_pool.go
Outdated
|
|
||
| // Determine the maximum exponent we need to support. | ||
| // bits.Len64(math.MaxUint64) is 63. | ||
| const maxExponent = 63 |
There was a problem hiding this comment.
Does this assume a 64-bit machine?
There was a problem hiding this comment.
It works for machines up to 64 bits, i.e. both 32 and 64 bits.
There was a problem hiding this comment.
If a value of 63 is passed to this function, capSize := 1 << exp will evaluate to a negative number on 64 bit machines, since capSize is a signed integer.
Also, how would these bit shift operators (where the exponent is greater than 32 bits) return expected values on 32 bit machines?
There was a problem hiding this comment.
I think we should have some tests for the edge cases for both 32 and 64 bit machines.
There was a problem hiding this comment.
You're right. I've updated the implementation to use bits.UintSize to determine the machine's word size and set that as the maximum tier size. I also added tests that mock bits.UintSize to simulate both 32-bit and 64-bit environments.
mem/buffer_pool.go
Outdated
| if exp > maxExponent || exp < 0 { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Are we silently ignoring certain exponents that don't meet our criteria? Should we error out instead?
There was a problem hiding this comment.
Added a comment explaining the reason to ignore the values.
- For values greater than 63, it's not possible to allocate such large slices. So ignoring them will not cause observable changes. I changed the param type to
uint8to help give linter errors if people pass constant values > 256. - For values < 1, the buffer sizes would be fractional, e.g.
$2^{-1}=0.5$ . Since slices are of integer sizes, a sized buffer pool with a fractional size will never get used. I changed the param type touint, making it impossible to pass negative values.
There was a problem hiding this comment.
Wouldn't it be better to return an error in such cases instead of ignoring certain exponents?
There was a problem hiding this comment.
Updated to return an error from the constructor.
I was waiting for the author of #8770 to raise a PR for exposing a function to set the default buffer pool. See the second part of #8770 (comment).
I'm not sure if we want to do this. Maybe people want to use different buffer pools for each channel. In this PR, I'm just improving the existing buffer pool implementation. |
b50277e to
8e7ecc4
Compare
8e7ecc4 to
36d34f7
Compare
|
Also, would it make sense to add some of the micro benchmarks that you used as part of this PR? Thanks. |
mem/buffer_pool.go
Outdated
| // NewBinaryTieredBufferPool returns a BufferPool implementation that uses | ||
| // multiple underlying pools of the given pool sizes. The pool sizes must be | ||
| // powers of 2. This enables O(1) lookup when getting or putting a buffer. |
There was a problem hiding this comment.
This section needs some updating I guess given that we accept exponents as arguments (and the next paragraph also says so).
mem/buffer_pool.go
Outdated
|
|
||
| // Determine the maximum exponent we need to support. | ||
| // bits.Len64(math.MaxUint64) is 63. | ||
| const maxExponent = 63 |
There was a problem hiding this comment.
If a value of 63 is passed to this function, capSize := 1 << exp will evaluate to a negative number on 64 bit machines, since capSize is a signed integer.
Also, how would these bit shift operators (where the exponent is greater than 32 bits) return expected values on 32 bit machines?
mem/buffer_pool.go
Outdated
|
|
||
| // Determine the maximum exponent we need to support. | ||
| // bits.Len64(math.MaxUint64) is 63. | ||
| const maxExponent = 63 |
There was a problem hiding this comment.
I think we should have some tests for the edge cases for both 32 and 64 bit machines.
mem/buffer_pool.go
Outdated
| if exp > maxExponent || exp < 0 { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Wouldn't it be better to return an error in such cases instead of ignoring certain exponents?
3f63cea to
e467b97
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new binaryTieredBufferPool which provides a significant performance improvement for buffer pool lookups by using power-of-2 tier sizes for O(1) lookups. The implementation is clever, leveraging math/bits for efficient calculations. The accompanying tests are thorough, including architecture-specific checks and benchmarks that demonstrate the performance gains. I've found a minor issue in one of the new benchmark tests that could lead to a panic. Overall, this is an excellent contribution that improves performance and is well-implemented.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
1502569 to
f32f229
Compare
This change adds a new tiered buffer pool that uses power-of-2 tier sizes. It reduces the lookup time for the relevant sizedBufferPool from$O(\log n)$ to $O(1)$ , where n is the number of tiers. This creates constant-time lookups independent of the tier count, allowing users to add more tiers without performance overhead.
Benchmarks
Micro-benchmark that measures only the pool query performance, ignoring the allocation time:
With 5 tiers:
With 9 tiers:
RELEASE NOTES:
NewBinaryTieredBufferPoolto create such pools.