HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Asynchronous object destruction service

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
asynchronousobjectservicedestruction

Problem

Inspired by the recent Stack Overflow question “Safely release a resource on a different thread”, I wrote the following destruction_service. Please refer to the class template's DocString for more information (and let me know in your review if it is insufficient).

destruction_service.hxx

``
/**
* @file destruction_service.hxx
*
* @brief
* Provides the
my::destruction_service class template for
* asynchronous object destruction.
*
*/

#ifndef DESTRUCTION_SERVICE_HXX
#define DESTRUCTION_SERVICE_HXX

#include // assert
#include // std::condition_variable
#include // std::mutex, std::unique_lock
#include // std::thread
#include // std::enable_if_t, std::is_nothrow_{move_constructible,destructible}
#include // std::move
#include // std::vector

#include "syncio.hxx" // syncio::print_log

namespace my
{

/**
* @brief
* An asynchronous object destruction service.
*
* An instance of this class owns a worker thread that asynchronously
* destroys objects. This might be useful for threads that have high
* responsiveness requirements and objects with (potentially) long-running
* destructors.
*
* To-be-destroyed objects can be scheduled for destruction by means of the
*
schedule_destruction member function. It takes its argument by rvalue
* reference assuming that destructing a moved-away-from object is
* considerably cheaper. If you are using this service to destroy smart
pointers, be aware that only scheduling the last* instance of a shared
* pointer will cause the managed object to be destroyed asynchronously.
T`
* is required to have a non-throwing move constructor and its destructor
* must not throw either.
*
* If you have to destruct objects of different types, consider using a
* polymorphic wrapper to perform type erasure. The default is
* non-polymorphic, however, becau

Solution

To SFINAE or not to SFINAE

Your class template definition starts with:

template
  ::value>,
    typename = std::enable_if_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 
class destruction_service final {
    static_assert(std::is_nothrow_move_constructible::value, "!");
    static_assert(std::is_nothrow_destructible::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 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 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.

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_()

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 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();


printer helper

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

template 
void print(std::ostream& os, Items const&... items)
{
    std::lock_guard 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.

Code Snippets

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
template <typename T>
class destruction_service final {
    static_assert(std::is_nothrow_move_constructible<T>::value, "!");
    static_assert(std::is_nothrow_destructible<T>::value, "!");
    ...
~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());
}
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();
}
for (auto stop = false; true; stop = this->is_done_()) { ... }

Context

StackExchange Code Review Q#106422, answer score: 4

Revisions (0)

No revisions yet.