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