Binding move-only objects to functions (for a thread pool implementation)

Question

I recently implemented a thread pool using std::functions. I only supported functions taking no arguments on the assumption that there would be no need to have them - one could just bind the argument to the std::function or capture it in a lambda.

I quickly realised the limitation with this approach - std::function requires bound objects to be copyable (I wanted to bind a std::unique_ptr). Binding by reference is also not an option since it's not safe in asynchronous contexts. The solution I've come up with is to support functions with arguments and store the parameters alongside the function. My implementation for storing parameters is below. I've left out the actual thread pool part and all the asynchronous bits and bobs because it's unnecessary noise.

#include <functional>
#include <utility>
#include <tuple>
#include <queue>
#include <iostream>

template <typename TupleT, std::size_t... I>
void invoke_impl(TupleT&& t, std::index_sequence<I...>)
{
    std::get<0>(t)(std::move(std::get<1 + I>(t))...);
}

template <typename... ArgTypes>
class thread_pool
{
    // First element is the function to be invoked. Remaining
    // elements are the function arguments.
    using job_t = std::tuple<
            std::function<void(ArgTypes...)>,
            ArgTypes...>;

    std::queue<job_t> _jobs;

    constexpr static auto param_index_sequence()
    {
        return std::make_index_sequence<sizeof...(ArgTypes)>();
    }

public:
    template <typename FuncT, typename ...Args>
    void post_job(FuncT&& func, Args&&... args)
    {
        _jobs.emplace(
                std::forward<FuncT>(func),
                std::forward<Args>(args)...);
    }

    // This would normally be run by the worker threads.
    void run_job()
    {
        job_t j = std::move(_jobs.front());
        _jobs.pop();
        invoke_impl(std::move(j), param_index_sequence());
    }
};

struct obj
{
    obj() { std::cout << "constructed\n"; }
    ~obj() { std::cout << "destructed\n"; }
    obj(const obj&) { std::cout << "copied\n"; }
    obj& operator=(const obj&) { std::cout << "copy-assigned\n"; return *this; }
    obj(obj&&) { std::cout << "moved\n"; }
    obj& operator=(obj&&) { std::cout << "move-assigned\n"; return *this; }
};

int main()
{
    thread_pool<obj> tp;

    // 4 moves
    tp.post_job([] (obj) {}, obj());
    tp.run_job();

    // 3 moves
    tp.post_job([] (const obj&) {}, obj());
    tp.run_job();
}

I'd especially appreciate thoughts on the following:

Is this the best approach?
Can I reduce the number of potentially-expensive moves required? Perhaps storing the arguments separately in a std::unique_ptr<std::tuple<ArgTypes...>> would be a better option...
Am I using std::move and std::forward optimally?
Can I make my templates more flexible/robust? e.g. by using std::remove_reference, std::remove_cv etc.
I've intentionally kept to C++14 (hence my using std::get<0>(j)(...) rather than std::invoke). I'd like to keep to C++14 but I'd be interested about newer features that make my implementation simpler or even completely redundant.

G. Sliepen · Accepted Answer · 2022-03-14 15:51:03Z

Is this the best approach?

While it is a nice workaround, it is not a generic solution to the problem. Ideally, you would create a C++14-compatible implementation of std::move_only_function.

Can I reduce the number of potentially-expensive moves required? Perhaps storing the arguments separately in a std::unique_ptr<std::tuple<ArgTypes...>> would be a better option...

std::unique_ptr introduces its own overhead, so it might not be a better option. You can however remove some std::moves. For example, in run_job(), you don't need to move, pop and then invoke, you can just do:

std::invoke_impl(std::move(_jobs.front()), param_index_sequence());
_jobs.pop();

Am I using std::move and std::forward optimally?

In post_job(), you declare the template parameter pack Args, but you should have used ArgTypes instead. This allows better type deduction, for example it allows you to write:

tp.post_job([] (const obj&) {}, {});

Can I make my templates more flexible/robust? e.g. by using std::remove_reference, std::remove_cv etc.

I don't think there is any reason here to use that.

I've intentionally kept to C++14 (hence my using std::get<0>(j)(...) rather than std::invoke()). I'd like to keep to C++14 but I'd be interested about newer features that make my implementation simpler or even completely redundant.

C++23's std::move_only_function will make it redundant.

If you would restructure job_t like so:

using job_t = std::pair<
    std::function<void(ArgTypes...)>,
    std::tuple<ArgTypes...>
>;

Then you can emplace jobs like so:

_jobs.emplace(
    std::forward<FuncT>(func),
    std::make_tuple<ArgTypes...>(std::forward<ArgTypes>(args)...)
);

And then the implementation fo invoke_impl() becomes much simpler; either:

t.first(std::move(std::get<I>(t.second))...);

Or using C++17's std::apply():

std::apply(t.first, std::move(t.second));

And at that point, invoke_impl() becomes redundant, and you could rewrite run_job() like so:

void run_job() {
    auto& j = _jobs.front();
    std::apply(j.first, std::move(j.second));
    _jobs.pop();
}

Thanks! Why is the the std::move in the call to std::forward_as_tuple required/desirable? Why that and not a std::forward? — jezza
– jezza, Commented Mar 14, 2022 at 12:31
Ah hm, I guess std::make_tuple<ArgTypes...>(std::forward<ArgTypes>(args)...) would be the best here. — G. Sliepen
– G. Sliepen, Commented Mar 14, 2022 at 15:50

Stack Exchange Network

Binding move-only objects to functions (for a thread pool implementation)

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Binding move-only objects to functions (for a thread pool implementation)

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions