scheduler: Post high priority tasks directly to the message loop

scheduler: Post high priority tasks directly to the message loop

This patch removes the scheduler's internal high priority task queues in
favor of posting high priority tasks directly to the main thread. This
fixes re-entrancy problems with running these tasks in a nested message
loop, but removes the option of having them jump ahead of other posted
tasks.

This is an interim solution while we research better ways to prioritize
compositor tasks over other (Blink and non-Blink) clients of the main
thread message loop.

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

[Sami, 2014-10-03 17:46:29]

skyostil@chromium.org changed reviewers:
+ alexclarke@chromium.org, petrcermak@chromium.org, picksi@chromium.org

[Sami, 2014-10-03 17:47:38]

Since I couldn't resist, here's something simple we could while we design a
better way to prioritize things. This should still give some queueing_times
benefit since timers will yield, and should serve as a good basis for more
scheduling work since the policy will now also be up to date.

[Sami, 2014-10-06 12:41:23]

I measured a 17% improvement with queueing duration with this patch on a Nexus
7v2 on key_mobile_sites. Seems like it might make sense to land this.

I'll do another comparison against the ToT scheduler which prioritizes input &
compositing more aggressively.

Anyone wanna do a review meanwhile?

[picksi1, 2014-10-06 13:21:02]

LGTM with one comment.

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

https://codereview.chromium.org/621363002/diff/40001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:245: }
Does this function need to check the policy too? If we're in normal mode should
we pretend we have no high priority work?

[Sami, 2014-10-06 14:00:47]

Thanks for the comments.

According to the perf test, this patch gives us a 17% improvement in
queueing_durations while the ToT code which prioritizes compositor work more
aggressively is at 26%. Given that this simplifies the code and should be easy
to integrate with whatever prioritization mechanism we end up with, this seems
like a reasonable thing to do.

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

https://codereview.chromium.org/621363002/diff/40001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:245: }
On 2014/10/06 13:21:02, picksi1 wrote:
> Does this function need to check the policy too? If we're in normal mode
should
> we pretend we have no high priority work?

No, I don't think there's a reason to lie here. This function should do what it
says. Actually what I'll do is fold this function into shouldYield() since
that's the only place this is used.

[alexclarke, 2014-10-06 14:11:03]

alexclarke@google.com changed reviewers:
+ alexclarke@google.com

[alexclarke, 2014-10-06 14:11:03]

https://codereview.chromium.org/621363002/diff/40001/Source/platform/schedule...
File Source/platform/scheduler/Scheduler.cpp (left):

https://codereview.chromium.org/621363002/diff/40001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:206: bool workDone =
runPendingHighPriorityTasksIfInCompositorPriority();
I suspect under some circumstances this patch will partially starve the blink
timers.  If a high priority task is already in the queue, only a single blink
timer will fire before the shared timer function yields and reposts itself.

That maybe OK for an interim solution.

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

https://codereview.chromium.org/621363002/diff/40001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:200: Locker<Mutex>
lock(m_pendingTasksMutex);
I think we can rename this mutex now.

https://codereview.chromium.org/621363002/diff/40001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:245: }
On 2014/10/06 13:21:02, picksi1 wrote:
> Does this function need to check the policy too? If we're in normal mode
should
> we pretend we have no high priority work?

That's done in shouldYieldForHighPriorityWork, this function is fine.

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

https://codereview.chromium.org/621363002/diff/40001/Source/platform/schedule...
Source/platform/scheduler/SchedulerTest.cpp:337: Scheduler::shutdown();
Might be worth adding something between shutdown and runPendinTasks to
illustrate that nothing was executed during the shutdown.

[Sami, 2014-10-06 14:59:18]

https://codereview.chromium.org/621363002/diff/40001/Source/platform/schedule...
File Source/platform/scheduler/Scheduler.cpp (left):

https://codereview.chromium.org/621363002/diff/40001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:206: bool workDone =
runPendingHighPriorityTasksIfInCompositorPriority();
On 2014/10/06 14:11:03, alexclarke wrote:
> I suspect under some circumstances this patch will partially starve the blink
> timers.  If a high priority task is already in the queue, only a single blink
> timer will fire before the shared timer function yields and reposts itself.
> 
> That maybe OK for an interim solution.

We figured out offline that this should be fine because the timer tasks are
never completely starved: we always run at least one every time the shared timer
fires and another shared timer is scheduled when that yields.

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

https://codereview.chromium.org/621363002/diff/40001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:200: Locker<Mutex>
lock(m_pendingTasksMutex);
On 2014/10/06 14:11:03, alexclarke wrote:
> I think we can rename this mutex now.

Done.

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

https://codereview.chromium.org/621363002/diff/40001/Source/platform/schedule...
Source/platform/scheduler/SchedulerTest.cpp:337: Scheduler::shutdown();
On 2014/10/06 14:11:03, alexclarke wrote:
> Might be worth adding something between shutdown and runPendinTasks to
> illustrate that nothing was executed during the shutdown.

Done.

[Sami, 2014-10-06 14:59:26]

skyostil@chromium.org changed reviewers:
+ eseidel@chromium.org

[eseidel, 2014-10-06 16:39:41]

lgtm

[Sami, 2014-10-06 16:41:17]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-10-06 16:41:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/621363002/80001

[commit-bot: I haz the power, 2014-10-06 21:01:07]

Committed patchset #5 (id:80001) as 183287