----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68490/#review208538 -----------------------------------------------------------
Almost there! I'm wondering about the casting to `Resource` in loops, seems broken since we lose the shared count? :O src/common/resources.cpp Line 1641 (original), 1655 (patched) <https://reviews.apache.org/r/68490/#comment292565> Actually, I was thinking about this, isn't the casting to `Resource` broken anywhere we use it? We lose the shared count? src/common/resources.cpp Lines 1671-1674 (patched) <https://reviews.apache.org/r/68490/#comment292563> The std::move in add doesn't do anything right now since we don't have an ravlue overload for shared_ptr? Wouldn't this be simpler? ``` if (isReserved(resource_->resource)) { Resource_ r_ = *resource_; r_->resource.clear_reservations(); result.add(std::move(r_)); } else { ``` src/common/resources.cpp Line 1969 (original), 1992 (patched) <https://reviews.apache.org/r/68490/#comment292562> Ditto here. src/v1/resources.cpp Lines 1687-1690 (patched) <https://reviews.apache.org/r/68490/#comment292564> Ditto here src/v1/resources.cpp Line 1976 (original), 1997 (patched) <https://reviews.apache.org/r/68490/#comment292561> We don't have an rvalue overload for -=, can we just leave this as is for now by taking out the std::move? - Benjamin Mahler On Sept. 7, 2018, 3:29 a.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68490/ > ----------------------------------------------------------- > > (Updated Sept. 7, 2018, 3:29 a.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 > > ## Sorter benchmark > > ### BENCHMARK_FullSort > > Master: > > Using 30000 agents and 500 clients > Added 500 clients in 2.261373ms > Added 30000 agents in 322.252441ms > Added allocations for 30000 agents in 823.631826ms > Full sort of 500 clients took 8662ns > No-op sort of 500 clients took 809ns > Removed allocations for 30000 agents in 8.482659084secs > Removed 30000 agents in 417.988004ms > Removed 500 clients in 17.855575ms > > > Copy-on-write: > > Using 30000 agents and 500 clients (**normalized to the master above**) > Added 500 clients in 2.464647ms (**1.09**) > Added 30000 agents in 205.071423ms (**0.64**) > Added allocations for 30000 agents in 156.340883ms (**0.19**) > Full sort of 500 clients took 1007ns (**0.12**) > No-op sort of 500 clients took 204ns (**0.02**) > Removed allocations for 30000 agents in 7.215175016secs (**0.85**) > Removed 30000 agents in 304.998116ms (**0.73**) > Removed 500 clients in 3.195079ms (**0.18**) > > > ### BENCHMARK_HierarchyFullSort > > Master: > > Added 1056 clients in 36.571505ms > Added 10000 agents in 114.165111ms > Added allocations for 10000 agents in 569.144686ms > Full sort of 1056 clients took 43485ns > No-op sort of 1056 clients took 676ns > Removed allocations for 10000 agents in 5.656517904secs > Removed 10000 agents in 153.016733ms > Removed 1056 clients in 7.169549ms > > Copy-on-write: > > Added 1056 clients in 36.468822ms (**0.997**) > Added 10000 agents in 67.733365ms (**0.59**) > Added allocations for 10000 agents in 99.695723ms (**0.175**) > Full sort of 1056 clients took 29729ns (**0.68**) > No-op sort of 1056 clients took 704ns (**1.04**) > Removed allocations for 10000 agents in 4.824389788secs (**0.85**) > Removed 10000 agents in 101.319295ms (**0.66**) > Removed 1056 clients in 2.480967ms (**0.35**) > > > Thanks, > > Meng Zhu > >
