Adds the concept of Policies to the Blink Scheduler

Adds the concept of Policies to the Blink Scheduler

In Normal mode (the default), there's virtually no 
prioritization of compositor events, and
shouldYieldForHighPriorityWork always returns false. In 
CompositorPriority mode, there *is* prioritization of 
compositor tasks and shouldYieldForHighPriorityWork will 
return false if there are pending high priority events.

Currently there is no way for client code to set the policy
but that will be introduced in a follow up CL.

Also adds some disabled by default tracing in
"blink.scheduler" to track the number of pending high
priority tasks and which policy mode the scheduler is in.

BUG=391005, 411520
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181818

[alexclarke, 2014-09-10 11:31:54]

alexclarke@google.com changed reviewers:
+ alexclarke@google.com, skyostil@chromium.org

[alexclarke, 2014-09-10 14:28:04]

alexclarke@google.com changed reviewers:
+ eseidel@chromium.org

[Sami, 2014-09-10 16:00:39]

Thanks Alex, I think functionally this looks fine but as usual I'm being picky
about naming :)

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
File Source/platform/scheduler/Scheduler.cpp (right):

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:68: // NOTE if we're not in low latency
mode, this is the only place they will actually get executed.
nit: reword? (it's not clear what |they| is referring to.)

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:158: setLatencyMode(LowLatency);
Could you leave out this call so we'll only do this through WebSchedulerProxy?
Otherwise it can happen twice for every input event.

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:204: if (latencyMode() != LowLatency)
ASSERT(isMainThread());

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
File Source/platform/scheduler/Scheduler.h (right):

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.h:46: // Returns true if we're in low
latency mode and there is high priority work pending on the
It feels like we don't need to mention the fact that this function depends on
the mode. I think it is adequately captured in the word "should" and the callers
don't really need to know exactly why the scheduler wants them to yield. I would
therefore keep this comment as is.

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.h:63: enum LatencyMode {
Unfortunately the word latency is used for various things over the code base and
it's not obvious what it is referring to. How about something a little more
explicit like this -- mostly because I don't think optimizing for latency is the
only mode we need:

enum SchedulingMode {
    Passthrough,  // or NoOp, or whatever
    PreferHighPriorityWork
}

WDYT?

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.h:92: bool
runPendingHighPrioirtyTasksIfInLowLatencyMode();
s/Prioirty/Priority/

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.h:109: // Should only be accessed from the
main thread.
Could you group the thread-specific members next to each other so it is easier
to see at a glance what goes where?

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.h:113: void setLatencyMode(LatencyMode);
Maybe call this |enter| instead of |set| to distinguish it from a pure setter as
it also updates the deadline for going back to normal mode.

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.h:125: volatile int m_latencyMode;
Can we make this type safe?

I wonder if we should make this & m_pendingHighPriorityTasks a packed bitfield
so we only have one atomic load/store. Premature optimization?

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.h:129: static double
s_lowLatencyModeAfterTouchTimeSeconds;
This is constant right? I'd just move it to the anonymous namespace in
Scheduler.cpp (and use kConstantNaming).

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
File Source/platform/scheduler/SchedulerTest.cpp (right):

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/SchedulerTest.cpp:79:
WTF::setMonotonicallyIncreasingTimeFunction(getDebugTime);
Maybe getMonotonicTimeForTest?

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/SchedulerTest.cpp:84:
WTF::setMonotonicallyIncreasingTimeFunction(0);
I hope we're not stomping over some other test by doing this :P

[eseidel, 2014-09-10 16:09:01]

lgtm

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

https://codereview.chromium.org/559973003/diff/100001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.h:65: LowLatency,
What does LowLatency mode mean?  Maybe just a comment to explain?

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

https://codereview.chromium.org/559973003/diff/100001/Source/platform/schedul...
Source/platform/scheduler/SchedulerTest.cpp:145: static double
getDebugTime(void)
Atypical to have "get" as a method prefix in blink.  But for unittest code I
don't care horribly. :)

[alexclarke, 2014-09-10 17:35:38]

PTAL

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
File Source/platform/scheduler/Scheduler.cpp (right):

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:68: // NOTE if we're not in low latency
mode, this is the only place they will actually get executed.
On 2014/09/10 16:00:38, Sami wrote:
> nit: reword? (it's not clear what |they| is referring to.)

Done.

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:158: setLatencyMode(LowLatency);
On 2014/09/10 16:00:38, Sami wrote:
> Could you leave out this call so we'll only do this through WebSchedulerProxy?
> Otherwise it can happen twice for every input event.

Done.

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:204: if (latencyMode() != LowLatency)
On 2014/09/10 16:00:38, Sami wrote:
> ASSERT(isMainThread());

Done.

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
File Source/platform/scheduler/Scheduler.h (right):

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.h:46: // Returns true if we're in low
latency mode and there is high priority work pending on the
On 2014/09/10 16:00:38, Sami wrote:
> It feels like we don't need to mention the fact that this function depends on
> the mode. I think it is adequately captured in the word "should" and the
callers
> don't really need to know exactly why the scheduler wants them to yield. I
would
> therefore keep this comment as is.

Done.

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.h:92: bool
runPendingHighPrioirtyTasksIfInLowLatencyMode();
On 2014/09/10 16:00:38, Sami wrote:
> s/Prioirty/Priority/

Done.

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.h:109: // Should only be accessed from the
main thread.
On 2014/09/10 16:00:38, Sami wrote:
> Could you group the thread-specific members next to each other so it is easier
> to see at a glance what goes where?

Done.

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.h:113: void setLatencyMode(LatencyMode);
On 2014/09/10 16:00:38, Sami wrote:
> Maybe call this |enter| instead of |set| to distinguish it from a pure setter
as
> it also updates the deadline for going back to normal mode.

Done.

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.h:125: volatile int m_latencyMode;
On 2014/09/10 16:00:38, Sami wrote:
> Can we make this type safe?
Well it's not supposed to be accessed directly.  I've added a comment.

> 
> I wonder if we should make this & m_pendingHighPriorityTasks a packed bitfield
> so we only have one atomic load/store. Premature optimization?
Maybe we could, but I think we should defer those optimizations until we're more
certain what overall algorithm we want.

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.h:129: static double
s_lowLatencyModeAfterTouchTimeSeconds;
On 2014/09/10 16:00:38, Sami wrote:
> This is constant right? I'd just move it to the anonymous namespace in
> Scheduler.cpp (and use kConstantNaming).

Done.

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
File Source/platform/scheduler/SchedulerTest.cpp (right):

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/SchedulerTest.cpp:79:
WTF::setMonotonicallyIncreasingTimeFunction(getDebugTime);
On 2014/09/10 16:00:39, Sami wrote:
> Maybe getMonotonicTimeForTest?

Done.

https://codereview.chromium.org/559973003/diff/80001/Source/platform/schedule...
Source/platform/scheduler/SchedulerTest.cpp:84:
WTF::setMonotonicallyIncreasingTimeFunction(0);
On 2014/09/10 16:00:39, Sami wrote:
> I hope we're not stomping over some other test by doing this :P

Likewise.  If it turns out we are, then we can just save and restore it. 
Although that means adding a (trivial) function in WTF to get the current
function.

[Sami, 2014-09-11 11:32:19]

Thanks for the comprehensive tests!

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

https://codereview.chromium.org/559973003/diff/160001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.cpp:241:
ASSERT(m_pendingTasksMutex.locked());
Also add ASSERT(isMainThread());

https://codereview.chromium.org/559973003/diff/160001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.cpp:312: void
Scheduler::enterSchedulerPolicy(SchedulerPolicy SchedulerPolicy)
scheduler shouldn't be capitalized here.

https://codereview.chromium.org/559973003/diff/160001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.cpp:315:
m_compositorPriorityPolicyEndTimeSeconds =
Platform::current()->monotonicallyIncreasingTime() +
kLowSchedulerPolicyAfterTouchTimeSeconds;
We need to grab m_pendingTasksMutex here, right?

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

https://codereview.chromium.org/559973003/diff/160001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.h:103: void
maybeEnterNormalschedulerPolicy();
Please capitalize "scheduler" here.

https://codereview.chromium.org/559973003/diff/160001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.h:134: double
m_compositorPriorityPolicyEndTimeSeconds;
This is protected by m_pendingTasksMutex, right? Could you move it over to that
section?

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

https://codereview.chromium.org/559973003/diff/160001/Source/platform/schedul...
Source/platform/scheduler/SchedulerTest.cpp:29: using Scheduler::Normal;
Neat!

https://codereview.chromium.org/559973003/diff/160001/Source/platform/schedul...
Source/platform/scheduler/SchedulerTest.cpp:91:
WTF::setMonotonicallyIncreasingTimeFunction(getMonotonicTimeForTest);
Drop the "get", it's cleaner :)

Do we actually need to override this if everyone's going through
Platform::current()->monotonicallyIncreasingTime()?

https://codereview.chromium.org/559973003/diff/160001/Source/platform/schedul...
Source/platform/scheduler/SchedulerTest.cpp:112: return s_debugTime;
nit: also call this s_monotonicallyIncreasingTime.

https://codereview.chromium.org/559973003/diff/160001/Source/platform/schedul...
Source/platform/scheduler/SchedulerTest.cpp:151: static void setDebugTime(double
time)
setMonotonicTimeForTest?

https://codereview.chromium.org/559973003/diff/160001/Source/platform/schedul...
Source/platform/scheduler/SchedulerTest.cpp:308:
m_scheduler->enterSchedulerPolicy(SchedulerForTest::Normal);
Instead of doing this in every test, let's add a setUp() method that does the
same.

https://codereview.chromium.org/559973003/diff/160001/Source/platform/schedul...
Source/platform/scheduler/SchedulerTest.cpp:439: TEST_F(SchedulerTest,
HighPriorityTasksOnlyDontRunBecasueOfSharedTimerFiring_InNormalMode)
s/Becasue/Because/

[alexclarke, 2014-09-11 11:52:06]

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

https://codereview.chromium.org/559973003/diff/160001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.cpp:241:
ASSERT(m_pendingTasksMutex.locked());
On 2014/09/11 11:32:18, Sami wrote:
> Also add ASSERT(isMainThread());

Done.

https://codereview.chromium.org/559973003/diff/160001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.cpp:312: void
Scheduler::enterSchedulerPolicy(SchedulerPolicy SchedulerPolicy)
On 2014/09/11 11:32:18, Sami wrote:
> scheduler shouldn't be capitalized here.

Done.

https://codereview.chromium.org/559973003/diff/160001/Source/platform/schedul...
Source/platform/scheduler/Scheduler.cpp:315:
m_compositorPriorityPolicyEndTimeSeconds =
Platform::current()->monotonicallyIncreasingTime() +
kLowSchedulerPolicyAfterTouchTimeSeconds;
On 2014/09/11 11:32:18, Sami wrote:
> We need to grab m_pendingTasksMutex here, right?

Done.

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

https://codereview.chromium.org/559973003/diff/160001/Source/platform/schedul...
Source/platform/scheduler/SchedulerTest.cpp:29: using Scheduler::Normal;
On 2014/09/11 11:32:18, Sami wrote:
> Neat!

Acknowledged.

https://codereview.chromium.org/559973003/diff/160001/Source/platform/schedul...
Source/platform/scheduler/SchedulerTest.cpp:91:
WTF::setMonotonicallyIncreasingTimeFunction(getMonotonicTimeForTest);
On 2014/09/11 11:32:19, Sami wrote:
> Drop the "get", it's cleaner :)
> 
> Do we actually need to override this if everyone's going through
> Platform::current()->monotonicallyIncreasingTime()?

Turns out we don't.  Nice.

https://codereview.chromium.org/559973003/diff/160001/Source/platform/schedul...
Source/platform/scheduler/SchedulerTest.cpp:112: return s_debugTime;
On 2014/09/11 11:32:18, Sami wrote:
> nit: also call this s_monotonicallyIncreasingTime.

Done.

https://codereview.chromium.org/559973003/diff/160001/Source/platform/schedul...
Source/platform/scheduler/SchedulerTest.cpp:151: static void setDebugTime(double
time)
On 2014/09/11 11:32:18, Sami wrote:
> setMonotonicTimeForTest?

Done.

https://codereview.chromium.org/559973003/diff/160001/Source/platform/schedul...
Source/platform/scheduler/SchedulerTest.cpp:308:
m_scheduler->enterSchedulerPolicy(SchedulerForTest::Normal);
On 2014/09/11 11:32:18, Sami wrote:
> Instead of doing this in every test, let's add a setUp() method that does the
> same.

Done.

https://codereview.chromium.org/559973003/diff/160001/Source/platform/schedul...
Source/platform/scheduler/SchedulerTest.cpp:439: TEST_F(SchedulerTest,
HighPriorityTasksOnlyDontRunBecasueOfSharedTimerFiring_InNormalMode)
On 2014/09/11 11:32:18, Sami wrote:
> s/Becasue/Because/

Done.

[Sami, 2014-09-11 12:10:58]

Thanks, lgtm!

[alexclarke, 2014-09-11 12:11:43]

The CQ bit was checked by alexclarke@google.com

[commit-bot: I haz the power, 2014-09-11 12:12:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/559973003/230001

[commit-bot: I haz the power, 2014-09-11 13:15:27]

Committed patchset #13 (id:230001) as 181818