Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2896603002:
[scheduler] Move task alignment into WakeUpBudgetPool. (Closed)
Description[scheduler] Move task alignment into WakeUpBudgetPool.
This fixes the regression where real-world throttling regressed to
two-seconds task alignment instead of one-second alignment.
This patch also improves test coverage for background throttling.
BUG=722410
Review-Url: https://codereview.chromium.org/2896603002
Cr-Original-Commit-Position: refs/heads/master@{#473609}
Committed: https://chromium.googlesource.com/chromium/src/+/188fc72030f13187af5cff7e9802f3abebdea017
Review-Url: https://codereview.chromium.org/2896603002
Cr-Commit-Position: refs/heads/master@{#473867}
Committed: https://chromium.googlesource.com/chromium/src/+/c37e844a6f87088261bf3c595ea34b64b15f1633
Patch Set 1 #
Total comments: 4
Patch Set 2 : addressed a comment from alexclarke@ #Patch Set 3 : uploaded correct version #Patch Set 4 : fix typo #
Total comments: 2
Patch Set 5 : Fix test #Patch Set 6 : no iostream #
Total comments: 2
Messages
Total messages: 42 (28 generated)
altimin@chromium.org changed reviewers: + alexclarke@chromium.org, skyostil@chromium.org
PTAL
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2896603002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc (right): https://codereview.chromium.org/2896603002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc:30: wake_up_interval_ = base::TimeDelta::FromSeconds(1 / wake_ups_per_second); Shouldn't that be base::TimeDelta::FromSecondsD(1.0 / wake_ups_per_second)? Otherwise I'm reasonably certain that returns 0 most of the time.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2896603002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc (right): https://codereview.chromium.org/2896603002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc:30: wake_up_interval_ = base::TimeDelta::FromSeconds(1 / wake_ups_per_second); On 2017/05/19 19:48:39, alex clarke wrote: > Shouldn't that be base::TimeDelta::FromSecondsD(1.0 / wake_ups_per_second)? > Otherwise I'm reasonably certain that returns 0 most of the time. It's fine because the type of wake_ups_per_second is double.
https://codereview.chromium.org/2896603002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc (right): https://codereview.chromium.org/2896603002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc:30: wake_up_interval_ = base::TimeDelta::FromSeconds(1 / wake_ups_per_second); On 2017/05/19 22:15:15, altimin wrote: > On 2017/05/19 19:48:39, alex clarke wrote: > > Shouldn't that be base::TimeDelta::FromSecondsD(1.0 / wake_ups_per_second)? > > Otherwise I'm reasonably certain that returns 0 most of the time. > > It's fine because the type of wake_ups_per_second is double. But base::TimeDelta::FromSeconds takes an integer number of seconds.
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2896603002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc (right): https://codereview.chromium.org/2896603002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc:30: wake_up_interval_ = base::TimeDelta::FromSeconds(1 / wake_ups_per_second); On 2017/05/20 09:09:51, alex clarke wrote: > On 2017/05/19 22:15:15, altimin wrote: > > On 2017/05/19 19:48:39, alex clarke wrote: > > > Shouldn't that be base::TimeDelta::FromSecondsD(1.0 / wake_ups_per_second)? > > > Otherwise I'm reasonably certain that returns 0 most of the time. > > > > It's fine because the type of wake_ups_per_second is double. > > But base::TimeDelta::FromSeconds takes an integer number of seconds. Oh, thank you. I noticed "1.0" (which is not needed), but missed the important part.
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM % possible test https://codereview.chromium.org/2896603002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc (right): https://codereview.chromium.org/2896603002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc:30: wake_up_interval_ = base::TimeDelta::FromSecondsD(1 / wake_ups_per_second); Did TEST_F(BudgetPoolTest, WakeUpBudgetPool) fail? If not can we add a test that would have caught this?
https://codereview.chromium.org/2896603002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc (right): https://codereview.chromium.org/2896603002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc:30: wake_up_interval_ = base::TimeDelta::FromSecondsD(1 / wake_ups_per_second); On 2017/05/22 13:26:35, alex clarke wrote: > Did TEST_F(BudgetPoolTest, WakeUpBudgetPool) fail? If not can we add a test > that would have caught this? No, because wake-up interval is 1s or 10s in the tests. The problem is that smaller intervals don't quite work at the moment due to forced alignment to whole seconds in task_queue_throttler.cc. Do you think that we should make this alignment configurable and and a test with, let's say, 60Hz frequency throttling?
On 2017/05/22 13:31:24, altimin wrote: > https://codereview.chromium.org/2896603002/diff/60001/third_party/WebKit/Sour... > File > third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc > (right): > > https://codereview.chromium.org/2896603002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc:30: > wake_up_interval_ = base::TimeDelta::FromSecondsD(1 / wake_ups_per_second); > On 2017/05/22 13:26:35, alex clarke wrote: > > Did TEST_F(BudgetPoolTest, WakeUpBudgetPool) fail? If not can we add a test > > that would have caught this? > > No, because wake-up interval is 1s or 10s in the tests. The problem is that > smaller intervals don't quite work at the moment due to forced alignment to > whole seconds in task_queue_throttler.cc. Do you think that we should make this > alignment configurable and and a test with, let's say, 60Hz frequency > throttling? I think I'll land this patch as is and will think about adding a test in a separate patch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexclarke@chromium.org Link to the patchset: https://codereview.chromium.org/2896603002/#ps80001 (title: "Fix test")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495473101056310,
"parent_rev": "eec54a3ad6f426699e9aa5754b0e553de13fe75e", "commit_rev":
"188fc72030f13187af5cff7e9802f3abebdea017"}
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Move task alignment into WakeUpBudgetPool. This fixes the regression where real-world throttling regressed to two-seconds task alignment instead of one-second alignment. This patch also improves test coverage for background throttling. BUG=722410 ========== to ========== [scheduler] Move task alignment into WakeUpBudgetPool. This fixes the regression where real-world throttling regressed to two-seconds task alignment instead of one-second alignment. This patch also improves test coverage for background throttling. BUG=722410 Review-Url: https://codereview.chromium.org/2896603002 Cr-Commit-Position: refs/heads/master@{#473609} Committed: https://chromium.googlesource.com/chromium/src/+/188fc72030f13187af5cff7e9802... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/188fc72030f13187af5cff7e9802...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2893363002/ by mek@chromium.org. The reason for reverting is: Regresses sizes on https://build.chromium.org/p/chromium/buildstatus?builder=Linux%20x64&number=... (due to include of iostream).
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Move task alignment into WakeUpBudgetPool. This fixes the regression where real-world throttling regressed to two-seconds task alignment instead of one-second alignment. This patch also improves test coverage for background throttling. BUG=722410 Review-Url: https://codereview.chromium.org/2896603002 Cr-Commit-Position: refs/heads/master@{#473609} Committed: https://chromium.googlesource.com/chromium/src/+/188fc72030f13187af5cff7e9802... ========== to ========== [scheduler] Move task alignment into WakeUpBudgetPool. This fixes the regression where real-world throttling regressed to two-seconds task alignment instead of one-second alignment. This patch also improves test coverage for background throttling. BUG=722410 Review-Url: https://codereview.chromium.org/2896603002 Cr-Commit-Position: refs/heads/master@{#473609} Committed: https://chromium.googlesource.com/chromium/src/+/188fc72030f13187af5cff7e9802... ==========
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexclarke@chromium.org Link to the patchset: https://codereview.chromium.org/2896603002/#ps100001 (title: "no iostream")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495542381021070,
"parent_rev": "dc768d48cd58d1b0c0690e9ba866991088196996", "commit_rev":
"c37e844a6f87088261bf3c595ea34b64b15f1633"}
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Move task alignment into WakeUpBudgetPool. This fixes the regression where real-world throttling regressed to two-seconds task alignment instead of one-second alignment. This patch also improves test coverage for background throttling. BUG=722410 Review-Url: https://codereview.chromium.org/2896603002 Cr-Commit-Position: refs/heads/master@{#473609} Committed: https://chromium.googlesource.com/chromium/src/+/188fc72030f13187af5cff7e9802... ========== to ========== [scheduler] Move task alignment into WakeUpBudgetPool. This fixes the regression where real-world throttling regressed to two-seconds task alignment instead of one-second alignment. This patch also improves test coverage for background throttling. BUG=722410 Review-Url: https://codereview.chromium.org/2896603002 Cr-Original-Commit-Position: refs/heads/master@{#473609} Committed: https://chromium.googlesource.com/chromium/src/+/188fc72030f13187af5cff7e9802... Review-Url: https://codereview.chromium.org/2896603002 Cr-Commit-Position: refs/heads/master@{#473867} Committed: https://chromium.googlesource.com/chromium/src/+/c37e844a6f87088261bf3c595ea3... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/c37e844a6f87088261bf3c595ea3...
Message was sent while issue was closed.
Some drive by after the fact comments... https://codereview.chromium.org/2896603002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2896603002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:254: // TODO(altimin): Consider removing alignment here. Ditto. https://codereview.chromium.org/2896603002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2896603002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:135: // TODO(altimin): Remove it. In the future, please give more descriptive TODOs. If you don't get around to it, someone looking at this 3 years from now won't know why this should be removed. Similarly, if someone wants to add a new call to this, they won't know whether it's OK or not based off this TODO. |
