Introduce TaskRunnerHelper::get()

Introduce TaskRunnerHelper::get()

Per the discussion in https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/uRvrk4CYSEc/discussion,
this CL introduces TaskRunnerHelper::get(), which maps TaskRunnerTypes
to actual task runners.

BUG=624689
Committed: https://crrev.com/19b8ee3aff5d67dd49f83e98c65f2410a9d4cf62
Cr-Commit-Position: refs/heads/master@{#409982}

[haraken, 2016-08-04 01:51:40]

haraken@chromium.org changed reviewers:
+ alexclarke@chromium.org, hiroshige@chromium.org, nhiroki@chromium.org,
skyostil@chromium.org, tzik@chromium.org

[haraken, 2016-08-04 01:51:41]

Sami, Alex: What do you think about this API?

[Sami, 2016-08-04 10:32:29]

https://codereview.chromium.org/2209023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/TaskRunnerHelper.h (right):

https://codereview.chromium.org/2209023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.h:19: enum TaskRunnerType {
FWIW we could use an enum class for this, e.g., TaskRunnerType::Networking. I'll
let you pick the style that best fits the rest of Blink :) The current scheme
requires less typing which is important too.

https://codereview.chromium.org/2209023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.h:42: IdleTask,
Idle tasks need a different kind of task runner (one that gives them a
deadline), so it doesn't seem like we can support them with this API?

[haraken, 2016-08-04 13:45:31]

PTAL

https://codereview.chromium.org/2209023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/TaskRunnerHelper.h (right):

https://codereview.chromium.org/2209023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.h:19: enum TaskRunnerType {
On 2016/08/04 10:32:29, Sami wrote:
> FWIW we could use an enum class for this, e.g., TaskRunnerType::Networking.
I'll
> let you pick the style that best fits the rest of Blink :) The current scheme
> requires less typing which is important too.

Used an enum class.

https://codereview.chromium.org/2209023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.h:42: IdleTask,
On 2016/08/04 10:32:29, Sami wrote:
> Idle tasks need a different kind of task runner (one that gives them a
> deadline), so it doesn't seem like we can support them with this API?

Agreed. Removed IdleTask.

[Sami, 2016-08-04 14:44:32]

lgtm, thanks.

[alex clarke (OOO till 29th), 2016-08-04 14:46:50]

lgtm

[haraken, 2016-08-04 15:36:03]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-08-04 15:36:30]

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, 2016-08-04 15:50:47]

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

[commit-bot: I haz the power, 2016-08-04 15:50:48]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[haraken, 2016-08-05 01:34:43]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-08-05 01:34:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexclarke@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2209023002/#ps60001
(title: "temp")

[commit-bot: I haz the power, 2016-08-05 01:34:58]

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, 2016-08-05 03:30:24]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-05 03:33:14]

Description was changed from

==========
Introduce TaskRunnerHelper::get()

Per the discussion in
https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/uR...,
this CL introduces TaskRunnerHelper::get(), which maps TaskRunnerTypes
to actual task runners.

BUG=624689
==========

to

==========
Introduce TaskRunnerHelper::get()

Per the discussion in
https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/uR...,
this CL introduces TaskRunnerHelper::get(), which maps TaskRunnerTypes
to actual task runners.

BUG=624689

Committed: https://crrev.com/19b8ee3aff5d67dd49f83e98c65f2410a9d4cf62
Cr-Commit-Position: refs/heads/master@{#409982}
==========

[commit-bot: I haz the power, 2016-08-05 03:33:15]

Patchset 4 (id:??) landed as
https://crrev.com/19b8ee3aff5d67dd49f83e98c65f2410a9d4cf62
Cr-Commit-Position: refs/heads/master@{#409982}

[kinuko, 2016-08-11 14:21:57]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2016-08-11 14:21:58]

https://codereview.chromium.org/2209023002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp (right):

https://codereview.chromium.org/2209023002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:27: case
TaskType::WebSocket:
IIRC some of WebSocket tasks are posted as loading tasks, this might cause
unwanted task reordering?

While introducing task types based on spec looks reasonable I'm slightly worried
that introducing this many task types at once might cause more confusion than
what we want to  achieve first (e.g. per-frame task scheduling).  Say, how do we
think about an internal task that could unblock multiple things that might fall
into multiple categories?

[Sami, 2016-08-11 14:52:46]

https://codereview.chromium.org/2209023002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp (right):

https://codereview.chromium.org/2209023002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:27: case
TaskType::WebSocket:
On 2016/08/11 14:21:58, kinuko wrote:
> IIRC some of WebSocket tasks are posted as loading tasks, this might cause
> unwanted task reordering?

Good point -- we probably want to fix that.

> While introducing task types based on spec looks reasonable I'm slightly
worried
> that introducing this many task types at once might cause more confusion than
> what we want to  achieve first (e.g. per-frame task scheduling).  Say, how do
we
> think about an internal task that could unblock multiple things that might
fall
> into multiple categories?

I'm not sure what you mean by unblocking? Or did you just mean some task that
does, say, WebSocket and DOMManipulation work? I think that's a good point and
requires some amount of caution. I can think of two options:

1) Make sure those tasks can be safely reordered w.r.t. the
WebSocket/DOMManipulation task runners (and vice versa).

2) Introduce a barrier object that can be used to enforce serialization at the
expense of possible extra delays.

It might be that the right option is different depending on the subsystem.

(We might also want to do some fuzz testing with a scheduler that maximizes task
reordering to weed out bugs.)

[haraken, 2016-08-11 15:05:04]

On 2016/08/11 14:52:46, Sami wrote:
>
https://codereview.chromium.org/2209023002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp (right):
> 
>
https://codereview.chromium.org/2209023002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:27: case
> TaskType::WebSocket:
> On 2016/08/11 14:21:58, kinuko wrote:
> > IIRC some of WebSocket tasks are posted as loading tasks, this might cause
> > unwanted task reordering?
> 
> Good point -- we probably want to fix that.
> 
> > While introducing task types based on spec looks reasonable I'm slightly
> worried
> > that introducing this many task types at once might cause more confusion
than
> > what we want to  achieve first (e.g. per-frame task scheduling).  

As far as I understand the super long discussion on the
platform-architecture-dev's thread, our conclusion is that we want to move tasks
to the per-frame scheduler improving the spec conformance, instead of anyway
moving tasks to the per-frame scheduler keeping the existing behavior. So it's
expected (and amazing!) that this kind of spec-conformance discussion happens
during the migration :)

> Say, how do
> we
> > think about an internal task that could unblock multiple things that might
> fall
> > into multiple categories?
> 
> I'm not sure what you mean by unblocking? Or did you just mean some task that
> does, say, WebSocket and DOMManipulation work? I think that's a good point and
> requires some amount of caution. I can think of two options:
> 
> 1) Make sure those tasks can be safely reordered w.r.t. the
> WebSocket/DOMManipulation task runners (and vice versa).
> 
> 2) Introduce a barrier object that can be used to enforce serialization at the
> expense of possible extra delays.
> 
> It might be that the right option is different depending on the subsystem.
> 
> (We might also want to do some fuzz testing with a scheduler that maximizes
task
> reordering to weed out bugs.)