GPU: Only allow the UI channel to preempt if all stubs are scheduled.

GPU: Only allow the UI channel to preempt if all stubs are scheduled.

Necessary for uber-comp with --ui-prioritize-in-gpu-process. Currently will not affect preemption.

BUG=173650
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=185445

[jonathan.backer, 2013-02-27 21:21:45]

I think this solves the problem in crbug.com/173650 without adding more
complication to the preemption state machine. There are more refined strategies
that we could employ (e.g. pay attention to which stub is descheduled or have
descheduling move us out of a PREEMPTING state). But this seemed the simplest
and easiest to reason about to me.

[piman, 2013-02-27 21:54:43]

https://codereview.chromium.org/12340118/diff/1/content/common/gpu/gpu_channe...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/12340118/diff/1/content/common/gpu/gpu_channe...
content/common/gpu/gpu_channel.cc:218: void UpdateStubSchedulingState(bool
a_stub_is_descheduled) {
I'm afraid this will kick preemption more than you'd generally want. Basically
any time passed unscheduled counts towards the preemption timeout.
OTOH, I think changing the state machine to take a_stub_is_descheduled_ into
account isn't bad at all, all you have to change is UpdatePreemtionState, to
test a_stub_is_descheduled_ any time you check pending_messages_.empty()
(and call UpdateStubSchedulingState)

https://codereview.chromium.org/12340118/diff/1/content/common/gpu/gpu_channe...
content/common/gpu/gpu_channel.cc:570: !it.IsAtEnd(); it.Advance()) {
Could we count scheduled stubs? If we fix the callbacks so that this is called
exactly once for each change (see below), we can avoid walking the map every
time.

https://codereview.chromium.org/12340118/diff/1/content/common/gpu/gpu_comman...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/12340118/diff/1/content/common/gpu/gpu_comman...
content/common/gpu/gpu_command_buffer_stub.cc:493: base::Unretained(channel_)));
Could we merge the 2 callbacks (eg passing the scheduled flag, or having the
callback check IsScheduled() )?
It's easier to follow if both paths get routed to the same class, and it avoids
the explosion of callbacks.

https://codereview.chromium.org/12340118/diff/1/gpu/command_buffer/service/gp...
File gpu/command_buffer/service/gpu_scheduler.cc (right):

https://codereview.chromium.org/12340118/diff/1/gpu/command_buffer/service/gp...
gpu/command_buffer/service/gpu_scheduler.cc:160: if
(!descheduled_callback_.is_null())
call this only if unscheduled_count_ is 1 (otherwise, seems overkill)

[jonathan.backer, 2013-02-28 21:43:56]

https://codereview.chromium.org/12340118/diff/1/content/common/gpu/gpu_channe...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/12340118/diff/1/content/common/gpu/gpu_channe...
content/common/gpu/gpu_channel.cc:218: void UpdateStubSchedulingState(bool
a_stub_is_descheduled) {
On 2013/02/27 21:54:43, piman wrote:
> I'm afraid this will kick preemption more than you'd generally want. Basically
> any time passed unscheduled counts towards the preemption timeout.
> OTOH, I think changing the state machine to take a_stub_is_descheduled_ into
> account isn't bad at all, all you have to change is UpdatePreemtionState, to
> test a_stub_is_descheduled_ any time you check pending_messages_.empty()
> (and call UpdateStubSchedulingState)

Done.

https://codereview.chromium.org/12340118/diff/1/content/common/gpu/gpu_channe...
content/common/gpu/gpu_channel.cc:570: !it.IsAtEnd(); it.Advance()) {
On 2013/02/27 21:54:43, piman wrote:
> Could we count scheduled stubs? If we fix the callbacks so that this is called
> exactly once for each change (see below), we can avoid walking the map every
> time.

Done. I'm concerned about teardown. I think I've handled that appropriately in
OnDestroyCommandBuffer.

https://codereview.chromium.org/12340118/diff/1/content/common/gpu/gpu_comman...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/12340118/diff/1/content/common/gpu/gpu_comman...
content/common/gpu/gpu_command_buffer_stub.cc:493: base::Unretained(channel_)));
On 2013/02/27 21:54:43, piman wrote:
> Could we merge the 2 callbacks (eg passing the scheduled flag, or having the
> callback check IsScheduled() )?
> It's easier to follow if both paths get routed to the same class, and it
avoids
> the explosion of callbacks.

Done.

https://codereview.chromium.org/12340118/diff/1/gpu/command_buffer/service/gp...
File gpu/command_buffer/service/gpu_scheduler.cc (right):

https://codereview.chromium.org/12340118/diff/1/gpu/command_buffer/service/gp...
gpu/command_buffer/service/gpu_scheduler.cc:160: if
(!descheduled_callback_.is_null())
On 2013/02/27 21:54:43, piman wrote:
> call this only if unscheduled_count_ is 1 (otherwise, seems overkill)

Done.

[piman, 2013-02-28 22:27:48]

LGTM + nit

Teardown looks fine - the assumption is that you want to count it down when you
remove an unscheduled stub - and the only place that happens is
OnDestroyCommandBuffer (and teardown of the whole channel, where it doesn't
matter anymore).

https://codereview.chromium.org/12340118/diff/11002/content/common/gpu/gpu_ch...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/12340118/diff/11002/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel.cc:896: OnScheduled();
nit: this will be called from StubsSchedulingChanged below, so I think you can
remove now.

[jonathan.backer, 2013-03-01 00:42:34]

https://codereview.chromium.org/12340118/diff/11002/content/common/gpu/gpu_ch...
File content/common/gpu/gpu_channel.cc (right):

https://codereview.chromium.org/12340118/diff/11002/content/common/gpu/gpu_ch...
content/common/gpu/gpu_channel.cc:896: OnScheduled();
On 2013/02/28 22:27:48, piman wrote:
> nit: this will be called from StubsSchedulingChanged below, so I think you can
> remove now.

Done.

[commit-bot: I haz the power, 2013-03-01 00:43:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/backer@chromium.org/12340118/6002

[commit-bot: I haz the power, 2013-03-01 03:42:12]

Change committed as 185445