Introduce a basic Blink Scheduler

Introduce a basic Blink Scheduler
    
This patch adds a bare-bones scheduler for prioritizing tasks that run
on the Blink main thread. The initial scheduling policy is to simply run
all tasks in their posted order.

The shared timer, WTF::callOnMainThread and by extension
WebCore::MainThreadTaskRunner are modified to route their tasks through
the Scheduler for eventual prioritization. A proxy for the scheduler is
also exposed in public/platform so that modules in content will be able
to post high priority (e.g., input and compositor-related) tasks.

Design document:
https://docs.google.com/a/chromium.org/document/d/11N2WTV3M0IkZ-kQlKWlBcwkOkKTCuLXGVNylK5E2zvc/edit#heading=h.3ay9sj44f0zd

BUG=391005
TEST=SchedulerTest.*

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178267

[mithro-old, 2014-07-08 10:38:11]

Hi Sami,

I know you mentioned this wasn't for review yet, but wanted to better understand
how this WIP fit in with the compositor code.

Thanks!

Tim

https://codereview.chromium.org/364873002/diff/100001/public/platform/WebSche...
File public/platform/WebScheduler.h (right):

https://codereview.chromium.org/364873002/diff/100001/public/platform/WebSche...
public/platform/WebScheduler.h:65: BLINK_PLATFORM_EXPORT void
postBeginFrameTask(WebThread::Task*, double frameTime, double deadline, double
interval);
If I understand correctly (which I'm never sure with the code under cc/) than in
many cases the system currently roughly goes,
 <begin frame generated>
 (on impl thread) ImplThread::Scheduler
 (on impl thread) ThreadProxy::ScheduledActionSendBeginMainFrame
 (on main thread) ThreadProxy::BeginMainFrame
 (on main thread) LayerTreeHost::UpdateClientAnimations
 (on main thread) LayerTreeHostClient::Animate
 (on main thread) RenderWidgetCompositor::Animate
 (on main thread) WebWidget::animate

Where would the "postBeginFrameTask" function replace?

[Sami, 2014-07-08 11:08:05]

Hi Tim,

Have a look here: https://codereview.chromium.org/363383002 :)

[Sami, 2014-07-08 15:58:29]

https://codereview.chromium.org/364873002/diff/100001/Source/platform/ThreadT...
File Source/platform/ThreadTimers.cpp (right):

https://codereview.chromium.org/364873002/diff/100001/Source/platform/ThreadT...
Source/platform/ThreadTimers.cpp:142: if (!m_firingTimers || timeToQuit <
monotonicallyIncreasingTime() ||
Scheduler::current()->shouldYieldForHighPriorityWork())
Note to self: isMainThread()

[eseidel, 2014-07-08 19:00:05]

Very interesting.  At first glance it's not clear to me why WebScheduler
straddles the web boundary as it does.  I guess cc needs to talk to it?  If so,
is this something which sits below blink (hence platform/) or is exposed by
blink to something above it (where it would go in web/).

I wasn't expecting to see you hook into the shared timer system, but now that i
see it, it makes total sense.  Right now all blink tasks go through that, but
eventually I suspect they won't.

Looking forward to seeing the finaler-patch.

[Sami, 2014-07-11 16:05:29]

Hi all, I think we can now turn this into a proper review. To keep things
simple this patch just implements a pass-through scheduling policy.

On 2014/07/08 19:00:05, eseidel wrote:
> Very interesting.  At first glance it's not clear to me why WebScheduler
> straddles the web boundary as it does.  I guess cc needs to talk to it?  If
so,
> is this something which sits below blink (hence platform/) or is exposed by
> blink to something above it (where it would go in web/).

Hopefully the design doc shed some light onto why we want to expose this. To
answer your question this kind of falls into both categories. I've renamed the
web-part to WebSchedulerProxy to make it clearer that it is really a separate
interface.

> I wasn't expecting to see you hook into the shared timer system, but now that
i
> see it, it makes total sense.  Right now all blink tasks go through that, but
> eventually I suspect they won't.

Right, I think we'll eventually remove shared timers but for now the path of
least resistance seem to be to divert them through the scheduler.

[eseidel, 2014-07-15 15:59:11]

looks good.  Lets talk more at our meeting.

This looks like it has no functional change (which is good).  I'm curious to see
what numbers you've seen when you've added a functional change to this.

https://codereview.chromium.org/364873002/diff/140001/Source/platform/ThreadT...
File Source/platform/ThreadTimers.cpp (right):

https://codereview.chromium.org/364873002/diff/140001/Source/platform/ThreadT...
Source/platform/ThreadTimers.cpp:142: if (!m_firingTimers || timeToQuit <
monotonicallyIncreasingTime() || (isMainThread() &&
Scheduler::current()->shouldYieldForHighPriorityWork()))
Do we ever have a sharedTimer not on the main thread?  I'm surprised
isMainThread() is necessary here.

https://codereview.chromium.org/364873002/diff/140001/Source/platform/schedul...
File Source/platform/scheduler/Scheduler.cpp (right):

https://codereview.chromium.org/364873002/diff/140001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.cpp:40: class MainThreadTaskAdapter : public
blink::WebThread::Task {
What is this for?

https://codereview.chromium.org/364873002/diff/140001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.cpp:73: Scheduler* Scheduler::current()
I believe this method is normally called 'shared' in the codebase.

https://codereview.chromium.org/364873002/diff/140001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.cpp:90: m_mainThread->postTask(new
MainThreadTaskAdapter(task));
I see, you want to speak to the Scheduler in terms of SchedulerTasks so you have
something to wrap the SchedulerTasks as Chromium tasks for the chormium message
loop.

https://codereview.chromium.org/364873002/diff/140001/Source/platform/schedul...
File Source/platform/scheduler/Scheduler.h (right):

https://codereview.chromium.org/364873002/diff/140001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.h:2: * Copyright (C) 2014 Google Inc. All
rights reserved.
Please use the updated 2-line license. instead of this giant blob. :)

https://codereview.chromium.org/364873002/diff/140001/Source/platform/schedul...
File Source/platform/scheduler/SchedulerTest.cpp (right):

https://codereview.chromium.org/364873002/diff/140001/Source/platform/schedul...
Source/platform/scheduler/SchedulerTest.cpp:2: * Copyright (C) 2014 Google Inc.
All rights reserved.
Wrong license block.

[Sami, 2014-07-15 19:20:06]

Thanks Eric. Another look please?

https://codereview.chromium.org/364873002/diff/140001/Source/platform/ThreadT...
File Source/platform/ThreadTimers.cpp (right):

https://codereview.chromium.org/364873002/diff/140001/Source/platform/ThreadT...
Source/platform/ThreadTimers.cpp:142: if (!m_firingTimers || timeToQuit <
monotonicallyIncreasingTime() || (isMainThread() &&
Scheduler::current()->shouldYieldForHighPriorityWork()))
On 2014/07/15 15:59:11, eseidel wrote:
> Do we ever have a sharedTimer not on the main thread?  I'm surprised
> isMainThread() is necessary here.

We don't have a shared timer there, but can have a ThreadTimers instance on a
non-main thread. See the initialization in ThreadTimers::ThreadTimers().

https://codereview.chromium.org/364873002/diff/140001/Source/platform/schedul...
File Source/platform/scheduler/Scheduler.cpp (right):

https://codereview.chromium.org/364873002/diff/140001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.cpp:73: Scheduler* Scheduler::current()
On 2014/07/15 15:59:11, eseidel wrote:
> I believe this method is normally called 'shared' in the codebase.

Done.

https://codereview.chromium.org/364873002/diff/140001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.cpp:90: m_mainThread->postTask(new
MainThreadTaskAdapter(task));
On 2014/07/15 15:59:11, eseidel wrote:
> I see, you want to speak to the Scheduler in terms of SchedulerTasks so you
have
> something to wrap the SchedulerTasks as Chromium tasks for the chormium
message
> loop.

Right. It's a bit unfortunate we can't share the same closures across that
interface -- perhaps something to revisit later.

https://codereview.chromium.org/364873002/diff/140001/Source/platform/schedul...
File Source/platform/scheduler/Scheduler.h (right):

https://codereview.chromium.org/364873002/diff/140001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.h:2: * Copyright (C) 2014 Google Inc. All
rights reserved.
On 2014/07/15 15:59:11, eseidel wrote:
> Please use the updated 2-line license. instead of this giant blob. :)

Ah, somehow I thought that was only for Chromium code but thanks for correcting
me.

https://codereview.chromium.org/364873002/diff/140001/Source/platform/schedul...
File Source/platform/scheduler/SchedulerTest.cpp (right):

https://codereview.chromium.org/364873002/diff/140001/Source/platform/schedul...
Source/platform/scheduler/SchedulerTest.cpp:2: * Copyright (C) 2014 Google Inc.
All rights reserved.
On 2014/07/15 15:59:11, eseidel wrote:
> Wrong license block.

Done.

[eseidel, 2014-07-15 19:32:37]

lgtm

https://codereview.chromium.org/364873002/diff/180001/Source/platform/schedul...
File Source/platform/scheduler/Scheduler.h (right):

https://codereview.chromium.org/364873002/diff/180001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.h:13: #include <deque>
Why is this needed?

[Sami, 2014-07-16 10:28:07]

https://codereview.chromium.org/364873002/diff/180001/Source/platform/schedul...
File Source/platform/scheduler/Scheduler.h (right):

https://codereview.chromium.org/364873002/diff/180001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.h:13: #include <deque>
On 2014/07/15 19:32:37, eseidel wrote:
> Why is this needed?

Oops, left over from an earlier version. I've cleaned up some other unnecessary
includes too.

[Sami, 2014-07-16 10:28:12]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-07-16 10:29:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/364873002/200001

[commit-bot: I haz the power, 2014-07-16 11:41:57]

Change committed as 178267

[picksi1, 2014-08-01 10:36:12]

https://codereview.chromium.org/364873002/diff/200001/Source/platform/schedul...
File Source/platform/scheduler/Scheduler.cpp (right):

https://codereview.chromium.org/364873002/diff/200001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.cpp:41: s_sharedScheduler = new Scheduler();
nit: Is it worth checking that s_sharedScheduler is null (or call shutdown?)
before this allocation in case we get multiple initialize calls and this leaks?