[scheduler] Add unthrottled-by-blockable task queue to fix touch latency regression.

[scheduler] Add general task queue to fix touch latency regression.

Every problem can be solved by adding an additional task queue, except
for the problem of too many task queues.

This should fix touch latency regression which a) is considered timer
task queue from renderer scheduler point of view and can be blocked
during touch move if expensive b) is not throttled or suspended,
therefore not breaking any other use cases.

This is a temporary fix and all tasks should be moved to suspendable or
unthrottled task queues.

NOTE: this patch temporary disables background throttling of the default timer queue because otherwise suspendable tasks would be throttled too.

R=skyostil@chromium.org,haraken@chromium.org
CC=tdresser@chromium.org,nzolghadr@chromium.org

BUG=704939
Review-Url: https://codereview.chromium.org/2895673002
Cr-Commit-Position: refs/heads/master@{#475591}
Committed: https://chromium.googlesource.com/chromium/src/+/f39f12be44e4d35eb25c7a58b1895c751da723f5

[altimin, 2017-05-19 16:55:24]

PTAL

[altimin, 2017-05-19 17:43:20]

The CQ bit was checked by altimin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 17:44:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 18:12:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 18:12:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[haraken, 2017-05-19 23:11:20]

Would you elaborate on this change more?

I'm really concerned about adding random task runners.

[altimin, 2017-05-19 23:22:03]

On 2017/05/19 23:11:20, haraken wrote:
> Would you elaborate on this change more?
> 
> I'm really concerned about adding random task runners.

At the moment we map almost all task types to unthrottled queue. That means that
we can't delay them during user gesture. We want to move them to suspendable
task queue, which means that we a) can suspend them, b) can delay them for user
gesture latency improvements. But there are subtle problems with suspending
tasks (my patches got reverted a number of times) and input folks want a quick
fix. So I'm introducing a temporary queue which can be delayed for user gesture
latency improvements, but can't be suspended or throttled. The plan is to move
all task types one-by-one to suspended or unthrottled queue after landing this
and fixing the scroll latency regression.

[haraken, 2017-05-20 00:10:02]

On 2017/05/19 23:22:03, altimin wrote:
> On 2017/05/19 23:11:20, haraken wrote:
> > Would you elaborate on this change more?
> > 
> > I'm really concerned about adding random task runners.
> 
> At the moment we map almost all task types to unthrottled queue. That means
that
> we can't delay them during user gesture. We want to move them to suspendable
> task queue, which means that we a) can suspend them, b) can delay them for
user
> gesture latency improvements. But there are subtle problems with suspending
> tasks (my patches got reverted a number of times) and input folks want a quick
> fix. So I'm introducing a temporary queue which can be delayed for user
gesture
> latency improvements, but can't be suspended or throttled. The plan is to move
> all task types one-by-one to suspended or unthrottled queue after landing this
> and fixing the scroll latency regression.

What happens if we simply make the unthrottled task queue delay tasks for user
gestures?

[altimin, 2017-05-22 10:54:02]

On 2017/05/20 00:10:02, haraken wrote:
> On 2017/05/19 23:22:03, altimin wrote:
> > On 2017/05/19 23:11:20, haraken wrote:
> > > Would you elaborate on this change more?
> > > 
> > > I'm really concerned about adding random task runners.
> > 
> > At the moment we map almost all task types to unthrottled queue. That means
> that
> > we can't delay them during user gesture. We want to move them to suspendable
> > task queue, which means that we a) can suspend them, b) can delay them for
> user
> > gesture latency improvements. But there are subtle problems with suspending
> > tasks (my patches got reverted a number of times) and input folks want a
quick
> > fix. So I'm introducing a temporary queue which can be delayed for user
> gesture
> > latency improvements, but can't be suspended or throttled. The plan is to
move
> > all task types one-by-one to suspended or unthrottled queue after landing
this
> > and fixing the scroll latency regression.
> 
> What happens if we simply make the unthrottled task queue delay tasks for user
> gestures?

I'm not sure that this is safe — there are strange regressions (media-related,
for example) which can happen because of this. (After struggling to land several
patches) I'd prefer to land a safe version to address regression and iterate
from there.

[altimin, 2017-05-22 10:54:57]

The CQ bit was checked by altimin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 10:55:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 11:36:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 11:36:25]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Sami, 2017-05-22 12:14:45]

We probably need something like this at least temporarily as we make our task
taxonomy more rigorous. We need a better name than 'general' however. Right now
we have (roughly in decreasing QoS order):

- unthrottled (unsuspendable)
- this new task queue (unsuspendable but throttled)
- timer (suspendable)
- loading (suspendable)

Is that right? How about something like UnthrottledButBlockable? I'm not crazy
about that name, but basically we have three levels on 'intervention'
(suspending, throttling and blocking) and this would make that obvious? I'm open
to suggestion here.

https://codereview.chromium.org/2895673002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp (right):

https://codereview.chromium.org/2895673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:56: // TODO(altimin):
Move all this tasks to suspendable or unthrottled
s/this/these/

[altimin, 2017-05-22 12:40:43]

On 2017/05/22 12:14:45, Sami wrote:
> We probably need something like this at least temporarily as we make our task
> taxonomy more rigorous. We need a better name than 'general' however. Right
now
> we have (roughly in decreasing QoS order):
> 
> - unthrottled (unsuspendable)
> - this new task queue (unsuspendable but throttled)
> - timer (suspendable)
> - loading (suspendable)
> 
> Is that right? How about something like UnthrottledButBlockable? I'm not crazy
> about that name, but basically we have three levels on 'intervention'
> (suspending, throttling and blocking) and this would make that obvious? I'm
open
> to suggestion here.

It's slightly more complex :)

- unthrottled (realtime, not affected at all)
- this new one (can be blocked during user gesture)
- suspendable (same as above + can be suspended)
- loading (same as suspendable, but a clear use case — different priorities)
- timer (same as suspendable + background throttling)

We need to move all tasks from the new one to unthrottled or suspendable task
queue. I'm inclined towards the "deprecated task queue" option :)

[haraken, 2017-05-22 13:42:45]

On 2017/05/22 12:40:43, altimin wrote:
> On 2017/05/22 12:14:45, Sami wrote:
> > We probably need something like this at least temporarily as we make our
task
> > taxonomy more rigorous. We need a better name than 'general' however. Right
> now
> > we have (roughly in decreasing QoS order):
> > 
> > - unthrottled (unsuspendable)
> > - this new task queue (unsuspendable but throttled)
> > - timer (suspendable)
> > - loading (suspendable)
> > 
> > Is that right? How about something like UnthrottledButBlockable? I'm not
crazy
> > about that name, but basically we have three levels on 'intervention'
> > (suspending, throttling and blocking) and this would make that obvious? I'm
> open
> > to suggestion here.
> 
> It's slightly more complex :)
> 
> - unthrottled (realtime, not affected at all)
> - this new one (can be blocked during user gesture)
> - suspendable (same as above + can be suspended)
> - loading (same as suspendable, but a clear use case — different priorities)
> - timer (same as suspendable + background throttling)
> 
> We need to move all tasks from the new one to unthrottled or suspendable task
> queue. I'm inclined towards the "deprecated task queue" option :)

I want to understand your overall plan before adding this new queue :)

[altimin, 2017-05-22 13:46:34]

On 2017/05/22 13:42:45, haraken wrote:
> On 2017/05/22 12:40:43, altimin wrote:
> > On 2017/05/22 12:14:45, Sami wrote:
> > > We probably need something like this at least temporarily as we make our
> task
> > > taxonomy more rigorous. We need a better name than 'general' however.
Right
> > now
> > > we have (roughly in decreasing QoS order):
> > > 
> > > - unthrottled (unsuspendable)
> > > - this new task queue (unsuspendable but throttled)
> > > - timer (suspendable)
> > > - loading (suspendable)
> > > 
> > > Is that right? How about something like UnthrottledButBlockable? I'm not
> crazy
> > > about that name, but basically we have three levels on 'intervention'
> > > (suspending, throttling and blocking) and this would make that obvious?
I'm
> > open
> > > to suggestion here.
> > 
> > It's slightly more complex :)
> > 
> > - unthrottled (realtime, not affected at all)
> > - this new one (can be blocked during user gesture)
> > - suspendable (same as above + can be suspended)
> > - loading (same as suspendable, but a clear use case — different priorities)
> > - timer (same as suspendable + background throttling)
> > 
> > We need to move all tasks from the new one to unthrottled or suspendable
task
> > queue. I'm inclined towards the "deprecated task queue" option :)
> 
> I want to understand your overall plan before adding this new queue :)

The plan here is rather simple:
a) add this new queue.
b) move tasks from it to other queue one-by-one.

There are certain technical unknowns and challenges in the second part, but
high-level overview is simple.

[haraken, 2017-05-22 13:53:51]

On 2017/05/22 13:46:34, altimin wrote:
> On 2017/05/22 13:42:45, haraken wrote:
> > On 2017/05/22 12:40:43, altimin wrote:
> > > On 2017/05/22 12:14:45, Sami wrote:
> > > > We probably need something like this at least temporarily as we make our
> > task
> > > > taxonomy more rigorous. We need a better name than 'general' however.
> Right
> > > now
> > > > we have (roughly in decreasing QoS order):
> > > > 
> > > > - unthrottled (unsuspendable)
> > > > - this new task queue (unsuspendable but throttled)
> > > > - timer (suspendable)
> > > > - loading (suspendable)
> > > > 
> > > > Is that right? How about something like UnthrottledButBlockable? I'm not
> > crazy
> > > > about that name, but basically we have three levels on 'intervention'
> > > > (suspending, throttling and blocking) and this would make that obvious?
> I'm
> > > open
> > > > to suggestion here.
> > > 
> > > It's slightly more complex :)
> > > 
> > > - unthrottled (realtime, not affected at all)
> > > - this new one (can be blocked during user gesture)
> > > - suspendable (same as above + can be suspended)
> > > - loading (same as suspendable, but a clear use case — different
priorities)
> > > - timer (same as suspendable + background throttling)
> > > 
> > > We need to move all tasks from the new one to unthrottled or suspendable
> task
> > > queue. I'm inclined towards the "deprecated task queue" option :)
> > 
> > I want to understand your overall plan before adding this new queue :)
> 
> The plan here is rather simple:
> a) add this new queue.
> b) move tasks from it to other queue one-by-one.
> 
> There are certain technical unknowns and challenges in the second part, but
> high-level overview is simple.

OK. +1 to UnthrottledButBlockable.

Also add a comment about explain the relationship of the task runners somewhere.

> > > - unthrottled (realtime, not affected at all)
> > > - this new one (can be blocked during user gesture)
> > > - suspendable (same as above + can be suspended)
> > > - loading (same as suspendable, but a clear use case — different
priorities)
> > > - timer (same as suspendable + background throttling)

^^^ this relationship.

With that change, LGTM.

[altimin, 2017-05-22 17:06:33]

Description was changed from

==========
[scheduler] Add general task queue to fix touch latency regression.

Every problem can be solved by adding an additional task queue, except
for the problem of too many task queues.

This should fix touch latency regression which a) is considered timer
task queue from renderer scheduler point of view and can be blocked
during touch move if expensive b) is not throttled or suspended,
therefore not breaking any other use cases.

This is a temporary fix and all tasks should be moved to suspendable or
unthrottled task queues.

R=skyostil@chromium.org,haraken@chromium.org
CC=tdresser@chromium.org,nzolghadr@chromium.org

BUG=704939
==========

to

==========
[scheduler] Add general task queue to fix touch latency regression.

Every problem can be solved by adding an additional task queue, except
for the problem of too many task queues.

This should fix touch latency regression which a) is considered timer
task queue from renderer scheduler point of view and can be blocked
during touch move if expensive b) is not throttled or suspended,
therefore not breaking any other use cases.

This is a temporary fix and all tasks should be moved to suspendable or
unthrottled task queues.

R=skyostil@chromium.org,haraken@chromium.org
CC=tdresser@chromium.org,nzolghadr@chromium.org

BUG=704939
==========

[altimin, 2017-05-22 17:57:48]

Description was changed from

==========
[scheduler] Add general task queue to fix touch latency regression.

Every problem can be solved by adding an additional task queue, except
for the problem of too many task queues.

This should fix touch latency regression which a) is considered timer
task queue from renderer scheduler point of view and can be blocked
during touch move if expensive b) is not throttled or suspended,
therefore not breaking any other use cases.

This is a temporary fix and all tasks should be moved to suspendable or
unthrottled task queues.

R=skyostil@chromium.org,haraken@chromium.org
CC=tdresser@chromium.org,nzolghadr@chromium.org

BUG=704939
==========

to

==========
[scheduler] Add general task queue to fix touch latency regression.

Every problem can be solved by adding an additional task queue, except
for the problem of too many task queues.

This should fix touch latency regression which a) is considered timer
task queue from renderer scheduler point of view and can be blocked
during touch move if expensive b) is not throttled or suspended,
therefore not breaking any other use cases.

This is a temporary fix and all tasks should be moved to suspendable or
unthrottled task queues.

NOTE: this patch temporary disables background throttling of the default timer
queue because otherwise suspendable tasks would be throttled too.

R=skyostil@chromium.org,haraken@chromium.org
CC=tdresser@chromium.org,nzolghadr@chromium.org

BUG=704939
==========

[altimin, 2017-05-22 17:59:43]

The CQ bit was checked by altimin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 18:00:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 19:13:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 19:13:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[altimin, 2017-05-23 12:44:44]

The CQ bit was checked by altimin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 12:45:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[altimin, 2017-05-23 12:53:20]

The CQ bit was checked by altimin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 12:53:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 13:38:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 13:38:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[altimin, 2017-05-26 14:17:48]

The CQ bit was checked by altimin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 14:17:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sami, 2017-05-26 15:07:31]

lgtm, but please fix the type in the title and update the patch description to
match what we're doing.

https://codereview.chromium.org/2895673002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/WebFrameScheduler.h (right):

https://codereview.chromium.org/2895673002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/WebFrameScheduler.h:91: virtual
RefPtr<WebTaskRunner> UnthrottledButBlockableTaskRunner() = 0;
nit: please move this up to match the order in the comment.

https://codereview.chromium.org/2895673002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
(right):

https://codereview.chromium.org/2895673002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:3840:
TEST_F(RendererSchedulerImplTest,
TODO(altimin@): Re-enable after splitting the timer policy into separate
policies.

https://codereview.chromium.org/2895673002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc
(right):

https://codereview.chromium.org/2895673002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc:201:
TaskQueue::QueueType::FRAME_UNTHROTTLED);
I guess we'll add a new queue type for this later too?

[Sami, 2017-05-26 15:07:43]

*erm, typo in the title

[commit-bot: I haz the power, 2017-05-26 15:36:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 15:36:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[altimin, 2017-05-30 15:29:03]

The CQ bit was checked by altimin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 15:29:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 17:40:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-30 17:40:52]

Dry run: This issue passed the CQ dry run.

[altimin, 2017-05-30 17:46:39]

https://codereview.chromium.org/2895673002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp (right):

https://codereview.chromium.org/2895673002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:56: // TODO(altimin):
Move all this tasks to suspendable or unthrottled
On 2017/05/22 12:14:44, Sami wrote:
> s/this/these/

Done.

https://codereview.chromium.org/2895673002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/WebFrameScheduler.h (right):

https://codereview.chromium.org/2895673002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/WebFrameScheduler.h:91: virtual
RefPtr<WebTaskRunner> UnthrottledButBlockableTaskRunner() = 0;
On 2017/05/26 15:07:31, Sami wrote:
> nit: please move this up to match the order in the comment.

Done.

https://codereview.chromium.org/2895673002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
(right):

https://codereview.chromium.org/2895673002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:3840:
TEST_F(RendererSchedulerImplTest,
On 2017/05/26 15:07:31, Sami wrote:
> TODO(altimin@): Re-enable after splitting the timer policy into separate
> policies.

Done.

https://codereview.chromium.org/2895673002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc
(right):

https://codereview.chromium.org/2895673002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc:201:
TaskQueue::QueueType::FRAME_UNTHROTTLED);
On 2017/05/26 15:07:31, Sami wrote:
> I guess we'll add a new queue type for this later too?

Yes. I have a modest proposal of removing the hard-coded queue types completely
and instead just save queue permissions inside the queue (is_throtteable,
is_blockable, is_suspendable). This will scale better and will simplify the
code.

[altimin, 2017-05-30 17:46:53]

The CQ bit was checked by altimin@chromium.org

[altimin, 2017-05-30 17:46:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2895673002/#ps100001
(title: "Address comments and disable a failing test")

[commit-bot: I haz the power, 2017-05-30 17:47:33]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 17:53:15]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496166413262910,
"parent_rev": "5dc1f110c5a1fc9fab3e79a83206000bf0aa61e0", "commit_rev":
"f39f12be44e4d35eb25c7a58b1895c751da723f5"}

[commit-bot: I haz the power, 2017-05-30 17:53:29]

Description was changed from

==========
[scheduler] Add general task queue to fix touch latency regression.

Every problem can be solved by adding an additional task queue, except
for the problem of too many task queues.

This should fix touch latency regression which a) is considered timer
task queue from renderer scheduler point of view and can be blocked
during touch move if expensive b) is not throttled or suspended,
therefore not breaking any other use cases.

This is a temporary fix and all tasks should be moved to suspendable or
unthrottled task queues.

NOTE: this patch temporary disables background throttling of the default timer
queue because otherwise suspendable tasks would be throttled too.

R=skyostil@chromium.org,haraken@chromium.org
CC=tdresser@chromium.org,nzolghadr@chromium.org

BUG=704939
==========

to

==========
[scheduler] Add general task queue to fix touch latency regression.

Every problem can be solved by adding an additional task queue, except
for the problem of too many task queues.

This should fix touch latency regression which a) is considered timer
task queue from renderer scheduler point of view and can be blocked
during touch move if expensive b) is not throttled or suspended,
therefore not breaking any other use cases.

This is a temporary fix and all tasks should be moved to suspendable or
unthrottled task queues.

NOTE: this patch temporary disables background throttling of the default timer
queue because otherwise suspendable tasks would be throttled too.

R=skyostil@chromium.org,haraken@chromium.org
CC=tdresser@chromium.org,nzolghadr@chromium.org

BUG=704939

Review-Url: https://codereview.chromium.org/2895673002
Cr-Commit-Position: refs/heads/master@{#475591}
Committed:
https://chromium.googlesource.com/chromium/src/+/f39f12be44e4d35eb25c7a58b189...
==========

[commit-bot: I haz the power, 2017-05-30 17:53:31]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/f39f12be44e4d35eb25c7a58b189...