----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50551/#review151852 -----------------------------------------------------------
Thanks looks pretty good! Just a few suggestions for simplifying things a bit below. Also, 42 seconds for these seems too long for a benchmark test, can you reduce it to be no more than a few seconds to run these? src/tests/resources_tests.cpp (lines 2959 - 2963) <https://reviews.apache.org/r/50551/#comment220463> How about calling this `Parameter` like the one above and nesting both of them within the test class? i.e. `Resources_Contains_BENCHMARK_Test::Parameter` src/tests/resources_tests.cpp (lines 2971 - 2972) <https://reviews.apache.org/r/50551/#comment220468> Showing a list of the parameters up here in the comment would be helpful. src/tests/resources_tests.cpp (lines 2983 - 2992) <https://reviews.apache.org/r/50551/#comment220470> Can we re-use the fragment function we introduced in hierarchical_allocator_tests.cpp? src/tests/resources_tests.cpp (line 3024) <https://reviews.apache.org/r/50551/#comment220465> How about making this a part of the parameter so we have more control over the time? (It currently takes too long to run) src/tests/resources_tests.cpp (lines 3042 - 3053) <https://reviews.apache.org/r/50551/#comment220467> It looks like we should just perform this portion (`superset.contains(subset)`) of the benchmark, rather than also doing `subset.contains(superset)` and `subset.contains(subset)`. If we want to test other contains checks we can simply add parameters. - Benjamin Mahler On Oct. 6, 2016, 9:30 a.m., Guangya Liu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50551/ > ----------------------------------------------------------- > > (Updated Oct. 6, 2016, 9:30 a.m.) > > > Review request for mesos, Benjamin Mahler, Klaus Ma, and Jiang Yan Xu. > > > Bugs: MESOS-5700 > https://issues.apache.org/jira/browse/MESOS-5700 > > > Repository: mesos > > > Description > ------- > > Added benchmark test for `Resources::contains`. > > > Diffs > ----- > > src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a > > Diff: https://reviews.apache.org/r/50551/diff/ > > > Testing > ------- > > make > make check > > ``` > ./bin/mesos-tests.sh --benchmark > --gtest_filter="*Resources_Contains_BENCHMARK_Test.Contains/*" > [==========] Running 3 tests from 1 test case. > [----------] Global test environment set-up. > [----------] 3 tests from ResourcesContains/Resources_Contains_BENCHMARK_Test > [ RUN ] ResourcesContains/Resources_Contains_BENCHMARK_Test.Contains/0 > Took 8629us to perform 5000 'subset.contains(superset)' operations on subset > resources cpus(*):1; mem(*):128 contains superset resources cpus(*):1; > gpus(*):1; mem(*):128; disk(*):256 > Took 20378us to perform 5000 'superset.contains(subset)' operations on > superset resources cpus(*):1; gpus(*):1; mem(*):128; disk(*):256 contains > subset resources cpus(*):1; mem(*):128 > Took 9526us to perform 5000 'subset.contains(subset)' operations on subset > resources cpus(*):1; mem(*):128 contains subset resources cpus(*):1; > mem(*):128 > [ OK ] ResourcesContains/Resources_Contains_BENCHMARK_Test.Contains/0 > (39 ms) > [ RUN ] ResourcesContains/Resources_Contains_BENCHMARK_Test.Contains/1 > Took 1.165801secs to perform 5000 'subset.contains(superset)' operations on > subset resources ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1... contains > superset resources ports(*):[1-1000] > Took 895058us to perform 5000 'superset.contains(subset)' operations on > superset resources ports(*):[1-1000] contains subset resources ports(*):[1-2, > 4-5, 7-8, 10-11, 13-14, 16-17, 1... > Took 19.768667secs to perform 5000 'subset.contains(subset)' operations on > subset resources ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1... contains > subset resources ports(*):[1-2, 4-5, 7-8, 10-11, 13-14, 16-17, 1... > [ OK ] ResourcesContains/Resources_Contains_BENCHMARK_Test.Contains/1 > (21831 ms) > [ RUN ] ResourcesContains/Resources_Contains_BENCHMARK_Test.Contains/2 > Took 326768us to perform 5000 'subset.contains(superset)' operations on > subset resources cpus(*):1; mem(*):128; ports(*):[1-2, 4-5, 7-8,... contains > superset resources cpus(*):1; gpus(*):1; mem(*):128; disk(*):256; ... > Took 1.003977secs to perform 5000 'superset.contains(subset)' operations on > superset resources cpus(*):1; gpus(*):1; mem(*):128; disk(*):256; ... > contains subset resources cpus(*):1; mem(*):128; ports(*):[1-2, 4-5, 7-8,... > Took 19.641502secs to perform 5000 'subset.contains(subset)' operations on > subset resources cpus(*):1; mem(*):128; ports(*):[1-2, 4-5, 7-8,... contains > subset resources cpus(*):1; mem(*):128; ports(*):[1-2, 4-5, 7-8,... > [ OK ] ResourcesContains/Resources_Contains_BENCHMARK_Test.Contains/2 > (20974 ms) > [----------] 3 tests from ResourcesContains/Resources_Contains_BENCHMARK_Test > (42844 ms total) > > [----------] Global test environment tear-down > [==========] 3 tests from 1 test case ran. (42853 ms total) > [ PASSED ] 3 tests. > ``` > > > Thanks, > > Guangya Liu > >
