Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2412323003:
[scheduler] Support setting maximal throttling duration (Closed)
Description[scheduler] Support setting maximal throttling duration
This patch adds support of maximal throttling duration to Time Budget API
and makes both maximal throttling duration and maximal budget level optional.
Maximal throttling duration is enforced by setting a lower bound on budget level,
making sure that we will not throttle a queue for more than 1 minute.
R=alexclarke@chromium.org,skyostil@chromium.org
CC=ojan@chromium.org
BUG=639852
Committed: https://crrev.com/19cf23b8ffe72e4d1d069e4bdf4a996be67285c8
Cr-Commit-Position: refs/heads/master@{#425938}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed comments from skyostil@ #
Total comments: 6
Patch Set 3 : Addressed comments from alexclarke@ #
Total comments: 6
Patch Set 4 : Addressed nits from alexclarke@ #
Messages
Total messages: 34 (20 generated)
Description was changed from ========== [scheduler] Support setting maximal throttling duration This patch adds support of maximal throttling duration to Time Budget API and makes both maximal throttling duration and maximal budget level optional. R=alexclarke@chromium.org,skyostil@chromium.org CC=ojan@chromium.org BUG=639852 ========== to ========== [scheduler] Support setting maximal throttling duration This patch adds support of maximal throttling duration to Time Budget API and makes both maximal throttling duration and maximal budget level optional. Maximal throttling duration is enforced by setting a lower bound on budget level, making sure that we will not throttle a queue for more than 1 minute. R=alexclarke@chromium.org,skyostil@chromium.org CC=ojan@chromium.org BUG=639852 ==========
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I may have missed the thread about this but IIRC we decided not to have a max limit because it would defeat much of the purpose of throttling and for a site to need a max limit it would have to be doing something pretty egregious already. Is there a reason we think we need this after all? Also, this is setting the max to 1 minute while purge & suspend uses two minutes. I'm not sure why the two should be different :) (+cc p&s folks)
https://codereview.chromium.org/2412323003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2412323003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:126: // is allowed to run for after a very long period of inactivity. Hmm, is this the maximum budget we can accrue? This description is a little confusing. https://codereview.chromium.org/2412323003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:128: // Max throttling duration is the amount of time after which the task will Would it be simpler to say this is the maximum delay added to a task by throttling? Is that accurate?
On 2016/10/14 00:29:42, Sami (OoO 0x1F1EF 0x1F1F5) wrote: > I may have missed the thread about this but IIRC we decided not to have a max > limit because it would defeat much of the purpose of throttling and for a site > to need a max limit it would have to be doing something pretty egregious > already. Is there a reason we think we need this after all? > > Also, this is setting the max to 1 minute while purge & suspend uses two > minutes. I'm not sure why the two should be different :) (+cc p&s folks) We talked about the fact that guaranteeing that every task will run in N minutes after desired run time is not a good idea. Here we are introducing a we-will-run-at-least-one-task-every-minute guarantee. This will give us more fine-grained control via Finch and will allow to experiment more aggressively. Updated comments, PTAL
https://codereview.chromium.org/2412323003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2412323003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:126: // is allowed to run for after a very long period of inactivity. On 2016/10/14 00:33:02, Sami (OoO 0x1F1EF 0x1F1F5) wrote: > Hmm, is this the maximum budget we can accrue? This description is a little > confusing. Rephrased. https://codereview.chromium.org/2412323003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:128: // Max throttling duration is the amount of time after which the task will On 2016/10/14 00:33:02, Sami (OoO 0x1F1EF 0x1F1F5) wrote: > Would it be simpler to say this is the maximum delay added to a task by > throttling? Is that accurate? No. In fact, it's just a lower bound on time budget level (we're using throttling duration instead of min budget level to make it throttling aggressiveness-agnostic). Rephrased.
Description was changed from ========== [scheduler] Support setting maximal throttling duration This patch adds support of maximal throttling duration to Time Budget API and makes both maximal throttling duration and maximal budget level optional. Maximal throttling duration is enforced by setting a lower bound on budget level, making sure that we will not throttle a queue for more than 1 minute. R=alexclarke@chromium.org,skyostil@chromium.org CC=ojan@chromium.org BUG=639852 ========== to ========== [scheduler] Support setting maximal throttling duration This patch adds support of maximal throttling duration to Time Budget API and makes both maximal throttling duration and maximal budget level optional. Maximal throttling duration is enforced by setting a lower bound on budget level, making sure that we will not throttle a queue for more than 1 minute. R=alexclarke@chromium.org,skyostil@chromium.org CC=ojan@chromium.org BUG=639852 ==========
https://codereview.chromium.org/2412323003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2412323003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:185: Advance(end_time); Maybe DCHECK_LE(start_time, end_time); https://codereview.chromium.org/2412323003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:243: void TaskQueueThrottler::TimeBudgetPool::EnforceMaximalThrottlingDuration() { Out of curiosity would it make sense to enforce max_budget_level_ and max_throttling_duration_ in the same place? https://codereview.chromium.org/2412323003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:246: std::max(current_budget_level_, Maybe add a comment pointing out the obvious: current_budget_level_ can be negative.
On 2016/10/14 16:18:39, altimin wrote: > On 2016/10/14 00:29:42, Sami (OoO 0x1F1EF 0x1F1F5) wrote: > > I may have missed the thread about this but IIRC we decided not to have a max > > limit because it would defeat much of the purpose of throttling and for a site > > to need a max limit it would have to be doing something pretty egregious > > already. Is there a reason we think we need this after all? > > > > Also, this is setting the max to 1 minute while purge & suspend uses two > > minutes. I'm not sure why the two should be different :) (+cc p&s folks) > > We talked about the fact that guaranteeing that every task will run in N minutes > after desired run time is not a good idea. Here we are introducing a > we-will-run-at-least-one-task-every-minute guarantee. This will give us more > fine-grained control via Finch and will allow to experiment more aggressively. > > Updated comments, PTAL I think we need something like that to make sure that tasks are not stalled for a long time, but I'm not sure if it should be we-will-run-at-least-one-task-every-minute. As Sami mentioned, I'm a bit concerned that this will defeat much of the purpose of throttling. Sites that need the max limit are already doing something pretty bad, so what we should try would be to throttle the tasks instead of making the bad sites work. FWIW, tasak@ is going to start a Finch experiment of the tab suspension (which should eventually merged to the timer-based throttling of Blink Scheduler) by resuming the tab for 10 seconds every 2 mins. tasak@ added UMAs to measure how many tasks are remaining when the 10-second budget is over.
ojan@chromium.org changed reviewers: + ojan@chromium.org
IMO, having this as a value we can control via finch increases the likelihood we can ship something in M56 since we can tweak as needed to if we find web compatibility problems. We can leave this value very large to start with (e.g. 1 hour), but having the ability to tweak it in the wild seems like a good thing to me.
PTAL https://codereview.chromium.org/2412323003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2412323003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:185: Advance(end_time); On 2016/10/14 16:37:22, alex clarke wrote: > Maybe DCHECK_LE(start_time, end_time); Done. https://codereview.chromium.org/2412323003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:243: void TaskQueueThrottler::TimeBudgetPool::EnforceMaximalThrottlingDuration() { On 2016/10/14 16:37:22, alex clarke wrote: > Out of curiosity would it make sense to enforce max_budget_level_ and > max_throttling_duration_ in the same place? Thanks! Done. https://codereview.chromium.org/2412323003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:246: std::max(current_budget_level_, On 2016/10/14 16:37:22, alex clarke wrote: > Maybe add a comment pointing out the obvious: current_budget_level_ can be > negative. Done.
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.
LGTM % nits https://codereview.chromium.org/2412323003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2412323003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:261: max_throttling_duration_(kMaxThrottlingDuration), Add a TODO to optionally control these values from finch. https://codereview.chromium.org/2412323003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2412323003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:117: // Increase current_budget_level_ to satisfy max throttling duration Consider reflowing comment here and below. https://codereview.chromium.org/2412323003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:119: // Decreate current_budget_level_ to satisfy max budget level Increase |current_budget_level_| ... Decrease |current_budget_level_| ...
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
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/2412323003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2412323003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:261: max_throttling_duration_(kMaxThrottlingDuration), On 2016/10/17 14:55:55, alex clarke wrote: > Add a TODO to optionally control these values from finch. Done. https://codereview.chromium.org/2412323003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2412323003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:117: // Increase current_budget_level_ to satisfy max throttling duration On 2016/10/17 14:55:55, alex clarke wrote: > Consider reflowing comment here and below. Done. https://codereview.chromium.org/2412323003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:119: // Decreate current_budget_level_ to satisfy max budget level On 2016/10/17 14:55:55, alex clarke wrote: > Increase |current_budget_level_| ... > Decrease |current_budget_level_| ... Done.
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/2412323003/#ps60001 (title: "Addressed nits from alexclarke@")
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 ========== [scheduler] Support setting maximal throttling duration This patch adds support of maximal throttling duration to Time Budget API and makes both maximal throttling duration and maximal budget level optional. Maximal throttling duration is enforced by setting a lower bound on budget level, making sure that we will not throttle a queue for more than 1 minute. R=alexclarke@chromium.org,skyostil@chromium.org CC=ojan@chromium.org BUG=639852 ========== to ========== [scheduler] Support setting maximal throttling duration This patch adds support of maximal throttling duration to Time Budget API and makes both maximal throttling duration and maximal budget level optional. Maximal throttling duration is enforced by setting a lower bound on budget level, making sure that we will not throttle a queue for more than 1 minute. R=alexclarke@chromium.org,skyostil@chromium.org CC=ojan@chromium.org BUG=639852 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Support setting maximal throttling duration This patch adds support of maximal throttling duration to Time Budget API and makes both maximal throttling duration and maximal budget level optional. Maximal throttling duration is enforced by setting a lower bound on budget level, making sure that we will not throttle a queue for more than 1 minute. R=alexclarke@chromium.org,skyostil@chromium.org CC=ojan@chromium.org BUG=639852 ========== to ========== [scheduler] Support setting maximal throttling duration This patch adds support of maximal throttling duration to Time Budget API and makes both maximal throttling duration and maximal budget level optional. Maximal throttling duration is enforced by setting a lower bound on budget level, making sure that we will not throttle a queue for more than 1 minute. R=alexclarke@chromium.org,skyostil@chromium.org CC=ojan@chromium.org BUG=639852 Committed: https://crrev.com/19cf23b8ffe72e4d1d069e4bdf4a996be67285c8 Cr-Commit-Position: refs/heads/master@{#425938} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/19cf23b8ffe72e4d1d069e4bdf4a996be67285c8 Cr-Commit-Position: refs/heads/master@{#425938} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
