mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Meng Zhu <m...@mesosphere.io>
Subject Re: Review Request 68490: Optimized `class Resources` with copy-on-write.
Date Fri, 07 Sep 2018 03:00:38 GMT


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


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