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 50017: Do not validate resource when add/subtract `Resources` object.
Date Sat, 23 Jul 2016 02:29:55 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50017/#review143307
-----------------------------------------------------------


Fix it, then Ship it!




Looks great, thanks!

How about the following description:

```
    Avoid unnecessary validation in Resources arithmetic.

    The 'Resource' objects within the 'Resources' class are always
    valid. However, we currently unnecessarily re-validate during
    the += and -= operators with a 'Resources' right hand side. This
    was done incidentally because these operators happened to just
    call into the += and -= operators to add each 'Resource'. These
    operators must validate, since they take the 'Resource' protobuf,
    which could be invalid!

    This updates the += and -= operators to both call into common
    'add' and 'subtract' methods that do not perform validation. In
    the case of performing a += or -= with a 'Resource', we perform
    validation before calling into 'add' or 'subtract'.

    This provides a significant performance improvement, as the
    validation consumed a lot of time during 'Resources' arithmetic.
```

The current summary doesn't mention that the validation is unnecessary. It would be nice to
mention in the description that this provides a significant performance improvement, and it
would be nice to give more context as to why we can make this change (note that while I have
context, others that encounter this patch may not have the same level of context).


include/mesos/resources.hpp (lines 399 - 421)
<https://reviews.apache.org/r/50017/#comment209083>

    Hm.. how about the following?
    
    ```
      // Validation-free versions of += and -= `Resource` operators.
      // These can be used when `r` is already validated.
      void add(const Resource& r);
      void subtract(const Resource& r);
    ```
    
    I like your comment! But I'm a bit worried that it encodes too much non-local information
that may become out-of-date as the code evolves. It seems like if we just document that these
can be called instead of += and -= when 'r' is validated, then the calling code (e.g. += Resources)
can document why it calls `add`. This would be less likely to go out of date than if were
were to document the details of += and -= here.


- Benjamin Mahler


On July 20, 2016, 3:22 a.m., Guangya Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50017/
> -----------------------------------------------------------
> 
> (Updated July 20, 2016, 3:22 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Klaus Ma.
> 
> 
> Bugs: MESOS-5869
>     https://issues.apache.org/jira/browse/MESOS-5869
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Do not validate resource when add/subtract `Resources` object.
> 
> 
> Diffs
> -----
> 
>   include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
>   include/mesos/v1/resources.hpp a5ba8fec4c9c3643646308f75a4b28cefe0b3df3 
>   src/common/resources.cpp b1bd2784aefdebf91892638b40b215373b998574 
>   src/v1/resources.cpp 6d4ec75fbb7edab013563142aaf13d5302cc50af 
> 
> Diff: https://reviews.apache.org/r/50017/diff/
> 
> 
> Testing
> -------
> 
> The `qcachegrind` result here: https://docs.google.com/document/d/1oilen04e8trIOgYbj-jxAd7esTRDaySll8y2BQ6p5nU/edit?usp=sharing
> 
> Based on the test, the performance of resources `+=` and `-=` will be improved by 10x
for sorter test, and the performance for port range `+=` was improved by 5x and port range
`-=` was improved 1000x.
> 
> Sorter Benchmark test before fix:
> ```
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test
> [ RUN      ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35
> Using 50000 agents and 1000 clients
> Added 1000 clients in 23305us
> Added 50000 agents in 1.174069secs
> Added allocations for 50000 agents in 40.562802secs
> Full sort of 1000 clients took 38193us
> No-op sort of 1000 clients took 382us
> [       OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35 (43032 ms)
> [----------] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test (43032 ms total)
> 
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (43054 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> Sorter Benchmark test after fix:
> ```
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test
> [ RUN      ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35
> Using 50000 agents and 1000 clients
> Added 1000 clients in 25846us
> Added 50000 agents in 1.092462secs
> Added allocations for 50000 agents in 4.397859secs
> Full sort of 1000 clients took 35051us
> No-op sort of 1000 clients took 551us
> [       OK ] AgentAndClientCount/Sorter_BENCHMARK_Test.FullSort/35 (6897 ms)
> [----------] 1 test from AgentAndClientCount/Sorter_BENCHMARK_Test (6897 ms total)
> 
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (6920 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> Ports resources benchmark test before fix:
> ```
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from ResourcesOperators/Resources_BENCHMARK_Test
> [ RUN      ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2
> Took 12.478841secs to perform 1000 'total += r' operations on ports(*):[1-2, 4-5, 7-8,
10-11, 13-14, 16-17, 1...
> Took 8.512399secs to perform 1000 'total -= r' operations on ports(*):[1-2, 4-5, 7-8,
10-11, 13-14, 16-17, 1...
> Took 11.296542secs to perform 1000 'total = total + r' operations on ports(*):[1-2, 4-5,
7-8, 10-11, 13-14, 16-17, 1...
> Took 8.517692secs to perform 1000 'total = total - r' operations on ports(*):[1-2, 4-5,
7-8, 10-11, 13-14, 16-17, 1...
> [       OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 (40808 ms)
> [----------] 1 test from ResourcesOperators/Resources_BENCHMARK_Test (40808 ms total)
> 
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (40832 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> Ports resources benchmark test after fix:
> ```
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from ResourcesOperators/Resources_BENCHMARK_Test
> [ RUN      ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2
> Took 2.827012secs to perform 1000 'total += r' operations on ports(*):[1-2, 4-5, 7-8,
10-11, 13-14, 16-17, 1...
> Took 8841us to perform 1000 'total -= r' operations on ports(*):[1-2, 4-5, 7-8, 10-11,
13-14, 16-17, 1...
> Took 3.313112secs to perform 1000 'total = total + r' operations on ports(*):[1-2, 4-5,
7-8, 10-11, 13-14, 16-17, 1...
> Took 12415us to perform 1000 'total = total - r' operations on ports(*):[1-2, 4-5, 7-8,
10-11, 13-14, 16-17, 1...
> [       OK ] ResourcesOperators/Resources_BENCHMARK_Test.Arithmetic/2 (6164 ms)
> [----------] 1 test from ResourcesOperators/Resources_BENCHMARK_Test (6164 ms total)
> 
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (6187 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>


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