patterncppMinor
Asynchronous object destruction service
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.hxx
``
* 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
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:
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:
Locking
Your
The bracing makes the locking intent clear and takes advantage of RAII. A similar construction can be used for
Prefer
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.
Your loop is:
That's... weird construction. Why not just:
Next,
printer helper
Behold, the wonder of pack expansion. No template specialization with
Usage of
You don't really need
Your class template definition starts with:
template
::value>,
typename = std::enable_if_t::value>
>
class destruction_service finalbut 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
thisYou 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 finaltemplate <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.