Bug 79948 – [Chromium] Release accelerated Canvas2D GPU resources for backgrounded tabs

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Jeff Timanus]

Recent work has landed changes that purge the Ganesh texture cache when a page is backgrounded.  See crbug.com/114801.

The purge is not sufficient, because any rendering that takes place in backgrounded tabs will result in the Ganesh cache being re-populated.  The cache size needs to be set to zero for canvases residing in backgrounded tabs.

[Jeff Timanus]

Created attachment 129527 [details]
Patch

[Nat Duca]

Comment on attachment 129527 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129527&action=review

> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:877
> +    if (visible && m_grContext)

Can't we set this when we get the nonzero allocation?

That this is being called here indicates to me a problem with our strategy when a tab activates...

[Jeff Timanus]

Comment on attachment 129527 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129527&action=review

>> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:877
>> +    if (visible && m_grContext)
> 
> Can't we set this when we get the nonzero allocation?
> 
> That this is being called here indicates to me a problem with our strategy when a tab activates...

I took this approach, as recommended by mmocny@.  

You're suggesting relying entirely on the memory allocation changed callback mechanism?  From an API viewpoint, I think that would work.  Michal, is the extension machinery sufficiently complete to be used for renderer visibility?

[Michal Mocny]

Jeff, yes I originally recommended that approach so that there would be no delay in waiting for the next allocation.

However, the new direction for all of these gpu resource related changes is to not directly use visibility hooks (sorry for misdirecting you there), but to instead use the new memory allocation hints.

The extension is sufficiently complete, but there will always be some chance of delay (and currently a delay is certain).  When switching back to a tab with a canvas that was hibernated, I would expect a temporary visible blip in performance since the cache was purged, and I am curious to know if there is any user visible difference between upping the cache limit here vs after the next memory allocation is received.  I expect not, but would you mind comparing?

[Nat Duca]

My sense is if the latency of the callback proves to be an issue, I wonder:
a) if we can add a sync call from the render process to force-pull an allocation to the renderer
b) if we cant do (a), then maybe we can modify the cache policy to go above its limit for 1 frame or some similar heuristic...

[Michal Mocny]

When would the render process make this sync pull? on visibility changes?  That is interesting for several reasons...

Still, my hope/assumption is that the delay won't prove to be an issue, and am curious of the results.

Also, the delay will be lower once visibility is sent from browser->gpu and maybe it isn't worth engineering big workarounds until we do that.

[Nat Duca]

Actually, thats a great point Michal. I'm comfortable with just doing this in a callback and focusing on routing the visibilility state directly to the GPU process... Jeff, are we making any sense? :)

[Jeff Timanus]

(In reply to comment #7)
> Actually, thats a great point Michal. I'm comfortable with just doing this in a callback and focusing on routing the visibilility state directly to the GPU process... Jeff, are we making any sense? :)

Yes, you guys are making sense.

Like Michal, I do wonder about when the renderer would make the explicit pull to the renderer.  Wouldn't this revert to the same philosophy as the patch I uploaded?

I haven't yet had a moment to check the performance, but I'll get back to you guys on the delay when switching back to a tab.  (With a trace.)

[Jeff Timanus]

(In reply to comment #8)
> (In reply to comment #7)
> > Actually, thats a great point Michal. I'm comfortable with just doing this in a callback and focusing on routing the visibilility state directly to the GPU process... Jeff, are we making any sense? :)
> 
> Yes, you guys are making sense.
> 
> Like Michal, I do wonder about when the renderer would make the explicit pull to the renderer.  Wouldn't this revert to the same philosophy as the patch I uploaded?
> 
> I haven't yet had a moment to check the performance, but I'll get back to you guys on the delay when switching back to a tab.  (With a trace.)

I'm looking into capturing the performance difference between the two approaches, and I think there's a fundamental flaw in patch #1.  GraphicsContext3DPrivate::setVisibilityCHROMIUM is never being called with a non-null GrContext.  

Is the GraphicsContext3DPrivate instance that is used to construct the memory allocation callback different from the instance used for tab visibility?

This doesn't quite make sense to me.  Can others comment?

[Michal Mocny]

Jeff, yes this makes sense:
- It is the compositor context which received the setVisibility call only
- It is for the ganesh shared graphics context which we set up the memory allocation callback you are referring to.

There may be a way to reference the shared graphics context from that visibility call, but its becoming quite apparent that using visibility hooks is not the right way to do this.

Sorry to have misled you down this path.

[Jeff Timanus]

Created attachment 130480 [details]
Patch

[Jeff Timanus]

Created attachment 130626 [details]
Performance tracing file.

Attached is a trace demonstrating the GPU resource purging behaviour of the memory allocation changed callback.

Special events have been added to indicate the timing of the purge, and cache resize.
Process 23843 
7905.94ms - Purged Event
10091.836 ms - Cache Resize Event

The trace indicates that the system is working properly.  

The cache resize happens after a single frame is drawn.  I believe this may be the operation of drawing the previously displayed tab contents, and not an actual render of the page.

[Jeff Timanus]

(In reply to comment #11)
> Created an attachment (id=130480) [details]
> Patch

Can a reviewer comment on this patch?

It's very simple, and the attached trace indicates that (I think) it's working correctly.

[Nat Duca]

Comment on attachment 130480 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130480&action=review

Unofficially looks good to me. Ping senorblanco or jamesr for official.

> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:179
> +        if (m_context->grContext()) {

In general, prefer
   if(!x)
     return
   y

over
   if (x)  {
     y
   }

Where y is long.

So, if (!m_context=>grContext) return

[Jeff Timanus]

Created attachment 130862 [details]
Patch

[Jeff Timanus]

Comment on attachment 130480 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130480&action=review

>> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:179
>> +        if (m_context->grContext()) {
> 
> In general, prefer
>    if(!x)
>      return
>    y
> 
> over
>    if (x)  {
>      y
>    }
> 
> Where y is long.
> 
> So, if (!m_context=>grContext) return

Done.

[Jeff Timanus]

Stephen, can you please CQ+ this change?

[Stephen White]

Comment on attachment 130862 [details]
Patch

Looks good.  r=me

[WebKit Review Bot]

Comment on attachment 130862 [details]
Patch

Clearing flags on attachment: 130862

Committed r110245: <http://trac.webkit.org/changeset/110245>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.