Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2427413005:
Improve slave-preference function (Closed)
|
Created:
4 years, 2 months ago by Michael Achenbach Modified:
4 years, 1 month ago CC:
chromium-reviews, infra-reviews+build_chromium.org, kjellander-cc_chromium.org Target Ref:
refs/heads/master Project:
build Visibility:
Public. |
DescriptionImprove slave-preference function
The current slave-preference function has the following
flaw:
When the system is under medium load, preferred slaves
are often occupied, so that each new build starts taking
slaves preferring other builders. This in turn leads to more
slaves not being available for their preferred builders.
The new implementation has the following strategy:
Let slaves(builder) be the number of available slaves
preferring that builder. For builder A and available
slaves S, choose s in S such that (order is preference):
1. s prefers A, or
2. s prefers nothing (an optional fall-over pool), or
3. s prefers some B != A and there's no other builder C != B
such that slaves(C) > slaves(B)
BUG=658189
Committed: https://chromium.googlesource.com/chromium/tools/build/+/f71cca8a090ef8d7608af034b5e2bdcdc7df2bc5
Patch Set 1 #Patch Set 2 : Improve slave preference function #
Total comments: 4
Patch Set 3 : Keep new function side-by-side with old #Patch Set 4 : Better and simpler implementation #Patch Set 5 : Test readability #
Total comments: 18
Patch Set 6 : Review tandrii #Patch Set 7 : Presubmit #Messages
Total messages: 27 (12 generated)
Description was changed from ========== Improve slave preference function BUG= ========== to ========== Improve slave-preference function The current slave-preference function has the following flaw: When the system is under medium load, preferred slaves are often occupied, so that each new build starts taking slaves preferring other builders. This in turn leads to more slaves not being available for their preferred builders. The new implementation has the following strategy: Be slaves(builder) the number of available slaves preferring that builder. For builder A and available slaves S, choose s in S such that (order is preference): 1. s prefers A 2. s prefers nothing (a fall-over pool) 3. s prefers some B != A and there's no other builder C != B such that slaves(C) > slaves(B) ==========
On 2016/10/21 09:28:21, machenbach (slow) wrote: > Description was changed from > > ========== > Improve slave preference function > > BUG= > ========== > > to > > ========== > Improve slave-preference function > > The current slave-preference function has the following > flaw: > > When the system is under medium load, preferred slaves > are often occupied, so that each new build starts taking > slaves preferring other builders. This in turn leads to more s/slaves preferring other builders/preferred slaves of other builders .. but now I get it that you used "preference" as a two-way relationship slave prefers builder <=> builder prefers slave. So maybe leave as is, it makes sense now. > slaves not being available for their preferred builders. > > The new implementation has the following strategy: > > Be slaves(builder) the number of available slaves preferring how about s/Be/Let s.t. Let slaves(builder) be the ....
Description was changed from ========== Improve slave-preference function The current slave-preference function has the following flaw: When the system is under medium load, preferred slaves are often occupied, so that each new build starts taking slaves preferring other builders. This in turn leads to more slaves not being available for their preferred builders. The new implementation has the following strategy: Be slaves(builder) the number of available slaves preferring that builder. For builder A and available slaves S, choose s in S such that (order is preference): 1. s prefers A 2. s prefers nothing (a fall-over pool) 3. s prefers some B != A and there's no other builder C != B such that slaves(C) > slaves(B) ========== to ========== Improve slave-preference function The current slave-preference function has the following flaw: When the system is under medium load, preferred slaves are often occupied, so that each new build starts taking slaves preferring other builders. This in turn leads to more slaves not being available for their preferred builders. The new implementation has the following strategy: Let slaves(builder) be the number of available slaves preferring that builder. For builder A and available slaves S, choose s in S such that (order is preference): 1. s prefers A 2. s prefers nothing (a fall-over pool) 3. s prefers some B != A and there's no other builder C != B such that slaves(C) > slaves(B) ==========
tandrii@chromium.org changed reviewers: + tandrii@chromium.org
I'm on board with this change. The suggested implementation will run super fast when master is under load, as there will be few slaves, and otherwise master is not under load and code essentially loops over all available slaves exactly twice regardless of preferences distribution. https://codereview.chromium.org/2427413005/diff/20001/scripts/master/master_u... File scripts/master/master_utils.py (right): https://codereview.chromium.org/2427413005/diff/20001/scripts/master/master_u... scripts/master/master_utils.py:671: first, consider adding new functionality behind a switch. second, how about this (as brainstormed offline): prefs = collections.Counter(s.preferred_builder for s in slave_builders) if builder in prefs: key = builder elif None in prefs: key = None else: key = prefs.most_common()[0] return choice(s for s in slave_builders if s.preferred_builder == key) # As discussed, last loop over all slaves_builders can be optimized by storing randomly selected slave next to count of slave so far in prefs{}. https://codereview.chromium.org/2427413005/diff/20001/scripts/master/unittest... File scripts/master/unittests/master_utils_test.py (right): https://codereview.chromium.org/2427413005/diff/20001/scripts/master/unittest... scripts/master/unittests/master_utils_test.py:80: del slaves[4] def remove(slaves): iterate and find and remove remove('s6') is nicer.
PTAL. New function now suffixed with "NG" :) If it looks good to your, I'll include more people in the review. https://codereview.chromium.org/2427413005/diff/20001/scripts/master/master_u... File scripts/master/master_utils.py (right): https://codereview.chromium.org/2427413005/diff/20001/scripts/master/master_u... scripts/master/master_utils.py:671: On 2016/10/21 10:39:16, tandrii(chromium) wrote: > first, consider adding new functionality behind a switch. > second, how about this (as brainstormed offline): > > prefs = collections.Counter(s.preferred_builder for s in slave_builders) > if builder in prefs: > key = builder > elif None in prefs: > key = None > else: > key = prefs.most_common()[0] > return choice(s for s in slave_builders if s.preferred_builder == key) > > > # As discussed, last loop over all slaves_builders can be optimized by storing > randomly selected slave next to count of slave so far in prefs{}. Used the first simple method with minor modifications. I don't think more optimization is necessary at that point. Note that the semantics now change: a set of several sets of equally available slaves is chosen now arbitrarily (not randomly) based on dictionary order. Only the slave from the set will be chosen randomly. I don't think it matters and it makes the algorithm faster. https://codereview.chromium.org/2427413005/diff/20001/scripts/master/unittest... File scripts/master/unittests/master_utils_test.py (right): https://codereview.chromium.org/2427413005/diff/20001/scripts/master/unittest... scripts/master/unittests/master_utils_test.py:80: del slaves[4] On 2016/10/21 10:39:17, tandrii(chromium) wrote: > def remove(slaves): > iterate and find and remove > > remove('s6') is nicer. Done.
Description was changed from ========== Improve slave-preference function The current slave-preference function has the following flaw: When the system is under medium load, preferred slaves are often occupied, so that each new build starts taking slaves preferring other builders. This in turn leads to more slaves not being available for their preferred builders. The new implementation has the following strategy: Let slaves(builder) be the number of available slaves preferring that builder. For builder A and available slaves S, choose s in S such that (order is preference): 1. s prefers A 2. s prefers nothing (a fall-over pool) 3. s prefers some B != A and there's no other builder C != B such that slaves(C) > slaves(B) ========== to ========== Improve slave-preference function The current slave-preference function has the following flaw: When the system is under medium load, preferred slaves are often occupied, so that each new build starts taking slaves preferring other builders. This in turn leads to more slaves not being available for their preferred builders. The new implementation has the following strategy: Let slaves(builder) be the number of available slaves preferring that builder. For builder A and available slaves S, choose s in S such that (order is preference): 1. s prefers A 2. s prefers nothing (an optional fall-over pool) 3. s prefers some B != A and there's no other builder C != B such that slaves(C) > slaves(B) ==========
LGTM % suggetsions https://codereview.chromium.org/2427413005/diff/80001/scripts/master/master_u... File scripts/master/master_utils.py (right): https://codereview.chromium.org/2427413005/diff/80001/scripts/master/master_u... scripts/master/master_utils.py:678: prefer any builder if such a slave is available. If not, a slave is randomly if such a slave is available => if any :) https://codereview.chromium.org/2427413005/diff/80001/scripts/master/master_u... scripts/master/master_utils.py:679: chosen with preference on slaves with higher capacity. If several sets of with preference on => preferring and maybe slaves with higher capacity => slaves whose preferred builder has largest currently available capacity ? https://codereview.chromium.org/2427413005/diff/80001/scripts/master/master_u... scripts/master/master_utils.py:680: slaves have equally high capacity, a set is chosen arbitrarily dependent on i'd not be that specific, but whatever :) https://codereview.chromium.org/2427413005/diff/80001/scripts/master/master_u... scripts/master/master_utils.py:686: self.choice = choice nit: self._choice is nicer, IMO https://codereview.chromium.org/2427413005/diff/80001/scripts/master/unittest... File scripts/master/unittests/master_utils_test.py (right): https://codereview.chromium.org/2427413005/diff/80001/scripts/master/unittest... scripts/master/unittests/master_utils_test.py:20: return list(iterable) imo, use lambda x: list(x) in place of actual need. It's easier to read then looking what mock_random_choice is. https://codereview.chromium.org/2427413005/diff/80001/scripts/master/unittest... scripts/master/unittests/master_utils_test.py:27: break for ... ... else: assert False, 'slave %s does not exist' % name https://codereview.chromium.org/2427413005/diff/80001/scripts/master/unittest... scripts/master/unittests/master_utils_test.py:112: # Remove slave 3. no need for comment. https://codereview.chromium.org/2427413005/diff/80001/scripts/master/unittest... scripts/master/unittests/master_utils_test.py:118: # Remove slave 6. ditto https://codereview.chromium.org/2427413005/diff/80001/scripts/master/unittest... scripts/master/unittests/master_utils_test.py:129: # Remove slaves 1, 7 and 8. ditto
Done, thanks for review! https://codereview.chromium.org/2427413005/diff/80001/scripts/master/master_u... File scripts/master/master_utils.py (right): https://codereview.chromium.org/2427413005/diff/80001/scripts/master/master_u... scripts/master/master_utils.py:678: prefer any builder if such a slave is available. If not, a slave is randomly On 2016/10/21 13:39:45, tandrii(chromium) wrote: > if such a slave is available => if any :) Done. https://codereview.chromium.org/2427413005/diff/80001/scripts/master/master_u... scripts/master/master_utils.py:679: chosen with preference on slaves with higher capacity. If several sets of On 2016/10/21 13:39:45, tandrii(chromium) wrote: > with preference on => preferring > > and maybe > > slaves with higher capacity => slaves whose preferred builder has largest > currently available capacity > > ? Done. https://codereview.chromium.org/2427413005/diff/80001/scripts/master/master_u... scripts/master/master_utils.py:680: slaves have equally high capacity, a set is chosen arbitrarily dependent on On 2016/10/21 13:39:45, tandrii(chromium) wrote: > i'd not be that specific, but whatever :) Lets keep it. https://codereview.chromium.org/2427413005/diff/80001/scripts/master/master_u... scripts/master/master_utils.py:686: self.choice = choice On 2016/10/21 13:39:45, tandrii(chromium) wrote: > nit: self._choice is nicer, IMO Done. https://codereview.chromium.org/2427413005/diff/80001/scripts/master/unittest... File scripts/master/unittests/master_utils_test.py (right): https://codereview.chromium.org/2427413005/diff/80001/scripts/master/unittest... scripts/master/unittests/master_utils_test.py:20: return list(iterable) On 2016/10/21 13:39:45, tandrii(chromium) wrote: > imo, use lambda x: list(x) in place of actual need. It's easier to read then > looking what mock_random_choice is. Done. https://codereview.chromium.org/2427413005/diff/80001/scripts/master/unittest... scripts/master/unittests/master_utils_test.py:27: break On 2016/10/21 13:39:45, tandrii(chromium) wrote: > for ... > ... > else: > assert False, 'slave %s does not exist' % name Done. https://codereview.chromium.org/2427413005/diff/80001/scripts/master/unittest... scripts/master/unittests/master_utils_test.py:112: # Remove slave 3. On 2016/10/21 13:39:45, tandrii(chromium) wrote: > no need for comment. Done. https://codereview.chromium.org/2427413005/diff/80001/scripts/master/unittest... scripts/master/unittests/master_utils_test.py:118: # Remove slave 6. On 2016/10/21 13:39:45, tandrii(chromium) wrote: > ditto Done. https://codereview.chromium.org/2427413005/diff/80001/scripts/master/unittest... scripts/master/unittests/master_utils_test.py:129: # Remove slaves 1, 7 and 8. On 2016/10/21 13:39:46, tandrii(chromium) wrote: > ditto Done.
machenbach@chromium.org changed reviewers: + phajdan.jr@chromium.org, sergeyberezin@chromium.org, shinyak@chromium.org
+ other infra reviewers working on the original method. WDYT?
Description was changed from ========== Improve slave-preference function The current slave-preference function has the following flaw: When the system is under medium load, preferred slaves are often occupied, so that each new build starts taking slaves preferring other builders. This in turn leads to more slaves not being available for their preferred builders. The new implementation has the following strategy: Let slaves(builder) be the number of available slaves preferring that builder. For builder A and available slaves S, choose s in S such that (order is preference): 1. s prefers A 2. s prefers nothing (an optional fall-over pool) 3. s prefers some B != A and there's no other builder C != B such that slaves(C) > slaves(B) ========== to ========== Improve slave-preference function The current slave-preference function has the following flaw: When the system is under medium load, preferred slaves are often occupied, so that each new build starts taking slaves preferring other builders. This in turn leads to more slaves not being available for their preferred builders. The new implementation has the following strategy: Let slaves(builder) be the number of available slaves preferring that builder. For builder A and available slaves S, choose s in S such that (order is preference): 1. s prefers A 2. s prefers nothing (an optional fall-over pool) 3. s prefers some B != A and there's no other builder C != B such that slaves(C) > slaves(B) BUG=658189 ==========
Description was changed from ========== Improve slave-preference function The current slave-preference function has the following flaw: When the system is under medium load, preferred slaves are often occupied, so that each new build starts taking slaves preferring other builders. This in turn leads to more slaves not being available for their preferred builders. The new implementation has the following strategy: Let slaves(builder) be the number of available slaves preferring that builder. For builder A and available slaves S, choose s in S such that (order is preference): 1. s prefers A 2. s prefers nothing (an optional fall-over pool) 3. s prefers some B != A and there's no other builder C != B such that slaves(C) > slaves(B) BUG=658189 ========== to ========== Improve slave-preference function The current slave-preference function has the following flaw: When the system is under medium load, preferred slaves are often occupied, so that each new build starts taking slaves preferring other builders. This in turn leads to more slaves not being available for their preferred builders. The new implementation has the following strategy: Let slaves(builder) be the number of available slaves preferring that builder. For builder A and available slaves S, choose s in S such that (order is preference): 1. s prefers A, or 2. s prefers nothing (an optional fall-over pool), or 3. s prefers some B != A and there's no other builder C != B such that slaves(C) > slaves(B) BUG=658189 ==========
lgtm
Code LGTM (though I didn't look as thoroughly as tandrii@). For the actual logic - I'm not sure how it would help you. At the end of the day, you allow any builder to take any slave when capacity is really tight, and you'll still get pending queues regardless. Basically, given the load and the desire to process CLs fast, I don't see a way around the fundamental need for computing power. http://go/guemi says you might need up to 60 slaves in this pool. Even discounting the "fudge factor", you still need ~40 slaves, which is double what is configured. These are ccompute slaves, they should be easy to add - just ask Infra>Labs. Of course, you also need to worry about swarming capacity, and here's a quick peak: http://go/vdmcr (I probably didn't capture the regexp exactly, but it should be close). While Ubuntu-12 pools are approaching 100% utilization, v8 share is only 12-13%, so doubling that shouldn't be too bad. And we need to increase these pools anayway (unless they are already autoscaled - at least one of them seems to be).
On 2016/10/21 18:19:53, Sergey Berezin wrote: > Code LGTM (though I didn't look as thoroughly as tandrii@). > > For the actual logic - I'm not sure how it would help you. At the end of the > day, you allow any builder to take any slave when capacity is really tight, and > you'll still get pending queues regardless. Basically, given the load and the > desire to process CLs fast, I don't see a way around the fundamental need for > computing power. Right now, I mainly want to improve utilization of incremental builds for a small subset of the builders where this is most crucial. I'll deploy this for V8 CQ to test and see if it helps in any way. > http://go/guemi says you might need up to 60 slaves in this pool. Even > discounting the "fudge factor", you still need ~40 slaves, which is double what > is configured. These are ccompute slaves, they should be easy to add - just ask > Infra>Labs. Will do that next. > Of course, you also need to worry about swarming capacity, and here's a quick > peak: http://go/vdmcr (I probably didn't capture the regexp exactly, but it > should be close). > > While Ubuntu-12 pools are approaching 100% utilization, v8 share is only 12-13%, > so doubling that shouldn't be too bad. And we need to increase these pools > anayway (unless they are already autoscaled - at least one of them seems to be). We're gonna upgrade a few bots to trusty anyways soon. Also, V8 usage is mainly during EMEA day time. And therefore disjoint with the Chromium peak times. Is this captured in the estimations?
On 2016/10/21 at 20:34:06, machenbach wrote: > On 2016/10/21 18:19:53, Sergey Berezin wrote: > > > > While Ubuntu-12 pools are approaching 100% utilization, v8 share is only 12-13%, > > so doubling that shouldn't be too bad. And we need to increase these pools > > anayway (unless they are already autoscaled - at least one of them seems to be). > > We're gonna upgrade a few bots to trusty anyways soon. Also, V8 usage is mainly > during EMEA day time. And therefore disjoint with the Chromium peak times. Is > this captured in the estimations? Not for swarming - these are just raw utilization graphs. Being disjoint with Chromium peaks should help dramatically - the pools should be pretty much idle. So, swarming capacity seems plenty for now :-)
lgtm Basically this effort is to reduce the amount of tasks for incremental builds, and will make build faster, and will reduce goma load.
The CQ bit was checked by machenbach@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/320e69ba29533810)
The CQ bit was checked by machenbach@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyberezin@chromium.org, shinyak@chromium.org, tandrii@chromium.org Link to the patchset: https://codereview.chromium.org/2427413005/#ps120001 (title: "Presubmit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Improve slave-preference function The current slave-preference function has the following flaw: When the system is under medium load, preferred slaves are often occupied, so that each new build starts taking slaves preferring other builders. This in turn leads to more slaves not being available for their preferred builders. The new implementation has the following strategy: Let slaves(builder) be the number of available slaves preferring that builder. For builder A and available slaves S, choose s in S such that (order is preference): 1. s prefers A, or 2. s prefers nothing (an optional fall-over pool), or 3. s prefers some B != A and there's no other builder C != B such that slaves(C) > slaves(B) BUG=658189 ========== to ========== Improve slave-preference function The current slave-preference function has the following flaw: When the system is under medium load, preferred slaves are often occupied, so that each new build starts taking slaves preferring other builders. This in turn leads to more slaves not being available for their preferred builders. The new implementation has the following strategy: Let slaves(builder) be the number of available slaves preferring that builder. For builder A and available slaves S, choose s in S such that (order is preference): 1. s prefers A, or 2. s prefers nothing (an optional fall-over pool), or 3. s prefers some B != A and there's no other builder C != B such that slaves(C) > slaves(B) BUG=658189 Committed: https://chromium.googlesource.com/chromium/tools/build/+/f71cca8a090ef8d7608a... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/tools/build/+/f71cca8a090ef8d7608a... |
