Re: Review Request 60003: Reduced copying in defer, dispatch and Future.

[Mesos Reviewbot Windows]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60003/#review182333
---



Patch looks great!

Reviews applied: [60002, 60003]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On Aug. 4, 2017, 10:40 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60003/
> ---
> 
> (Updated Aug. 4, 2017, 10:40 a.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 piece of code like this:
> 
> ```
> future.then(defer(pid, ::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 
> 245ff7742b1e2c01c42a66d35d8e4f686603244f 
> 
> 
> Diff: https://reviews.apache.org/r/60003/diff/4/
> 
> 
> 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
> 
>

Re: Review Request 60003: Reduced copying in defer, dispatch and Future.

[Dmitry Zhuk]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60003/
---

(Updated Aug. 4, 2017, 10:40 a.m.)


Review request for mesos, Benjamin Hindman, haosdent huang, James Peach, and 
Michael Park.


Changes
---

Replaced `Dispatcher` with `std::bind`.


Bugs: MESOS-7713
https://issues.apache.org/jira/browse/MESOS-7713


Repository: mesos


Description (updated)
---

This reduces number of copies made for each parameter in a code like this:
future.then(defer(pid, ::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 (updated)
-

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


Diff: https://reviews.apache.org/r/60003/diff/4/

Changes: https://reviews.apache.org/r/60003/diff/3-4/


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

Re: Review Request 60003: Reduced copying in defer, dispatch and Future.

[Michael Park]

> On July 28, 2017, 1:42 p.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/dispatch.hpp
> > Lines 193-197 (original), 201-225 (patched)
> > 
> >
> > 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), ...]() 
> {...}`. `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> f(   \
new std::function(  \
std::bind([method](ENUM(N, DECL, _),\
   ProcessBase* process) {  \
assert(process != nullptr); \
T* t = dynamic_cast(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::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, ::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
> 
>

Re: Review Request 60003: Reduced copying in defer, dispatch and Future.

[Dmitry Zhuk]

> On July 28, 2017, 8:42 p.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/dispatch.hpp
> > Lines 193-197 (original), 201-225 (patched)
> > 
> >
> > 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.

This is an equivalent of C++14's `[a1=std::forward(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.


- Dmitry


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60003/#review181717
---


On July 31, 2017, 8: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, 8: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, ::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
> 
>

Re: Review Request 60003: Reduced copying in defer, dispatch and Future.

[Dmitry Zhuk]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60003/
---

(Updated July 31, 2017, 8:31 p.m.)


Review request for mesos, Benjamin Hindman, haosdent huang, James Peach, and 
Michael Park.


Changes
---

Addressed review comments.

Added benchmark.
Before changes:
Movable elapsed: 7.763873472secs
Copyable elapsed: 8.20426283secs

After changes:
Movable elapsed: 4.749548906secs
Copyable elapsed: 6.266501858secs


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, ::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 (updated)
-

  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/

Changes: https://reviews.apache.org/r/60003/diff/2-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

Re: Review Request 60003: Reduced copying in defer, dispatch and Future.

[Michael Park]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60003/#review181717
---



Looks good overall!


3rdparty/libprocess/include/process/defer.hpp
Lines 62 (patched)


Let's just call this `FORWARD`. Ideally we'd take the `A` and `a` as 
parameters into the macro, but I don't think it's worth it since we'll be 
removing all of these macros anyway. Meanwhile, could you just leave a comment 
here mentioning the implicit use of `A` and `a` please?



3rdparty/libprocess/include/process/defer.hpp
Lines 83-85 (original), 93-97 (patched)


Could we just fix the formatting here?

Specifically, something like:

```cpp
return defer(process.self(),
 method,
 ENUM(N, FORWARD, _));
```

Here and below.



3rdparty/libprocess/include/process/deferred.hpp
Line 83 (original), 85 (patched)


We should be able to save an extra copy/move here by taking it by doing 
`F&& f_ = std::forward(f);`



3rdparty/libprocess/include/process/dispatch.hpp
Lines 193-197 (original), 201-225 (patched)


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.


- Michael Park


On July 17, 2017, 4:42 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60003/
> ---
> 
> (Updated July 17, 2017, 4:42 a.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, ::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 
> 
> 
> Diff: https://reviews.apache.org/r/60003/diff/2/
> 
> 
> 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
> 
>

Re: Review Request 60003: Reduced copying in defer, dispatch and Future.

[Mesos Reviewbot Windows]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60003/#review180672
---



Patch looks great!

Reviews applied: [60002, 60003]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On July 17, 2017, 11:42 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60003/
> ---
> 
> (Updated July 17, 2017, 11:42 a.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, ::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 
> 
> 
> Diff: https://reviews.apache.org/r/60003/diff/2/
> 
> 
> 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
> 
>

Re: Review Request 60003: Reduced copying in defer, dispatch and Future.

[Dmitry Zhuk]

> On July 4, 2017, 10:55 a.m., Ilya Pronin wrote:
> > 3rdparty/libprocess/include/process/deferred.hpp
> > Line 77 (original), 79 (patched)
> > 
> >
> > This is more a question than an issue. Do we plan to forward functor 
> > references through `_Deferred`, hence the use of `std::forward()` here 
> > instead of `std::move()`?
> > 
> > Carrying a functor reference is unsafe. `template  
> > _Deferred defer(F&&)` overload silently captures references to lvalues 
> > in `_Deferred`. But as long as `_Deferred` is immediately used for 
> > conversion to `Deferred` or `std::function` it should be fine.

This is intentional, as in this case move/copy happens only once on conversion. 
Saving `f` by value would add an extra move/copy on `_Deferred` construction.
This implementation is relatively safe, as `_Deferred` is documented as an 
intermediate type, which is not supposed to be used directly, and it's somewhat 
protected from misuse by having public methods callable on rvalues only.


> On July 4, 2017, 10:55 a.m., Ilya Pronin wrote:
> > 3rdparty/libprocess/include/process/deferred.hpp
> > Lines 83-88 (original), 85-90 (patched)
> > 
> >
> > Can we eliminate more copyings here? `f_` is captured by value by 
> > lambda and then passed as a `const` to `dispatch`. We can make the lambda 
> > `mutable` and `move()` captured `f_`. Or, for example, we could store `f` 
> > value and `pid` in a structure, that will be captured by a 
> > `std::shared_ptr`.

We can't assume that lambda is called only once here, so we must make a copy. 
Avoiding this last copy is doable, but I left it out of scope of this review.


- Dmitry


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60003/#review179373
---


On July 17, 2017, 11:42 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60003/
> ---
> 
> (Updated July 17, 2017, 11:42 a.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, ::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 
> 
> 
> Diff: https://reviews.apache.org/r/60003/diff/2/
> 
> 
> 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
> 
>

Re: Review Request 60003: Reduced copying in defer, dispatch and Future.

[Dmitry Zhuk]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60003/
---

(Updated July 17, 2017, 11:42 a.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 (updated)
---

This reduces number of copies made for each parameter in a code like this:
future.then(defer(pid, ::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 (updated)
-

  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 


Diff: https://reviews.apache.org/r/60003/diff/2/

Changes: https://reviews.apache.org/r/60003/diff/1-2/


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

Re: Review Request 60003: Reduced copying in defer, dispatch and Future.

[Ilya Pronin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60003/#review179373
---




3rdparty/libprocess/include/process/deferred.hpp
Line 77 (original), 79 (patched)


This is more a question than an issue. Do we plan to forward functor 
references through `_Deferred`, hence the use of `std::forward()` here 
instead of `std::move()`?

Carrying a functor reference is unsafe. `template  _Deferred 
defer(F&&)` overload silently captures references to lvalues in 
`_Deferred`. But as long as `_Deferred` is immediately used for conversion 
to `Deferred` or `std::function` it should be fine.



3rdparty/libprocess/include/process/deferred.hpp
Lines 83-88 (original), 85-90 (patched)


Can we eliminate more copyings here? `f_` is captured by value by lambda 
and then passed as a `const` to `dispatch`. We can make the lambda `mutable` 
and `move()` captured `f_`. Or, for example, we could store `f` value and `pid` 
in a structure, that will be captured by a `std::shared_ptr`.



3rdparty/libprocess/include/process/dispatch.hpp
Lines 203-205 (patched)


Style: `Dispatcher` internals are overindented. Here and in other places 
where it's defined.


- Ilya Pronin


On June 15, 2017, 8:32 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60003/
> ---
> 
> (Updated June 15, 2017, 8:32 a.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
> ---
> 
> Reduced copying in defer, dispatch and Future.
> 
> This reduces number of copies made for each parameter in a code like this:
> future.then(defer(pid, ::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 
> 
> 
> Diff: https://reviews.apache.org/r/60003/diff/1/
> 
> 
> 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
> 
>

Re: Review Request 60003: Reduced copying in defer, dispatch and Future.

[Mesos Reviewbot Windows]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60003/#review177625
---



Patch looks great!

Reviews applied: [60002, 60003]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 12, 2017, 2:53 p.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60003/
> ---
> 
> (Updated June 12, 2017, 2:53 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reduced copying in defer, dispatch and Future.
> 
> This reduces number of copies made for each parameter in a code like this:
> future.then(defer(pid, ::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 
> 
> 
> Diff: https://reviews.apache.org/r/60003/diff/1/
> 
> 
> 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
> 
>

Review Request 60003: Reduced copying in defer, dispatch and Future.

[Dmitry Zhuk]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60003/
---

Review request for mesos, haosdent huang and Michael Park.


Repository: mesos


Description
---

Reduced copying in defer, dispatch and Future.

This reduces number of copies made for each parameter in a code like this:
future.then(defer(pid, ::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 


Diff: https://reviews.apache.org/r/60003/diff/1/


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