oneTBB
oneTBB copied to clipboard
oneapi-src
Porting from tbb::task to tbb::task_group for non-const execute() methods?
In the old tbb::task API, execute() methods were not marked as const. Therefore, it could mutate state within itself. This was often just needed for the duration of the execute method. eg. for calling different non-const methods on an data member that it owned.
In the oneTBB migration docs, it suggests that we use functors instead with tbb::task_group. However, a naïve conversion does not work since the operator() of the functor is required to be const (even though there seems to be no documentation to that effect). A contrived example that fails to compile: https://godbolt.org/z/jTxoh8dqn
#include <tbb/task_group.h>
struct Functor
{
int x_ = 0;
#if 1
void operator()() { ++x_; } // this fails
#else
void operator()() const { } // this works
#endif
};
int main()
{
tbb::task_group tg;
tg.run_and_wait(Functor{});
return 0;
}
Hi @e4lam,
This interface was designed in such a way because the task_group instance can copy the callable when you pass it as an argument and in this case, you will not see expected effects during the operator() execution. The recommended solution is to store the modifiable state in some shared object (e.g. std::shared_ptr) or make it mutable.
The callable will see the side effects within itself because it's operating on a class data member. Consider something like this except far more convoluted because it's been evolved TBB usage for over a decade:
class LegacyObject {
int x_ = 0;
public:
void foo() { ++x_; }
void bar() {
if (x_ % 2 == 0)
do_something();
else
do_something_else();
}
};
struct Functor
{
LegacyObject obj_;
void operator()() {
obj_.foo();
obj_.bar();
}
};
int doAlgorithm() {
tbb::task_group tg;
tg.run_and_wait(Functor{});
return 0;
}
It's not that doAlgorithm() cares about the state of Functor::obj_, Functor::operator() is the one that actually cares.
It seems that some time ago (~5 years ago) we considered it is a bug that non-const functor can be used: https://github.com/oneapi-src/oneTBB/blob/tbb_2020/CHANGES#L930-L932
Usage of mutable functors with task_group::run_and_wait() and task_arena::enqueue() is disabled. An attempt to pass a functor which operator()() is not const will produce compilation errors.
I believe that it was done for unification with task_group::run and to prevent the issue mentioned above. I will try to dig into the past to understand if there were some other reasons that motivated us to change the behavior.
Just some random thoughts:
From C++ perspective it seems slightly unusual that a temporary object is passed (Functor{}) but non-const methods are expected to be called. I suppose you thought about something like this:
Functor f;
tg.run_and_wait(f);
Additionally, in this simple example, we can workaround the issue with stack object. However, it is not a solution for general case...
tg.run_and_wait([]{
LegacyObject obj_;
obj_.foo();
obj_.bar();
});
Is there any news about this? I fail to see a valid reason to disallow non-const operator()'s for callables that are only called once, anyway. On the contrary, I would even suggest they should be called via std::move(m_func)() so that the operator() can even have a && qualifier.
The current state of affairs basically precludes using move semantics with tbb, and forces the user into useless shared_ptr's. That the user may inadvertently copy the functor instead of moving it is IMO not nearly good enough reason to prevent a ton of valid use-cases...
I'd like to be able to do things like:
auto async_foo(task_group& tg, Foo item) -> std::future<Bar> {
auto promise = std::promise<Bar>();
auto future = promise.get_future();
tg.run([item = std::move(item), promise = std::move(promise)]() mutable {
promise.set_value(foo(std::move(item)));
});
return future;
}
Please consider removing this restriction.
I am adding onto this.
I understand there are some instances where the task needs to be copied to be executed on multiple threads, there is may be reasonable to force const. But in cases where I only want to enqueue a task to a task_arena, I expect the function to be moved around and executed once, therefore there should be no need to require const as I understand.
Why aren't there 2 kinds of tasks? That would make it also more transparent about the intended usage of the functions (single vs multiple execution).
Also note that having const data member of a struct really limits what you can do with the object. It prohibits move-assignment (but you can still use placement-new / std::construct_at). More importantly, it makes const_cast-ing such objects undefined behaviour. Where const_cast-ing a const reference to a non-const object is not.
As the original submitter of this issue, yes, this is very much still relevant. Can we please stop having these bots touch every old issue every few months? We need real devs reading these issues to actually understand the deficiencies of the library.