Copilot commented on code in PR #3640:
URL: https://github.com/apache/brpc/pull/3640#discussion_r4344175161
##########
src/butil/resource_pool_inl.h:
##########
@@ -184,28 +187,32 @@ class BAIDU_CACHELINE_ALIGNMENT ResourcePool {
} \
T* p = nullptr;
\
/* Fetch memory from local block */ \
- if (_cur_block && _cur_block->nitem < BLOCK_NITEM) { \
- id->value = _cur_block_index * BLOCK_NITEM + _cur_block->nitem; \
- auto item = _cur_block->items + _cur_block->nitem; \
- p = new (item->void_data()) T CTOR_ARGS; \
- if (!ResourcePoolValidator<T>::validate(p)) { \
- p->~T(); \
- return nullptr;
\
+ if (_cur_block) { \
+ const size_t nitem =
_cur_block->nitem.load(butil::memory_order_relaxed); \
+ if (nitem < BLOCK_NITEM) { \
+ id->value = _cur_block_index * BLOCK_NITEM + nitem; \
+ auto item = _cur_block->items + nitem; \
+ p = new (item->void_data()) T CTOR_ARGS; \
+ if (!ResourcePoolValidator<T>::validate(p)) { \
+ p->~T(); \
+ return nullptr; \
+ } \
+ _cur_block->nitem.fetch_add(1, butil::memory_order_release); \
+ return p; \
} \
- ++_cur_block->nitem; \
- return p; \
} \
/* Fetch a Block from global */ \
_cur_block = add_block(&_cur_block_index); \
if (_cur_block != nullptr) {
\
- id->value = _cur_block_index * BLOCK_NITEM + _cur_block->nitem; \
- auto item = _cur_block->items + _cur_block->nitem; \
+ const size_t nitem =
_cur_block->nitem.load(butil::memory_order_relaxed); \
+ id->value = _cur_block_index * BLOCK_NITEM + nitem; \
+ auto item = _cur_block->items + nitem; \
p = new (item->void_data()) T CTOR_ARGS; \
if (!ResourcePoolValidator<T>::validate(p)) { \
p->~T(); \
return nullptr;
\
} \
- ++_cur_block->nitem; \
+ _cur_block->nitem.fetch_add(1, butil::memory_order_release); \
return p; \
} \
Review Comment:
`nitem` is documented as being written only by the owning `LocalPool`
(single-writer), so using `fetch_add` introduces an unnecessary
read-modify-write atomic operation on the hot path. Consider replacing
`fetch_add(1, memory_order_release)` with `store(nitem + 1,
memory_order_release)` using the already-loaded `nitem` value; this preserves
the release publication semantics while typically being cheaper than an RMW.
##########
src/butil/resource_pool_inl.h:
##########
@@ -184,28 +187,32 @@ class BAIDU_CACHELINE_ALIGNMENT ResourcePool {
} \
T* p = nullptr;
\
/* Fetch memory from local block */ \
- if (_cur_block && _cur_block->nitem < BLOCK_NITEM) { \
- id->value = _cur_block_index * BLOCK_NITEM + _cur_block->nitem; \
- auto item = _cur_block->items + _cur_block->nitem; \
- p = new (item->void_data()) T CTOR_ARGS; \
- if (!ResourcePoolValidator<T>::validate(p)) { \
- p->~T(); \
- return nullptr;
\
+ if (_cur_block) { \
+ const size_t nitem =
_cur_block->nitem.load(butil::memory_order_relaxed); \
+ if (nitem < BLOCK_NITEM) { \
+ id->value = _cur_block_index * BLOCK_NITEM + nitem; \
+ auto item = _cur_block->items + nitem; \
+ p = new (item->void_data()) T CTOR_ARGS; \
+ if (!ResourcePoolValidator<T>::validate(p)) { \
+ p->~T(); \
+ return nullptr; \
+ } \
+ _cur_block->nitem.fetch_add(1, butil::memory_order_release); \
+ return p; \
} \
- ++_cur_block->nitem; \
- return p; \
} \
Review Comment:
`nitem` is documented as being written only by the owning `LocalPool`
(single-writer), so using `fetch_add` introduces an unnecessary
read-modify-write atomic operation on the hot path. Consider replacing
`fetch_add(1, memory_order_release)` with `store(nitem + 1,
memory_order_release)` using the already-loaded `nitem` value; this preserves
the release publication semantics while typically being cheaper than an RMW.
--
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]
