Retry gpu compositing when fallback is not true.

Retry gpu compositing when fallback is not true.

If the GPU process goes down, and the renderer tries to recreate its
context too soon, it will fail. We should retry using GPU compositing
until fallback is given as true from the compositor to allow fallback
into software compositing.

R=jbauman@chromium.org, piman@chromium.org
BUG=356453, 327220
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259668

[jbauman, 2014-03-26 17:17:25]

https://codereview.chromium.org/212103005/diff/1/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/212103005/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:938: if (!context_provider.get() && !fallback)
{
We should check for use_software here, because if gpu compositing is disabled on
the command line then creating a context will never succeed and we don't want to
wait for multiple retries.

https://codereview.chromium.org/212103005/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:940: return scoped_ptr<cc::OutputSurface>();
Does this always cause it to retry, even with the single-threaded compositor?

[danakj, 2014-03-26 17:21:21]

https://codereview.chromium.org/212103005/diff/1/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/212103005/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:938: if (!context_provider.get() && !fallback)
{
On 2014/03/26 17:17:26, jbauman wrote:
> We should check for use_software here, because if gpu compositing is disabled
on
> the command line then creating a context will never succeed and we don't want
to
> wait for multiple retries.

Oh, that's what I was going for but didn't capture, ya.

https://codereview.chromium.org/212103005/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:940: return scoped_ptr<cc::OutputSurface>();
On 2014/03/26 17:17:26, jbauman wrote:
> Does this always cause it to retry, even with the single-threaded compositor?

Yes.
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

[danakj, 2014-03-26 17:22:39]

PTAL

https://codereview.chromium.org/212103005/diff/1/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/212103005/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:938: if (!context_provider.get() && !fallback)
{
On 2014/03/26 17:21:22, danakj wrote:
> On 2014/03/26 17:17:26, jbauman wrote:
> > We should check for use_software here, because if gpu compositing is
disabled
> on
> > the command line then creating a context will never succeed and we don't
want
> to
> > wait for multiple retries.
> 
> Oh, that's what I was going for but didn't capture, ya.

Done.

[jbauman, 2014-03-26 17:30:20]

lgtm

[danakj, 2014-03-26 17:34:00]

piman PTAL for OWNERS

[piman, 2014-03-26 18:17:12]

LGTM to unblock, but, do you know why the renderer fails to create its context?
We used to block the reply in the GpuMessageFilter to wait until the UI has
recreated its context, to make sure both the UI and the renderer talk to the
same GPU process. Is that not happening any more?

I worry that the "try 3 times" logic will paper over the race, but across
hundreds of millions of users, some will run into the case where it's not
enough.

[danakj, 2014-03-26 18:24:13]

I guess I'll land this for the GPU bots though to green up.

[danakj, 2014-03-26 18:24:20]

The CQ bit was checked by danakj@chromium.org

[danakj, 2014-03-26 18:24:23]

On Wed, Mar 26, 2014 at 2:17 PM, <piman@chromium.org> wrote:

> LGTM to unblock, but, do you know why the renderer fails to create its
> context?
> We used to block the reply in the GpuMessageFilter to wait until the UI has
> recreated its context, to make sure both the UI and the renderer talk to
> the
> same GPU process. Is that not happening any more?
>

Thanks, I always forget how that's supposed to work since I've never
found/read the code before. I'll go poke around and try figure out.


>
> I worry that the "try 3 times" logic will paper over the race, but across
> hundreds of millions of users, some will run into the case where it's not
> enough.
>
> https://codereview.chromium.org/212103005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-03-26 18:25:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/212103005/80001

[danakj, 2014-03-26 18:54:29]

On 2014/03/26 18:17:12, piman wrote:
> LGTM to unblock, but, do you know why the renderer fails to create its
context?
> We used to block the reply in the GpuMessageFilter to wait until the UI has
> recreated its context, to make sure both the UI and the renderer talk to the
> same GPU process. Is that not happening any more?

What happens is that after chrome://gpucrash, the next EstablishGpuChannel is
returning channel_handle.socket.fd = -1, and channel_handle.name = "". So I
guess it is not blocking.

What looks like another renderer trying the same thing, does work correctly
shortly after in the log.

Going deeper.

[danakj, 2014-03-26 19:24:59]

On 2014/03/26 18:54:29, danakj wrote:
> On 2014/03/26 18:17:12, piman wrote:
> > LGTM to unblock, but, do you know why the renderer fails to create its
> context?
> > We used to block the reply in the GpuMessageFilter to wait until the UI has
> > recreated its context, to make sure both the UI and the renderer talk to the
> > same GPU process. Is that not happening any more?
> 
> What happens is that after chrome://gpucrash, the next EstablishGpuChannel is
> returning channel_handle.socket.fd = -1, and channel_handle.name = "". So I
> guess it is not blocking.
> 
> What looks like another renderer trying the same thing, does work correctly
> shortly after in the log.
> 
> Going deeper.

We go to GpuMessageFilter::OnEstablishGpuChannel.
It gets a host from GpuProcessHost::FromID. So the host is valid at that point.
We call into GpuProcessHost::EstablishGpuChannel.

We send a GpuMsg_EstablishChannel successfully.

Then we hit GpuProcessHost::OnProcessCrashed, which does
GpuProcessHost::SendOutstandingReplies, which sends back failed channels to the
renderer.

Any requests that come in after the OnProcessCrashed will succeed.

[commit-bot: I haz the power, 2014-03-26 19:59:44]

Change committed as 259668

[danakj, 2014-03-26 23:08:24]

On 2014/03/26 19:24:59, danakj wrote:
> On 2014/03/26 18:54:29, danakj wrote:
> > On 2014/03/26 18:17:12, piman wrote:
> > > LGTM to unblock, but, do you know why the renderer fails to create its
> > context?
> > > We used to block the reply in the GpuMessageFilter to wait until the UI
has
> > > recreated its context, to make sure both the UI and the renderer talk to
the
> > > same GPU process. Is that not happening any more?
> > 
> > What happens is that after chrome://gpucrash, the next EstablishGpuChannel
is
> > returning channel_handle.socket.fd = -1, and channel_handle.name = "". So I
> > guess it is not blocking.
> > 
> > What looks like another renderer trying the same thing, does work correctly
> > shortly after in the log.
> > 
> > Going deeper.
> 
> We go to GpuMessageFilter::OnEstablishGpuChannel.
> It gets a host from GpuProcessHost::FromID. So the host is valid at that
point.
> We call into GpuProcessHost::EstablishGpuChannel.
> 
> We send a GpuMsg_EstablishChannel successfully.
> 
> Then we hit GpuProcessHost::OnProcessCrashed, which does
> GpuProcessHost::SendOutstandingReplies, which sends back failed channels to
the
> renderer.
> 
> Any requests that come in after the OnProcessCrashed will succeed.

Analysis/discussion of how to improve this situation moved to here:
https://code.google.com/p/chromium/issues/detail?id=356876