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 2017/12/07 20:32:22 UTC
Review Request 64428: Eliminated some copying of tasks / executors in
agent re-registration.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64428/
-----------------------------------------------------------
Review request for mesos, Dmitry Zhuk and Michael Park.
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/master/master.hpp 2658312b0d10a72fefda68c7d3137b94afbd8249
src/master/master.cpp 2fd66c072e9a194680d7653c664bd8a68ea1d2f0
Diff: https://reviews.apache.org/r/64428/diff/1/
Testing
-------
About a 15% improvement.
Before:
Reregistered 2000 agents with a total of 100000 running tasks and 100000 completed tasks in 2.977767303secs
Reregistered 2000 agents with a total of 200000 running tasks and 0 completed tasks in 5.642229947secs
Reregistered 20000 agents with a total of 100000 running tasks and 0 completed tasks in 6.966318315secs
After:
Reregistered 2000 agents with a total of 100000 running tasks and 100000 completed tasks in 2.623647005secs
Reregistered 2000 agents with a total of 200000 running tasks and 0 completed tasks in 5.032204158secs
Reregistered 20000 agents with a total of 100000 running tasks and 0 completed tasks in 6.187945743secs
Thanks,
Benjamin Mahler
Re: Review Request 64428: Eliminated some copying of tasks /
executors in agent re-registration.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Dec. 7, 2017, 10:33 p.m., Michael Park wrote:
> > src/master/master.cpp
> > Lines 6675-6684 (original)
> > <https://reviews.apache.org/r/64428/diff/3/?file=1910803#file1910803line6677>
> >
> > It looks like we still do all of these (via move) except `tasks`. I would've expected to see these still here but turned into moves. What was the motivation for the new pattern?
Mainly to have the mutation of the message more directly mutating the message, per offline chat.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64428/#review193184
-----------------------------------------------------------
On Dec. 7, 2017, 8:46 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64428/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2017, 8:46 p.m.)
>
>
> Review request for mesos, Dmitry Zhuk and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp 2658312b0d10a72fefda68c7d3137b94afbd8249
> src/master/master.cpp 2fd66c072e9a194680d7653c664bd8a68ea1d2f0
>
>
> Diff: https://reviews.apache.org/r/64428/diff/3/
>
>
> Testing
> -------
>
> About a 10-20% improvement.
>
> Before:
> Reregistered 2000 agents with a total of 100000 running tasks and 100000 completed tasks in 2.977767303secs
> Reregistered 2000 agents with a total of 200000 running tasks and 0 completed tasks in 5.642229947secs
> Reregistered 20000 agents with a total of 100000 running tasks and 0 completed tasks in 6.966318315secs
>
> After:
> Reregistered 2000 agents with a total of 100000 running tasks and 100000 completed tasks in 2.591612374secs
> Reregistered 2000 agents with a total of 200000 running tasks and 0 completed tasks in 4.52633125secs
> Reregistered 20000 agents with a total of 100000 running tasks and 0 completed tasks in 6.449984263secs
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 64428: Eliminated some copying of tasks /
executors in agent re-registration.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64428/#review193184
-----------------------------------------------------------
Fix it, then Ship it!
src/master/master.cpp
Lines 6675-6684 (original)
<https://reviews.apache.org/r/64428/#comment271721>
It looks like we still do all of these (via move) except `tasks`. I would've expected to see these still here but turned into moves. What was the motivation for the new pattern?
src/master/master.cpp
Lines 6690 (patched)
<https://reviews.apache.org/r/64428/#comment271720>
Shouldn't need `std::cref` here.
- Michael Park
On Dec. 7, 2017, 12:46 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64428/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2017, 12:46 p.m.)
>
>
> Review request for mesos, Dmitry Zhuk and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp 2658312b0d10a72fefda68c7d3137b94afbd8249
> src/master/master.cpp 2fd66c072e9a194680d7653c664bd8a68ea1d2f0
>
>
> Diff: https://reviews.apache.org/r/64428/diff/3/
>
>
> Testing
> -------
>
> About a 10-20% improvement.
>
> Before:
> Reregistered 2000 agents with a total of 100000 running tasks and 100000 completed tasks in 2.977767303secs
> Reregistered 2000 agents with a total of 200000 running tasks and 0 completed tasks in 5.642229947secs
> Reregistered 20000 agents with a total of 100000 running tasks and 0 completed tasks in 6.966318315secs
>
> After:
> Reregistered 2000 agents with a total of 100000 running tasks and 100000 completed tasks in 2.591612374secs
> Reregistered 2000 agents with a total of 200000 running tasks and 0 completed tasks in 4.52633125secs
> Reregistered 20000 agents with a total of 100000 running tasks and 0 completed tasks in 6.449984263secs
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 64428: Eliminated some copying of tasks /
executors in agent re-registration.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Dec. 7, 2017, 10:15 p.m., Dmitry Zhuk wrote:
> > src/master/master.hpp
> > Line 713 (original), 713 (patched)
> > <https://reviews.apache.org/r/64428/diff/3/?file=1910802#file1910802line713>
> >
> > shall we keep a default value?
I can update this to take an rvalue reference, at which point I can't have a default right? That was originally why I removed it.
> On Dec. 7, 2017, 10:15 p.m., Dmitry Zhuk wrote:
> > src/master/master.cpp
> > Line 6700 (original), 6694 (patched)
> > <https://reviews.apache.org/r/64428/diff/3/?file=1910803#file1910803line6704>
> >
> > btw, if large number of frameworks is a concern, we could also avoid double lookup in `hashmap` with something like
> > ```
> > auto it = fameworks.find(task.framework_id());
> >
> > CHECK(it != frameworks.end());
> >
> > injectAllocationInfo(
> > task.mutable_resources(),
> > it->second);
> > ```
Good point, we indeed have some excessive lookups, will look into the impact of this in a separate patch.
> On Dec. 7, 2017, 10:15 p.m., Dmitry Zhuk wrote:
> > src/master/master.cpp
> > Lines 6770 (patched)
> > <https://reviews.apache.org/r/64428/diff/3/?file=1910803#file1910803line6780>
> >
> > it seems that we can convert directly to `Resources` instead. `Slave`'s constructor for some reason requires `vector<Resources>`, but then converts to `Resources` anyway
It was a little messy to do this since the registration path also uses the constructor, will maybe look into it as a follow up.
> On Dec. 7, 2017, 10:15 p.m., Dmitry Zhuk wrote:
> > src/master/master.cpp
> > Lines 6837-6839 (patched)
> > <https://reviews.apache.org/r/64428/diff/3/?file=1910803#file1910803line6847>
> >
> > we don't really need a conversion to `vector` for `updateSlaveFrameworks` if performance here is a concern. it could take `RepeatedPtrField<FrameworkInfo>` as parameter instead.
> > this is also true for `addSlave`
Hm.. yeah, good point, I will explore that in a subsequent patch, see if it's helpful.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64428/#review193167
-----------------------------------------------------------
On Dec. 7, 2017, 8:46 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64428/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2017, 8:46 p.m.)
>
>
> Review request for mesos, Dmitry Zhuk and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp 2658312b0d10a72fefda68c7d3137b94afbd8249
> src/master/master.cpp 2fd66c072e9a194680d7653c664bd8a68ea1d2f0
>
>
> Diff: https://reviews.apache.org/r/64428/diff/3/
>
>
> Testing
> -------
>
> About a 10-20% improvement.
>
> Before:
> Reregistered 2000 agents with a total of 100000 running tasks and 100000 completed tasks in 2.977767303secs
> Reregistered 2000 agents with a total of 200000 running tasks and 0 completed tasks in 5.642229947secs
> Reregistered 20000 agents with a total of 100000 running tasks and 0 completed tasks in 6.966318315secs
>
> After:
> Reregistered 2000 agents with a total of 100000 running tasks and 100000 completed tasks in 2.591612374secs
> Reregistered 2000 agents with a total of 200000 running tasks and 0 completed tasks in 4.52633125secs
> Reregistered 20000 agents with a total of 100000 running tasks and 0 completed tasks in 6.449984263secs
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 64428: Eliminated some copying of tasks /
executors in agent re-registration.
Posted by Dmitry Zhuk <dz...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64428/#review193167
-----------------------------------------------------------
src/master/master.hpp
Line 713 (original), 713 (patched)
<https://reviews.apache.org/r/64428/#comment271703>
shall we keep a default value?
src/master/master.cpp
Line 6700 (original), 6694 (patched)
<https://reviews.apache.org/r/64428/#comment271700>
btw, if large number of frameworks is a concern, we could also avoid double lookup in `hashmap` with something like
```
auto it = fameworks.find(task.framework_id());
CHECK(it != frameworks.end());
injectAllocationInfo(
task.mutable_resources(),
it->second);
```
src/master/master.cpp
Lines 6770 (patched)
<https://reviews.apache.org/r/64428/#comment271702>
it seems that we can convert directly to `Resources` instead. `Slave`'s constructor for some reason requires `vector<Resources>`, but then converts to `Resources` anyway
src/master/master.cpp
Lines 6837-6839 (patched)
<https://reviews.apache.org/r/64428/#comment271706>
we don't really need a conversion to `vector` for `updateSlaveFrameworks` if performance here is a concern. it could take `RepeatedPtrField<FrameworkInfo>` as parameter instead.
this is also true for `addSlave`
- Dmitry Zhuk
On Dec. 7, 2017, 8:46 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64428/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2017, 8:46 p.m.)
>
>
> Review request for mesos, Dmitry Zhuk and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp 2658312b0d10a72fefda68c7d3137b94afbd8249
> src/master/master.cpp 2fd66c072e9a194680d7653c664bd8a68ea1d2f0
>
>
> Diff: https://reviews.apache.org/r/64428/diff/3/
>
>
> Testing
> -------
>
> About a 10-20% improvement.
>
> Before:
> Reregistered 2000 agents with a total of 100000 running tasks and 100000 completed tasks in 2.977767303secs
> Reregistered 2000 agents with a total of 200000 running tasks and 0 completed tasks in 5.642229947secs
> Reregistered 20000 agents with a total of 100000 running tasks and 0 completed tasks in 6.966318315secs
>
> After:
> Reregistered 2000 agents with a total of 100000 running tasks and 100000 completed tasks in 2.591612374secs
> Reregistered 2000 agents with a total of 200000 running tasks and 0 completed tasks in 4.52633125secs
> Reregistered 20000 agents with a total of 100000 running tasks and 0 completed tasks in 6.449984263secs
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 64428: Eliminated some copying of tasks /
executors in agent re-registration.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64428/
-----------------------------------------------------------
(Updated Dec. 7, 2017, 8:46 p.m.)
Review request for mesos, Dmitry Zhuk and Michael Park.
Changes
-------
Simplified the patch by taking a `T` as arguments.
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/master/master.hpp 2658312b0d10a72fefda68c7d3137b94afbd8249
src/master/master.cpp 2fd66c072e9a194680d7653c664bd8a68ea1d2f0
Diff: https://reviews.apache.org/r/64428/diff/1/
Testing (updated)
-------
About a 10-20% improvement.
Before:
Reregistered 2000 agents with a total of 100000 running tasks and 100000 completed tasks in 2.977767303secs
Reregistered 2000 agents with a total of 200000 running tasks and 0 completed tasks in 5.642229947secs
Reregistered 20000 agents with a total of 100000 running tasks and 0 completed tasks in 6.966318315secs
After:
Reregistered 2000 agents with a total of 100000 running tasks and 100000 completed tasks in 2.591612374secs
Reregistered 2000 agents with a total of 200000 running tasks and 0 completed tasks in 4.52633125secs
Reregistered 20000 agents with a total of 100000 running tasks and 0 completed tasks in 6.449984263secs
Thanks,
Benjamin Mahler
Re: Review Request 64428: Eliminated some copying of tasks /
executors in agent re-registration.
Posted by Dmitry Zhuk <dz...@twopensource.com>.
> On Dec. 7, 2017, 8:37 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Line 9480 (original), 9495 (patched)
> > <https://reviews.apache.org/r/64428/diff/1/?file=1910739#file1910739line9505>
> >
> > Should I just take a copy here to avoid forcing the caller to provide an rvalue reference? Much like we did in the `Slave` struct.
Pass-by-value in `Slave` was mainly directed by compatibility with `registerSlave`. Ideally, we should migrate `registerSlave` to direct usage of message too.
We can stick to https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rf-conventional, which advices against using pass-by-value here.
- Dmitry
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64428/#review193163
-----------------------------------------------------------
On Dec. 7, 2017, 8:46 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64428/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2017, 8:46 p.m.)
>
>
> Review request for mesos, Dmitry Zhuk and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp 2658312b0d10a72fefda68c7d3137b94afbd8249
> src/master/master.cpp 2fd66c072e9a194680d7653c664bd8a68ea1d2f0
>
>
> Diff: https://reviews.apache.org/r/64428/diff/3/
>
>
> Testing
> -------
>
> About a 10-20% improvement.
>
> Before:
> Reregistered 2000 agents with a total of 100000 running tasks and 100000 completed tasks in 2.977767303secs
> Reregistered 2000 agents with a total of 200000 running tasks and 0 completed tasks in 5.642229947secs
> Reregistered 20000 agents with a total of 100000 running tasks and 0 completed tasks in 6.966318315secs
>
> After:
> Reregistered 2000 agents with a total of 100000 running tasks and 100000 completed tasks in 2.591612374secs
> Reregistered 2000 agents with a total of 200000 running tasks and 0 completed tasks in 4.52633125secs
> Reregistered 20000 agents with a total of 100000 running tasks and 0 completed tasks in 6.449984263secs
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 64428: Eliminated some copying of tasks /
executors in agent re-registration.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64428/#review193163
-----------------------------------------------------------
src/master/master.cpp
Line 9480 (original), 9495 (patched)
<https://reviews.apache.org/r/64428/#comment271679>
Should I just take a copy here to avoid forcing the caller to provide an rvalue reference? Much like we did in the `Slave` struct.
- Benjamin Mahler
On Dec. 7, 2017, 8:32 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64428/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2017, 8:32 p.m.)
>
>
> Review request for mesos, Dmitry Zhuk and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp 2658312b0d10a72fefda68c7d3137b94afbd8249
> src/master/master.cpp 2fd66c072e9a194680d7653c664bd8a68ea1d2f0
>
>
> Diff: https://reviews.apache.org/r/64428/diff/1/
>
>
> Testing
> -------
>
> About a 15% improvement.
>
> Before:
> Reregistered 2000 agents with a total of 100000 running tasks and 100000 completed tasks in 2.977767303secs
> Reregistered 2000 agents with a total of 200000 running tasks and 0 completed tasks in 5.642229947secs
> Reregistered 20000 agents with a total of 100000 running tasks and 0 completed tasks in 6.966318315secs
>
> After:
> Reregistered 2000 agents with a total of 100000 running tasks and 100000 completed tasks in 2.623647005secs
> Reregistered 2000 agents with a total of 200000 running tasks and 0 completed tasks in 5.032204158secs
> Reregistered 20000 agents with a total of 100000 running tasks and 0 completed tasks in 6.187945743secs
>
>
> Thanks,
>
> Benjamin Mahler
>
>