mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 27113: Libprocess benchmark cleanup
Date Thu, 23 Oct 2014 22:18:37 GMT

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


Thanks for following up Joris!

Initial pass, have not looked at the test body yet.
Could we call this file process_benchmarks.cpp? I'm thinking of when we want other benchmarks
(e.g. future_benchmarks.cpp).


3rdparty/libprocess/src/tests/benchmarks.cpp
<https://reviews.apache.org/r/27113/#comment99034>

    Just use lambda::function for consistency, until we migrate std::function.



3rdparty/libprocess/src/tests/benchmarks.cpp
<https://reviews.apache.org/r/27113/#comment99033>

    I thought we weren't checking for this feature yet..
    
    Looks like the first one introduced in the code base!
    
    ```
    ?  mesos git:(master) ? grep -R unique_ptr 3rdparty
    3rdparty/libprocess/3rdparty/stout/README.md:std::unique_ptr (although, likely wrapped
as Owned) in order to
    3rdparty/libprocess/include/process/c++11/dispatch.hpp:#include <memory> // TODO(benh):
Replace shared_ptr with unique_ptr.
    3rdparty/libprocess/include/process/c++11/dispatch.hpp:// will probably change in the
future to unique_ptr (or a variant).
    3rdparty/libprocess/include/process/dispatch.hpp:#include <stout/memory.hpp> //
TODO(benh): Replace shared_ptr with unique_ptr.
    3rdparty/libprocess/include/process/dispatch.hpp:// will probably change in the future
to unique_ptr (or a variant).
    3rdparty/libprocess/include/process/event.hpp:#include <stout/memory.hpp> // TODO(benh):
Replace shared_ptr with unique_ptr.
    3rdparty/libprocess/include/process/future.hpp:#include <stout/memory.hpp> // TODO(benh):
Replace shared_ptr with unique_ptr.
    3rdparty/libprocess/include/process/owned.hpp:// unique_ptr semantics. Consequently, each
usage of Owned that
    3rdparty/libprocess/include/process/run.hpp:#include <stout/memory.hpp> // TODO(benh):
Replace shared_ptr with unique_ptr.
    3rdparty/libprocess/m4/ax_cxx_compile_stdcxx_11.m4:  int foo(std::unique_ptr<int>
i)
    3rdparty/libprocess/m4/ax_cxx_compile_stdcxx_11.m4:  std::unique_ptr<int> i(new
int());
    3rdparty/libprocess/src/process.cpp:#include <stout/memory.hpp> // TODO(benh): Replace
shared_ptr with unique_ptr.
    ```



3rdparty/libprocess/src/tests/benchmarks.cpp
<https://reviews.apache.org/r/27113/#comment99036>

    We don't use this anywhere yet, can you use  <stout/hashset.hpp> or <set>
instead?
    
    ```
    ?  mesos git:(master) ? grep -R unordered_set 3rdparty
    3rdparty/libprocess/3rdparty/glog-0.3.3.patch:+# include <unordered_set>
    3rdparty/libprocess/3rdparty/glog-0.3.3.patch:+OUTPUT_FOUR_ARG_CONTAINER(std::unordered_set)
    3rdparty/libprocess/3rdparty/stout/include/stout/hashset.hpp:#include <boost/unordered_set.hpp>
    3rdparty/libprocess/3rdparty/stout/include/stout/hashset.hpp:// Provides a hash set via
Boost's 'unordered_set'. For most intensive
    3rdparty/libprocess/3rdparty/stout/include/stout/hashset.hpp:class hashset : public boost::unordered_set<Elem>
    3rdparty/libprocess/3rdparty/stout/include/stout/hashset.hpp:    return boost::unordered_set<Elem>::count(elem)
> 0;
    ```



3rdparty/libprocess/src/tests/benchmarks.cpp
<https://reviews.apache.org/r/27113/#comment99038>

    I think we need to follow the same pattern as the mesos benchmarks, that is, they are
included within the test binary, not as separate mains.



3rdparty/libprocess/src/tests/benchmarks.cpp
<https://reviews.apache.org/r/27113/#comment99046>

    Could we just have a Pinger and a Ponger, is there an advantage to conflating the client
and server in one Process? Or am I missing something here?



3rdparty/libprocess/src/tests/benchmarks.cpp
<https://reviews.apache.org/r/27113/#comment99043>

    What is the point of this link() (typically we would put this in initialize()).
    
    The other process isn't up necessarily, so the only reliable link() call needs to occur
either after receiving a message.



3rdparty/libprocess/src/tests/benchmarks.cpp
<https://reviews.apache.org/r/27113/#comment99050>

    Please dispatch into these instead of calling directly!



3rdparty/libprocess/src/tests/benchmarks.cpp
<https://reviews.apache.org/r/27113/#comment99049>

    Whoa! Why did you need a latch? Please use dispatch instead, and return a Future if you
need the caller to block on it. In general we avoid calling directly into a process like.



3rdparty/libprocess/src/tests/benchmarks.cpp
<https://reviews.apache.org/r/27113/#comment99053>

    A comment somewhere would be great, it's hard to tell what is happening with these without
some guidance :)



3rdparty/libprocess/src/tests/benchmarks.cpp
<https://reviews.apache.org/r/27113/#comment99047>

    Why is this a set, don't you only need to know whether you're linked? (i.e. boolean).
    
    This is just 1:1 client:server in terms of messaging, right? It's a bit confusing as to
why there is 1 'other', but many 'linkedPorts'..



3rdparty/libprocess/src/tests/benchmarks.cpp
<https://reviews.apache.org/r/27113/#comment99055>

    Why not just 'remaining', or even more straightforward and consistent, just count upwards
with 'i' :)



3rdparty/libprocess/src/tests/benchmarks.cpp
<https://reviews.apache.org/r/27113/#comment99054>

    I guess the style checker is not catching these `if(` constructs without spaces.


- Ben Mahler


On Oct. 23, 2014, 9:54 p.m., Joris Van Remoortere wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27113/
> -----------------------------------------------------------
> 
> (Updated Oct. 23, 2014, 9:54 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Niklas Nielsen.
> 
> 
> Bugs: MESOS-1980
>     https://issues.apache.org/jira/browse/MESOS-1980
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Add a comment to BenchmarkProcess.
> Remove setLink() as it is not strictly necessary.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/tests/benchmarks.cpp 79a650b 
> 
> Diff: https://reviews.apache.org/r/27113/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>


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