Summary: | [Chromium] Release accelerated Canvas2D GPU resources for backgrounded tabs | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jeff Timanus <twiz> | ||||||||||
Component: | Canvas | Assignee: | Jeff Timanus <twiz> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bsalomon, mdelaney7, mmocny, nduca, senorblanco, twiz, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Jeff Timanus
2012-02-29 14:26:03 PST
Created attachment 129527 [details]
Patch
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... 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? 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? 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... 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. 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? :) (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.) (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? 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. Created attachment 130480 [details]
Patch
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.
(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. 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 Created attachment 130862 [details]
Patch
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. Stephen, can you please CQ+ this change? Comment on attachment 130862 [details]
Patch
Looks good. r=me
Comment on attachment 130862 [details] Patch Clearing flags on attachment: 130862 Committed r110245: <http://trac.webkit.org/changeset/110245> All reviewed patches have been landed. Closing bug. |