mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Park <mp...@apache.org>
Subject Re: Review Request 60003: Reduced copying in defer, dispatch and Future.
Date Wed, 02 Aug 2017 02:28:17 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message