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 Fri, 28 Jul 2017 20:42:24 GMT

-----------------------------------------------------------
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)
<https://reviews.apache.org/r/60003/#comment257367>

    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)
<https://reviews.apache.org/r/60003/#comment257368>

    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)
<https://reviews.apache.org/r/60003/#comment257371>

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



3rdparty/libprocess/include/process/dispatch.hpp
Lines 193-197 (original), 201-225 (patched)
<https://reviews.apache.org/r/60003/#comment257370>

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

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


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