Re: Review Request 27113: Libprocess benchmark cleanup
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/ --- (Updated April 17, 2015, 5:28 p.m.) Review request for mesos, Ben Mahler, Cody Maloney, Joerg Schad, and Michael Park. Changes --- Address bmahler's comments. Rebased on 33315 which allows us to disable short-circuit passing of messages between actors. This means we can get rid of the forking code path, which simplifies the test significantly. Bugs: MESOS-1980 https://issues.apache.org/jira/browse/MESOS-1980 Repository: mesos Description --- Clean up Libprocess for readability / extensibility. Diffs (updated) - 3rdparty/libprocess/src/tests/benchmarks.cpp a927e4ecd8c8955cd9f85e716173a73a9a21c6cd Diff: https://reviews.apache.org/r/27113/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 27113: Libprocess benchmark cleanup
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/#review80495 --- Patch looks great! Reviews applied: [33315, 27113] All tests passed. - Mesos ReviewBot On April 17, 2015, 5:28 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/ --- (Updated April 17, 2015, 5:28 p.m.) Review request for mesos, Ben Mahler, Cody Maloney, Joerg Schad, and Michael Park. Bugs: MESOS-1980 https://issues.apache.org/jira/browse/MESOS-1980 Repository: mesos Description --- Clean up Libprocess for readability / extensibility. Diffs - 3rdparty/libprocess/src/tests/benchmarks.cpp a927e4ecd8c8955cd9f85e716173a73a9a21c6cd Diff: https://reviews.apache.org/r/27113/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 27113: Libprocess benchmark cleanup
On April 14, 2015, 9:28 p.m., Ben Mahler wrote: 3rdparty/libprocess/src/tests/benchmarks.cpp, lines 108-114 https://reviews.apache.org/r/27113/diff/4/?file=925777#file925777line108 This seems to suggest support for multiple runs of the client. However, if a /run request arrives while the previous run is in progress, it looks like this will behave strangely. Did you want to deny requests while a run is in progress, or chain the runs after one another? Caught this state and return an error message for now. On April 14, 2015, 9:28 p.m., Ben Mahler wrote: 3rdparty/libprocess/src/tests/benchmarks.cpp, line 266 https://reviews.apache.org/r/27113/diff/4/?file=925777#file925777line266 Could you do a self review of this test body, from the perspective of someone approaching this code for the first time? I had a hard time understanding this, at a basic level what is the overall structure of the forking here? It's also a bit tricky to figure out the lower level details, like what the `requestPipes` and `resultPipes` are for, and why they're needed. What the vectors of pipes are used for, why they're needed. Etc. In the process of being able to articulate what this does, we might figure out how we can make it more easily understandable :) Take a look at the updated review. I'll let you mark this as fixed if you approve :-) - Joris --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/#review80101 --- On April 17, 2015, 5:28 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/ --- (Updated April 17, 2015, 5:28 p.m.) Review request for mesos, Ben Mahler, Cody Maloney, Joerg Schad, and Michael Park. Bugs: MESOS-1980 https://issues.apache.org/jira/browse/MESOS-1980 Repository: mesos Description --- Clean up Libprocess for readability / extensibility. Diffs - 3rdparty/libprocess/src/tests/benchmarks.cpp a927e4ecd8c8955cd9f85e716173a73a9a21c6cd Diff: https://reviews.apache.org/r/27113/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 27113: Libprocess benchmark cleanup
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/#review80540 --- Ship it! Wow, nice work, this is a lot cleaner! I left some comments but since they are pretty minor, I've gone ahead and taken care of them to get this committed for you. For the dependent change of disabling local messages, I've left it as a TODO here so we can get these great cleanups in. Let's follow up with your dependent patch to add the ability for the tests to run both the local and remote code paths, sound good? :) 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130567 Looks like we're missing some includes here? 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130574 Weird wrapping here? You could make this comment more succinct here if you omit the request parameters. We probably don't need a /help for now. 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130572 Maybe a newline in between these? 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130563 It seems a bit strange to have a `running` that isn't set to false? 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130570 Since we're capturing the duration for the run, how about just calling it 'duration'? Finished makes it clear that it's a signal for finishing but that seems to express how it's used as opposed to what it is. 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130575 It's probably time we give this a more meaningful name than Test. :) 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130561 No need for `` here. 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130568 How about: ``` // Start the ping / pongs! const string query = strings::join( , server= + stringify(serverPid), requests= + stringify(numRequests), concurrency= + stringify(concurrency), messageSize= + stringify(messageSize)); ``` 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130559 Can we use std::array in the future? 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130543 Hm.. we call this 'requests' but we store 'Responses'? 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130548 It seems like the iterate and wait ... aspect of this comment re-iterates what the code says? 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130556 A 'request' is actually a 'Response'? 3rdparty/libprocess/src/tests/benchmarks.cpp https://reviews.apache.org/r/27113/#comment130553 Printing the total rpc throughput like this seems prone to error. For example, if each client ran __serially__ at throughput X, we would print X*N without realizing that each client's throughput was not overlapping. Could we just print the throughput for each individual client for now? For server throughput, we can track that more precisely in the server itself, yes? For now, I'd propose just tracking the total time in the test and estimating total throughput based on that. Also, why not use cout like we did elsewhere? - Ben Mahler On April 17, 2015, 5:28 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/ --- (Updated April 17, 2015, 5:28 p.m.) Review request for mesos, Ben Mahler, Cody Maloney, Joerg Schad, and Michael Park. Bugs: MESOS-1980 https://issues.apache.org/jira/browse/MESOS-1980 Repository: mesos Description --- Clean up Libprocess for readability / extensibility. Diffs - 3rdparty/libprocess/src/tests/benchmarks.cpp a927e4ecd8c8955cd9f85e716173a73a9a21c6cd Diff: https://reviews.apache.org/r/27113/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 27113: Libprocess benchmark cleanup
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/ --- (Updated April 14, 2015, 7:46 p.m.) Review request for mesos, Ben Mahler, Cody Maloney, Joerg Schad, and Michael Park. Changes --- minor fixes for Joerg. Bugs: MESOS-1980 https://issues.apache.org/jira/browse/MESOS-1980 Repository: mesos Description --- Clean up Libprocess for readability / extensibility. Diffs (updated) - 3rdparty/libprocess/src/tests/benchmarks.cpp a927e4ecd8c8955cd9f85e716173a73a9a21c6cd Diff: https://reviews.apache.org/r/27113/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 27113: Libprocess benchmark cleanup
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/#review80103 --- Patch looks great! Reviews applied: [25448, 27113] All tests passed. - Mesos ReviewBot On April 14, 2015, 7:46 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/ --- (Updated April 14, 2015, 7:46 p.m.) Review request for mesos, Ben Mahler, Cody Maloney, Joerg Schad, and Michael Park. Bugs: MESOS-1980 https://issues.apache.org/jira/browse/MESOS-1980 Repository: mesos Description --- Clean up Libprocess for readability / extensibility. Diffs - 3rdparty/libprocess/src/tests/benchmarks.cpp a927e4ecd8c8955cd9f85e716173a73a9a21c6cd Diff: https://reviews.apache.org/r/27113/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 27113: Libprocess benchmark cleanup
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27113/#review80101
---
Wow, the client and server parts of this are looking a lot clearer, thanks
Joris!
Let's focus on the test body next, it was difficult for me to grasp it fully, I
left a comment below :)
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/27113/#comment129861
How about 'concurrency' or 'concurrentRequests' instead of 'queueDepth'?
Which queue is this variable referring to?
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/27113/#comment129863
This seems to suggest support for multiple runs of the client. However,
if a /run request arrives while the previous run is in progress, it looks like
this will behave strangely. Did you want to deny requests while a run is in
progress, or chain the runs after one another?
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/27113/#comment129864
Why do totalRequests and the concurrency factor need to be reset here?
Looks like these are required.
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/27113/#comment129871
How about:
```
hashmapstring, Optionstring parameters {
{server, request.query.get(server)},
{messageSize, request.query.get(messageSize)},
{totalRequests, request.query.get(totalRequests)},
{concurrentRequests, request.query.get(concurrentRequests)},
};
// Ensure all parameters were provided.
foreachpair (const string parameter, const Optionstring value,
parameters) {
if (value.isNone()) {
return http::BadRequest(Missing ' + parameter + ' parameter);
}
}
server = UPID(parameters[server].get());
TryBytes bytes = Bytes::parse(parameters[messageSize].get());
if (bytes.isError()) {
return http::BadRequest(Invalid 'messageSize': + bytes.error());
}
messageSize = bytes.get();
Trysize_t numify = numifysize_t(parameters[totalRequests].get());
if (numify.isError()) {
return http::BadRequest(Invalid 'totalRequests': + numify.error());
}
totalRequests = numify.get();
numify = numifysize_t(parameters[concurrentRequests].get());
if (numify.isError()) {
return http::BadRequest(Invalid 'concurrentRequests': +
numify.error());
}
concurrentRequests = numify.get();
```
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/27113/#comment129865
How about using a `Bytes` for this?
```
TryBytes messageSize = Bytes::parse(value.get());
if (messageSize.isError()) {
}
message = string(messageSize.get().bytes(), '1');
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/27113/#comment129866
Can you include the error message from the Try here and elsewhere?
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/27113/#comment129873
Why do you need to reject this? If the number of concurrent requests is
larger than the total number of requests, that seems well defined
(concurrentRequests == totalRequests). Any reason not to just do the right
thing for what was requested?
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/27113/#comment129875
'concurrency' or 'concurrentRequests' might make this clearer
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/27113/#comment129877
Comment seems unnecessary?
3rdparty/libprocess/src/tests/benchmarks.cpp
https://reviews.apache.org/r/27113/#comment129881
Could you do a self review of this test body, from the perspective of
someone approaching this code for the first time?
I had a hard time understanding this, at a basic level what is the overall
structure of the forking here? It's also a bit tricky to figure out the lower
level details, like what the `requestPipes` and `resultPipes` are for, and why
they're needed. What the vectors of pipes are used for, why they're needed. Etc.
In the process of being able to articulate what this does, we might figure
out how we can make it more easily understandable :)
- Ben Mahler
On April 14, 2015, 7:46 p.m., Joris Van Remoortere wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27113/
---
(Updated April 14, 2015, 7:46 p.m.)
Review request for mesos, Ben Mahler, Cody Maloney, Joerg Schad, and Michael
Park.
Bugs: MESOS-1980
https://issues.apache.org/jira/browse/MESOS-1980
Repository: mesos
Description
---
Clean up Libprocess for readability / extensibility.
Re: Review Request 27113: Libprocess benchmark cleanup
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/ --- (Updated April 13, 2015, 5:43 p.m.) Review request for Cody Maloney, Joerg Schad and Michael Park. Changes --- addressed Cody's comments. Bugs: MESOS-1980 https://issues.apache.org/jira/browse/MESOS-1980 Repository: mesos Description --- Clean up Libprocess for readability / extensibility. Diffs (updated) - 3rdparty/libprocess/src/tests/benchmarks.cpp a927e4ecd8c8955cd9f85e716173a73a9a21c6cd Diff: https://reviews.apache.org/r/27113/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 27113: Libprocess benchmark cleanup
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/ --- (Updated April 10, 2015, 8:59 p.m.) Review request for Cody Maloney, Joerg Schad and Michael Park. Changes --- Cleaning up per BenM's review. Bugs: MESOS-1980 https://issues.apache.org/jira/browse/MESOS-1980 Repository: mesos Description (updated) --- Clean up Libprocess for readability / extensibility. Diffs (updated) - 3rdparty/libprocess/src/tests/benchmarks.cpp a927e4ecd8c8955cd9f85e716173a73a9a21c6cd Diff: https://reviews.apache.org/r/27113/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 27113: Libprocess benchmark cleanup
On Nov. 8, 2014, 1:19 a.m., Ben Mahler wrote: 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! Just to be clear, please feel free to punt with TODOs for (2)! - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/#review60425 --- 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
Re: Review Request 27113: Libprocess benchmark cleanup
---
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 SharedT and OwnedT 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));
}
FutureDuration _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`:
```
FutureDuration 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
Re: Review Request 27113: Libprocess benchmark cleanup
On Oct. 23, 2014, 10:18 p.m., Ben Mahler wrote: 3rdparty/libprocess/src/tests/benchmarks.cpp, line 149 https://reviews.apache.org/r/27113/diff/1/?file=731328#file731328line149 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'.. There are many clients connecting to a single server. The server keeps track of which clients it has linked with to ensure that it links back to all of them. - Joris --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/#review58124 --- On Oct. 24, 2014, 3:09 a.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, 3:09 a.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
Re: Review Request 27113: Libprocess benchmark cleanup
--- 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. Changes --- address bmahler's issues. 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 (updated) - 3rdparty/libprocess/src/tests/benchmarks.cpp 3177a8e Diff: https://reviews.apache.org/r/27113/diff/ Testing --- make check Thanks, Joris Van Remoortere
Re: Review Request 27113: Libprocess benchmark cleanup
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/#review58386 --- Patch looks great! Reviews applied: [25448, 27113] All tests passed. - Mesos ReviewBot 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
Re: Review Request 27113: Libprocess benchmark cleanup
--- 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_ptrint i) 3rdparty/libprocess/m4/ax_cxx_compile_stdcxx_11.m4: std::unique_ptrint 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_setElem 3rdparty/libprocess/3rdparty/stout/include/stout/hashset.hpp:return boost::unordered_setElem::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
Re: Review Request 27113: Libprocess benchmark cleanup
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/ --- (Updated Oct. 24, 2014, 3:09 a.m.) Review request for mesos, Ben Mahler and Niklas Nielsen. Changes --- add std::unique_ptr configure check as dependency. 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
Re: Review Request 27113: Libprocess benchmark cleanup
On Oct. 23, 2014, 10:18 p.m., Ben Mahler wrote: 3rdparty/libprocess/src/tests/benchmarks.cpp, lines 40-53 https://reviews.apache.org/r/27113/diff/1/?file=731328#file731328line40 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. The problem with including this in the regular libprocess test suite is that we require libprocess to be initialized after the forks have been instantiated. Once we can safely terminate the global libprocess state, we could merge this with the regular tests, until then it needs to be in a separate binary (or forcibly run before the other tests). - Joris --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/#review58124 --- On Oct. 24, 2014, 3:09 a.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, 3:09 a.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
Re: Review Request 27113: Libprocess benchmark cleanup
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/#review58238 --- Patch looks great! Reviews applied: [27113] All tests passed. - Mesos ReviewBot On Oct. 24, 2014, 3:09 a.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, 3:09 a.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
