Review Request 64028: Removed duplicate resources conversions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64028/ --- Review request for mesos, Benjamin Mahler and Michael Park. Repository: mesos Description --- `RepeatedPtrField` can be implicitly converted to `Resources`, leading to hidden multiple resources conversions on performance-critical paths in master. For example, `operator +=` relies on implicit conversion, when invoked with `RepeatedPtrField` argument. Diffs - src/master/master.hpp 2a2e830354db4a2191fb8321beb8174b80f7ba7d src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206 Diff: https://reviews.apache.org/r/64028/diff/1/ Testing --- make check Thanks, Dmitry Zhuk
Re: Review Request 64028: Removed duplicate resources conversions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64028/#review191755 --- Should we be implementing a += overload against `RepeatedPtrField` instead of doing this? Would that be more efficient than doing a conversion then adding? I'm also hoping to avoid code having to this type of conversion, ideally :) - Benjamin Mahler On Nov. 22, 2017, 1:24 p.m., Dmitry Zhuk wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64028/ > --- > > (Updated Nov. 22, 2017, 1:24 p.m.) > > > Review request for mesos, Benjamin Mahler and Michael Park. > > > Repository: mesos > > > Description > --- > > `RepeatedPtrField` can be implicitly converted to `Resources`, > leading to hidden multiple resources conversions on performance-critical > paths in master. For example, `operator +=` relies on implicit > conversion, when invoked with `RepeatedPtrField` argument. > > > Diffs > - > > src/master/master.hpp 2a2e830354db4a2191fb8321beb8174b80f7ba7d > src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206 > > > Diff: https://reviews.apache.org/r/64028/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Dmitry Zhuk > >
Re: Review Request 64028: Removed duplicate resources conversions.
> On Nov. 22, 2017, 7:05 p.m., Benjamin Mahler wrote: > > Should we be implementing a += overload against > > `RepeatedPtrField` instead of doing this? Would that be more > > efficient than doing a conversion then adding? I'm also hoping to avoid > > code having to this type of conversion, ideally :) There's definitely some room for improvements in `Resources`, and `+=` overload could save some CPU cycles, but not in this case. It will not be as efficient here as having explicit conversion followed by more than one `+=`/`<<`. In current approach we don't trust data in protobuf, and we must do validation (see `Resources::operator+=(const Resource_& that)`, invoked from `Resources(const RepeatedPtrField&)`). So in ``` const Resources r = ...; r1 += r; r2 += r; ``` we do validation only once during conversion, and `+=(const Resources&)` bypasses validation since `Resources` are known to be valid. However `+=(const RepeatedPtrField&)` overload will have to do validatation on each call, so we will validate data twice. - Dmitry --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64028/#review191755 --- On Nov. 22, 2017, 1:24 p.m., Dmitry Zhuk wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64028/ > --- > > (Updated Nov. 22, 2017, 1:24 p.m.) > > > Review request for mesos, Benjamin Mahler and Michael Park. > > > Repository: mesos > > > Description > --- > > `RepeatedPtrField` can be implicitly converted to `Resources`, > leading to hidden multiple resources conversions on performance-critical > paths in master. For example, `operator +=` relies on implicit > conversion, when invoked with `RepeatedPtrField` argument. > > > Diffs > - > > src/master/master.hpp 2a2e830354db4a2191fb8321beb8174b80f7ba7d > src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206 > > > Diff: https://reviews.apache.org/r/64028/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Dmitry Zhuk > >
Re: Review Request 64028: Removed duplicate resources conversions.
> On Nov. 22, 2017, 7:05 p.m., Benjamin Mahler wrote: > > Should we be implementing a += overload against > > `RepeatedPtrField` instead of doing this? Would that be more > > efficient than doing a conversion then adding? I'm also hoping to avoid > > code having to this type of conversion, ideally :) > > Dmitry Zhuk wrote: > There's definitely some room for improvements in `Resources`, and `+=` > overload could save some CPU cycles, but not in this case. It will not be as > efficient here as having explicit conversion followed by more than one > `+=`/`<<`. In current approach we don't trust data in protobuf, and we must > do validation (see `Resources::operator+=(const Resource_& that)`, invoked > from `Resources(const RepeatedPtrField&)`). So in > ``` > const Resources r = ...; > r1 += r; > r2 += r; > ``` > we do validation only once during conversion, and `+=(const Resources&)` > bypasses validation since `Resources` are known to be valid. > However `+=(const RepeatedPtrField&)` overload will have to do > validatation on each call, so we will validate data twice. Ah yes, thanks for the clarification! Can you mention this in the description and the code? Also, if there was a performance impact on the benchmark, could you include it for posterity? - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64028/#review191755 --- On Nov. 22, 2017, 1:24 p.m., Dmitry Zhuk wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64028/ > --- > > (Updated Nov. 22, 2017, 1:24 p.m.) > > > Review request for mesos, Benjamin Mahler and Michael Park. > > > Repository: mesos > > > Description > --- > > `RepeatedPtrField` can be implicitly converted to `Resources`, > leading to hidden multiple resources conversions on performance-critical > paths in master. For example, `operator +=` relies on implicit > conversion, when invoked with `RepeatedPtrField` argument. > > > Diffs > - > > src/master/master.hpp 2a2e830354db4a2191fb8321beb8174b80f7ba7d > src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206 > > > Diff: https://reviews.apache.org/r/64028/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Dmitry Zhuk > >
Re: Review Request 64028: Removed duplicate resources conversions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64028/ --- (Updated Nov. 28, 2017, 5:41 p.m.) Review request for mesos, Benjamin Mahler and Michael Park. Changes --- Rebased. Added comments to code. Added benchmark results. Repository: mesos Description (updated) --- `RepeatedPtrField` can be implicitly converted to `Resources`, leading to hidden multiple resources conversions on performance-critical paths in master. For example, `operator +=` relies on implicit conversion, when invoked with `RepeatedPtrField` argument. Using protobuf also implies data validation and sanitization, e.g. when converting to `Resources`, as protobuf generally comes from untrusted sources. By doing conversion only once, and then reusing the result, we save on these checks as well, as operations on `Resources` are generally faster as they can trust data in `Resources`. Diffs (updated) - src/master/master.hpp 2a2e830354db4a2191fb8321beb8174b80f7ba7d src/master/master.cpp 53263e499d88b906b6406c24c0dfb737e589e813 Diff: https://reviews.apache.org/r/64028/diff/2/ Changes: https://reviews.apache.org/r/64028/diff/1-2/ Testing (updated) --- make check ./mesos-tests.sh --benchmark --gtest_filter=AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.* Before changes: [==] Running 3 tests from 1 test case. [--] Global test environment set-up. [--] 3 tests from AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test [ RUN ] AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0 Starting reregistration for all agents Reregistered 2000 agents with a total of 10 running tasks and 10 completed tasks in 13.267079264secs [ OK ] AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0 (25274 ms) [ RUN ] AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1 Starting reregistration for all agents Reregistered 2000 agents with a total of 20 running tasks and 0 completed tasks in 26.212404584secs [ OK ] AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1 (52153 ms) [ RUN ] AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2 Starting reregistration for all agents Reregistered 2 agents with a total of 10 running tasks and 0 completed tasks in 20.269919213secs [ OK ] AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2 (34072 ms) [--] 3 tests from AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test (111499 ms total) [--] Global test environment tear-down [==] 3 tests from 1 test case ran. (111515 ms total) [ PASSED ] 3 tests. After cahnges: [==] Running 3 tests from 1 test case. [--] Global test environment set-up. [--] 3 tests from AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test [ RUN ] AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0 Starting reregistration for all agents Reregistered 2000 agents with a total of 10 running tasks and 10 completed tasks in 12.550849382secs [ OK ] AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0 (24289 ms) [ RUN ] AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1 Starting reregistration for all agents Reregistered 2000 agents with a total of 20 running tasks and 0 completed tasks in 24.63363665secs [ OK ] AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1 (44266 ms) [ RUN ] AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2 Starting reregistration for all agents Reregistered 2 agents with a total of 10 running tasks and 0 completed tasks in 18.150451886secs [ OK ] AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2 (32117 ms) [--] 3 tests from AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test (100672 ms total) [--] Global test environment tear-down [==] 3 tests from 1 test case ran. (100688 ms total) [ PASSED ] 3 tests. Thanks, Dmitry Zhuk
Re: Review Request 64028: Removed duplicate resources conversions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64028/#review192226 --- Ship it! Ship It! - Benjamin Mahler On Nov. 28, 2017, 5:41 p.m., Dmitry Zhuk wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64028/ > --- > > (Updated Nov. 28, 2017, 5:41 p.m.) > > > Review request for mesos, Benjamin Mahler and Michael Park. > > > Repository: mesos > > > Description > --- > > `RepeatedPtrField` can be implicitly converted to `Resources`, > leading to hidden multiple resources conversions on performance-critical > paths in master. For example, `operator +=` relies on implicit > conversion, when invoked with `RepeatedPtrField` argument. > Using protobuf also implies data validation and sanitization, e.g. when > converting to `Resources`, as protobuf generally comes from untrusted > sources. By doing conversion only once, and then reusing the result, > we save on these checks as well, as operations on `Resources` are > generally faster as they can trust data in `Resources`. > > > Diffs > - > > src/master/master.hpp 2a2e830354db4a2191fb8321beb8174b80f7ba7d > src/master/master.cpp 53263e499d88b906b6406c24c0dfb737e589e813 > > > Diff: https://reviews.apache.org/r/64028/diff/2/ > > > Testing > --- > > make check > > ./mesos-tests.sh --benchmark > --gtest_filter=AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.* > > Before changes: > [==] Running 3 tests from 1 test case. > [--] Global test environment set-up. > [--] 3 tests from > AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test > [ RUN ] > AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0 > Starting reregistration for all agents > Reregistered 2000 agents with a total of 10 running tasks and 10 > completed tasks in 13.267079264secs > [ OK ] > AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0 > (25274 ms) > [ RUN ] > AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1 > Starting reregistration for all agents > Reregistered 2000 agents with a total of 20 running tasks and 0 completed > tasks in 26.212404584secs > [ OK ] > AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1 > (52153 ms) > [ RUN ] > AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2 > Starting reregistration for all agents > Reregistered 2 agents with a total of 10 running tasks and 0 > completed tasks in 20.269919213secs > [ OK ] > AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2 > (34072 ms) > [--] 3 tests from > AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test (111499 ms total) > > [--] Global test environment tear-down > [==] 3 tests from 1 test case ran. (111515 ms total) > [ PASSED ] 3 tests. > > > After cahnges: > [==] Running 3 tests from 1 test case. > [--] Global test environment set-up. > [--] 3 tests from > AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test > [ RUN ] > AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0 > Starting reregistration for all agents > Reregistered 2000 agents with a total of 10 running tasks and 10 > completed tasks in 12.550849382secs > [ OK ] > AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0 > (24289 ms) > [ RUN ] > AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1 > Starting reregistration for all agents > Reregistered 2000 agents with a total of 20 running tasks and 0 completed > tasks in 24.63363665secs > [ OK ] > AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1 > (44266 ms) > [ RUN ] > AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2 > Starting reregistration for all agents > Reregistered 2 agents with a total of 10 running tasks and 0 > completed tasks in 18.150451886secs > [ OK ] > AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2 > (32117 ms) > [--] 3 tests from > AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test (100672 ms total) > > [--] Global test environment tear-down > [==] 3 tests from 1 test case ran. (100688 ms total) > [ PASSED ] 3 tests. > > > Thanks, > > Dmitry Zhuk > >
