scheduler: Support moved tast captures / arguments #247
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
ben-clayton
added
the
kokoro:run
kokoro-team
removed
the
kokoro:run
4kangjc
force-pushed
the
main
branch
4 times, most recently
from
bb9c4d0
to
37d4a54
Compare
ben-clayton
added
the
kokoro:run
kokoro-team
removed
the
kokoro:run
| @@ -62,28 +56,15 @@ class Task { | |||
| }; | |||
|
|
|||
| Task::Task() {} | |||
| Task::Task(const Task& o) : function(o.function), flags(o.flags) {} | |||
There was a problem hiding this comment.
Removal of task copy constructors is a breaking API change, which I've tried hard to avoid.
The following used to compile, but no longer does:
TEST_P(WithBoundScheduler, ScheduleWithCopy) {
auto fn = []() {};
auto task = marl::Task(fn);
marl::schedule(task);
marl::schedule(task);
}
It looks like it should be feasible to make the new function class support copying.
| } | ||
|
|
||
| template <class T> | ||
| const TypeOps* EraseCopySmall(void* buffer, T&& obejct) { |
| struct TypeOps { | ||
| using Invoker = R (*)(void* object, Args&&... args); | ||
| using Manager = void (*)(void* dest, void* src); | ||
|
|
||
| Invoker invoker; | ||
| Manager manager; | ||
| }; |
There was a problem hiding this comment.
Manager doesn't describe what it does. AFAICT, this is a move or destruct, depending on whether src is nullptr.
I'd recommend splitting it into two methods:
struct TypeOps {
using Invoke = R (*)(void* object, Args&&... args);
using Destruct = void (*)(void* object);
using Move = void (*)(void* dest, void* src);
Invoke invoke;
Destruct destruct;
Move move;
};
| return &ops; | ||
| } | ||
|
|
||
| mutable marl::aligned_storage<kMaximumOptimizableSize, 1>::type object_; |
There was a problem hiding this comment.
need typename before marl::
| #endif // __cplusplus > 201402L | ||
|
|
||
| template <class R> | ||
| class move_only_function; |
There was a problem hiding this comment.
See my comment about removal of copy constructors. If we can make this support copying as well, then we might as well just call this marl::Function.
| } | ||
|
|
||
| move_only_function& operator=(std::nullptr_t) { | ||
| ops_->manager(&object_, nullptr); |
There was a problem hiding this comment.
need to check ops_ is nullptr.
|
|
||
| template <class T> | ||
| const TypeOps* EraseCopySmall(void* buffer, T&& obejct) { | ||
| using Decayed = typename std::decay<T>::type; |
There was a problem hiding this comment.
Suggest renaming Decayed to Functor.
| template <class T> | ||
| const TypeOps* EraseCopyLarge(void* buffer, T&& object) { | ||
| using Decayed = typename std::decay<T>::type; | ||
| using Stored = Decayed*; |
There was a problem hiding this comment.
Suggest renaming Stored to FunctorPtr
| return &ops; | ||
| } | ||
|
|
||
| mutable marl::aligned_storage<kMaximumOptimizableSize, 1>::type object_; |
There was a problem hiding this comment.
I'd use an alignment of at least 16.
| !std::is_same<typename std::decay<F>::type, | ||
| move_only_function>::value>::type> | ||
| move_only_function(F&& function) { | ||
| if (sizeof(typename std::decay<F>::type) <= kMaximumOptimizableSize) { |
There was a problem hiding this comment.
You'll need to check that the alignment is compatible for std::decay<F>::type too.
Closes #211
c++ 23 move_only_function
#216