> On Nov. 22, 2017, 7:05 p.m., Benjamin Mahler wrote: > > Should we be implementing a += overload against > > `RepeatedPtrField<Resource>` 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<Resource>&)`). 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<Resource>&)` 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<Resource>` 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<Resource>` 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 > >
