content/gpu: Simplify stub scheduling.

content/gpu: Simplify stub scheduling.

Get rid of the reschedule timeout in GpuScheduler. Also let the channel
know which stub is scheduled/descheduled so that it can map it to the
correct stream.

BUG=514813
Committed: https://crrev.com/baab86ff3d07870373266e101b0cf8916d4af188
Cr-Commit-Position: refs/heads/master@{#349788}

[sunnyps, 2015-09-16 21:24:17]

sunnyps@chromium.org changed reviewers:
+ jbauman@chromium.org, piman@chromium.org

[sunnyps, 2015-09-16 21:24:17]

PTAL

I've let the refcounting for schedule/unschedule remain but it's not being used
so I can remove it if you all think it's ok.

[jbauman, 2015-09-16 21:31:26]

lgtm, with one suggestion

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

https://codereview.chromium.org/1348363003/diff/1/content/common/gpu/gpu_comm...
content/common/gpu/gpu_command_buffer_stub.cc:919:
scheduler_->SetScheduled(false);
DCHECK(!waiting_for_sync_point_);

[piman, 2015-09-16 21:46:29]

I'm ok if we remove refcounting of unschedules, if we replace it by a DCHECK.

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

https://codereview.chromium.org/1348363003/diff/1/content/common/gpu/gpu_comm...
content/common/gpu/gpu_command_buffer_stub.cc:926: return false;
AddSyncPointCallback may call the callback synchronously, if the sync point was
already retired. While there is the IsSyncPointRetired test above, you can't
rely on its result if the sync point is retired on another thread.

https://codereview.chromium.org/1348363003/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gpu_scheduler.cc (left):

https://codereview.chromium.org/1348363003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gpu_scheduler.cc:153: #endif
What is the reason this is not needed any more? Is it because NPAPI is gone?
Not that I'm in love with this hack, but unless I'm confused, we still use the
browser's HWND, so we have synchronous win32 messages between this thread and
the browser's UI thread. Are we guaranteed cycles are gone?

[sunnyps, 2015-09-16 23:08:32]

PTAL

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

https://codereview.chromium.org/1348363003/diff/1/content/common/gpu/gpu_comm...
content/common/gpu/gpu_command_buffer_stub.cc:919:
scheduler_->SetScheduled(false);
On 2015/09/16 21:31:26, jbauman wrote:
> DCHECK(!waiting_for_sync_point_);

Done.

https://codereview.chromium.org/1348363003/diff/1/content/common/gpu/gpu_comm...
content/common/gpu/gpu_command_buffer_stub.cc:926: return false;
On 2015/09/16 21:46:29, piman (slow to review) wrote:
> AddSyncPointCallback may call the callback synchronously, if the sync point
was
> already retired. While there is the IsSyncPointRetired test above, you can't
> rely on its result if the sync point is retired on another thread.

Done.

https://codereview.chromium.org/1348363003/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/gpu_scheduler.cc (left):

https://codereview.chromium.org/1348363003/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/gpu_scheduler.cc:153: #endif
On 2015/09/16 21:46:29, piman (slow to review) wrote:
> What is the reason this is not needed any more? Is it because NPAPI is gone?
> Not that I'm in love with this hack, but unless I'm confused, we still use the
> browser's HWND, so we have synchronous win32 messages between this thread and
> the browser's UI thread. Are we guaranteed cycles are gone?

I do not know what everything else looked like when this code was added but:
1. SetScheduled is *only* called by GpuCommandBufferStub when it waits for a
sync point and that use case certainly doesn't need a reschedule timeout.
2. When this code was added it was for all windows versions - in fact the
original bug was a crash when closing the page while youtube was running on
windows 7. crbug.com/111514
3. After some time it was found that this code was inconvenient for debugging
because a DCHECK triggers if you pause in the debugger so it was made XP only
because no developers use XP. crrev.com/12315112

[jbauman, 2015-09-16 23:18:06]

On 2015/09/16 23:08:32, sunnyps wrote:
> PTAL
> 
>
https://codereview.chromium.org/1348363003/diff/1/content/common/gpu/gpu_comm...
> File content/common/gpu/gpu_command_buffer_stub.cc (right):
> 
>
https://codereview.chromium.org/1348363003/diff/1/content/common/gpu/gpu_comm...
> content/common/gpu/gpu_command_buffer_stub.cc:919:
> scheduler_->SetScheduled(false);
> On 2015/09/16 21:31:26, jbauman wrote:
> > DCHECK(!waiting_for_sync_point_);
> 
> Done.
> 
>
https://codereview.chromium.org/1348363003/diff/1/content/common/gpu/gpu_comm...
> content/common/gpu/gpu_command_buffer_stub.cc:926: return false;
> On 2015/09/16 21:46:29, piman (slow to review) wrote:
> > AddSyncPointCallback may call the callback synchronously, if the sync point
> was
> > already retired. While there is the IsSyncPointRetired test above, you can't
> > rely on its result if the sync point is retired on another thread.
> 
> Done.
> 
>
https://codereview.chromium.org/1348363003/diff/1/gpu/command_buffer/service/...
> File gpu/command_buffer/service/gpu_scheduler.cc (left):
> 
>
https://codereview.chromium.org/1348363003/diff/1/gpu/command_buffer/service/...
> gpu/command_buffer/service/gpu_scheduler.cc:153: #endif
> On 2015/09/16 21:46:29, piman (slow to review) wrote:
> > What is the reason this is not needed any more? Is it because NPAPI is gone?
> > Not that I'm in love with this hack, but unless I'm confused, we still use
the
> > browser's HWND, so we have synchronous win32 messages between this thread
and
> > the browser's UI thread. Are we guaranteed cycles are gone?
> 
> I do not know what everything else looked like when this code was added but:
> 1. SetScheduled is *only* called by GpuCommandBufferStub when it waits for a
> sync point and that use case certainly doesn't need a reschedule timeout.
> 2. When this code was added it was for all windows versions - in fact the
> original bug was a crash when closing the page while youtube was running on
> windows 7. crbug.com/111514
> 3. After some time it was found that this code was inconvenient for debugging
> because a DCHECK triggers if you pause in the debugger so it was made XP only
> because no developers use XP. crrev.com/12315112

In particular, at the time we used to send a message to the browser process UI
thread for it to resize the child window, deschedule the stub, then wait for the
ack before rescheduling. We never do that sort of thing anymore (on windows at
least), so it's not an issue.

[piman, 2015-09-16 23:21:09]

https://codereview.chromium.org/1348363003/diff/20001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1348363003/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:927: return
waiting_for_sync_point_;
return !waiting_for_sync_point_;

[sunnyps, 2015-09-16 23:38:42]

PTAL

Removed the refcounting for SetScheduled as well.

https://codereview.chromium.org/1348363003/diff/20001/content/common/gpu/gpu_...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1348363003/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_command_buffer_stub.cc:927: return
waiting_for_sync_point_;
On 2015/09/16 23:21:09, piman (slow to review) wrote:
> return !waiting_for_sync_point_;

Done.

[piman, 2015-09-16 23:48:43]

https://codereview.chromium.org/1348363003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_scheduler.cc (right):

https://codereview.chromium.org/1348363003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_scheduler.cc:32: unscheduled_count_(0),
You still need an initializer for scheduled_.

https://codereview.chromium.org/1348363003/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_scheduler.cc (right):

https://codereview.chromium.org/1348363003/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_scheduler.cc:43: return;
I don't think this should happen any more (i.e. you can replace with a DCHECK).
If it does, we would actually need to handle it at the GpuChannel level.

[sunnyps, 2015-09-17 00:04:53]

PTAL

https://codereview.chromium.org/1348363003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_scheduler.cc (right):

https://codereview.chromium.org/1348363003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_scheduler.cc:32: unscheduled_count_(0),
On 2015/09/16 23:48:42, piman (slow to review) wrote:
> You still need an initializer for scheduled_.

Done.

https://codereview.chromium.org/1348363003/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_scheduler.cc (right):

https://codereview.chromium.org/1348363003/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_scheduler.cc:43: return;
On 2015/09/16 23:48:42, piman (slow to review) wrote:
> I don't think this should happen any more (i.e. you can replace with a
DCHECK).
> If it does, we would actually need to handle it at the GpuChannel level.

Done.

[piman, 2015-09-17 00:13:01]

lgtm

[sunnyps, 2015-09-18 17:47:07]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2015-09-18 17:47:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbauman@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1348363003/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2015-09-18 17:48:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1348363003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1348363003/100001

[commit-bot: I haz the power, 2015-09-18 18:14:32]

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

[commit-bot: I haz the power, 2015-09-18 18:14:33]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[sunnyps, 2015-09-18 18:20:16]

+sky for mus/components/gles2 rubber stamp

[sunnyps, 2015-09-18 18:20:37]

sunnyps@chromium.org changed reviewers:
+ sky@chromium.org

[sunnyps, 2015-09-18 18:20:37]

actually +sky

[sky, 2015-09-18 20:15:25]

LGTM

[sunnyps, 2015-09-18 20:22:25]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2015-09-18 20:22:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbauman@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1348363003/#ps120001
(title: "fix android compile error")

[commit-bot: I haz the power, 2015-09-18 20:23:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1348363003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1348363003/120001

[commit-bot: I haz the power, 2015-09-18 20:41:38]

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

[commit-bot: I haz the power, 2015-09-18 20:41:38]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[sunnyps, 2015-09-18 20:48:00]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2015-09-18 20:48:01]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
jbauman@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1348363003/#ps140001
(title: "fix android compile error sigh")

[commit-bot: I haz the power, 2015-09-18 20:49:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1348363003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1348363003/140001

[commit-bot: I haz the power, 2015-09-18 22:34:17]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-09-18 22:34:43]

Patchset 8 (id:??) landed as
https://crrev.com/baab86ff3d07870373266e101b0cf8916d4af188
Cr-Commit-Position: refs/heads/master@{#349788}