-
Notifications
You must be signed in to change notification settings - Fork 4.6k
mem: Allow overriding the default buffer pool. #8806
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
Conversation
The default buffer pool only has a only a few tiers, which hurts performance in some applications. In, it was decided that instead of adding more tiers to the default pool, we should allow changing the default pool.
|
I didn't know if you |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8806 +/- ##
==========================================
- Coverage 83.30% 83.00% -0.31%
==========================================
Files 418 414 -4
Lines 33004 32779 -225
==========================================
- Hits 27493 27207 -286
- Misses 4102 4115 +13
- Partials 1409 1457 +48
🚀 New features to boost your workflow:
|
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
I can't figure out how to get the PR validation to pass. My initial PR title didn't start with "mem:", but now it does. |
|
@vanja-p can you check if the test failing is related to the changes or not? |
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@eshitachandwani, looks like it's not related to my change. Rerunning them passed, and also, my 2 new lines don't have any coverage. |
I think it's better to move |
Done. |
arjan-bal
left a comment
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.
Just one minor comment; otherwise, LGTM. Adding a second reviewer.
| // can't be changed otherwise. The provided buffer pool must be non-nil. This | ||
| // must be called before creating any grpc clients or servers. The default value | ||
| // is mem.DefaultBufferPool. | ||
| func SetDefaultBufferPool(bufferPool mem.BufferPool) { |
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.
Instead of requiring users to set this before making RPCs, we recommend calling these functions within an init() function. This prevents data races that can occur when multiple packages invoke the same method simultaneously. For example, the balancer.Register function includes the following requirement:
NOTE: this function must only be called during initialization time (i.e., in an init() function) and is not thread-safe...
Can you please add a similar note here?
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.
yep, done.
internal/leakcheck/leakcheck.go
Outdated
| defaultPool := mem.DefaultBufferPool() | ||
| globalPool.Store(&defaultPool) | ||
| (internal.SetDefaultBufferPoolForTesting.(func(mem.BufferPool)))(&globalPool) | ||
| (internal.SetDefaultBufferPool.(func(mem.BufferPool)))(&globalPool) |
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.
Nit: Can the extra parenthesis be avoided? Can this be
internal.SetDefaultBufferPool.(func(mem.BufferPool))(&globalPool) ?
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.
Yep. done.
The default buffer pool only has a only a few tiers, which hurts performance in some applications. In #8770, it was decided that instead of adding more tiers to the default pool, we should allow changing the default pool.
RELEASE NOTES: