scheduler: Support moved tast captures / arguments #216
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hello, is there any update for this? |
Replace the internal use of `std::function` with `std::packaged_task`. `std::function` requires that the wrapped function is CopyConstructable, where as a `std::packaged_task` does not. This allows the tasks to hold `std::move`'d values. This is an API / ABI breaking change, but I believe few people would be copying `marl::Task`s.
ben-clayton
force-pushed
the
support-moved-captures
branch
from
6058c63 to
a2ca890
Compare
I have another solution. #include <iostream>
#include <type_traits>
#include <functional>
#include <memory>
#include <utility>
#include <cassert>
template <typename Callable>
void task_proxy(void* vp) {
auto invoker_ptr(static_cast<Callable*>(vp));
if (invoker_ptr) {
(*invoker_ptr)();
}
}
class task {
public:
template <class Fn, typename = std::enable_if_t<std::is_invocable_v<Fn&&>>>
task(Fn&& func) {
using Wrapper = typename std::decay_t<Fn>;
auto __vp = new Wrapper(std::forward<Fn>(func));
proxy = &task_proxy<Wrapper>;
vp.reset(__vp);
}
void operator()() const { proxy(vp.get()); }
explicit operator bool() { return proxy.operator bool(); }
private:
std::function<void(void*)> proxy;
std::shared_ptr<void> vp;
};
void print() {
std::cout << "Hello World\n";
}
int main() {
const char* s = "Hello World";
auto uptr = std::make_unique<int>(2);
// Test lambda
task t([u = std::move(uptr), s]() {
assert(*u = 2);
std::cout << s << std::endl;
});
t();
// Test function name
task t2(print);
t2();
// Test function pointer
task t3(&print);
t3();
// Test member method
struct A {
void operator()() {}
};
A a;
task t4(a);
t4();
// Test bind
task t5(std::bind(&A::operator(), a));
t5();
} |
|
MSVC doesn't like this change: I haven't had time to see if I can find a work around for this.
Thank you for the suggestion. The heap allocation is costly to do for all tasks. I guess there's nothing stopping a user from using this work-around as something they'd pass to |
|
Implement a |
amaiorano
approved these changes
Mar 27, 2023
There was a problem hiding this comment.
This looks okay to me. Not super familiar with std::packaged_task, but reading about it, it looks like there would be some size and runtime overhead as compared to std::function because it stores a std::future of the callable's return value; however, in this case, the return type is void so this probably doesn't make much difference.
|
This is a solution referring to struct Task {
// template <class Fn, typename = std::enable_if_t<std::is_invocable_v<Fn>>>
template <class Fn>
Task(Fn&& func) {
using Wrapper = typename std::decay<Fn>::type;
init<Wrapper>(std::forward<Fn>(func));
invoker = &Invoke<Wrapper>;
}
~Task() {
if (manager) {
manager(storage, nullptr);
}
}
void operator()() const {
invoker(&storage);
}
struct Storage {
void* addr() noexcept { return &bytes[0]; }
const void* addr() const noexcept { return &bytes[0]; }
struct Delegate { void (Storage::*pfm)(); Storage* obj; };
union {
void* ptr;
alignas(Delegate) alignas(void(*)())
unsigned char bytes[sizeof(Delegate)];
};
};
template<typename T>
static constexpr bool stored_locally
= sizeof(T) <= sizeof(Storage) && alignof(T) <= alignof(Storage);
// && std::is_nothrow_move_constructible_v<T>;
template <class T>
static void Manage(Storage& target, Storage* src) noexcept {
if (stored_locally<T>) {
if (src) {
T* rval = static_cast<T*>(src->addr());
::new (target.addr()) T(std::move(*rval));
rval->~T();
} else {
static_cast<T*>(target.addr())->~T();
}
} else {
if (src) {
target.ptr = src->ptr;
} else {
delete static_cast<T*>(target.ptr);
}
}
}
template <class T>
static void Invoke(Storage* vp) {
if (stored_locally<T>) {
auto invoker_ptr(static_cast<T*>(vp->addr()));
if (invoker_ptr) {
(*invoker_ptr)();
}
} else {
auto invoker_ptr(static_cast<T*>(vp->ptr));
if (invoker_ptr) {
(*invoker_ptr)();
}
}
}
template <class T, class... Args>
void init(Args&&... args) {
if (stored_locally<T>) {
new (storage.addr()) T(std::forward<Args>(args)...);
} else {
storage.ptr = new T(std::forward<Args>(args)...);
}
manager = &Manage<T>;
}
using Invoker = void(*)(Storage*);
using Manager = void(*)(Storage&, Storage*);
Invoker invoker = nullptr;
mutable Storage storage;
Manager manager = nullptr;
}; |
|
The master branch did not see this commit yet. lambda still not support unique_ptr. Why is this request not submitted? |
Because the PR does not compile with MSVC, and suggested alternatives will add unacceptable overhead to each scheduled task. If someone can create a PR that adds move-support with no additional overhead, then I'd happily review it. Closing this PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replace the internal use of
std::functionwithstd::packaged_task.std::functionrequires that the wrapped function is CopyConstructable, where as astd::packaged_taskdoes not.This allows the tasks to hold
std::move'd values.This is an API / ABI breaking change, but I believe few people would be copying
marl::Tasks.Fixes: #211