Mpmcqueue #19358
Mpmcqueue #19358
Conversation
|
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
|
Re-verify CLA |
guantaol
added
lang/core
release notes: yes
|
|
||
| namespace grpc_core { | ||
|
|
||
| DebugOnlyTraceFlag thread_pool(false, "thread_pool_trace"); |
There was a problem hiding this comment.
Trace name: "thread_pool"
Also, thread_pool is too generic as the name of a trace flag. Consistent with the existing naming convention, grpc_thread_pool_trace may be more appropriate.
| DebugOnlyTraceFlag thread_pool(false, "thread_pool_trace"); | ||
|
|
||
| inline void* InfLenFIFOQueue::PopFront() { | ||
| // Caller should already checked queue is not empty and has already hold the |
There was a problem hiding this comment.
checked -> check
hold -> held
| MutexLock l(&mu_); | ||
|
|
||
| Node* new_node = New<Node>(elem); | ||
| if (count_.Load(MemoryOrder::RELAXED) == 0) { |
There was a problem hiding this comment.
Why not directly calling FetchAdd() which will return the previous value? Even though the Load will be cheap here, it seems redundant to have an extra Load.
| // Creates a new MPMC Queue. The queue created will have infinite length. | ||
| InfLenFIFOQueue(); | ||
|
|
||
| // Releases all resources hold by the queue. The queue must be empty, and no |
| Node* head_to_remove = queue_head_; | ||
| queue_head_ = queue_head_->next; | ||
|
|
||
| count_.FetchSub(1, MemoryOrder::RELAXED); |
There was a problem hiding this comment.
As mentioned offline, assuming count_ is not modified outside of lock this wants to be load, subtract 1, store. FetchAdd/FetchSub are more expensive (even relaxed) than load-add-store because they guarantee atomicity.
| MutexLock l(&mu_); | ||
|
|
||
| Node* new_node = New<Node>(elem); | ||
| if (count_.FetchAdd(1, MemoryOrder::RELAXED) == 0) { |
There was a problem hiding this comment.
Ditto that this should be load+add+store
Add MPMCQ implementations and unit tests.
Modified BUILD and build.yaml for build and make.