> On Sept. 6, 2018, 3:27 a.m., Benjamin Bannier wrote: > > include/mesos/resources.hpp > > Line 588 (original), 590 (patched) > > <https://reviews.apache.org/r/68490/diff/3/?file=2082742#file2082742line590> > > > > `s/Resource/Resource_/`?
`Resource_` is not exposed outside, it is implicitly converted to `Resource`. Will comment on this. > On Sept. 6, 2018, 3:27 a.m., Benjamin Bannier wrote: > > src/v1/resources.cpp > > Lines 2117-2118 (original), 2154-2178 (patched) > > <https://reviews.apache.org/r/68490/diff/3/?file=2082745#file2082745line2155> > > > > This function looks redundant and identical to `Resources::add(const > > Resource&)` to me. It also introduces another place where we leak > > implementation details into the interface. > > > > Suggest to remove and instead let callers perform the deref. We could > > always add something similar back once we want to support `move`'ing a > > `that`. > > Benjamin Mahler wrote: > I think Meng forgot to publish a reply here, but you can see the reply on > my similar comment above. If we don't have this and the caller has to de-ref, > then we have to copy the resource object, whereas this interface allows us to > copy the shared_ptr. > > Also note that this is not part of the public interface, it's private. The difference lies in: ``` // Cannot be combined with any existing Resource object. if (!found) { resources.push_back(that); } ``` I will copy my earlier reply to BenM: Consider adding two non-mergeble Resources(e.g. 1 cpu + 1 mem), if we only have Resources::add(const Resource_& that), we will always need to make_shared(Resource_). If we directly pass shared_ptr, we can then just push_back. For any non-mergable Resource_, we will be able to save one make_shared. One rule of thumb I figured is that, we should avoid make_shared when possible (because it makes a copy). This means if we already have a shared_ptr we should just use it and pass it around. This also explains why I use shared_ptr for foreach loops. - Meng ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68490/#review208402 ----------------------------------------------------------- On Sept. 6, 2018, 2:56 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68490/ > ----------------------------------------------------------- > > (Updated Sept. 6, 2018, 2:56 p.m.) > > > Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Gastón > Kleiman. > > > Bugs: MESOS-6765 > https://issues.apache.org/jira/browse/MESOS-6765 > > > Repository: mesos > > > Description > ------- > > This patch lets `class Resources` only store shared > pointers to the underlying resource objects, so that > read-only filter operations such as `reserved()`, > `unreserved()` and etc. can avoid making copies of > the whole resource objects. Instead, only shared pointers > are copied. > > In write operations, we check if there are more than one > references to the resource object. If so, a copy is made > for safe mutation without affecting owners. > > To maintain the usage abstraction that `class Resources` > still holds resource objects, we utilize > `boost::indirect_iterator` iterator adapter to deference > the shared pointers as we iterate. > > > Diffs > ----- > > include/mesos/resources.hpp 6f81b14f8bc090a144eeae8f15639c370366166d > include/mesos/v1/resources.hpp 09110530da16678abf6bf6b308906dd8ccc8180a > src/common/resources.cpp 3e63cdedb9261970dbeb9bb9f97eed65819f68a7 > src/v1/resources.cpp 3683a331e0859cd6f2ad061db6ba67112ecfcb0d > > > Diff: https://reviews.apache.org/r/68490/diff/4/ > > > Testing > ------- > > make check > > Benchmarked on a Ubuntu box using clang -O2 optimized build with an i5 > processor, frequency fixed at 1.6GHz. > > Resources benchmarks (Measure geo-mean of various operations): > > Normalized Geomean latency > > Master Copy-on-write > Arithmetic 1 1.09 > Filter 1 0.28 > Contains 1 0.27 > Parse 1 1.01 > > Sample allocator benchmark (Measure first allocation latency): > > HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/1 (1k agents, 50 > frameworks): > > Master 1 > Copy-on-write 0.72 > > HierarchicalAllocator_BENCHMARK_Test.ResourceLabels/1 (1k agents, 50 > frameworks): > > Master 1 > Copy-on-write 0.72 > > > Thanks, > > Meng Zhu > >
