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 68490: Optimized `class Resources` with copy-on-write.
Date Fri, 07 Sep 2018 00:53:26 GMT


> On Sept. 6, 2018, 10: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`.

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.


- Benjamin


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


On Sept. 6, 2018, 9: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, 9: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
> 
>


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