mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 68490: Optimized `class Resources` with copy-on-write.
Date Thu, 06 Sep 2018 10:27:53 GMT

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




include/mesos/resources.hpp
Line 588 (original), 590 (patched)
<https://reviews.apache.org/r/68490/#comment292306>

    `s/Resource/Resource_/`?



include/mesos/resources.hpp
Lines 596-597 (patched)
<https://reviews.apache.org/r/68490/#comment292307>

    I'd leave this above above the iterator types as documentation.



include/mesos/resources.hpp
Lines 691-694 (patched)
<https://reviews.apache.org/r/68490/#comment292308>

    nit: While this is likely true, copying a shared pointer can be suprisingly non-inexpensive
due to copying of its atomic ref count (likely protected with memory barries, and copies triggering
cache invalidations across threads holding ref counts).
    
    Your benchmarks strongly suggest that we still benefit from this patch.



src/v1/resources.cpp
Lines 2117-2118 (original), 2154-2178 (patched)
<https://reviews.apache.org/r/68490/#comment292305>

    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 Bannier


On Sept. 5, 2018, 11:47 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68490/
> -----------------------------------------------------------
> 
> (Updated Sept. 5, 2018, 11:47 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/3/
> 
> 
> 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