Skip to main content
Code Review

Return to Answer

added 87 characters in body
Source Link
Barry
Barry
  • 18.6k
  • 1
  • 41
  • 92
while (!this->is_done())
{
    {
        std::unique_lock<std::mutex> lk(this->mutex_); // now we need unique
        this->condvar_.wait(lk, [this]{
            // waiting for one of these things to be true
            return !this->queue_.empty() || this->done_; 
        });
        this->queue_.swap(things);
    }
    // and now we're unlocked
    things.clear();
} 

// we're done, so let's clean up. no lock necessary
this->queue_.clear(); 

while (!this->is_done())
{
    {
        std::unique_lock<std::mutex> lk(this->mutex_); // now we need unique
        this->condvar_.wait(lk, [this]{
            // waiting for one of these things to be true
            return !this->queue_.empty() || this->done_; 
        });
        this->queue_.swap(things);
    }
    // and now we're unlocked
    things.clear();
}

while (!this->is_done())
{
    {
        std::unique_lock<std::mutex> lk(this->mutex_); // now we need unique
        this->condvar_.wait(lk, [this]{
            // waiting for one of these things to be true
            return !this->queue_.empty() || this->done_; 
        });
        this->queue_.swap(things);
    }
    // and now we're unlocked
    things.clear();
} 

// we're done, so let's clean up. no lock necessary
this->queue_.clear();
added 166 characters in body
Source Link
Barry
Barry
  • 18.6k
  • 1
  • 41
  • 92

Lock-Free Queue

If this is an optimization, I'd consider using a lock-free queue. You may find that most of your time comes from lock acquisition/release.

do_work_()

do_work_()

Lock-Free Queue

If this is an optimization, I'd consider using a lock-free queue. You may find that most of your time comes from lock acquisition/release.

do_work_()

Source Link
Barry
Barry
  • 18.6k
  • 1
  • 41
  • 92

To SFINAE or not to SFINAE

Your class template definition starts with:

template
  <
    typename T,
    typename = std::enable_if_t<std::is_nothrow_move_constructible<T>::value>,
    typename = std::enable_if_t<std::is_nothrow_destructible<T>::value>
  >
  class destruction_service final

but SFINAE is basically intended to remove candidates from an overload set. That's not really an issue here, so this seems unnecessarily verbose. What you want in this case is just the simple static asserts:

template <typename T>
class destruction_service final {
    static_assert(std::is_nothrow_move_constructible<T>::value, "!");
    static_assert(std::is_nothrow_destructible<T>::value, "!");
    ...

Locking

Your lock_queue_ is weird and unnecessary. Just construct a lock object, it'll be cleaner. Also if you find yourself writing unlock(), you're probably doing something wrong. In the case of your destructor, I would rewrite it as:

~destruction_service() noexcept
{
    syncio::print_log(__PRETTY_FUNCTION__);
    {
        std::lock_guard<std::mutex> lk(this->mutex_);
        this->done_ = true;
    }
    this->condvar_.notify_all();
    if (this->worker_.joinable())
        this->worker_.join();
    assert(this->queue_.empty());
}

The bracing makes the locking intent clear and takes advantage of RAII. A similar construction can be used for schedule_destruction:

void schedule_destruction(object_type&& object)
{
    {
        std::lock_guard<std::mutex> lk(this->mutex_);
        this->queue_.push_back(std::move(object));
    }
    this->condvar_.notify_all();
}

Prefer lock_guard to unique_lock unless you really need a unique_lock.

do_work_()

Your loop is:

for (auto stop = false; true; stop = this->is_done_()) { ... }

That's... weird construction. Why not just:

while (!this->is_done_()) { ... }

Next, std::condition_variable::wait has an overload that takes a predicate. This clears intent a bit:

while (!this->is_done())
{
    {
        std::unique_lock<std::mutex> lk(this->mutex_); // now we need unique
        this->condvar_.wait(lk, [this]{
            // waiting for one of these things to be true
            return !this->queue_.empty() || this->done_; 
        });
        this->queue_.swap(things);
    }
    // and now we're unlocked
    things.clear();
}

printer helper

Behold, the wonder of pack expansion. No template specialization with printer_helper_ necessary:

template <typename... Items>
void print(std::ostream& os, Items const&... items)
{
    std::lock_guard<std::mutex> guard{...};

    using expander = int[];
    (void)expander{0,
        (void(os << items)
        , 0)...
    };
    os << std::endl;
}

Usage of this

You don't really need this in just about all of the places you use it.