----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/#review60425 -----------------------------------------------------------
Thanks for following up Joris!! Didn't look at the forking code since we might be able to avoid per (2): (1) Hm.. it might be easier for someone to understand how this benchmark works if we change the names of these processes to `ClientProcess` and `ServerProces`, from `PingerProcess` and `PongerProcess`, respectively. After which, let's rewrite the comments above the classes to be a bit less low-level. (2) Could we pull the client and server out into separate programs? That way, we could get two key benefits. First, we can start the server and as many clients as we like manually as well, and we can run the benchmark using http requests. That might make it a bit easier for us to collect `perf` traces of say, only the server process. Second, and less important, we might be able to leverage `Subprocess` to avoid the complicated forking logic currently in place. Let me know if I missed anything! 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment101822> Let's replace all of unique_ptr here with Owned, since we're thinking of keeping the Shared<T> and Owned<T> symmetry and using unique_ptr for low level library code. 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment101843> Can we take all of these in the http::Request rather than the constructor? That way, we can run the client/server benchmark manually as well, per my top-level comment. 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment101858> protected? 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment101859> private? 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment101842> Please defer into non-static member functions! I see two options here: (1) Create the Stopwatch within `start()` instead of as a member variable, and pass it into `finish()` via lambda::bind. That way, you could just make `finish()` a static method: ``` run(const http::Request& request) { Stopwatch watch; watch.start(); // Parse arguments out of request. // NOTE: Place .then() on the next line please. return run(arguments) .then(lambda::bind(&PingerProcess::finish, watch); } ``` (2) Even better, what about just having a `run()` which returns the Duration? ``` run(const http::Request& request) { // Parse arguments out of request. return _run(arguments) .then(lambda::bind(&PingerProcess::finish, labmda::_1)); } Future<Duration> _run(size_t totalRequests, size_t maxOutstandingRequests) { ... } http::Response finish(const Duration& duration) { return http::OK(stringify(duration)); } ``` 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment101845> Should this be inside an `else`? 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment101853> In line with my other comments above, we could place this inside `run`: ``` Future<Duration> run(...) { watch.start(); while (requests < maxOutstandingRequests) { send(server, "ping"); requests++; } return finished.future(); } void pong(const UPID& from, const string& body) { responses++; if (responses == totalRequests) { finished.set(watch.elapsed()); } if (requests < totalRequests) { send(server, "pong"); requests++; } } ``` Note that we should ensure 0 < maxOutstandingRequests <= totalRequests when we parse it! 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment101848> Ditto here, do we need the "hi"? For now I'd say let's kill it, and possibly introduce a custom message or a message size via a request parameter. 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment101855> Let's clean up the naming so that the names can be self describing, avoiding the need for all these comments, and I think we only need four of these per my comments above? ``` size_t requests; size_t responses; const size_t totalRequests; const size_t maxOutstandingRequests; ``` 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment101821> Can we rely on these to be generated? What does it mean for a zero argument constructor to be marked `explicit`? 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment101860> protected? 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment101820> Do you need to pass a tiny string here in the body? If not, let's just do: ``` send(from, "pong"); ``` Otherwise, it might be nice to add a `send` that takes a string, looks like there are only callers that want to pass a string anyway. 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment101819> Could we make `hashset<int> linkedPorts` just a `hashset<UPID> links` instead? hashset also supports `contains()` so let's use that instead of `.find()` above: ``` if (!links.contains(from)) { link(from); links.insert(from); } ``` 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment101856> Let's kill this typedef, we've never used this in all of our other pipe related code and it doesn't provide much benefit IMHO. 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment101857> Let's get some newlines in between these blocks of logic, it's a bit dense :) Would love to have MESOS-2054 to clean up this stuff! 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment101861> Can we use ABORT with strerror here? ``` Error error = ErrnoError(); ABORT(error.message.c_str()); ``` For the messaging, I'd suggest the following which is more in line with the rest of the code: ``` ErrnoError(); ErrnoError("Failed to set cloexec"); ErrnoError("Failed to set cloexec"); ``` 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment101862> ErrnoError("Failed to fork"); ABORT(error.message.c_str()); We should update ABORT to take an Error, or a string as well. =/ 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment101827> Let's remove all of the `auto` keywords in this file, they go against the style guide here in that they are not increasing readability. - Ben Mahler On Oct. 24, 2014, 8:46 p.m., Joris Van Remoortere wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27113/ > ----------------------------------------------------------- > > (Updated Oct. 24, 2014, 8:46 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 3177a8e > > Diff: https://reviews.apache.org/r/27113/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Joris Van Remoortere > >
