You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Michael Park <mp...@apache.org> on 2017/08/02 02:28:17 UTC
Re: Review Request 60003: Reduced copying in defer,
dispatch and Future.
> On July 28, 2017, 1:42 p.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/dispatch.hpp
> > Lines 193-197 (original), 201-225 (patched)
> > <https://reviews.apache.org/r/60003/diff/2/?file=1777463#file1777463line201>
> >
> > Could you explain how this is any different?
> >
> > It looks to me like the arguemnts are captured by-value in `Dispatcher`, which is what happens with the lambda anyway.
> >
> > Here and below.
>
> Dmitry Zhuk wrote:
> This is an equivalent of C++14's `[a1=std::forward<A1>(a1), ...]() {...}`. `aN` is always an lvalue in this context, so lambda should copy-construct a capture. `Dispatcher` forwards `aN` parameters to member fields via `INIT_VAR_TEMPLATE`, so it could move-construct in certain cases.
Ah, you're right. Thanks for clarifying. What do you think about using `std::bind` here
rather than defining the `Dispatcher`?
For example,
```cpp
std::shared_ptr<std::function<void(ProcessBase*)>> f( \
new std::function<void(ProcessBase*)>( \
std::bind([method](ENUM(N, DECL, _), \
ProcessBase* process) { \
assert(process != nullptr); \
T* t = dynamic_cast<T*>(process); \
assert(t != nullptr); \
(t->*method)(ENUM_PARAMS(N, a)); \
}, \
ENUM(N, FORWARD, _), \
lambda::_1))); \
```
where `DECL` is `#define DECL(Z, N, DATA) typename std::decay<A ## N>::type& a ## N`
With this approach we don't need `INIT_VAR_TEMPLATE`,
`DECLARE_VAR_TEMPLATE`, and the `Dispatcher` structs.
If you like the approach, this is essentially the diff:
https://github.com/mpark/mesos/commit/61f5408e78855b1155051a88ee5f833abcb03191
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60003/#review181717
-----------------------------------------------------------
On July 31, 2017, 1:31 p.m., Dmitry Zhuk wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60003/
> -----------------------------------------------------------
>
> (Updated July 31, 2017, 1:31 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, haosdent huang, James Peach, and Michael Park.
>
>
> Bugs: MESOS-7713
> https://issues.apache.org/jira/browse/MESOS-7713
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This reduces number of copies made for each parameter in a code like this:
> future.then(defer(pid, &SomeProcess::someMethod, param1, param2));
>
> For the objects that do not support move semantics (e.g. protobuf messages), number of copies is reduced from 8-10 to 6. If move semantics is supported, then number of copies is reduced from 6-7 to 1 if parameter is passed with std::move, or 2 otherwise.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/defer.hpp 7f3369e723cb244e97930621cbba89cf7873567d
> 3rdparty/libprocess/include/process/deferred.hpp e446621be11ac51f5f91c417cc8975e363c2f715
> 3rdparty/libprocess/include/process/dispatch.hpp 3a0793888dc0df5e3ec31b06f47cd920c71e0db9
> 3rdparty/libprocess/include/process/future.hpp cce950509f58022e79bb51a6e72ea1a005b9cb50
> 3rdparty/libprocess/include/process/http.hpp f637999174d92a98208b5fc49a65f9929efb11a0
> 3rdparty/libprocess/src/tests/benchmarks.cpp 694a842e8e18d82ac551749a71764825ba7cb3a9
>
>
> Diff: https://reviews.apache.org/r/60003/diff/3/
>
>
> Testing
> -------
>
> make check
>
> Number of copies was checked by using defer to subscribe process for Future callbacks, and passing parameters that count number of copies made.
>
>
> Thanks,
>
> Dmitry Zhuk
>
>