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 Wed, 12 Sep 2018 02:25:49 GMT

-----------------------------------------------------------
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
> 
>


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