songgaoye commented on code in PR #3641:
URL: https://github.com/apache/brpc/pull/3641#discussion_r4349861707
##########
src/bthread/remote_task_queue.h:
##########
@@ -49,11 +50,16 @@ class RemoteTaskQueue {
}
bool pop(bthread_t* task) {
- if (_tasks.empty()) {
+ if (_ntask.load(butil::memory_order_relaxed) == 0) {
return false;
}
_mutex.lock();
const bool result = _tasks.pop(task);
+ if (result) {
+ const size_t ntask = _ntask.load(butil::memory_order_relaxed);
+ DCHECK_GT(ntask, 0u);
+ _ntask.store(ntask - 1, butil::memory_order_relaxed);
+ }
Review Comment:
I don't think so. _ntask is intentionally a lock-free empty hint rather than
a strongly
synchronized mirror of _tasks. A stale zero may cause a spurious empty
result,
which callers tolerate and enqueueing handles via signaling. A nonzero hint
only causes pop() to acquire _mutex, where the queue is checked safely.
All _ntask updates are serialized by _mutex, so relaxed load/store cannot
lose
updates. Acquire/release would not prevent observing a stale zero: an acquire
load is only synchronized when it actually reads from the corresponding
release
store; it is still permitted to read the earlier value of zero. Thus it does
not provide the claimed stronger guarantee or eliminate this intentionally
permitted false-negative fast-path result.
--
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]
