----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49381/#review142096 -----------------------------------------------------------
Some of my comments are spread across your review revisions because updates were made after I started reviewing, sorry about that! Overall I like that you parameterized the tests by the 'Resources' that we're intereted in working with! Note that it was a bit confusing that these test parameters were called "test cases" because this terminology is already used in google test to refer to the tests themselves [1](https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#basic-concepts) [2](https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#simple-tests). I would suggest renaming to 'Parameter' to match the terminology. However, I think that the parameters could be improved. First, it seems we can do away with the 'initial' resources in the struct. Second, there is a 'total' parameter which specifies how many times to perform the operations (I noted below that this was a bit misleading since it's actually `total*resources.size()` operations!) and there is a vector of 'Resources' but it only ever contains one entry FWICT. So it seems we could simplify: ``` // Before. for (size_t i = 0; i < total; i++) { foreach (Resources& resources_, testCase.resources) { resources += resources_; } } // After. for (size_t i = 0; i < total; i++) { total += resources; } ``` Also the 'total' should likely be tied to the resources that we're operating on, since some are dramatically more expensive. src/tests/resources_tests.cpp (line 2415) <https://reviews.apache.org/r/49381/#comment207599> Recall that we generally avoid abbreviations, so `initResources` would be `initial` or `initialResources`. That being said, why did you need this? It seem so simplify things if we get rid of this. src/tests/resources_tests.cpp (line 2416) <https://reviews.apache.org/r/49381/#comment207598> `vector<Resources>` seems a bit strange here, because `Resources` is already capabable of aggregating multiple `Resources` together. Note that this makes your benchmark output misleading, it will print 'total' operations but really it is doing `total * resources.size()`! Let's simplify this and just store a `Resources`. src/tests/resources_tests.cpp (line 2447) <https://reviews.apache.org/r/49381/#comment207604> You need a using statement for vector, how did this compile? Looks like maybe you got it from here: ``` ? grep -R 'using std::vector' 3rdparty | grep hpp 3rdparty/libprocess/include/process/posix/subprocess.hpp:using std::vector; 3rdparty/libprocess/include/process/windows/subprocess.hpp:using std::vector; ``` src/tests/resources_tests.cpp (lines 2555 - 2556) <https://reviews.apache.org/r/49381/#comment207603> ValuesIn can directly take a container AFAICT, no need to pass iterators: https://github.com/google/googletest/blob/d225acc90bc3a8c420a9bcd1f033033c1ccd7fe0/googletest/include/gtest/gtest-param-test.h.pump#L321-L325 src/tests/resources_tests.cpp (line 2577) <https://reviews.apache.org/r/49381/#comment207602> No backticks in logging messages. src/tests/resources_tests.cpp (lines 2624 - 2641) <https://reviews.apache.org/r/49381/#comment207597> Hm.. this is a weird test because you're going to quickly go to 0 resources and subtraction will become a no-op. Perhaps it would be better if we had a single 'Arithmetic' benchmark for +,-,+=,-=. This would allow you to do something like the following: ``` Resources total; watch.start(); for (...) { total += resources; } watch.stop(); cout << "Took ..." << endl; watch.start(); for (...) { total -= resources; } watch.stop(); cout << "Took ..." << endl; ``` This way each subtraction operation should not be a no-op. src/tests/resources_tests.cpp (line 2669) <https://reviews.apache.org/r/49381/#comment207596> 'total' what? How about 'totalOperations'? src/tests/resources_tests.cpp (line 2414) <https://reviews.apache.org/r/49381/#comment207658> Maybe we just use a generic `abbreviate(string, maxLength)` instead of having descriptions? This would let us print out long resources as: ``` ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1... ``` It's not perfect (won't show if other resources are present), but it seems good enough for now. src/tests/resources_tests.cpp (lines 2488 - 2489) <https://reviews.apache.org/r/49381/#comment207611> We tend to use strings::format from stout rather than sprintf. src/tests/resources_tests.cpp (line 2598) <https://reviews.apache.org/r/49381/#comment207660> We do not allow non-POD static globals in the style guide due to the potential for crashing the program upon termination. Why did you need the global option? We can just call a function like so: ``` INSTANTIATE_TEST_CASE_P( ResourcesOperators, Resources_BENCHMARK_Test, ::testing::ValuesIn(Resources_BENCHMARK_TestCases::parameters())); ``` src/tests/resources_tests.cpp (line 2609) <https://reviews.apache.org/r/49381/#comment207662> Hard-coding 100 for all kinds of resources seems a bit unfortunate since the performance varies wildly across the parameters used here. Seems it would be better to include the number of operations alongside the resources instead of fixed for all resources? src/tests/resources_tests.cpp (lines 2634 - 2694) <https://reviews.apache.org/r/49381/#comment207663> To simplify this a bit, we can just have a single Arithmetic benchmark which aggregates into a += total and then subtracts them all back out with -=. Ditto for + and -. Seems a bit simpler than all of the separate test cases here? - Benjamin Mahler On July 13, 2016, 1:48 p.m., Klaus Ma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49381/ > ----------------------------------------------------------- > > (Updated July 13, 2016, 1:48 p.m.) > > > Review request for mesos, Benjamin Mahler, Guangya Liu, and Michael Park. > > > Bugs: MESOS-5700 > https://issues.apache.org/jira/browse/MESOS-5700 > > > Repository: mesos > > > Description > ------- > > Benchmark for Resources class. > > > Diffs > ----- > > src/tests/resources_tests.cpp dc12bd8f1e2da6972bc8aed598811c55d664036e > > Diff: https://reviews.apache.org/r/49381/diff/ > > > Testing > ------- > > make > MESOS_BENCHMARK=1 GTEST_FILTER="*Resources_BENCHMARK_Test.Operator*" make > check > > [----------] Global test environment set-up. > [----------] 30 tests from ResourcesOperators/Resources_BENCHMARK_Test > [ RUN ] > ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/0 > Took 5429us to `AddAndAssign` "cpus:1" 10000 times. > [ OK ] > ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/0 (6 ms) > [ RUN ] > ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/1 > Took 9317us to `AddAndAssign` "cpus:1;mem:2" 10000 times. > [ OK ] > ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/1 (10 ms) > [ RUN ] > ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/2 > **Took 5.271509secs to `AddAndAssign` "cpus(r0):1;cpus(r1):1; ... > cpus(r99):1" 10000 times.** > [ OK ] > ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/2 (5271 ms) > [ RUN ] > ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/3 > **Took 13.017557secs to `AddAndAssign` "cpus(r0):1;mem(r0):100; ... > cpus(r99):1;mem(r99):100" 10000 times.** > [ OK ] > ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/3 (13018 ms) > [ RUN ] > ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/4 > **Took 3.357184secs to `AddAndAssign` "[1-2, 4-5, ... , 298-299]" 10000 > times.** > [ OK ] > ResourcesOperators/Resources_BENCHMARK_Test.Operator_AddAndAssign/4 (3358 ms) > [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/0 > Took 20977us to `Add` "cpus:1" 10000 times. > [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/0 (21 > ms) > [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/1 > Took 26069us to `Add` "cpus:1;mem:2" 10000 times. > [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/1 (26 > ms) > [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/2 > **Took 5.891177secs to `Add` "cpus(r0):1;cpus(r1):1; ... cpus(r99):1" 10000 > times.** > [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/2 (5891 > ms) > [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/3 > **Took 14.894299secs to `Add` "cpus(r0):1;mem(r0):100; ... > cpus(r99):1;mem(r99):100" 10000 times.** > [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/3 > (14895 ms) > [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/4 > **Took 2.321527secs to `Add` "[1-2, 4-5, ... , 298-299]" 10000 times.** > [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Add/4 (2321 > ms) > [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/0 > Took 26362us to `Sub` "cpus:1" 10000 times. > [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/0 (27 > ms) > [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/1 > Took 30828us to `Sub` "cpus:1;mem:2" 10000 times. > [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/1 (31 > ms) > [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/2 > Took 431531us to `Sub` "cpus(r0):1;cpus(r1):1; ... cpus(r99):1" 10000 times. > [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/2 (431 > ms) > [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/3 > Took 837827us to `Sub` "cpus(r0):1;mem(r0):100; ... cpus(r99):1;mem(r99):100" > 10000 times. > [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/3 (838 > ms) > [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/4 > **Took 2.173558secs to `Sub` "[1-2, 4-5, ... , 298-299]" 10000 times.** > [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_Sub/4 (2174 > ms) > [ RUN ] > ResourcesOperators/Resources_BENCHMARK_Test.Operator_SubAndAssign/0 > Took 3536us to `SubAndAssign` "cpus:1" 10000 times. > [ OK ] > ResourcesOperators/Resources_BENCHMARK_Test.Operator_SubAndAssign/0 (4 ms) > [ RUN ] > ResourcesOperators/Resources_BENCHMARK_Test.Operator_SubAndAssign/1 > Took 6091us to `SubAndAssign` "cpus:1;mem:2" 10000 times. > [ OK ] > ResourcesOperators/Resources_BENCHMARK_Test.Operator_SubAndAssign/1 (6 ms) > [ RUN ] > ResourcesOperators/Resources_BENCHMARK_Test.Operator_SubAndAssign/2 > Took 399855us to `SubAndAssign` "cpus(r0):1;cpus(r1):1; ... cpus(r99):1" > 10000 times. > [ OK ] > ResourcesOperators/Resources_BENCHMARK_Test.Operator_SubAndAssign/2 (400 ms) > [ RUN ] > ResourcesOperators/Resources_BENCHMARK_Test.Operator_SubAndAssign/3 > Took 834715us to `SubAndAssign` "cpus(r0):1;mem(r0):100; ... > cpus(r99):1;mem(r99):100" 10000 times. > [ OK ] > ResourcesOperators/Resources_BENCHMARK_Test.Operator_SubAndAssign/3 (835 ms) > [ RUN ] > ResourcesOperators/Resources_BENCHMARK_Test.Operator_SubAndAssign/4 > **Took 2.096096secs to `SubAndAssign` "[1-2, 4-5, ... , 298-299]" 10000 > times.** > [ OK ] > ResourcesOperators/Resources_BENCHMARK_Test.Operator_SubAndAssign/4 (2096 ms) > [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_CPUS/0 > Took 2750us to `cpus()` "cpus:1" 10000 times. > [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_CPUS/0 (3 > ms) > [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_CPUS/1 > Took 3223us to `cpus()` "cpus:1;mem:2" 10000 times. > [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_CPUS/1 (3 > ms) > [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_CPUS/2 > Took 102915us to `cpus()` "cpus(r0):1;cpus(r1):1; ... cpus(r99):1" 10000 > times. > [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_CPUS/2 (103 > ms) > [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_CPUS/3 > Took 131005us to `cpus()` "cpus(r0):1;mem(r0):100; ... > cpus(r99):1;mem(r99):100" 10000 times. > [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_CPUS/3 (131 > ms) > [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_CPUS/4 > Took 1408us to `cpus()` "[1-2, 4-5, ... , 298-299]" 10000 times. > [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_CPUS/4 (2 > ms) > [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_MEM/0 > Took 1453us to `mem()` "cpus:1" 10000 times. > [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_MEM/0 (1 ms) > [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_MEM/1 > Took 2968us to `mem()` "cpus:1;mem:2" 10000 times. > [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_MEM/1 (3 ms) > [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_MEM/2 > Took 28500us to `mem()` "cpus(r0):1;cpus(r1):1; ... cpus(r99):1" 10000 times. > [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_MEM/2 (29 > ms) > [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_MEM/3 > Took 134330us to `mem()` "cpus(r0):1;mem(r0):100; ... > cpus(r99):1;mem(r99):100" 10000 times. > [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_MEM/3 (135 > ms) > [ RUN ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_MEM/4 > Took 1753us to `mem()` "[1-2, 4-5, ... , 298-299]" 10000 times. > [ OK ] ResourcesOperators/Resources_BENCHMARK_Test.Operator_MEM/4 (1 ms) > [----------] 30 tests from ResourcesOperators/Resources_BENCHMARK_Test (52070 > ms total) > > > Thanks, > > Klaus Ma > >
