Shared State#5
Conversation
| // We now transition into the busy wait state. | ||
| task_found = false; | ||
| num_busy_waiters.fetch_add(1, std::memory_order_acq_rel); | ||
| state->numBusyWaiters().fetch_add(1, std::memory_order_acq_rel); |
There was a problem hiding this comment.
would it be useful to cache a reference to the result of numBusyWaiters() so that we don't have to access their values through the reference every time they are called? or is the compiler smart enough to optimize that away?
(same goes for all the other items within the state object)
hadamyan
left a comment
hadamyan
left a comment
There was a problem hiding this comment.
Looks good with a few minor comments.
| m_failed_wakeup_retry_cap = rhs.m_failed_wakeup_retry_cap; | ||
| m_next_worker = rhs.m_next_worker.load(); | ||
| m_num_busy_waiters = rhs.m_num_busy_waiters.load(); | ||
| m_rouser = rhs.m_rouser; |
There was a problem hiding this comment.
Instead of copy assigning and then setting rhs to nullptr you have to just move it:
m_rouser = std::move(rhs.m_rouser);
This will automatically set the rhs.m_rouser to nullptr
| m_num_busy_waiters = rhs.m_num_busy_waiters.load(); | ||
| m_rouser = rhs.m_rouser; | ||
| rhs.m_rouser = nullptr; | ||
| m_state = rhs.m_state; |
There was a problem hiding this comment.
The same here, just move the smart pointer.
| */ | ||
| template <typename Task, template<typename> class Queue> | ||
| void start(std::vector<std::unique_ptr<Worker<Task, Queue>>>& workers, SlottedBag<Queue>& idle_workers, std::atomic<size_t>& num_busy_waiters); | ||
| void start(std::shared_ptr<ThreadPoolState<Task, Queue>> state); |
| std::atomic<size_t> m_num_busy_waiters; | ||
| std::shared_ptr<Rouser> m_rouser; | ||
| std::shared_ptr<ThreadPoolState<Task, Queue>> m_state; | ||
| std::atomic<bool> m_moved; |
There was a problem hiding this comment.
I think you don't need an additional flag m_moved because:
- Construction/assignment/destruction should be always synchronized and no other member function should be called asynchronously during construction/destruction.
- After the move m_state will be null and you can use that to check and throw if a member function is called on a moved thread pool.
There was a problem hiding this comment.
can we guarantee atomicity and memory ordering if we rely on the shared pointer being null after move to check whether or not we can make a new post?
| throw std::runtime_error("Cannot start Worker: it has previously been started or stopped."); | ||
|
|
||
| m_thread = std::thread(&Worker<Task, Queue>::threadFunc, this, id, std::ref(workers), std::ref(idle_workers), std::ref(num_busy_waiters)); | ||
| m_thread = std::thread(&Worker<Task, Queue>::threadFunc, this, id, std::move(state)); |
There was a problem hiding this comment.
No matter you use move or not, the outcome is the same here because the function argument "state" shared_ptr has another copy outside. And since you are not moving any ownership here it would be more readable if you just copy without a move. Currently when you read the code it seems that you are passing the ownership of the state object to the newly created thread, which is not the case.
There was a problem hiding this comment.
I thought moving the shared pointer saves the copy construction of the instance passed to std::thread, which saves one atomic ref counter increment (and subsequent decrement)?
|
All review comments addressed! |
5 participants
Hayk suggested combining the thread pool's shared state in order to clean up call signatures, and to allow the thread pool to be moved safely.