You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2018/09/17 02:59:16 UTC
Review Request 68731: Added a ScalarResourceQuantities type to improve
sorter performance.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68731/
-----------------------------------------------------------
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<string,Value::Scalar>`, in order
to avoid the performance penalty of using hashmaps.
Running *HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/21 shows
the following improvement:
Using 10000 agents and 1000 frameworks
Added 1000 frameworks in 42.49ms -> 42.85ms (no change)
Added 10000 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
Diff: https://reviews.apache.org/r/68731/diff/1/
Testing
-------
make check
Thanks,
Benjamin Mahler
Re: Review Request 68731: Added a ScalarResourceQuantities type to
improve sorters performance.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Sept. 19, 2018, 9:27 p.m., Meng Zhu wrote:
> > src/master/allocator/sorter/sorter.hpp
> > Lines 163-164 (patched)
> > <https://reviews.apache.org/r/68731/diff/2/?file=2090164#file2090164line163>
> >
> > 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)
> > <https://reviews.apache.org/r/68731/diff/2/?file=2090164#file2090164line194>
> >
> > 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<T1, T2>& 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<string,Value::Scalar>`, in order
> to avoid the performance penalty of using hashmaps.
>
> Running *HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/21 shows
> the following improvement:
>
> Using 10000 agents and 1000 frameworks
> Added 1000 frameworks in 42.49ms -> 42.85ms (no change)
> Added 10000 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.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/68731/#comment292956>
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)
<https://reviews.apache.org/r/68731/#comment292955>
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)
<https://reviews.apache.org/r/68731/#comment292951>
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<string,Value::Scalar>`, in order
> to avoid the performance penalty of using hashmaps.
>
> Running *HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/21 shows
> the following improvement:
>
> Using 10000 agents and 1000 frameworks
> Added 1000 frameworks in 42.49ms -> 42.85ms (no change)
> Added 10000 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.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/68731/#comment293187>
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)
<https://reviews.apache.org/r/68731/#comment293185>
Comments need to be updated regarding "first-class resource".
src/master/allocator/sorter/sorter.hpp
Lines 219-222 (patched)
<https://reviews.apache.org/r/68731/#comment293186>
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<string,Value::Scalar>`, in order
> to avoid the performance penalty of using hashmaps.
>
> Running *HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/21 shows
> the following improvement:
>
> Using 10000 agents and 1000 frameworks
> Added 1000 frameworks in 42.49ms -> 42.85ms (no change)
> Added 10000 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.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
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<string,Value::Scalar>`, in order
> to avoid the performance penalty of using hashmaps.
>
> Running *HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/21 shows
> the following improvement:
>
> Using 10000 agents and 1000 frameworks
> Added 1000 frameworks in 42.49ms -> 42.85ms (no change)
> Added 10000 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.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
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<string,Value::Scalar>`, in order
to avoid the performance penalty of using hashmaps.
Running *HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/21 shows
the following improvement:
Using 10000 agents and 1000 frameworks
Added 1000 frameworks in 42.49ms -> 42.85ms (no change)
Added 10000 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.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
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<string,Value::Scalar>`, in order
to avoid the performance penalty of using hashmaps.
Running *HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/21 shows
the following improvement:
Using 10000 agents and 1000 frameworks
Added 1000 frameworks in 42.49ms -> 42.85ms (no change)
Added 10000 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
Re: Review Request 68731: Added a ScalarResourceQuantities type to
improve sorter performance.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68731/#review208679
-----------------------------------------------------------
PASS: Mesos patch 68731 was successfully built and tested.
Reviews applied: `['68729', '68730', '68731']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2344/mesos-review-68731
- Mesos Reviewbot Windows
On Sept. 16, 2018, 7:59 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68731/
> -----------------------------------------------------------
>
> (Updated Sept. 16, 2018, 7:59 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<string,Value::Scalar>`, in order
> to avoid the performance penalty of using hashmaps.
>
> Running *HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/21 shows
> the following improvement:
>
> Using 10000 agents and 1000 frameworks
> Added 1000 frameworks in 42.49ms -> 42.85ms (no change)
> Added 10000 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
>
>
> Diff: https://reviews.apache.org/r/68731/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>