Re: Review Request 68731: Added a ScalarResourceQuantities type to improve sorters performance.

[Meng Zhu]

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


Ship it!




Ship It!

- Meng Zhu


On Sept. 21, 2018, 4:24 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68731/
> ---
> 
> (Updated Sept. 21, 2018, 4:24 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Meng Zhu.
> 
> 
> Bugs: MESOS-9239
> https://issues.apache.org/jira/browse/MESOS-9239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This type replaces the use of hashmaps keyed by resource names in
> favor of storing vectors of `pair`, in order
> to avoid the performance penalty of using hashmaps.
> 
> Running *HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/21 shows
> the following improvement:
> 
> Using 1 agents and 1000 frameworks
> Added 1000 frameworks in 42.49ms -> 42.85ms (no change)
> Added 1 agents in 7.69secs -> 4.89secs (normalized: 1 -> 0.64)
> round 0 allocate() took 5.42secs -> 3.53secs (nomralized: 1 -> 0.65)
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 71352c848e812b7c499dfbf0f09dc86fac3ee8e1 
>   src/master/allocator/sorter/random/sorter.hpp 
> 6bfeda0f0d02b4738a6d46a7798b1bf4751f0b38 
>   src/master/allocator/sorter/sorter.hpp 
> 432ccfe24ed2854df9cc186a8691009cbdb763c7 
> 
> 
> Diff: https://reviews.apache.org/r/68731/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68731: Added a ScalarResourceQuantities type to improve sorters performance.

[Meng Zhu]

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




src/master/allocator/sorter/sorter.hpp
Lines 177-180 (patched)


we can use `find_if` to make it more concise and readable. I will leave 
that to you. Ditto for `::at`.



src/master/allocator/sorter/sorter.hpp
Lines 204-207 (patched)


Comments need to be updated regarding "first-class resource".



src/master/allocator/sorter/sorter.hpp
Lines 219-222 (patched)


we should be able to do:

```
return quantities.insert(it, std::make_pair(name, Value::Scalar()))->second;
```

Furthur, we can replace the `break` above with this.


- Meng Zhu


On Sept. 21, 2018, 4:24 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68731/
> ---
> 
> (Updated Sept. 21, 2018, 4:24 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Meng Zhu.
> 
> 
> Bugs: MESOS-9239
> https://issues.apache.org/jira/browse/MESOS-9239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This type replaces the use of hashmaps keyed by resource names in
> favor of storing vectors of `pair`, in order
> to avoid the performance penalty of using hashmaps.
> 
> Running *HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/21 shows
> the following improvement:
> 
> Using 1 agents and 1000 frameworks
> Added 1000 frameworks in 42.49ms -> 42.85ms (no change)
> Added 1 agents in 7.69secs -> 4.89secs (normalized: 1 -> 0.64)
> round 0 allocate() took 5.42secs -> 3.53secs (nomralized: 1 -> 0.65)
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 71352c848e812b7c499dfbf0f09dc86fac3ee8e1 
>   src/master/allocator/sorter/random/sorter.hpp 
> 6bfeda0f0d02b4738a6d46a7798b1bf4751f0b38 
>   src/master/allocator/sorter/sorter.hpp 
> 432ccfe24ed2854df9cc186a8691009cbdb763c7 
> 
> 
> Diff: https://reviews.apache.org/r/68731/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68731: Added a ScalarResourceQuantities type to improve sorters performance.

[Benjamin Mahler]

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

(Updated Sept. 21, 2018, 11:24 p.m.)


Review request for mesos, Gastón Kleiman and Meng Zhu.


Changes
---

Updated per feedback.


Bugs: MESOS-9239
https://issues.apache.org/jira/browse/MESOS-9239


Repository: mesos


Description
---

This type replaces the use of hashmaps keyed by resource names in
favor of storing vectors of `pair`, in order
to avoid the performance penalty of using hashmaps.

Running *HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/21 shows
the following improvement:

Using 1 agents and 1000 frameworks
Added 1000 frameworks in 42.49ms -> 42.85ms (no change)
Added 1 agents in 7.69secs -> 4.89secs (normalized: 1 -> 0.64)
round 0 allocate() took 5.42secs -> 3.53secs (nomralized: 1 -> 0.65)


Diffs (updated)
-

  src/master/allocator/sorter/drf/sorter.hpp 
71352c848e812b7c499dfbf0f09dc86fac3ee8e1 
  src/master/allocator/sorter/random/sorter.hpp 
6bfeda0f0d02b4738a6d46a7798b1bf4751f0b38 
  src/master/allocator/sorter/sorter.hpp 
432ccfe24ed2854df9cc186a8691009cbdb763c7 


Diff: https://reviews.apache.org/r/68731/diff/3/

Changes: https://reviews.apache.org/r/68731/diff/2-3/


Testing
---

make check


Thanks,

Benjamin Mahler

Re: Review Request 68731: Added a ScalarResourceQuantities type to improve sorters performance.

[Benjamin Mahler]

> On Sept. 19, 2018, 9:27 p.m., Meng Zhu wrote:
> > src/master/allocator/sorter/sorter.hpp
> > Lines 163-164 (patched)
> > 
> >
> > Why we are enforcing alphabetically sorted? Seems to me unnecessary, 
> > especially we do not force it.

We don't rely on it yet, but it will be beneficial for arithmetic operations. 
I'll add a comment for that.


> On Sept. 19, 2018, 9:27 p.m., Meng Zhu wrote:
> > src/master/allocator/sorter/sorter.hpp
> > Lines 194 (patched)
> > 
> >
> > I think it is a good idea to print the whole vector as well.

I added a TODO, it's a little verbose unless we have a `stringify(const 
pair& p)` overload.


- Benjamin


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


On Sept. 18, 2018, 9:20 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68731/
> ---
> 
> (Updated Sept. 18, 2018, 9:20 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Meng Zhu.
> 
> 
> Bugs: MESOS-9239
> https://issues.apache.org/jira/browse/MESOS-9239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This type replaces the use of hashmaps keyed by resource names in
> favor of storing vectors of `pair`, in order
> to avoid the performance penalty of using hashmaps.
> 
> Running *HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/21 shows
> the following improvement:
> 
> Using 1 agents and 1000 frameworks
> Added 1000 frameworks in 42.49ms -> 42.85ms (no change)
> Added 1 agents in 7.69secs -> 4.89secs (normalized: 1 -> 0.64)
> round 0 allocate() took 5.42secs -> 3.53secs (nomralized: 1 -> 0.65)
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 5a4fa5e2dca61168923261230b1f5c245354cbb7 
>   src/master/allocator/sorter/random/sorter.hpp 
> 7f6c0de70e3ae03d7362fb9e140b93435e530499 
>   src/master/allocator/sorter/sorter.hpp 
> 432ccfe24ed2854df9cc186a8691009cbdb763c7 
> 
> 
> Diff: https://reviews.apache.org/r/68731/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68731: Added a ScalarResourceQuantities type to improve sorters performance.

[Meng Zhu]

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



I am honestly surprised by the improvement this patch brings. Did not expect 
the hashmap has such high overhead, given the keys are short strings. But 
numbers are numbers :)


src/master/allocator/sorter/sorter.hpp
Lines 163-164 (patched)


Why we are enforcing alphabetically sorted? Seems to me unnecessary, 
especially we do not force it.



src/master/allocator/sorter/sorter.hpp
Lines 176 (patched)


If the value is zero, we still return `true`? That is quite 
counter-intuitive.

Also, consider using `find_if` for simplicity. Ditt below.



src/master/allocator/sorter/sorter.hpp
Lines 194 (patched)


I think it is a good idea to print the whole vector as well.


- Meng Zhu


On Sept. 18, 2018, 2:20 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68731/
> ---
> 
> (Updated Sept. 18, 2018, 2:20 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Meng Zhu.
> 
> 
> Bugs: MESOS-9239
> https://issues.apache.org/jira/browse/MESOS-9239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This type replaces the use of hashmaps keyed by resource names in
> favor of storing vectors of `pair`, in order
> to avoid the performance penalty of using hashmaps.
> 
> Running *HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/21 shows
> the following improvement:
> 
> Using 1 agents and 1000 frameworks
> Added 1000 frameworks in 42.49ms -> 42.85ms (no change)
> Added 1 agents in 7.69secs -> 4.89secs (normalized: 1 -> 0.64)
> round 0 allocate() took 5.42secs -> 3.53secs (nomralized: 1 -> 0.65)
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 5a4fa5e2dca61168923261230b1f5c245354cbb7 
>   src/master/allocator/sorter/random/sorter.hpp 
> 7f6c0de70e3ae03d7362fb9e140b93435e530499 
>   src/master/allocator/sorter/sorter.hpp 
> 432ccfe24ed2854df9cc186a8691009cbdb763c7 
> 
> 
> Diff: https://reviews.apache.org/r/68731/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68731: Added a ScalarResourceQuantities type to improve sorters performance.

[Benjamin Mahler]

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

(Updated Sept. 18, 2018, 9:20 p.m.)


Review request for mesos, Gastón Kleiman and Meng Zhu.


Changes
---

Updated the random sorter as well.


Summary (updated)
-

Added a ScalarResourceQuantities type to improve sorters performance.


Bugs: MESOS-9239
https://issues.apache.org/jira/browse/MESOS-9239


Repository: mesos


Description
---

This type replaces the use of hashmaps keyed by resource names in
favor of storing vectors of `pair`, in order
to avoid the performance penalty of using hashmaps.

Running *HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/21 shows
the following improvement:

Using 1 agents and 1000 frameworks
Added 1000 frameworks in 42.49ms -> 42.85ms (no change)
Added 1 agents in 7.69secs -> 4.89secs (normalized: 1 -> 0.64)
round 0 allocate() took 5.42secs -> 3.53secs (nomralized: 1 -> 0.65)


Diffs (updated)
-

  src/master/allocator/sorter/drf/sorter.hpp 
5a4fa5e2dca61168923261230b1f5c245354cbb7 
  src/master/allocator/sorter/random/sorter.hpp 
7f6c0de70e3ae03d7362fb9e140b93435e530499 
  src/master/allocator/sorter/sorter.hpp 
432ccfe24ed2854df9cc186a8691009cbdb763c7 


Diff: https://reviews.apache.org/r/68731/diff/2/

Changes: https://reviews.apache.org/r/68731/diff/1-2/


Testing
---

make check


Thanks,

Benjamin Mahler