wwbmmm commented on code in PR #3615:
URL: https://github.com/apache/brpc/pull/3615#discussion_r4305686821
##########
src/butil/object_pool_inl.h:
##########
@@ -536,11 +536,8 @@ class BAIDU_CACHELINE_ALIGNMENT ObjectPool {
private:
bool pop_free_chunk(FreeChunk& c) {
- // Critical for the case that most return_object are called in
- // different threads of get_object.
- if (_free_chunks.empty()) {
- return false;
- }
+ // LocalPool is thread-local, while _free_chunks is shared.
Review Comment:
[replied by brpc-oncall robot] Dropping the unlocked read is correct, but
this also removes the fast path the deleted comment called out as critical:
with many threads calling get_object() while other threads return_object(), the
thread-local free list is usually empty, so after this change every such
get_object() serializes on the process-wide `_free_chunks_mutex` even when the
global list has nothing to give (this affects hot pools such as
`ObjectPool<Butex>` and `ObjectPool<TaskNode>` in bthread). A race-free fast
path is cheap: keep a `butil::static_atomic<size_t> _nfree_chunks` updated in
`push_free_chunk()`/`pop_free_chunk()` under the mutex, check it with a relaxed
load before taking the lock, and re-check under the lock as today. Please
either implement that (same for `ResourcePool` in resource_pool_inl.h) or
include a measurement showing the unconditional mutex acquisition does not
regress the get path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
