Modify the scheduler to queue idle work between idle periods.

Modify the scheduler to queue idle work between idle periods.

Modifies the idle time scheduling such that idle tasks which are posted during one
idle period will not become pending until the next idle period.  This enables idle
tasks to repost themselves at the end of their execution without causing a flood of
idle task execution in the remaining deadline period.

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

[rmcilroy, 2014-10-10 15:30:24]

rmcilroy@chromium.org changed reviewers:
+ jochen@chromium.org, skyostil@chromium.org

[rmcilroy, 2014-10-10 15:30:24]

Sami / Jochen, could you please take a look, thanks.

[Sami, 2014-10-10 17:31:31]

Looks great. Could use a test too.

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

https://codereview.chromium.org/640053003/diff/40001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:276: Locker<Mutex>
lock(m_incomingIdleTasksMutex);
I'm trying to decide whether we should only do this when the pending queue
becomes empty. If we did that, we could potentially need to flush less, but we'd
run the risk of draining the pending queue and delaying posted tasks until the
next frame. WDYT?

[jochen (gone - plz use gerrit), 2014-10-13 08:09:11]

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

https://codereview.chromium.org/640053003/diff/40001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:280: while
(!m_incomingIdleTasks.isEmpty()) {
nit. no { }

what happens if the idle task immediately posts a new idle task?

Maybe make a copy of the queue first, and just process the queue?

[picksi1, 2014-10-13 09:38:09]

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

[picksi1, 2014-10-13 09:38:10]

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

https://codereview.chromium.org/640053003/diff/40001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:279: 
Is it worth doing this empty check here? The following code will fall through
anyway and it bulks the code.

https://codereview.chromium.org/640053003/diff/40001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:282: }
Is there no quick way to append the entire queue here, rather than iterating
task by task?

[rmcilroy, 2014-10-13 11:41:19]

> Looks great. Could use a test too.

Done.

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

https://codereview.chromium.org/640053003/diff/40001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:276: Locker<Mutex>
lock(m_incomingIdleTasksMutex);
On 2014/10/10 17:31:31, Sami wrote:
> I'm trying to decide whether we should only do this when the pending queue
> becomes empty. If we did that, we could potentially need to flush less, but
we'd
> run the risk of draining the pending queue and delaying posted tasks until the
> next frame. WDYT?

I don't think it would make much of a difference either way.  I'd prefer to keep
it this way for now since it's simpler, but happy to change to the other
approach once your CL lands if it makes it easier.

https://codereview.chromium.org/640053003/diff/40001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:279: 
On 2014/10/13 09:38:09, picksi1 wrote:
> Is it worth doing this empty check here? The following code will fall through
> anyway and it bulks the code.

Makes sense, dropped it.

https://codereview.chromium.org/640053003/diff/40001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:280: while
(!m_incomingIdleTasks.isEmpty()) {
On 2014/10/13 08:09:11, jochen wrote:
> nit. no { }
> 
> what happens if the idle task immediately posts a new idle task?
> 
> Maybe make a copy of the queue first, and just process the queue?

If the idle task immediately posts a new task it will go into the incoming task
queue and only get run after that is flushed to the pending tasks queue.  No
tasks can get posted while we hold the lock, and no idle tasks get executed
until we have finished with this flush function. 

I'm not sure what you mean about taking a copy of the queue - this is
effectively what we are doing by having a separate incoming and pending queue -
could you clarify what you mean here?

https://codereview.chromium.org/640053003/diff/40001/Source/platform/schedule...
Source/platform/scheduler/Scheduler.cpp:282: }
On 2014/10/13 09:38:09, picksi1 wrote:
> Is there no quick way to append the entire queue here, rather than iterating
> task by task?

I couldn't find any faster append in Deque - let me know if you know of any?

[rmcilroy, 2014-10-15 08:43:25]

On 2014/10/13 11:41:19, rmcilroy wrote:
> > Looks great. Could use a test too.
> 
> Done.
> 
>
https://codereview.chromium.org/640053003/diff/40001/Source/platform/schedule...
> File Source/platform/scheduler/Scheduler.cpp (right):
> 
>
https://codereview.chromium.org/640053003/diff/40001/Source/platform/schedule...
> Source/platform/scheduler/Scheduler.cpp:276: Locker<Mutex>
> lock(m_incomingIdleTasksMutex);
> On 2014/10/10 17:31:31, Sami wrote:
> > I'm trying to decide whether we should only do this when the pending queue
> > becomes empty. If we did that, we could potentially need to flush less, but
> we'd
> > run the risk of draining the pending queue and delaying posted tasks until
the
> > next frame. WDYT?
> 
> I don't think it would make much of a difference either way.  I'd prefer to
keep
> it this way for now since it's simpler, but happy to change to the other
> approach once your CL lands if it makes it easier.
> 
>
https://codereview.chromium.org/640053003/diff/40001/Source/platform/schedule...
> Source/platform/scheduler/Scheduler.cpp:279: 
> On 2014/10/13 09:38:09, picksi1 wrote:
> > Is it worth doing this empty check here? The following code will fall
through
> > anyway and it bulks the code.
> 
> Makes sense, dropped it.
> 
>
https://codereview.chromium.org/640053003/diff/40001/Source/platform/schedule...
> Source/platform/scheduler/Scheduler.cpp:280: while
> (!m_incomingIdleTasks.isEmpty()) {
> On 2014/10/13 08:09:11, jochen wrote:
> > nit. no { }
> > 
> > what happens if the idle task immediately posts a new idle task?
> > 
> > Maybe make a copy of the queue first, and just process the queue?
> 
> If the idle task immediately posts a new task it will go into the incoming
task
> queue and only get run after that is flushed to the pending tasks queue.  No
> tasks can get posted while we hold the lock, and no idle tasks get executed
> until we have finished with this flush function. 
> 
> I'm not sure what you mean about taking a copy of the queue - this is
> effectively what we are doing by having a separate incoming and pending queue
-
> could you clarify what you mean here?
> 
>
https://codereview.chromium.org/640053003/diff/40001/Source/platform/schedule...
> Source/platform/scheduler/Scheduler.cpp:282: }
> On 2014/10/13 09:38:09, picksi1 wrote:
> > Is there no quick way to append the entire queue here, rather than iterating
> > task by task?
> 
> I couldn't find any faster append in Deque - let me know if you know of any?

Any more comments on this CL?

[jochen (gone - plz use gerrit), 2014-10-15 15:29:25]

lgtm

[rmcilroy, 2014-10-17 09:26:55]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2014-10-17 09:27:03]

The CQ bit was unchecked by rmcilroy@chromium.org

[rmcilroy, 2014-10-17 10:50:49]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2014-10-17 10:51:44]

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

[commit-bot: I haz the power, 2014-10-17 12:41:58]

Committed patchset #4 (id:150001) as 183884