Worker: Introduce per-global-scope task scheduler

Worker: Introduce per-global-scope task scheduler

This CL introduces WorkerGlobalScopeScheduler to make it simpler and saner to
manage tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, they should be handled in different ways based on their
task types though. For example, worker tasks shouldn't run after the global
scope is closed, while thread control tasks should run even after that. To
achieve it, worker tasks were always wrapped with WorkerThread's method to check
the current thread state. This was so tricky.

After this CL, each global scope has its own task queues like per-frame task
scheduler. Worker tasks are pushed into the task queues, control tasks are still
pushed into the thread's default task queue though. Thanks to this separation,
we can abort only worker tasks by simply unregistering the queues when a global
scope is closed.

In addition, this CL unifies postTask interfaces into TaskRunnerHelper (issue
694914), and enables any threads to post a task to worker threads because of
thread-safe refcounted WebTaskRunner.

BUG=670534, 694914, 710364
Review-Url: https://codereview.chromium.org/2806623004
Cr-Commit-Position: refs/heads/master@{#465841}
Committed: https://chromium.googlesource.com/chromium/src/+/d56a35fb25dc388a0506174b9e3fda67337313ce

[nhiroki, 2017-04-07 07:55:11]

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

[commit-bot: I haz the power, 2017-04-07 07:55:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nhiroki, 2017-04-07 08:48:35]

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

[commit-bot: I haz the power, 2017-04-07 08:48:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nhiroki, 2017-04-07 09:29:34]

Description was changed from

==========
(WIP) Worker: Introduce per-GlobalScope scheduler

BUG=
==========

to

==========
(WIP) Worker: Introduce per-GlobalScope task queue

BUG=
==========

[nhiroki, 2017-04-07 09:29:43]

Description was changed from

==========
(WIP) Worker: Introduce per-GlobalScope task queue

BUG=
==========

to

==========
(WIP) Worker: Introduce per-GlobalScope task queue

BUG=
==========

[commit-bot: I haz the power, 2017-04-07 10:42:25]

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

[commit-bot: I haz the power, 2017-04-07 10:42:26]

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_...)

[nhiroki, 2017-04-07 11:10:31]

Description was changed from

==========
(WIP) Worker: Introduce per-GlobalScope task queue

BUG=
==========

to

==========
(WIP) Worker: Introduce per-global-scope task scheduler

This CL introduces GlobalScopeScheduler to make it simpler and saner to manage
tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, we need to handle the tasks based on their types though.
For example, worker tasks shouldn't run after the global scope is closed, while
thread control tasks should run even after that. To achieve it, worker tasks
were always wrapped with WorkerThread's function to check the current state.
This was so tricky.

After this CL, each global scope has its own task queues. Worker tasks are
pushed into the task queues and control tasks are still pushed into the thread's
default task queue. Thanks to this separation, we can abort only worker tasks
by simply unregistering the queues when a global scope is closed. In addition,
this change enables to unify postTask interfaces into TaskRunnerHelper, and also
enables any threads to post tasks to worker threads because of thread-safe
refcounted WebTaskRunner.

BUG=694914
==========

[nhiroki, 2017-04-07 11:19:36]

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

[commit-bot: I haz the power, 2017-04-07 11:19:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nhiroki, 2017-04-07 11:20:05]

Description was changed from

==========
(WIP) Worker: Introduce per-global-scope task scheduler

This CL introduces GlobalScopeScheduler to make it simpler and saner to manage
tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, we need to handle the tasks based on their types though.
For example, worker tasks shouldn't run after the global scope is closed, while
thread control tasks should run even after that. To achieve it, worker tasks
were always wrapped with WorkerThread's function to check the current state.
This was so tricky.

After this CL, each global scope has its own task queues. Worker tasks are
pushed into the task queues and control tasks are still pushed into the thread's
default task queue. Thanks to this separation, we can abort only worker tasks
by simply unregistering the queues when a global scope is closed. In addition,
this change enables to unify postTask interfaces into TaskRunnerHelper, and also
enables any threads to post tasks to worker threads because of thread-safe
refcounted WebTaskRunner.

BUG=694914
==========

to

==========
(WIP) Worker: Introduce per-global-scope task scheduler

This CL introduces GlobalScopeScheduler to make it simpler and saner to manage
tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, we need to handle the tasks based on their types though.
For example, worker tasks shouldn't run after the global scope is closed, while
thread control tasks should run even after that. To achieve it, worker tasks
were always wrapped with WorkerThread's function to check the current state.
This was so tricky.

After this CL, each global scope has its own task queues. Worker tasks are
pushed into the task queues and control tasks are still pushed into the thread's
default task queue. Thanks to this separation, we can abort only worker tasks
by simply unregistering the queues when a global scope is closed. In addition,
this change enables to unify postTask interfaces into TaskRunnerHelper, and also
enables any threads to post tasks to worker threads because of thread-safe
refcounted WebTaskRunner.

BUG=694914
==========

[nhiroki, 2017-04-07 11:20:39]

Description was changed from

==========
(WIP) Worker: Introduce per-global-scope task scheduler

This CL introduces GlobalScopeScheduler to make it simpler and saner to manage
tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, we need to handle the tasks based on their types though.
For example, worker tasks shouldn't run after the global scope is closed, while
thread control tasks should run even after that. To achieve it, worker tasks
were always wrapped with WorkerThread's function to check the current state.
This was so tricky.

After this CL, each global scope has its own task queues. Worker tasks are
pushed into the task queues and control tasks are still pushed into the thread's
default task queue. Thanks to this separation, we can abort only worker tasks
by simply unregistering the queues when a global scope is closed. In addition,
this change enables to unify postTask interfaces into TaskRunnerHelper, and also
enables any threads to post tasks to worker threads because of thread-safe
refcounted WebTaskRunner.

BUG=694914
==========

to

==========
(WIP) Worker: Introduce per-global-scope task scheduler

This CL introduces GlobalScopeScheduler to make it simpler and saner to manage
tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, we need to handle the tasks based on their types though.
For example, worker tasks shouldn't run after the global scope is closed, while
thread control tasks should run even after that. To achieve it, worker tasks
were always wrapped with WorkerThread's function to check the current state.
This was so tricky.

After this CL, each global scope has its own task queues. Worker tasks are
pushed into the task queues and control tasks are still pushed into the thread's
default task queue. Thanks to this separation, we can abort only worker tasks
by simply unregistering the queues when a global scope is closed. In addition,
this change enables to unify postTask interfaces into TaskRunnerHelper, and also
enables any threads to post tasks to worker threads because of thread-safe
refcounted WebTaskRunner.

BUG=670534, 694914
==========

[nhiroki, 2017-04-07 11:20:47]

Patchset #1 (id:1) has been deleted

[nhiroki, 2017-04-07 11:21:03]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2017-04-07 11:31:25]

Description was changed from

==========
(WIP) Worker: Introduce per-global-scope task scheduler

This CL introduces GlobalScopeScheduler to make it simpler and saner to manage
tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, we need to handle the tasks based on their types though.
For example, worker tasks shouldn't run after the global scope is closed, while
thread control tasks should run even after that. To achieve it, worker tasks
were always wrapped with WorkerThread's function to check the current state.
This was so tricky.

After this CL, each global scope has its own task queues. Worker tasks are
pushed into the task queues and control tasks are still pushed into the thread's
default task queue. Thanks to this separation, we can abort only worker tasks
by simply unregistering the queues when a global scope is closed. In addition,
this change enables to unify postTask interfaces into TaskRunnerHelper, and also
enables any threads to post tasks to worker threads because of thread-safe
refcounted WebTaskRunner.

BUG=670534, 694914
==========

to

==========
(WIP) Worker: Introduce per-global-scope task scheduler

This CL introduces GlobalScopeScheduler to make it simpler and saner to manage
tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, they should be handled in different ways based on their
types though. For example, worker tasks shouldn't run after the global scope is
closed, while thread control tasks should run even after that. To achieve it,
worker tasks were always wrapped with WorkerThread's method to check the current
thread state. This was so tricky.

After this CL, each global scope has its own task queues. Worker tasks are
pushed into the task queues and control tasks are still pushed into the thread's
default task queue. Thanks to this separation, we can abort only worker tasks
by simply unregistering the queues when a global scope is closed.

In addition, this change enables to unify postTask interfaces into
TaskRunnerHelper (issue 694914), makes it easier to introduce task throttling /
suspending mechanism (see issue 670534), and also enables any threads to post a
task to worker threads because of thread-safe refcounted WebTaskRunner.

BUG=670534, 694914
==========

[nhiroki, 2017-04-07 11:33:43]

Description was changed from

==========
(WIP) Worker: Introduce per-global-scope task scheduler

This CL introduces GlobalScopeScheduler to make it simpler and saner to manage
tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, they should be handled in different ways based on their
types though. For example, worker tasks shouldn't run after the global scope is
closed, while thread control tasks should run even after that. To achieve it,
worker tasks were always wrapped with WorkerThread's method to check the current
thread state. This was so tricky.

After this CL, each global scope has its own task queues. Worker tasks are
pushed into the task queues and control tasks are still pushed into the thread's
default task queue. Thanks to this separation, we can abort only worker tasks
by simply unregistering the queues when a global scope is closed.

In addition, this change enables to unify postTask interfaces into
TaskRunnerHelper (issue 694914), makes it easier to introduce task throttling /
suspending mechanism (see issue 670534), and also enables any threads to post a
task to worker threads because of thread-safe refcounted WebTaskRunner.

BUG=670534, 694914
==========

to

==========
(WIP) Worker: Introduce per-global-scope task scheduler

This CL introduces GlobalScopeScheduler to make it simpler and saner to manage
tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, they should be handled in different ways based on their
types though. For example, worker tasks shouldn't run after the global scope is
closed, while thread control tasks should run even after that. To achieve it,
worker tasks were always wrapped with WorkerThread's method to check the current
thread state. This was so tricky.

After this CL, each global scope has its own task queues. Worker tasks are
pushed into the task queues and control tasks are still pushed into the thread's
default task queue. Thanks to this separation, we can abort only worker tasks
by simply unregistering the queues when a global scope is closed.

In addition, this CL unifies postTask interfaces into TaskRunnerHelper (issue
694914) and enables any threads to post a task to worker threads because of
thread-safe refcounted WebTaskRunner.

BUG=670534, 694914
==========

[nhiroki, 2017-04-07 11:39:10]

Description was changed from

==========
(WIP) Worker: Introduce per-global-scope task scheduler

This CL introduces GlobalScopeScheduler to make it simpler and saner to manage
tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, they should be handled in different ways based on their
types though. For example, worker tasks shouldn't run after the global scope is
closed, while thread control tasks should run even after that. To achieve it,
worker tasks were always wrapped with WorkerThread's method to check the current
thread state. This was so tricky.

After this CL, each global scope has its own task queues. Worker tasks are
pushed into the task queues and control tasks are still pushed into the thread's
default task queue. Thanks to this separation, we can abort only worker tasks
by simply unregistering the queues when a global scope is closed.

In addition, this CL unifies postTask interfaces into TaskRunnerHelper (issue
694914) and enables any threads to post a task to worker threads because of
thread-safe refcounted WebTaskRunner.

BUG=670534, 694914
==========

to

==========
(WIP) Worker: Introduce per-global-scope task scheduler

This CL introduces GlobalScopeScheduler to make it simpler and saner to manage
tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, they should be handled in different ways based on their
task types though. For example, worker tasks shouldn't run after the global
scope is closed, while thread control tasks should run even after that. To
achieve it, worker tasks were always wrapped with WorkerThread's method to check
the current thread state. This was so tricky.

After this CL, each global scope has its own task queues like per-frame task
scheduler. Worker tasks are pushed into the task queues, control tasks are still
pushed into the thread's default task queue though. Thanks to this separation,
we can abort only worker tasks by simply unregistering the queues when a global
scope is closed.

In addition, this CL unifies postTask interfaces into TaskRunnerHelper (issue
694914) and enables any threads to post a task to worker threads because of
thread-safe refcounted WebTaskRunner.

BUG=670534, 694914
==========

[nhiroki, 2017-04-07 11:55:50]

nhiroki@chromium.org changed reviewers:
+ altimin@chromium.org, haraken@chromium.org, kinuko@chromium.org,
skyostil@chromium.org, tzik@chromium.org

[nhiroki, 2017-04-07 11:55:50]

Hi everyone,

Could you take a look at this WIP CL? I'd like to confirm whether I'm going in
the right direction. Any feedback is welcome.

Thank you.

[nhiroki, 2017-04-07 11:57:09]

FYI: CompositorWorker/AnimationWorklet tests are still failing because of
unimplemented functions in CompositorScheduler.

[commit-bot: I haz the power, 2017-04-07 12:49:44]

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

[commit-bot: I haz the power, 2017-04-07 12:49:46]

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_...)

[kinuko, 2017-04-10 06:30:06]

Will take another look later, but sending some early comments.  To me it starts
to feel that it is (more) confusing (than before) that something that's named
'WorkerThread' is actually a per-global-scope thing at this point.  I think
you've been planning something for that too, what's your thought there?

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

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:68: return get(type,
static_cast<Document*>(nullptr));
You can/should use ToDocument instead.

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:85: WorkerThread*
workerThread) {
Could this just take WorkerOrWorkletGlobalScope instead (and hide the naming
gotcha around WorkerThread, which feels really non-intuitive here)?

Hm, but you probably want to limit access to GlobalScope / ExecutionContext from
other threads too.

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:110: return
workerThread->globalScopeScheduler()->unthrottledTaskRunner();
Would be nice to have some comments as 'unthrottled' thing should be generally
discouraged in future?

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:127:
crossThreadUnretained(&waitableEvent)));
Is it the requirement of NewTaskQueue implementation that this needs to run on
the new worker thread? (Looks like yes)

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.h:300: RefPtr<WebTaskRunner>
m_threadControlTaskRunner;
It feels a little confusing to have this and workerBackingThread() separately.
Could we clean up further / merge these two concepts?

[Sami, 2017-04-10 16:58:10]

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

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:110: return
workerThread->globalScopeScheduler()->unthrottledTaskRunner();
On 2017/04/10 06:30:06, kinuko wrote:
> Would be nice to have some comments as 'unthrottled' thing should be generally
> discouraged in future?

+1

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:127:
crossThreadUnretained(&waitableEvent)));
On 2017/04/10 06:30:06, kinuko wrote:
> Is it the requirement of NewTaskQueue implementation that this needs to run on
> the new worker thread? (Looks like yes)

Yes, that's the case.

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.h:295:
std::unique_ptr<scheduler::GlobalScopeScheduler> m_globalScopeScheduler;
Would it make sense to call this a WorkerScheduler instead (and merge with the
existing class with that name)? "Global scope" sounds, well, global, and makes
me wonder why there isn't one of these on the main thread too.

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/child/global_scope_scheduler.h
(right):

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/child/global_scope_scheduler.h:26:
void dispose();
Please document what this method does.

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/child/worker_scheduler_impl.cc
(right):

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/child/worker_scheduler_impl.cc:98:
// TODO: TimeDomain??
A real time time domain (i.e., nullptr) is fine for now. We'll probably look at
supporting virtual time on worker threads at some point.

[altimin, 2017-04-10 17:59:46]

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/scheduler/child/worker_scheduler.h
(right):

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/scheduler/child/worker_scheduler.h:31:
virtual scoped_refptr<TaskQueue> NewUnthrottledTaskRunner(
I'll make your life more interesting: we're changing public/platform/child API
surface and are trying to narrow it down. TaskQueue will be definitely moved to
Source/platform/scheduler, I'll try to move WorkerScheduler too. I'll keep you
posted.

[Sami, 2017-04-11 09:53:06]

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/scheduler/child/worker_scheduler.h
(right):

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/scheduler/child/worker_scheduler.h:31:
virtual scoped_refptr<TaskQueue> NewUnthrottledTaskRunner(
On 2017/04/10 17:59:46, altimin wrote:
> I'll make your life more interesting: we're changing public/platform/child API
> surface and are trying to narrow it down. TaskQueue will be definitely moved
to
> Source/platform/scheduler, I'll try to move WorkerScheduler too. I'll keep you
> posted.

That shouldn't affect things here since this patch is all in Blink, right? We
may need to tweak the way threads are initialized from content but that should
be largely orthogonal.

[nhiroki, 2017-04-11 10:41:18]

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

[commit-bot: I haz the power, 2017-04-11 10:41:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[altimin, 2017-04-11 10:47:01]

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/scheduler/child/worker_scheduler.h
(right):

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/scheduler/child/worker_scheduler.h:31:
virtual scoped_refptr<TaskQueue> NewUnthrottledTaskRunner(
On 2017/04/11 09:53:06, Sami wrote:
> On 2017/04/10 17:59:46, altimin wrote:
> > I'll make your life more interesting: we're changing public/platform/child
API
> > surface and are trying to narrow it down. TaskQueue will be definitely moved
> to
> > Source/platform/scheduler, I'll try to move WorkerScheduler too. I'll keep
you
> > posted.
> 
> That shouldn't affect things here since this patch is all in Blink, right? We
> may need to tweak the way threads are initialized from content but that should
> be largely orthogonal.

Not exactly: this patch touches public/platform and will be affected by layering
improvements

[nhiroki, 2017-04-11 10:47:54]

Thank you for your comments! Updated the patch set. Can you take another look,
this is still WIP though?

I'll implement CompositorScheduler part and brush up the entire patch tomorrow.

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

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:68: return get(type,
static_cast<Document*>(nullptr));
On 2017/04/10 06:30:06, kinuko wrote:
> You can/should use ToDocument instead.

Done.

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:85: WorkerThread*
workerThread) {
On 2017/04/10 06:30:06, kinuko wrote:
> Could this just take WorkerOrWorkletGlobalScope instead (and hide the naming
> gotcha around WorkerThread, which feels really non-intuitive here)?
> 
> Hm, but you probably want to limit access to GlobalScope / ExecutionContext
from
> other threads too.

We could do the same thing as ParentFrameTaskRunners, that is, capture a set of
task runners in WorkerThread::initializeSchedulerOnWorkerThread and make foreign
threads use the set instead of TaskRunnerHelper.

The patchset-3 introduces WorkerTaskRunners for that. What do you think about
this?

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.h:295:
std::unique_ptr<scheduler::GlobalScopeScheduler> m_globalScopeScheduler;
On 2017/04/10 16:58:10, Sami wrote:
> Would it make sense to call this a WorkerScheduler instead (and merge with the
> existing class with that name)? "Global scope" sounds, well, global, and makes
> me wonder why there isn't one of these on the main thread too.

Current WorkerScheduler is a per-worker-thread object, while
GlobalScopeScheduler is a per-global-scope object[*], so IMO keeping them
separate would be simpler. If this is just an naming issue, how about renaming
it to something else, for example, WorkerOrWorkletGlobalScopeScheduler?

[*] For worklets, a worker thread can host multiple global scopes. See the
figure in this doc:
https://docs.google.com/document/d/1a0qQwfquxsPLFRmVfaySHic02ll-TaR50J7Va91cL...

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.h:300: RefPtr<WebTaskRunner>
m_threadControlTaskRunner;
On 2017/04/10 06:30:06, kinuko wrote:
> It feels a little confusing to have this and workerBackingThread() separately.
> Could we clean up further / merge these two concepts?

For now, I cannot come up with an idea to clean it up further or merge them, so
reverted this. I'll revisit this later.

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/child/global_scope_scheduler.h
(right):

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/child/global_scope_scheduler.h:26:
void dispose();
On 2017/04/10 16:58:10, Sami wrote:
> Please document what this method does.

Done.

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scheduler/child/worker_scheduler_impl.cc
(right):

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/child/worker_scheduler_impl.cc:98:
// TODO: TimeDomain??
On 2017/04/10 16:58:10, Sami wrote:
> A real time time domain (i.e., nullptr) is fine for now. We'll probably look
at
> supporting virtual time on worker threads at some point.

Thank you for the information. Removed this comment.

[nhiroki, 2017-04-11 10:51:58]

On 2017/04/10 06:30:06, kinuko wrote:
> Will take another look later, but sending some early comments.  To me it
starts
> to feel that it is (more) confusing (than before) that something that's named
> 'WorkerThread' is actually a per-global-scope thing at this point.  I think
> you've been planning something for that too, what's your thought there?

Yes, I plan to clarify per-global-scope/per-platform-thread things and rename
some classes (e.g., WorkerThread -> GlobalScopeController). This is a design
document about it:
https://docs.google.com/document/d/1a0qQwfquxsPLFRmVfaySHic02ll-TaR50J7Va91cL...
(any feedback is welcome!)

[Sami, 2017-04-11 11:52:03]

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.h:295:
std::unique_ptr<scheduler::GlobalScopeScheduler> m_globalScopeScheduler;
On 2017/04/11 10:47:54, nhiroki wrote:
> On 2017/04/10 16:58:10, Sami wrote:
> > Would it make sense to call this a WorkerScheduler instead (and merge with
the
> > existing class with that name)? "Global scope" sounds, well, global, and
makes
> > me wonder why there isn't one of these on the main thread too.
> 
> Current WorkerScheduler is a per-worker-thread object, while
> GlobalScopeScheduler is a per-global-scope object[*], so IMO keeping them
> separate would be simpler. If this is just an naming issue, how about renaming
> it to something else, for example, WorkerOrWorkletGlobalScopeScheduler?
> 
> [*] For worklets, a worker thread can host multiple global scopes. See the
> figure in this doc:
>
https://docs.google.com/document/d/1a0qQwfquxsPLFRmVfaySHic02ll-TaR50J7Va91cL...

Ah, I see. So you can have multiple global scopes on a single worker thread?
WorkerGlobalScopeScheduler would make it obvious why there isn't one on the main
thread (not sure if it's accurate however), but I'd be happy with
GlobalScopeScheduler too if you prefer.

[commit-bot: I haz the power, 2017-04-11 12:06:30]

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

[commit-bot: I haz the power, 2017-04-11 12:06:31]

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_...)

[nhiroki, 2017-04-12 02:31:29]

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

[commit-bot: I haz the power, 2017-04-12 02:31:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nhiroki, 2017-04-12 03:23:19]

Description was changed from

==========
(WIP) Worker: Introduce per-global-scope task scheduler

This CL introduces GlobalScopeScheduler to make it simpler and saner to manage
tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, they should be handled in different ways based on their
task types though. For example, worker tasks shouldn't run after the global
scope is closed, while thread control tasks should run even after that. To
achieve it, worker tasks were always wrapped with WorkerThread's method to check
the current thread state. This was so tricky.

After this CL, each global scope has its own task queues like per-frame task
scheduler. Worker tasks are pushed into the task queues, control tasks are still
pushed into the thread's default task queue though. Thanks to this separation,
we can abort only worker tasks by simply unregistering the queues when a global
scope is closed.

In addition, this CL unifies postTask interfaces into TaskRunnerHelper (issue
694914) and enables any threads to post a task to worker threads because of
thread-safe refcounted WebTaskRunner.

BUG=670534, 694914
==========

to

==========
(WIP) Worker: Introduce per-global-scope task scheduler

This CL introduces GlobalScopeScheduler to make it simpler and saner to manage
tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, they should be handled in different ways based on their
task types though. For example, worker tasks shouldn't run after the global
scope is closed, while thread control tasks should run even after that. To
achieve it, worker tasks were always wrapped with WorkerThread's method to check
the current thread state. This was so tricky.

After this CL, each global scope has its own task queues like per-frame task
scheduler. Worker tasks are pushed into the task queues, control tasks are still
pushed into the thread's default task queue though. Thanks to this separation,
we can abort only worker tasks by simply unregistering the queues when a global
scope is closed.

In addition, this CL unifies postTask interfaces into TaskRunnerHelper (issue
694914) and enables any threads to post a task to worker threads because of
thread-safe refcounted WebTaskRunner.

BUG=670534, 694914, 710364
==========

[commit-bot: I haz the power, 2017-04-12 03:36:38]

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

[commit-bot: I haz the power, 2017-04-12 03:36:39]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nhiroki, 2017-04-12 04:44:28]

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

[commit-bot: I haz the power, 2017-04-12 04:44:44]

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-04-12 06:00:08]

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

[commit-bot: I haz the power, 2017-04-12 06:00:09]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[nhiroki, 2017-04-12 07:43:47]

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

[commit-bot: I haz the power, 2017-04-12 07:44:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nhiroki, 2017-04-12 08:41:29]

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

[commit-bot: I haz the power, 2017-04-12 08:41:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nhiroki, 2017-04-12 08:42:44]

Description was changed from

==========
(WIP) Worker: Introduce per-global-scope task scheduler

This CL introduces GlobalScopeScheduler to make it simpler and saner to manage
tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, they should be handled in different ways based on their
task types though. For example, worker tasks shouldn't run after the global
scope is closed, while thread control tasks should run even after that. To
achieve it, worker tasks were always wrapped with WorkerThread's method to check
the current thread state. This was so tricky.

After this CL, each global scope has its own task queues like per-frame task
scheduler. Worker tasks are pushed into the task queues, control tasks are still
pushed into the thread's default task queue though. Thanks to this separation,
we can abort only worker tasks by simply unregistering the queues when a global
scope is closed.

In addition, this CL unifies postTask interfaces into TaskRunnerHelper (issue
694914) and enables any threads to post a task to worker threads because of
thread-safe refcounted WebTaskRunner.

BUG=670534, 694914, 710364
==========

to

==========
Worker: Introduce per-global-scope task scheduler

This CL introduces WorkerGlobalScopeScheduler to make it simpler and saner to
manage tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, they should be handled in different ways based on their
task types though. For example, worker tasks shouldn't run after the global
scope is closed, while thread control tasks should run even after that. To
achieve it, worker tasks were always wrapped with WorkerThread's method to check
the current thread state. This was so tricky.

After this CL, each global scope has its own task queues like per-frame task
scheduler. Worker tasks are pushed into the task queues, control tasks are still
pushed into the thread's default task queue though. Thanks to this separation,
we can abort only worker tasks by simply unregistering the queues when a global
scope is closed.

In addition, this CL unifies postTask interfaces into TaskRunnerHelper and
WorkerTaskRunners (issue 694914), and enables any threads to post a task to
worker threads because of thread-safe refcounted WebTaskRunner.

BUG=670534, 694914, 710364
==========

[nhiroki, 2017-04-12 08:43:13]

Description was changed from

==========
Worker: Introduce per-global-scope task scheduler

This CL introduces WorkerGlobalScopeScheduler to make it simpler and saner to
manage tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, they should be handled in different ways based on their
task types though. For example, worker tasks shouldn't run after the global
scope is closed, while thread control tasks should run even after that. To
achieve it, worker tasks were always wrapped with WorkerThread's method to check
the current thread state. This was so tricky.

After this CL, each global scope has its own task queues like per-frame task
scheduler. Worker tasks are pushed into the task queues, control tasks are still
pushed into the thread's default task queue though. Thanks to this separation,
we can abort only worker tasks by simply unregistering the queues when a global
scope is closed.

In addition, this CL unifies postTask interfaces into TaskRunnerHelper and
WorkerTaskRunners (issue 694914), and enables any threads to post a task to
worker threads because of thread-safe refcounted WebTaskRunner.

BUG=670534, 694914, 710364
==========

to

==========
Worker: Introduce per-global-scope task scheduler

This CL introduces WorkerGlobalScopeScheduler to make it simpler and saner to
manage tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, they should be handled in different ways based on their
task types though. For example, worker tasks shouldn't run after the global
scope is closed, while thread control tasks should run even after that. To
achieve it, worker tasks were always wrapped with WorkerThread's method to check
the current thread state. This was so tricky.

After this CL, each global scope has its own task queues like per-frame task
scheduler. Worker tasks are pushed into the task queues, control tasks are still
pushed into the thread's default task queue though. Thanks to this separation,
we can abort only worker tasks by simply unregistering the queues when a global
scope is closed.

In addition, this CL unifies postTask interfaces into TaskRunnerHelper and
WorkerTaskRunners (issue 694914), and enables any threads to post a task to
worker threads because of thread-safe refcounted WebTaskRunner.

BUG=670534, 694914, 710364
==========

[nhiroki, 2017-04-12 08:54:45]

I addressed all your comments and this is now ready for formal review. Please
take another look. Thanks!

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerThread.h (right):

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerThread.h:295:
std::unique_ptr<scheduler::GlobalScopeScheduler> m_globalScopeScheduler;
On 2017/04/11 11:52:02, Sami wrote:
> On 2017/04/11 10:47:54, nhiroki wrote:
> > On 2017/04/10 16:58:10, Sami wrote:
> > > Would it make sense to call this a WorkerScheduler instead (and merge with
> the
> > > existing class with that name)? "Global scope" sounds, well, global, and
> makes
> > > me wonder why there isn't one of these on the main thread too.
> > 
> > Current WorkerScheduler is a per-worker-thread object, while
> > GlobalScopeScheduler is a per-global-scope object[*], so IMO keeping them
> > separate would be simpler. If this is just an naming issue, how about
renaming
> > it to something else, for example, WorkerOrWorkletGlobalScopeScheduler?
> > 
> > [*] For worklets, a worker thread can host multiple global scopes. See the
> > figure in this doc:
> >
>
https://docs.google.com/document/d/1a0qQwfquxsPLFRmVfaySHic02ll-TaR50J7Va91cL...
> 
> Ah, I see. So you can have multiple global scopes on a single worker thread?

Yes!

> WorkerGlobalScopeScheduler would make it obvious why there isn't one on the
main
> thread (not sure if it's accurate however), but I'd be happy with
> GlobalScopeScheduler too if you prefer.

Sounds good. I renamed it to WorkerGlobalScopeScheduler.

Under core/workers/, GlobalScopeScheduler would be understandable because it is
a directory for worker components. On the other hand, under platform/scheduler/,
it may be confusing because the directory is not specific to workers. Therefore,
WorkerGlobalScopeScheduler would be better.

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/scheduler/child/worker_scheduler.h
(right):

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/scheduler/child/worker_scheduler.h:31:
virtual scoped_refptr<TaskQueue> NewUnthrottledTaskRunner(
On 2017/04/11 10:47:00, altimin wrote:
> On 2017/04/11 09:53:06, Sami wrote:
> > On 2017/04/10 17:59:46, altimin wrote:
> > > I'll make your life more interesting: we're changing public/platform/child
> API
> > > surface and are trying to narrow it down. TaskQueue will be definitely
moved
> > to
> > > Source/platform/scheduler, I'll try to move WorkerScheduler too. I'll keep
> you
> > > posted.
> > 
> > That shouldn't affect things here since this patch is all in Blink, right?
We
> > may need to tweak the way threads are initialized from content but that
should
> > be largely orthogonal.
> 
> Not exactly: this patch touches public/platform and will be affected by
layering
> improvements 

Thank you for the heads-up. I rebased / will rebase this on your CLs.

[nhiroki, 2017-04-12 09:42:38]

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

[commit-bot: I haz the power, 2017-04-12 09:42:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kinuko, 2017-04-12 09:47:44]

(Response only, haven't taken a look at the latest patch yet)

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

https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:85: WorkerThread*
workerThread) {
On 2017/04/11 10:47:54, nhiroki wrote:
> On 2017/04/10 06:30:06, kinuko wrote:
> > Could this just take WorkerOrWorkletGlobalScope instead (and hide the naming
> > gotcha around WorkerThread, which feels really non-intuitive here)?
> > 
> > Hm, but you probably want to limit access to GlobalScope / ExecutionContext
> from
> > other threads too.
> 
> We could do the same thing as ParentFrameTaskRunners, that is, capture a set
of
> task runners in WorkerThread::initializeSchedulerOnWorkerThread and make
foreign
> threads use the set instead of TaskRunnerHelper.
> 
> The patchset-3 introduces WorkerTaskRunners for that. What do you think about
> this?

Sorry my comment was probably unclear, what I felt weird was the fact that we're
retrieving per-global-scope thing from something that is named WorkerThread, so
introducing WorkerTaskRunners doesn't seem very helpful here / given that you
have future plan having some TODO comment might be enough.

(I think commonly using TaskRunnerHelper itself makes sense, though cross-thread
usage might need some extra care)

[commit-bot: I haz the power, 2017-04-12 10:48:34]

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

[commit-bot: I haz the power, 2017-04-12 10:48:35]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Sami, 2017-04-12 15:54:07]

I think this looks great! Left a few comments below.

https://codereview.chromium.org/2806623004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerTaskRunners.cpp (right):

https://codereview.chromium.org/2806623004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerTaskRunners.cpp:31: return
task_runners_.at(type);
Is there some benefit to caching runners in |task_runners_| that I'm not seeing?
It seems like we could just call TaskRunnerHelper::Get directly here, no?

https://codereview.chromium.org/2806623004/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc
(right):

https://codereview.chromium.org/2806623004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc:18:
CompositorWorkerScheduler::CompositorWorkerScheduler(
FYI, last time we tried to do this there was a minor perf regression which we
mostly optimized away. The last thing remaining there is to add support for task
cancellation in base::MessageLoop. dcheng@ has a patch for that.

https://codereview.chromium.org/2806623004/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler.h
(right):

https://codereview.chromium.org/2806623004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler.h:27:
void Dispose();
Could you add a test for this please?

[kinuko, 2017-04-13 01:46:59]

https://codereview.chromium.org/2806623004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerTaskRunners.cpp (right):

https://codereview.chromium.org/2806623004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerTaskRunners.cpp:31: return
task_runners_.at(type);
On 2017/04/12 15:54:06, Sami wrote:
> Is there some benefit to caching runners in |task_runners_| that I'm not
seeing?
> It seems like we could just call TaskRunnerHelper::Get directly here, no?

Yeah, (in case it wasn't clear) I agree with this comment.

[haraken, 2017-04-13 07:03:36]

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp (right):

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:71: return Get(type,
ToDocument(executionContext));

Can we simply pass nullptr?

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:77: return Get(type,
ToDocument(executionContext));

Ditto.

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp
(right):

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp:77:
TaskRunnerHelper::Get(TaskType::kNetworking, worker_thread_.get())

Why does this use kNetworking?

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp
(right):

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:74:
TaskRunnerHelper::Get(TaskType::kUnthrottled, thread)

Why kUnthrottled?

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/webthread_impl_for_worker_scheduler.h
(right):

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/webthread_impl_for_worker_scheduler.h:52:
return worker_scheduler_.get();

It looks strange that:

- webthread_impl_for_worker_scheduler owns the scheduler by std::unique_ptr.

- the caller site also owns the scheduler by creating another std::unique_ptr by
calling MakeUnique(GetWorkerScheduler()).

Who should own the scheduler?

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler.h
(right):

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler.h:23:
WorkerGlobalScopeScheduler(WorkerScheduler* worker_scheduler);

Add explicit.

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler.h:40:
scoped_refptr<TaskQueue> unthrottled_task_queue_;

Why do we need to own unthrottled_task_queue_? I think the WebTaskRunner should
own it.

[haraken, 2017-04-13 07:03:51]

On 2017/04/13 07:03:36, haraken wrote:
>
https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp (right):
> 
>
https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:71: return Get(type,
> ToDocument(executionContext));
> 
> Can we simply pass nullptr?
> 
>
https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:77: return Get(type,
> ToDocument(executionContext));
> 
> Ditto.
> 
>
https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp
> (right):
> 
>
https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp:77:
> TaskRunnerHelper::Get(TaskType::kNetworking, worker_thread_.get())
> 
> Why does this use kNetworking?
> 
>
https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
> File
> third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp
> (right):
> 
>
https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:74:
> TaskRunnerHelper::Get(TaskType::kUnthrottled, thread)
> 
> Why kUnthrottled?
> 
>
https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
> File
>
third_party/WebKit/Source/platform/scheduler/child/webthread_impl_for_worker_scheduler.h
> (right):
> 
>
https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/platform/scheduler/child/webthread_impl_for_worker_scheduler.h:52:
> return worker_scheduler_.get();
> 
> It looks strange that:
> 
> - webthread_impl_for_worker_scheduler owns the scheduler by std::unique_ptr.
> 
> - the caller site also owns the scheduler by creating another std::unique_ptr
by
> calling MakeUnique(GetWorkerScheduler()).
> 
> Who should own the scheduler?
> 
>
https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
> File
>
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler.h
> (right):
> 
>
https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler.h:23:
> WorkerGlobalScopeScheduler(WorkerScheduler* worker_scheduler);
> 
> Add explicit.
> 
>
https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler.h:40:
> scoped_refptr<TaskQueue> unthrottled_task_queue_;
> 
> Why do we need to own unthrottled_task_queue_? I think the WebTaskRunner
should
> own it.

The overall approach looks great.

[nhiroki, 2017-04-13 08:17:59]

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

[commit-bot: I haz the power, 2017-04-13 08:18:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nhiroki, 2017-04-13 08:21:57]

Description was changed from

==========
Worker: Introduce per-global-scope task scheduler

This CL introduces WorkerGlobalScopeScheduler to make it simpler and saner to
manage tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, they should be handled in different ways based on their
task types though. For example, worker tasks shouldn't run after the global
scope is closed, while thread control tasks should run even after that. To
achieve it, worker tasks were always wrapped with WorkerThread's method to check
the current thread state. This was so tricky.

After this CL, each global scope has its own task queues like per-frame task
scheduler. Worker tasks are pushed into the task queues, control tasks are still
pushed into the thread's default task queue though. Thanks to this separation,
we can abort only worker tasks by simply unregistering the queues when a global
scope is closed.

In addition, this CL unifies postTask interfaces into TaskRunnerHelper and
WorkerTaskRunners (issue 694914), and enables any threads to post a task to
worker threads because of thread-safe refcounted WebTaskRunner.

BUG=670534, 694914, 710364
==========

to

==========
Worker: Introduce per-global-scope task scheduler

This CL introduces WorkerGlobalScopeScheduler to make it simpler and saner to
manage tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, they should be handled in different ways based on their
task types though. For example, worker tasks shouldn't run after the global
scope is closed, while thread control tasks should run even after that. To
achieve it, worker tasks were always wrapped with WorkerThread's method to check
the current thread state. This was so tricky.

After this CL, each global scope has its own task queues like per-frame task
scheduler. Worker tasks are pushed into the task queues, control tasks are still
pushed into the thread's default task queue though. Thanks to this separation,
we can abort only worker tasks by simply unregistering the queues when a global
scope is closed.

In addition, this CL unifies postTask interfaces into TaskRunnerHelper (issue
694914), and enables any threads to post a task to worker threads because of
thread-safe refcounted WebTaskRunner.

BUG=670534, 694914, 710364
==========

[nhiroki, 2017-04-13 08:23:50]

Thank you! Updated.

https://codereview.chromium.org/2806623004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerTaskRunners.cpp (right):

https://codereview.chromium.org/2806623004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerTaskRunners.cpp:31: return
task_runners_.at(type);
On 2017/04/13 01:46:59, kinuko wrote:
> On 2017/04/12 15:54:06, Sami wrote:
> > Is there some benefit to caching runners in |task_runners_| that I'm not
> seeing?
> > It seems like we could just call TaskRunnerHelper::Get directly here, no?

I removed WorkerTaskRunners. The original motivation of WorkerTaskRunenrs is
here:
https://codereview.chromium.org/2806623004/diff/40001/third_party/WebKit/Sour...

At first, I think it could be clearer to separate the interfaces into
TaskRunnerHelper for same-thread-postTask and
ParentFrameTaskRunners/WorkerTaskRunners for cross-thread postTask. After
offline chat with kinuko@, we decided to unify them into TaskRunnerHelper for
simplicity and reconsider the cross-thread postTask interface later.

> Yeah, (in case it wasn't clear) I agree with this comment.

https://codereview.chromium.org/2806623004/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc
(right):

https://codereview.chromium.org/2806623004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc:18:
CompositorWorkerScheduler::CompositorWorkerScheduler(
On 2017/04/12 15:54:06, Sami wrote:
> FYI, last time we tried to do this there was a minor perf regression which we
> mostly optimized away. The last thing remaining there is to add support for
task
> cancellation in base::MessageLoop. dcheng@ has a patch for that.

Thank you for the information. I'll take care of perf regression reports.

Should I wait for dcheng@'s patch before landing this?

https://codereview.chromium.org/2806623004/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler.h
(right):

https://codereview.chromium.org/2806623004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler.h:27:
void Dispose();
On 2017/04/12 15:54:07, Sami wrote:
> Could you add a test for this please?

Done.

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp (right):

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:71: return Get(type,
ToDocument(executionContext));
On 2017/04/13 07:03:36, haraken wrote:
> 
> Can we simply pass nullptr?

Hmmm... the clang compiler complains about the usage like this:

../../third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:71:22: error: call
to 'ToDocument' is ambiguous
    return Get(type, ToDocument(nullptr));
                     ^~~~~~~~~~

Also, we cannot pass only "nullptr" because Get() is overloaded:

../../third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:71:12: error: call
to 'Get' is ambiguous
    return Get(type, nullptr);

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp
(right):

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/ThreadedMessagingProxyBase.cpp:77:
TaskRunnerHelper::Get(TaskType::kNetworking, worker_thread_.get())
On 2017/04/13 07:03:36, haraken wrote:
> 
> Why does this use kNetworking?

Because this function (and PostTaskToLoader) implements WorkerLoaderProxy
interface that is supposed to be called from loading components.

(I found some non-loading functions wrongly call this. I'll fix them in a
separate CL. Also, we could rename this function to clearer name, or change the
interface using new TaskRunnerHelper(GlobalScope))

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp
(right):

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/time_zone_monitor/TimeZoneMonitorClient.cpp:74:
TaskRunnerHelper::Get(TaskType::kUnthrottled, thread)
On 2017/04/13 07:03:36, haraken wrote:
> 
> Why kUnthrottled?

Hm... this should be kUnspecedTimer. Replaced.

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/webthread_impl_for_worker_scheduler.h
(right):

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/webthread_impl_for_worker_scheduler.h:52:
return worker_scheduler_.get();
On 2017/04/13 07:03:36, haraken wrote:
> 
> It looks strange that:
> 
> - webthread_impl_for_worker_scheduler owns the scheduler by std::unique_ptr.
> 
> - the caller site also owns the scheduler by creating another std::unique_ptr
by
> calling MakeUnique(GetWorkerScheduler()).

That MakeUnique creates Worker*GlobalScope*Scheduler, and it does NOT own the
passed WorkerScheduler.

> Who should own the scheduler?

- WebThreadImplForWorkerScheduler owns WorkerScheduler
    (per-thread object owns per-thread scheduler).
- WorkerThread owns WorkerGlobalScopeScheduler
    (per-global-scope object the per-global-scope scheduler)

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler.h
(right):

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler.h:23:
WorkerGlobalScopeScheduler(WorkerScheduler* worker_scheduler);
On 2017/04/13 07:03:36, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/2806623004/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler.h:40:
scoped_refptr<TaskQueue> unthrottled_task_queue_;
On 2017/04/13 07:03:36, haraken wrote:
> 
> Why do we need to own unthrottled_task_queue_? I think the WebTaskRunner
should
> own it.

Done.

WebTaskRunner already owns it. I added WebTaskRunner::GetTaskQueue(), made
WorkerGlobalScopeScheduler::Dispose() use it and removed the field.

[commit-bot: I haz the power, 2017-04-13 09:51:25]

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

[commit-bot: I haz the power, 2017-04-13 09:51:26]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2017-04-17 01:49:20]

ping? :)

[kinuko, 2017-04-17 05:01:12]

lgtm % some more nits

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp (right):

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:123: // into other task
runners. See also comments in Get(LocalFrame).
Should this also have the crbug link, maybe 694925?

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/TaskRunnerHelper.h (right):

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.h:153: // Passing
WorkerThread could be confusing but it's valid because WorkerThread
Maybe it'd be clearer to document what this Get does before noting this caveat
(E.g. "Returns WebTaskRunners that are associated to the worker global scope
that corresponds to the given WorkerThread. Note that WorkerThread is a
per-global-scope object while the naming might sound differently. TODO:...")

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerEventQueue.cpp (right):

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerEventQueue.cpp:61: //
TODO(nhiroki): Callers of enqueueEvent() should specify the task type.
nit: enqueueEvent -> EnqueueEvent now

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerEventQueue.cpp:61: //
TODO(nhiroki): Callers of enqueueEvent() should specify the task type.
Currently window-side consumer of this class, e.g. DOMWindowEventQueue uses
kUnthrottled with a note about throttling IndexedDB events may break scenarios
in multiple tabs, should this use the same type & same comment, with the same
issue link if we have?  (Do we have one? > scheduler folks)

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (left):

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:577: new
CustomCountHistogram("WorkerThread.Task.Time", 0, 10000000, 50));
Are we deprecating this histogram?

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:424:
DCHECK(IsCurrentThread());
nit: DCHECK(!global_scope_scheduler_)

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler_unittest.cc
(right):

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler_unittest.cc:35:
mock_task_runner_(new cc::OrderedSimpleTaskRunner(clock_.get(), true)),
We don't have a handy one outside cc class? (Just asking)

[haraken, 2017-04-17 08:15:14]

(I'll take a look at this by tomorrow.)

[nhiroki, 2017-04-17 10:56:12]

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

[commit-bot: I haz the power, 2017-04-17 10:56:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nhiroki, 2017-04-17 11:03:47]

Thank you! Updated.

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp (right):

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.cpp:123: // into other task
runners. See also comments in Get(LocalFrame).
On 2017/04/17 05:01:11, kinuko wrote:
> Should this also have the crbug link, maybe 694925?

694925 is not relevant. Added 670534 instead.

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/TaskRunnerHelper.h (right):

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.h:153: // Passing
WorkerThread could be confusing but it's valid because WorkerThread
On 2017/04/17 05:01:11, kinuko wrote:
> Maybe it'd be clearer to document what this Get does before noting this caveat
> (E.g. "Returns WebTaskRunners that are associated to the worker global scope
> that corresponds to the given WorkerThread. Note that WorkerThread is a
> per-global-scope object while the naming might sound differently. TODO:...")

Yeah, that makes it much clearer. Done.

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerEventQueue.cpp (right):

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerEventQueue.cpp:61: //
TODO(nhiroki): Callers of enqueueEvent() should specify the task type.
On 2017/04/17 05:01:11, kinuko wrote:
> Currently window-side consumer of this class, e.g. DOMWindowEventQueue uses
> kUnthrottled with a note about throttling IndexedDB events may break scenarios
> in multiple tabs, should this use the same type & same comment, with the same
> issue link if we have?

Sounds reasonable. Added comments and changed the task type to kUnthrottled.

> (Do we have one? > scheduler folks)

I couldn't find it.

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (left):

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:577: new
CustomCountHistogram("WorkerThread.Task.Time", 0, 10000000, 50));
On 2017/04/17 05:01:11, kinuko wrote:
> Are we deprecating this histogram?

Oh, good catch.

I moved this into WorkerSchedulerImpl::DidProcessTask. I think this move won't
drastically change the trend of records and we don't have to make a new metric.
(please let me know if you don't think so)

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerThread.cpp:424:
DCHECK(IsCurrentThread());
On 2017/04/17 05:01:11, kinuko wrote:
> nit: DCHECK(!global_scope_scheduler_)

Done.

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler_unittest.cc
(right):

https://codereview.chromium.org/2806623004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler_unittest.cc:35:
mock_task_runner_(new cc::OrderedSimpleTaskRunner(clock_.get(), true)),
On 2017/04/17 05:01:11, kinuko wrote:
> We don't have a handy one outside cc class? (Just asking)

Apparently, base::TestSimpleTaskRunner would be available. Replaced.

[nhiroki, 2017-04-17 11:03:52]

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

[commit-bot: I haz the power, 2017-04-17 11:04:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nhiroki, 2017-04-17 11:04:28]

On 2017/04/17 08:15:14, haraken wrote:
> (I'll take a look at this by tomorrow.)

Acked. Thank you :)

[commit-bot: I haz the power, 2017-04-17 12:34:00]

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

[commit-bot: I haz the power, 2017-04-17 12:34:03]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-04-17 14:49:52]

LGTM

Let's wait for Sami's approval.

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp
(right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:126:
TaskRunnerHelper::Get(TaskType::kUnspecedTimer, global_scope), this,

Sorry for my previous comment but I begin to think that this timer task should
not be throttled (otherwise, the GC timing will be delayed). Would it make sense
to use kUnthrottled?

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerBackingThread.h:62:
CreateGlobalScopeScheduler();

Shall we inline the method in WorkerThread::InitializeSchedulerOnWorkerThread?

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerEventQueue.cpp (right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerEventQueue.cpp:70:
WrapPersistent(this), WrapWeakPersistent(event)));

Not related to this CL, do you know why we're using a weak persistent for event?
I think it can be a normal persistent because the event is anyway kept alive by
pending_events_ until DispatchEvent runs.

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc
(right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc:68:
scoped_refptr<TaskQueue> CompositorWorkerScheduler::NewUnthrottledTaskRunner(

Why does NewUnthrottledTaskRunner need to be virtualized? Do we want to get
different task runners between the compositor thread and the worker thread? Or
is it simply because helper_ is not defined on WorkerScheduler?

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.h
(right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.h:45:
scoped_refptr<TaskQueue> NewUnthrottledTaskRunner(

CreateUnthrottledTaskRunner ?

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler.h
(right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler.h:20:
// Unless stated otherwise, all methods must be called on the same thread.

on the worker thread?

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler.h:41:
bool is_enabled_ = true;

is_enabled_ => is_disposed_

Also can we put this under #if DCHECK_IS_ON?

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:278:
TaskRunnerHelper::Get(TaskType::kNetworking, worker_thread_.get())

If we want to use kNetworking, I think we should rename
PostTaskToWorkerGlobalScope. PostTaskToWorkerGlobalScope sounds general and it's
not clear that networking-related tasks will be posted.

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:258:
TaskRunnerHelper::Get(TaskType::kNetworking, GetWorkerThread())

Ditto.

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:273:
TaskRunnerHelper::Get(TaskType::kDOMManipulation, GetWorkerThread())

Why kDOMManipulation?

[kinuko, 2017-04-17 15:55:19]

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:278:
TaskRunnerHelper::Get(TaskType::kNetworking, worker_thread_.get())
On 2017/04/17 14:49:52, haraken wrote:
> 
> If we want to use kNetworking, I think we should rename
> PostTaskToWorkerGlobalScope. PostTaskToWorkerGlobalScope sounds general and
it's
> not clear that networking-related tasks will be posted.

(I believe we're deprecating / cleaning up this.  Also let me note that the
header file clearly says it's for WorkerLoaderProxy overrides)

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:273:
TaskRunnerHelper::Get(TaskType::kDOMManipulation, GetWorkerThread())
On 2017/04/17 14:49:52, haraken wrote:
> 
> Why kDOMManipulation?

It's in the spec.

https://html.spec.whatwg.org/multipage/workers.html#shared-workers-and-the-sh...

"10.2.4.25 If is shared is true, then queue a task, using the DOM manipulation
task source, to fire an event named connect at worker global scope, ..."

Hiroki: might be good to document that as a comment.

[altimin, 2017-04-18 00:46:40]

lgtm (and couple of unrelated nits)

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/TaskRunnerHelper.h (right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.h:147: static
RefPtr<WebTaskRunner> Get(TaskType, LocalFrame*);
Not related to this CL, just to start discussion.

Do you think that these should be methods of LocalFrame, Document,
ExecutionContext, etc? Something like local_frame->GetTaskRunner(task_type)?

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp
(right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:126:
TaskRunnerHelper::Get(TaskType::kUnspecedTimer, global_scope), this,
On 2017/04/17 14:49:52, haraken wrote:
> 
> Sorry for my previous comment but I begin to think that this timer task should
> not be throttled (otherwise, the GC timing will be delayed). Would it make
sense
> to use kUnthrottled?

Should we make a special GC task type? It seems that GC tasks have been
mentioned on a number of occasions.

[nhiroki, 2017-04-18 05:25:11]

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

[commit-bot: I haz the power, 2017-04-18 05:25:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nhiroki, 2017-04-18 05:34:38]

Thank you! Updated.

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/TaskRunnerHelper.h (right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/TaskRunnerHelper.h:147: static
RefPtr<WebTaskRunner> Get(TaskType, LocalFrame*);
On 2017/04/18 00:46:40, altimin (OOO until April 20) wrote:
> Not related to this CL, just to start discussion.
> 
> Do you think that these should be methods of LocalFrame, Document,
> ExecutionContext, etc? Something like local_frame->GetTaskRunner(task_type)?

I don't have a strong opinion, but I'd slightly prefer the current
TaskRunnerHelper because the TaskType/TaskRunner mappings for documents and
workers are placed in one file. I'd like to hear others' opinions.

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp
(right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:126:
TaskRunnerHelper::Get(TaskType::kUnspecedTimer, global_scope), this,
On 2017/04/18 00:46:40, altimin (OOO until April 20) wrote:
> On 2017/04/17 14:49:52, haraken wrote:
> > 
> > Sorry for my previous comment but I begin to think that this timer task
should
> > not be throttled (otherwise, the GC timing will be delayed). Would it make
> sense
> > to use kUnthrottled?

Yeah, that makes sense. Replaced.

> Should we make a special GC task type? It seems that GC tasks have been
> mentioned on a number of occasions.

SGTM. I added a TODO comment and filed an issue (https://crbug.com/712504)

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerBackingThread.h (right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerBackingThread.h:62:
CreateGlobalScopeScheduler();
On 2017/04/17 14:49:52, haraken wrote:
> 
> Shall we inline the method in WorkerThread::InitializeSchedulerOnWorkerThread?

Done.

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerEventQueue.cpp (right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerEventQueue.cpp:70:
WrapPersistent(this), WrapWeakPersistent(event)));
On 2017/04/17 14:49:52, haraken wrote:
> 
> Not related to this CL, do you know why we're using a weak persistent for
event?
> I think it can be a normal persistent because the event is anyway kept alive
by
> pending_events_ until DispatchEvent runs.
> 

It was introduced by https://codereview.chromium.org/2209613002

According to the CL description, it seems to be used for cancelling the event.
Now we could replace it with PostCancellableTask. I'll try it in a separate CL.

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc
(right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc:68:
scoped_refptr<TaskQueue> CompositorWorkerScheduler::NewUnthrottledTaskRunner(
On 2017/04/17 14:49:52, haraken wrote:
> 
> Why does NewUnthrottledTaskRunner need to be virtualized? Do we want to get
> different task runners between the compositor thread and the worker thread? Or
> is it simply because helper_ is not defined on WorkerScheduler?
> 

Good point. Moved this to WorkerScheduler.

When I initially created this patch, (Compositor)WorkerScheduler interfaces were
located under public/platform/scheduler/child/ and couldn't touch
SchedulerHelper under platform/scheduler/child (not public/). Thanks to
altimin@'s refactoring, they all are now under platform/scheduler/child and we
can consolidate the implementation.

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.h
(right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.h:45:
scoped_refptr<TaskQueue> NewUnthrottledTaskRunner(
On 2017/04/17 14:49:52, haraken wrote:
> 
> CreateUnthrottledTaskRunner ?

Done.

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler.h
(right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler.h:20:
// Unless stated otherwise, all methods must be called on the same thread.
On 2017/04/17 14:49:52, haraken wrote:
> 
> on the worker thread?

Done.

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/worker_global_scope_scheduler.h:41:
bool is_enabled_ = true;
On 2017/04/17 14:49:52, haraken wrote:
> 
> is_enabled_ => is_disposed_
> 
> Also can we put this under #if DCHECK_IS_ON?

Done.

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp:278:
TaskRunnerHelper::Get(TaskType::kNetworking, worker_thread_.get())
On 2017/04/17 15:55:19, kinuko wrote:
> On 2017/04/17 14:49:52, haraken wrote:
> > 
> > If we want to use kNetworking, I think we should rename
> > PostTaskToWorkerGlobalScope. PostTaskToWorkerGlobalScope sounds general and
> it's
> > not clear that networking-related tasks will be posted.
> 
> (I believe we're deprecating / cleaning up this.  Also let me note that the
> header file clearly says it's for WorkerLoaderProxy overrides)

Yes, these will be removed by a follow-up CL:
https://codereview.chromium.org/2811993007/

... so let me keep them as they are in this CL.

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp (right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:258:
TaskRunnerHelper::Get(TaskType::kNetworking, GetWorkerThread())
On 2017/04/17 14:49:52, haraken wrote:
> 
> Ditto.

(This will also be removed soon)

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp:273:
TaskRunnerHelper::Get(TaskType::kDOMManipulation, GetWorkerThread())
On 2017/04/17 15:55:19, kinuko wrote:
> On 2017/04/17 14:49:52, haraken wrote:
> > 
> > Why kDOMManipulation?
> 
> It's in the spec.
> 
>
https://html.spec.whatwg.org/multipage/workers.html#shared-workers-and-the-sh...
> 
> "10.2.4.25 If is shared is true, then queue a task, using the DOM manipulation
> task source, to fire an event named connect at worker global scope, ..."
> 
> Hiroki: might be good to document that as a comment.

Yes, that's required by the spec (thank you for the follow-up, kinuko). Added
comments.

[commit-bot: I haz the power, 2017-04-18 07:17:39]

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

[commit-bot: I haz the power, 2017-04-18 07:17:41]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-04-18 07:29:38]

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp
(right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:126:
TaskRunnerHelper::Get(TaskType::kUnspecedTimer, global_scope), this,
On 2017/04/18 05:34:38, nhiroki wrote:
> On 2017/04/18 00:46:40, altimin (OOO until April 20) wrote:
> > On 2017/04/17 14:49:52, haraken wrote:
> > > 
> > > Sorry for my previous comment but I begin to think that this timer task
> should
> > > not be throttled (otherwise, the GC timing will be delayed). Would it make
> > sense
> > > to use kUnthrottled?
> 
> Yeah, that makes sense. Replaced.
> 
> > Should we make a special GC task type? It seems that GC tasks have been
> > mentioned on a number of occasions.
> 
> SGTM. I added a TODO comment and filed an issue (https://crbug.com/712504)

I'm slightly negative about increasing # of unspeced tasks. Unless we're
planning to treat GC tasks in a special manner, I'd prefer just using
Unthrottled.

[haraken, 2017-04-18 07:30:51]

LGTM

[nhiroki, 2017-04-19 06:35:59]

(No updates. Reply only)

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp
(right):

https://codereview.chromium.org/2806623004/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.cpp:126:
TaskRunnerHelper::Get(TaskType::kUnspecedTimer, global_scope), this,
On 2017/04/18 07:29:38, haraken wrote:
> On 2017/04/18 05:34:38, nhiroki wrote:
> > On 2017/04/18 00:46:40, altimin (OOO until April 20) wrote:
> > > On 2017/04/17 14:49:52, haraken wrote:
> > > > 
> > > > Sorry for my previous comment but I begin to think that this timer task
> > should
> > > > not be throttled (otherwise, the GC timing will be delayed). Would it
make
> > > sense
> > > > to use kUnthrottled?
> > 
> > Yeah, that makes sense. Replaced.
> > 
> > > Should we make a special GC task type? It seems that GC tasks have been
> > > mentioned on a number of occasions.
> > 
> > SGTM. I added a TODO comment and filed an issue (https://crbug.com/712504)
> 
> I'm slightly negative about increasing # of unspeced tasks. Unless we're
> planning to treat GC tasks in a special manner, I'd prefer just using
> Unthrottled.

Thank you for the comment. For the record I copied-and-pasted your comment in
the issue.

[nhiroki, 2017-04-19 06:37:54]

Sami: ping? :)

[Sami, 2017-04-19 11:14:07]

lgtm, sorry for the delay!

https://codereview.chromium.org/2806623004/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc
(right):

https://codereview.chromium.org/2806623004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc:18:
CompositorWorkerScheduler::CompositorWorkerScheduler(
On 2017/04/13 08:23:50, nhiroki wrote:
> On 2017/04/12 15:54:06, Sami wrote:
> > FYI, last time we tried to do this there was a minor perf regression which
we
> > mostly optimized away. The last thing remaining there is to add support for
> task
> > cancellation in base::MessageLoop. dcheng@ has a patch for that.
> 
> Thank you for the information. I'll take care of perf regression reports.
> 
> Should I wait for dcheng@'s patch before landing this?

It's probably fine to try landing it at least since we haven't measured in a
while. If the issue turns out to be huge we can improve as a follow-up.

https://codereview.chromium.org/2806623004/diff/410001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc
(right):

https://codereview.chromium.org/2806623004/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc:25:
TRACE_DISABLED_BY_DEFAULT("compositor.scheduler"),
Note about these: because of the way tracing works, the compositor scheduler
won't actually get its own category but will be reported under
renderer.scheduler. This is just a FYI in case you're confused about the traces
:) (See crbug.com/706775 for details.)

https://codereview.chromium.org/2806623004/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc:34:
// TODO(sad): Implement a more robust scheduler that can do idle tasks for GC
I think you can remove this comment now :)

[nhiroki, 2017-04-19 23:10:50]

No problem! Thank you for reviewing this :)

https://codereview.chromium.org/2806623004/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc
(right):

https://codereview.chromium.org/2806623004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc:18:
CompositorWorkerScheduler::CompositorWorkerScheduler(
On 2017/04/19 11:14:07, Sami wrote:
> On 2017/04/13 08:23:50, nhiroki wrote:
> > On 2017/04/12 15:54:06, Sami wrote:
> > > FYI, last time we tried to do this there was a minor perf regression which
> we
> > > mostly optimized away. The last thing remaining there is to add support
for
> > task
> > > cancellation in base::MessageLoop. dcheng@ has a patch for that.
> > 
> > Thank you for the information. I'll take care of perf regression reports.
> > 
> > Should I wait for dcheng@'s patch before landing this?
> 
> It's probably fine to try landing it at least since we haven't measured in a
> while. If the issue turns out to be huge we can improve as a follow-up.

OK, let's try!

https://codereview.chromium.org/2806623004/diff/410001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc
(right):

https://codereview.chromium.org/2806623004/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc:25:
TRACE_DISABLED_BY_DEFAULT("compositor.scheduler"),
On 2017/04/19 11:14:07, Sami wrote:
> Note about these: because of the way tracing works, the compositor scheduler
> won't actually get its own category but will be reported under
> renderer.scheduler. This is just a FYI in case you're confused about the
traces
> :) (See crbug.com/706775 for details.)

Acknowledged.

https://codereview.chromium.org/2806623004/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc:34:
// TODO(sad): Implement a more robust scheduler that can do idle tasks for GC
On 2017/04/19 11:14:07, Sami wrote:
> I think you can remove this comment now :)

Done.

[nhiroki, 2017-04-19 23:12:05]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2017-04-19 23:12:05]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
altimin@chromium.org, haraken@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2806623004/#ps450001
(title: "remove unnecessary comments")

[commit-bot: I haz the power, 2017-04-19 23:12:38]

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-04-20 01:05:38]

CQ is committing da patch.

Bot data: {"patchset_id": 450001, "attempt_start_ts": 1492643525418820,
"parent_rev": "29a7c449f99e99807fcbbbe9a66d87032006fa03", "commit_rev":
"d56a35fb25dc388a0506174b9e3fda67337313ce"}

[commit-bot: I haz the power, 2017-04-20 01:05:50]

Description was changed from

==========
Worker: Introduce per-global-scope task scheduler

This CL introduces WorkerGlobalScopeScheduler to make it simpler and saner to
manage tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, they should be handled in different ways based on their
task types though. For example, worker tasks shouldn't run after the global
scope is closed, while thread control tasks should run even after that. To
achieve it, worker tasks were always wrapped with WorkerThread's method to check
the current thread state. This was so tricky.

After this CL, each global scope has its own task queues like per-frame task
scheduler. Worker tasks are pushed into the task queues, control tasks are still
pushed into the thread's default task queue though. Thanks to this separation,
we can abort only worker tasks by simply unregistering the queues when a global
scope is closed.

In addition, this CL unifies postTask interfaces into TaskRunnerHelper (issue
694914), and enables any threads to post a task to worker threads because of
thread-safe refcounted WebTaskRunner.

BUG=670534, 694914, 710364
==========

to

==========
Worker: Introduce per-global-scope task scheduler

This CL introduces WorkerGlobalScopeScheduler to make it simpler and saner to
manage tasks on worker threads.

Before this CL, tasks on worker threads were always pushed into the thread's
default task queue, they should be handled in different ways based on their
task types though. For example, worker tasks shouldn't run after the global
scope is closed, while thread control tasks should run even after that. To
achieve it, worker tasks were always wrapped with WorkerThread's method to check
the current thread state. This was so tricky.

After this CL, each global scope has its own task queues like per-frame task
scheduler. Worker tasks are pushed into the task queues, control tasks are still
pushed into the thread's default task queue though. Thanks to this separation,
we can abort only worker tasks by simply unregistering the queues when a global
scope is closed.

In addition, this CL unifies postTask interfaces into TaskRunnerHelper (issue
694914), and enables any threads to post a task to worker threads because of
thread-safe refcounted WebTaskRunner.

BUG=670534, 694914, 710364

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

[commit-bot: I haz the power, 2017-04-20 01:05:52]

Committed patchset #22 (id:450001) as
https://chromium.googlesource.com/chromium/src/+/d56a35fb25dc388a0506174b9e3f...

[aboxhall, 2017-04-20 03:22:25]

A revert of this CL (patchset #22 id:450001) has been created in
https://codereview.chromium.org/2831843002/ by aboxhall@chromium.org.

The reason for reverting is: Seems to have caused a crash in
fast/workers/termination-early.html on Webkit Linux Trusty Leak -
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Tru...
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu...

https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Tru...
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....