mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 49381: Benchmark for Resources class.
Date Wed, 13 Jul 2016 23:13:08 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message