gpu: Optimize in-process idle task scheduling

gpu: Match in-process idle work delay

Use 2ms which is used by GpuCommandBufferStub. This helps
throughput of idle work like the idle uploader.

BUG=448168
Committed: https://crrev.com/348699274b88544017815231a333b0ed02055986
Cr-Commit-Position: refs/heads/master@{#312969}

[boliu, 2015-01-12 22:34:38]

boliu@chromium.org changed reviewers:
+ sievers@chromium.org

[boliu, 2015-01-12 22:34:39]

PTAL

I think the failure on win_gpu is a flake

[no sievers, 2015-01-13 21:00:00]

https://codereview.chromium.org/851473003/diff/1/gpu/command_buffer/service/i...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/851473003/diff/1/gpu/command_buffer/service/i...
gpu/command_buffer/service/in_process_command_buffer.cc:109:
message_loop()->PostTask(FROM_HERE, task);
Should this also change then and have its own queue which we handle here?

I.e. CheckIdleTasks() -> CheckTasks()
ScheduleTask(task) similar as ScheduleIdleWork() but put in different queue.

RunIdleTask() -> RunLoop() which could do something like:
get the current number of tasks and run all of them
then run idle tasks while there are no normal tasks

[boliu, 2015-01-13 22:07:23]

https://codereview.chromium.org/851473003/diff/1/gpu/command_buffer/service/i...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/851473003/diff/1/gpu/command_buffer/service/i...
gpu/command_buffer/service/in_process_command_buffer.cc:109:
message_loop()->PostTask(FROM_HERE, task);
On 2015/01/13 21:00:00, sievers wrote:
> Should this also change then and have its own queue which we handle here?
> 
> I.e. CheckIdleTasks() -> CheckTasks()
> ScheduleTask(task) similar as ScheduleIdleWork() but put in different queue.
> 
> RunIdleTask() -> RunLoop() which could do something like:
> get the current number of tasks and run all of them
> then run idle tasks while there are no normal tasks

I think we get this behavior with the current code. It's just the queue for
non-idle tasks you describe here is the task queue inside the message loop. No?

[no sievers, 2015-01-13 22:53:27]

On 2015/01/13 22:07:23, boliu wrote:
>
https://codereview.chromium.org/851473003/diff/1/gpu/command_buffer/service/i...
> File gpu/command_buffer/service/in_process_command_buffer.cc (right):
> 
>
https://codereview.chromium.org/851473003/diff/1/gpu/command_buffer/service/i...
> gpu/command_buffer/service/in_process_command_buffer.cc:109:
> message_loop()->PostTask(FROM_HERE, task);
> On 2015/01/13 21:00:00, sievers wrote:
> > Should this also change then and have its own queue which we handle here?
> > 
> > I.e. CheckIdleTasks() -> CheckTasks()
> > ScheduleTask(task) similar as ScheduleIdleWork() but put in different queue.
> > 
> > RunIdleTask() -> RunLoop() which could do something like:
> > get the current number of tasks and run all of them
> > then run idle tasks while there are no normal tasks
> 
> I think we get this behavior with the current code. It's just the queue for
> non-idle tasks you describe here is the task queue inside the message loop.
No?

In your patch idle tasks are ordered with normal tasks when they are posted.
However, once an idle task runs it's similar in that it yields for all
already-posted normal tasks.

Maybe it doesn't make a big difference. What was the problem you wanted to fix
again? :)

[boliu, 2015-01-13 23:03:06]

On 2015/01/13 22:53:27, sievers wrote:
> On 2015/01/13 22:07:23, boliu wrote:
> >
>
https://codereview.chromium.org/851473003/diff/1/gpu/command_buffer/service/i...
> > File gpu/command_buffer/service/in_process_command_buffer.cc (right):
> > 
> >
>
https://codereview.chromium.org/851473003/diff/1/gpu/command_buffer/service/i...
> > gpu/command_buffer/service/in_process_command_buffer.cc:109:
> > message_loop()->PostTask(FROM_HERE, task);
> > On 2015/01/13 21:00:00, sievers wrote:
> > > Should this also change then and have its own queue which we handle here?
> > > 
> > > I.e. CheckIdleTasks() -> CheckTasks()
> > > ScheduleTask(task) similar as ScheduleIdleWork() but put in different
queue.
> > > 
> > > RunIdleTask() -> RunLoop() which could do something like:
> > > get the current number of tasks and run all of them
> > > then run idle tasks while there are no normal tasks
> > 
> > I think we get this behavior with the current code. It's just the queue for
> > non-idle tasks you describe here is the task queue inside the message loop.
> No?
> 
> In your patch idle tasks are ordered with normal tasks when they are posted.

Doesn't sound so bad :)

> However, once an idle task runs it's similar in that it yields for all
> already-posted normal tasks.
> 
> Maybe it doesn't make a big difference. What was the problem you wanted to fix
> again? :)

Idle should be lower priority than (ie yield to) regular tasks (which is not the
same thing as post idle tasks with a delay). But then if there are no regular
tasks (most of the case in webview), don't have the frigging 5ms delay.

[no sievers, 2015-01-14 02:59:02]

On 2015/01/13 23:03:06, boliu wrote:
> On 2015/01/13 22:53:27, sievers wrote:
> > On 2015/01/13 22:07:23, boliu wrote:
> > >
> >
>
https://codereview.chromium.org/851473003/diff/1/gpu/command_buffer/service/i...
> > > File gpu/command_buffer/service/in_process_command_buffer.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/851473003/diff/1/gpu/command_buffer/service/i...
> > > gpu/command_buffer/service/in_process_command_buffer.cc:109:
> > > message_loop()->PostTask(FROM_HERE, task);
> > > On 2015/01/13 21:00:00, sievers wrote:
> > > > Should this also change then and have its own queue which we handle
here?
> > > > 
> > > > I.e. CheckIdleTasks() -> CheckTasks()
> > > > ScheduleTask(task) similar as ScheduleIdleWork() but put in different
> queue.
> > > > 
> > > > RunIdleTask() -> RunLoop() which could do something like:
> > > > get the current number of tasks and run all of them
> > > > then run idle tasks while there are no normal tasks
> > > 
> > > I think we get this behavior with the current code. It's just the queue
for
> > > non-idle tasks you describe here is the task queue inside the message
loop.
> > No?
> > 
> > In your patch idle tasks are ordered with normal tasks when they are posted.
> 
> Doesn't sound so bad :)
> 
> > However, once an idle task runs it's similar in that it yields for all
> > already-posted normal tasks.
> > 
> > Maybe it doesn't make a big difference. What was the problem you wanted to
fix
> > again? :)
> 
> Idle should be lower priority than (ie yield to) regular tasks (which is not
the
> same thing as post idle tasks with a delay). But then if there are no regular
> tasks (most of the case in webview), don't have the frigging 5ms delay.

But we also want to keep the delay, it's actually intentional (I think
GpuCommandBufferStub does something similar).
One iteration of PerformIdleWork() checks a bunch of state (queries, async
transfers), so if those are not completed yet, we should yield.
A better way to think of this is that we need to trigger one specific callback
every once in a while. HasMoreWork() then means that we should try again after a
while. So I'd keep the delay in your patch.

The other call site for idle tasks (SignalSyncPoint) is actually similar, but
from a different thread. What we could do is make SignalSyncPoint store the
(wrapped) callback in the SyncPointManager if the sync point is not retired yet.
And then trigger callbacks when the sync point retires on the gpu thread. This
would also make the code closer to sync_point_manager.cc, and for the bug you
reference here we eventually have to merge the sync point managers into one
anyway.

Overall we wouldn't need a lock or a queue, just a bool for
'idle_already_scheduled'.

[boliu, 2015-01-14 17:22:57]

On 2015/01/14 02:59:02, sievers wrote:
> On 2015/01/13 23:03:06, boliu wrote:
> > Idle should be lower priority than (ie yield to) regular tasks (which is not
> the
> > same thing as post idle tasks with a delay). But then if there are no
regular
> > tasks (most of the case in webview), don't have the frigging 5ms delay.
> 
> But we also want to keep the delay, it's actually intentional (I think
> GpuCommandBufferStub does something similar).
> One iteration of PerformIdleWork() checks a bunch of state (queries, async
> transfers), so if those are not completed yet, we should yield.
> A better way to think of this is that we need to trigger one specific callback
> every once in a while. HasMoreWork() then means that we should try again after
a
> while. So I'd keep the delay in your patch.

Interesting. Read a bit about GpuCommandBufferStub and it's rather complicated.
1) Idle tasks are scheduled per stub, there is no global queue that limits how
often idle runs
2) The delay is either 1ms or 2ms
3) However in GpuCommandBufferStub::ScheduleDelayedWork, there is a block that
sets the delay to 0 if IsScheduled and HasMoreIdleWork is true. I'm not really
sure what IsScheduled here means..
4) Also there's non-idle delayed work. And in PollWork, it really does check
there are no non-idle work, so idle yields even more aggressively than this
current implementation. And idle is not starved by checking idle runs at least
once every 10ms.

For 1), it seems I don't need to keep a global queue of idle work.

I'm concerned about 3). According to the comment, it sounds if there is *no
other work*, then idle runs as fast as it can. Should we do the same here, and
check IsScheduled and make the delay 0?

> 
> The other call site for idle tasks (SignalSyncPoint) is actually similar, but
> from a different thread. What we could do is make SignalSyncPoint store the
> (wrapped) callback in the SyncPointManager if the sync point is not retired
yet.
> And then trigger callbacks when the sync point retires on the gpu thread. This
> would also make the code closer to sync_point_manager.cc, and for the bug you
> reference here we eventually have to merge the sync point managers into one
> anyway.

Sounds easy to fix :)

> 
> Overall we wouldn't need a lock or a queue, just a bool for
> 'idle_already_scheduled'.

[no sievers, 2015-01-16 18:28:44]

On 2015/01/14 17:22:57, boliu wrote:
> On 2015/01/14 02:59:02, sievers wrote:
> > On 2015/01/13 23:03:06, boliu wrote:
> > > Idle should be lower priority than (ie yield to) regular tasks (which is
not
> > the
> > > same thing as post idle tasks with a delay). But then if there are no
> regular
> > > tasks (most of the case in webview), don't have the frigging 5ms delay.
> > 
> > But we also want to keep the delay, it's actually intentional (I think
> > GpuCommandBufferStub does something similar).
> > One iteration of PerformIdleWork() checks a bunch of state (queries, async
> > transfers), so if those are not completed yet, we should yield.
> > A better way to think of this is that we need to trigger one specific
callback
> > every once in a while. HasMoreWork() then means that we should try again
after
> a
> > while. So I'd keep the delay in your patch.
> 
> Interesting. Read a bit about GpuCommandBufferStub and it's rather
complicated.
> 1) Idle tasks are scheduled per stub, there is no global queue that limits how
> often idle runs
> 2) The delay is either 1ms or 2ms
> 3) However in GpuCommandBufferStub::ScheduleDelayedWork, there is a block that
> sets the delay to 0 if IsScheduled and HasMoreIdleWork is true. I'm not really
> sure what IsScheduled here means..
> 4) Also there's non-idle delayed work. And in PollWork, it really does check
> there are no non-idle work, so idle yields even more aggressively than this
> current implementation. And idle is not starved by checking idle runs at least
> once every 10ms.
> 
> For 1), it seems I don't need to keep a global queue of idle work.
> 
> I'm concerned about 3). According to the comment, it sounds if there is *no
> other work*, then idle runs as fast as it can. Should we do the same here, and
> check IsScheduled and make the delay 0?
> 


I think we can keep it simple for now since we don't deal with descheduling
stubs here.
And it's not terribly important how exactly we tweak it in terms of milliseconds
of delay etc.
You might want to just fix the problem of the over-posting of tasks once we have
a general PerformIdleWork() entry point (with sync callbacks being independent
of idle work).

[boliu, 2015-01-21 01:12:14]

On 2015/01/16 18:28:44, sievers wrote:
> On 2015/01/14 17:22:57, boliu wrote:
> > On 2015/01/14 02:59:02, sievers wrote:
> > > On 2015/01/13 23:03:06, boliu wrote:
> > > > Idle should be lower priority than (ie yield to) regular tasks (which is
> not
> > > the
> > > > same thing as post idle tasks with a delay). But then if there are no
> > regular
> > > > tasks (most of the case in webview), don't have the frigging 5ms delay.
> > > 
> > > But we also want to keep the delay, it's actually intentional (I think
> > > GpuCommandBufferStub does something similar).
> > > One iteration of PerformIdleWork() checks a bunch of state (queries, async
> > > transfers), so if those are not completed yet, we should yield.
> > > A better way to think of this is that we need to trigger one specific
> callback
> > > every once in a while. HasMoreWork() then means that we should try again
> after
> > a
> > > while. So I'd keep the delay in your patch.
> > 
> > Interesting. Read a bit about GpuCommandBufferStub and it's rather
> complicated.
> > 1) Idle tasks are scheduled per stub, there is no global queue that limits
how
> > often idle runs
> > 2) The delay is either 1ms or 2ms
> > 3) However in GpuCommandBufferStub::ScheduleDelayedWork, there is a block
that
> > sets the delay to 0 if IsScheduled and HasMoreIdleWork is true. I'm not
really
> > sure what IsScheduled here means..
> > 4) Also there's non-idle delayed work. And in PollWork, it really does check
> > there are no non-idle work, so idle yields even more aggressively than this
> > current implementation. And idle is not starved by checking idle runs at
least
> > once every 10ms.
> > 
> > For 1), it seems I don't need to keep a global queue of idle work.
> > 
> > I'm concerned about 3). According to the comment, it sounds if there is *no
> > other work*, then idle runs as fast as it can. Should we do the same here,
and
> > check IsScheduled and make the delay 0?
> > 
> 
> 
> I think we can keep it simple for now since we don't deal with descheduling
> stubs here.
> And it's not terribly important how exactly we tweak it in terms of
milliseconds
> of delay etc.

Doesn't the delay affect throughput of idle uploader? Ok with me lowering it to
2ms?

> You might want to just fix the problem of the over-posting of tasks once we
have
> a general PerformIdleWork() entry point (with sync callbacks being independent
> of idle work).

I think this isn't much a problem. With the SyncPointManager change
(https://codereview.chromium.org/849103002/) and this one from 5 months ago
(https://codereview.chromium.org/450793002), there's at most one idle task per
InProcessCommandBuffer. And this is the same as the stub implementation, one
pending task per stub.

[no sievers, 2015-01-23 21:33:51]

On 2015/01/21 01:12:14, boliu wrote:
> On 2015/01/16 18:28:44, sievers wrote:
> > On 2015/01/14 17:22:57, boliu wrote:
> > > On 2015/01/14 02:59:02, sievers wrote:
> > > > On 2015/01/13 23:03:06, boliu wrote:
> > > > > Idle should be lower priority than (ie yield to) regular tasks (which
is
> > not
> > > > the
> > > > > same thing as post idle tasks with a delay). But then if there are no
> > > regular
> > > > > tasks (most of the case in webview), don't have the frigging 5ms
delay.
> > > > 
> > > > But we also want to keep the delay, it's actually intentional (I think
> > > > GpuCommandBufferStub does something similar).
> > > > One iteration of PerformIdleWork() checks a bunch of state (queries,
async
> > > > transfers), so if those are not completed yet, we should yield.
> > > > A better way to think of this is that we need to trigger one specific
> > callback
> > > > every once in a while. HasMoreWork() then means that we should try again
> > after
> > > a
> > > > while. So I'd keep the delay in your patch.
> > > 
> > > Interesting. Read a bit about GpuCommandBufferStub and it's rather
> > complicated.
> > > 1) Idle tasks are scheduled per stub, there is no global queue that limits
> how
> > > often idle runs
> > > 2) The delay is either 1ms or 2ms
> > > 3) However in GpuCommandBufferStub::ScheduleDelayedWork, there is a block
> that
> > > sets the delay to 0 if IsScheduled and HasMoreIdleWork is true. I'm not
> really
> > > sure what IsScheduled here means..
> > > 4) Also there's non-idle delayed work. And in PollWork, it really does
check
> > > there are no non-idle work, so idle yields even more aggressively than
this
> > > current implementation. And idle is not starved by checking idle runs at
> least
> > > once every 10ms.
> > > 
> > > For 1), it seems I don't need to keep a global queue of idle work.
> > > 
> > > I'm concerned about 3). According to the comment, it sounds if there is
*no
> > > other work*, then idle runs as fast as it can. Should we do the same here,
> and
> > > check IsScheduled and make the delay 0?
> > > 
> > 
> > 
> > I think we can keep it simple for now since we don't deal with descheduling
> > stubs here.
> > And it's not terribly important how exactly we tweak it in terms of
> milliseconds
> > of delay etc.
> 
> Doesn't the delay affect throughput of idle uploader? Ok with me lowering it
to
> 2ms?
> 

2ms sounds fine.

> > You might want to just fix the problem of the over-posting of tasks once we
> have
> > a general PerformIdleWork() entry point (with sync callbacks being
independent
> > of idle work).
> 
> I think this isn't much a problem. With the SyncPointManager change
> (https://codereview.chromium.org/849103002/) and this one from 5 months ago
> (https://codereview.chromium.org/450793002), there's at most one idle task per
> InProcessCommandBuffer. And this is the same as the stub implementation, one
> pending task per stub.

[boliu, 2015-01-23 22:16:25]

Check out how small this patch is! :p

[no sievers, 2015-01-23 22:59:40]

lgtm

[boliu, 2015-01-23 23:00:16]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2015-01-23 23:01:06]

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

[commit-bot: I haz the power, 2015-01-23 23:53:27]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-23 23:54:17]

Patchset 2 (id:??) landed as
https://crrev.com/348699274b88544017815231a333b0ed02055986
Cr-Commit-Position: refs/heads/master@{#312969}