Fix a bug in the implementation of DescheduleUntilFinishedCHROMIUM.

Fix a bug in the implementation of DescheduleUntilFinishedCHROMIUM.

The logic for checking whether an executor was idle was only checking if all of
the commands submitted had been processed. This always returns false for
DescheduleUntilFinishedCHROMIUM, since there are unprocessed commands.

I added the functions HasPollingWork() and PerformPollingWork() to
CommandExecutor to separate DescheduleUntilFinishedCHROMIUM work from idle work.

BUG=617249
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/63170daf2b1bc70388d39d8b3635058b8269e3ea
Cr-Commit-Position: refs/heads/master@{#401525}

[erikchen, 2016-06-10 19:43:24]

Description was changed from

==========
Fix a bug in the implementation of DescheduleUntilFinishedCHROMIUM.

The logic for checking whether an executor was idle was only checking if all of
the commands submitted have been processed. This always returns false for
DescheduleUntilFinishedCHROMIUM, since there are unprocessed commands.

The solution is to also consider the executor as idle if it is descheduled.

BUG=617249
==========

to

==========
Fix a bug in the implementation of DescheduleUntilFinishedCHROMIUM.

The logic for checking whether an executor was idle was only checking if all of
the commands submitted have been processed. This always returns false for
DescheduleUntilFinishedCHROMIUM, since there are unprocessed commands.

The solution is to also consider the executor as idle if it is descheduled.

BUG=617249
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[erikchen, 2016-06-10 19:45:17]

erikchen@chromium.org changed reviewers:
+ piman@chromium.org

[erikchen, 2016-06-10 19:45:18]

piman: Please review.

[erikchen, 2016-06-10 19:46:37]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-10 19:47:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2056413003/1

[piman, 2016-06-10 21:52:27]

https://codereview.chromium.org/2056413003/diff/1/gpu/ipc/service/gpu_command...
File gpu/ipc/service/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/2056413003/diff/1/gpu/ipc/service/gpu_command...
gpu/ipc/service/gpu_command_buffer_stub.cc:310: is_idle |=
!executor_->scheduled();
Trying to think a bit more about this...

We want to do idle work when nothing else is executing. The check on the
unprocessed message is global - looking at all channels, all command buffers,
making sure we don't have any unprocessed message.

With this change, we would be spending cycles trying to perform idle work on any
command buffer that is unscheduled, even if something else (other command
buffer), possibly higher priority, would be running otherwise - e.g. in case of
a WaitSyncToken.

So what we would really want is: do idle work if either:
1- there's nothing else to do (no unprocessed message), or
2- every command buffer that has messages to process is unscheduled (assuming,
the idle work will eventually reschedule it)

This is something that Sunny's global GPU scheduler should make easy,
unfortunately it has not landed yet.


In the mean time my suggestion is to implement something similar to
GpuChannelManager::GetUnprocessedOrderNum(): iterate over all the channels, and
in each channel iterate over all the streams (GpuChannelMessageQueue), and check
scheduled_.
If they're all false, then we can run idle work. WDYT?

[commit-bot: I haz the power, 2016-06-11 00:16:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-11 00:16:10]

Dry run: This issue passed the CQ dry run.

[erikchen, 2016-06-22 17:07:24]

Description was changed from

==========
Fix a bug in the implementation of DescheduleUntilFinishedCHROMIUM.

The logic for checking whether an executor was idle was only checking if all of
the commands submitted have been processed. This always returns false for
DescheduleUntilFinishedCHROMIUM, since there are unprocessed commands.

The solution is to also consider the executor as idle if it is descheduled.

BUG=617249
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix a bug in the implementation of DescheduleUntilFinishedCHROMIUM.

The logic for checking whether an executor was idle was only checking if all of
the commands submitted have been processed. This always returns false for
DescheduleUntilFinishedCHROMIUM, since there are unprocessed commands.

The solution is to also consider the executor as idle if it is descheduled.

BUG=617249
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[erikchen, 2016-06-22 19:23:07]

piman: PTAL

https://codereview.chromium.org/2056413003/diff/1/gpu/ipc/service/gpu_command...
File gpu/ipc/service/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/2056413003/diff/1/gpu/ipc/service/gpu_command...
gpu/ipc/service/gpu_command_buffer_stub.cc:310: is_idle |=
!executor_->scheduled();
On 2016/06/10 21:52:27, piman wrote:
> Trying to think a bit more about this...
> 
> We want to do idle work when nothing else is executing. The check on the
> unprocessed message is global - looking at all channels, all command buffers,
> making sure we don't have any unprocessed message.
> 
> With this change, we would be spending cycles trying to perform idle work on
any
> command buffer that is unscheduled, even if something else (other command
> buffer), possibly higher priority, would be running otherwise - e.g. in case
of
> a WaitSyncToken.
> 
> So what we would really want is: do idle work if either:
> 1- there's nothing else to do (no unprocessed message), or
> 2- every command buffer that has messages to process is unscheduled (assuming,
> the idle work will eventually reschedule it)
> 
> This is something that Sunny's global GPU scheduler should make easy,
> unfortunately it has not landed yet.
> 
> 
> In the mean time my suggestion is to implement something similar to
> GpuChannelManager::GetUnprocessedOrderNum(): iterate over all the channels,
and
> in each channel iterate over all the streams (GpuChannelMessageQueue), and
check
> scheduled_.
> If they're all false, then we can run idle work. WDYT?

I think that your proposed solution still has edge cases. If there is a tiny
amount of unprocessed work each time a descheduled executor tries to reschedule
itself, it will never be rescheduled. In the more common case, it will
reschedule itself later than it otherwise should have been rescheduled.

Perhaps this is a nomenclature problem? Attempting to reschedule the executor is
a more urgent priority than idle work. I could add some additional logic and
rename some methods to check for "mandatory work"?

[piman, 2016-06-22 21:47:41]

https://codereview.chromium.org/2056413003/diff/1/gpu/ipc/service/gpu_command...
File gpu/ipc/service/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/2056413003/diff/1/gpu/ipc/service/gpu_command...
gpu/ipc/service/gpu_command_buffer_stub.cc:310: is_idle |=
!executor_->scheduled();
On 2016/06/22 19:23:07, erikchen wrote:
> On 2016/06/10 21:52:27, piman wrote:
> > Trying to think a bit more about this...
> > 
> > We want to do idle work when nothing else is executing. The check on the
> > unprocessed message is global - looking at all channels, all command
buffers,
> > making sure we don't have any unprocessed message.
> > 
> > With this change, we would be spending cycles trying to perform idle work on
> any
> > command buffer that is unscheduled, even if something else (other command
> > buffer), possibly higher priority, would be running otherwise - e.g. in case
> of
> > a WaitSyncToken.
> > 
> > So what we would really want is: do idle work if either:
> > 1- there's nothing else to do (no unprocessed message), or
> > 2- every command buffer that has messages to process is unscheduled
(assuming,
> > the idle work will eventually reschedule it)
> > 
> > This is something that Sunny's global GPU scheduler should make easy,
> > unfortunately it has not landed yet.
> > 
> > 
> > In the mean time my suggestion is to implement something similar to
> > GpuChannelManager::GetUnprocessedOrderNum(): iterate over all the channels,
> and
> > in each channel iterate over all the streams (GpuChannelMessageQueue), and
> check
> > scheduled_.
> > If they're all false, then we can run idle work. WDYT?
> 
> I think that your proposed solution still has edge cases. If there is a tiny
> amount of unprocessed work each time a descheduled executor tries to
reschedule
> itself, it will never be rescheduled. In the more common case, it will
> reschedule itself later than it otherwise should have been rescheduled.
> 
> Perhaps this is a nomenclature problem? Attempting to reschedule the executor
is
> a more urgent priority than idle work. I could add some additional logic and
> rename some methods to check for "mandatory work"?
> 

Yeah, I guess we should separate "polling work", which blocks forwards progress
from "idle work", which doesn't block forward progress and that we should only
do if nothing else wants to run.
I.e. for the purpose of rescheduling, it should behave like the pending queries
rather than idle work.

[erikchen, 2016-06-22 23:53:37]

Description was changed from

==========
Fix a bug in the implementation of DescheduleUntilFinishedCHROMIUM.

The logic for checking whether an executor was idle was only checking if all of
the commands submitted have been processed. This always returns false for
DescheduleUntilFinishedCHROMIUM, since there are unprocessed commands.

The solution is to also consider the executor as idle if it is descheduled.

BUG=617249
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix a bug in the implementation of DescheduleUntilFinishedCHROMIUM.

The logic for checking whether an executor was idle was only checking if all of
the commands submitted had been processed. This always returns false for
DescheduleUntilFinishedCHROMIUM, since there are unprocessed commands.

I added the functions HasPollingWork() and PerformPollingWork() to
CommandExecutor to separate DescheduleUntilFinishedCHROMIUM work from idle work.

BUG=617249
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[erikchen, 2016-06-22 23:57:10]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[erikchen, 2016-06-22 23:57:16]

piman: PTAL

[commit-bot: I haz the power, 2016-06-22 23:58:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2056413003/60001

[piman, 2016-06-23 00:02:10]

LGTM, thanks!

[erikchen, 2016-06-23 00:29:37]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-06-23 00:29:37]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2056413003/#ps80001
(title: "compile error on windows.")

[commit-bot: I haz the power, 2016-06-23 00:30:28]

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

[commit-bot: I haz the power, 2016-06-23 02:39:43]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-23 02:46:16]

Description was changed from

==========
Fix a bug in the implementation of DescheduleUntilFinishedCHROMIUM.

The logic for checking whether an executor was idle was only checking if all of
the commands submitted had been processed. This always returns false for
DescheduleUntilFinishedCHROMIUM, since there are unprocessed commands.

I added the functions HasPollingWork() and PerformPollingWork() to
CommandExecutor to separate DescheduleUntilFinishedCHROMIUM work from idle work.

BUG=617249
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix a bug in the implementation of DescheduleUntilFinishedCHROMIUM.

The logic for checking whether an executor was idle was only checking if all of
the commands submitted had been processed. This always returns false for
DescheduleUntilFinishedCHROMIUM, since there are unprocessed commands.

I added the functions HasPollingWork() and PerformPollingWork() to
CommandExecutor to separate DescheduleUntilFinishedCHROMIUM work from idle work.

BUG=617249
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/63170daf2b1bc70388d39d8b3635058b8269e3ea
Cr-Commit-Position: refs/heads/master@{#401525}
==========

[commit-bot: I haz the power, 2016-06-23 02:46:18]

Patchset 5 (id:??) landed as
https://crrev.com/63170daf2b1bc70388d39d8b3635058b8269e3ea
Cr-Commit-Position: refs/heads/master@{#401525}