3
\$\begingroup\$

This question follows up on this question.

After turning the while-loop into a conditional wait using std::condition_variable, I would like to know if there are still any flaws/problems with this code which implements the std::latch class from C++20 in C++11:

#include <condition_variable>
#include <cstdint>
#include <atomic>
#include <mutex>

class latch {
    std::atomic<std::ptrdiff_t> counter_;
    mutable std::condition_variable cv_;
    mutable std::mutex mut_;

public:
    explicit latch(std::ptrdiff_t const def = 1) : counter_(def) {}

    void count_down(std::ptrdiff_t const n = 1) {
        counter_ -= n;
        cv_.notify_all();
    }

    void wait() const {
        if (counter_.load(std::memory_order_relaxed) == 0) return;
        std::unique_lock<std::mutex> lock(mut_);
        cv_.wait(lock, [=] { return counter_.load(std::memory_order_relaxed) == 0; });
    }

    bool try_wait() const noexcept {
        return counter_.load(std::memory_order_relaxed) == 0;
    }

    void arrive_and_wait(std::ptrdiff_t const n = 1) {
        count_down(n);
        wait();
    }

    static constexpr std::ptrdiff_t max() noexcept {
        return std::numeric_limits<std::ptrdiff_t>::max();
    }
};
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

Some minor nitpicks about the constructor:

  • It should be constexpr.

  • It shouldn't have a default argument.

  • We should check that the argument is valid (not negative, and not greater than max()). The standard says it's undefined behavior otherwise, so we don't technically have to, but it's nicer to assert than to silently carry on.


Another thing:

    counter_ -= n;
    cv_.notify_all();

operator-=() here is doing counter.fetch_sub(n, std::memory_order_seq_cst);.

While this memory order will be safe, I'm not sure it's necessary. I suspect you could get away with std::memory_order_release (note: I'm not very familiar with atomic<>s, so you might want a second opinion).

In any case, using an explicit fetch_sub call is clearer, and also allows us to add an assertion that the previous value of counter was greater than or equal to n.

\$\endgroup\$
1
\$\begingroup\$

The biggest offender here in terms of performance here is

void count_down(std::ptrdiff_t const n = 1) {
    counter_ -= n;
    cv_.notify_all();
}

While the std::memory_order_seq_cst is surely too strong here, as std::memory_order_release should be sufficient, this is but a minor issue compared to the cv_.notify_all() that you trigger all the time completely unnecessarily. Think what it does: it awakens all the threads that wait for counter to reach 0 only finding out that the variable is not zero. Awakening of each thread costs thousands of cycles and you do it on all waiting thread on each count down. Then putting it to sleep also costs thousands of cycles. It is a total waste.

Instead, you should trigger the notify_all() only when the counter reached 0.

Next: memory order bugs with wait() and try_wait(). Think what happens when counter reached 0. You've only triggered relaxed memory order instructions. It means that whatever changes occurred on other threads on other variables they aren't necessarily synchronized with the currently running thread. You ought to trigger memory order acquire synchronization with the atomic variable when that happens. Also make sure that it is triggered on the atomic variable regardless of the fact that mutex applies memory synchronization routines. They aren't necessarily the same routines or matching routines considering that you try to utilize efficiency of the std::atomic for lightweight operations. Mixing atomics with mutex is always problematic.

arrive_and_wait routine. You can mildly optimize it for the case when the counter reaches zero on the current thread. Just make sure you apply proper memory synchronization routines - both release and acquire on the atomic.

It would be much simpler and straightforward if you were to implement all of this with mutex and non-atomic variable it guards. But it would cause count_down and try_wait to be less efficient due to unnecessary calls to acquire atomic memory instructions.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.