Bug 78265 – [chromium] Purge Skia's GPU texture cache using GL_CHROMIUM_gpu_memory_manager

• Component: New Bugs
• Status: RESOLVED FIXED

[Michal Mocny]

[chromium][not for review] Purge Skia's GPU texture cache using GL_CHROMIUM_gpu_memory_manager

[Michal Mocny]

Created attachment 126346 [details]
Patch

[Michal Mocny]

*** Bug 78264 has been marked as a duplicate of this bug. ***

[Nat Duca]

Comment on attachment 126346 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:154
> +    layerTreeHost()->client()->addAffectingContext(m_context);

Ok, this looks good. But I've had a bit of a brainwave on how to make this cleaner.

CCLayerTreeHost tracks a bool saying "hasAccelerated2DCanvasLayer"

It gets cleared to false in the CCLTh::willCommit function.

This code just says layerTreeHost->setHasAccelerated2DCanvas(). That function should set the flag to true.

The post commit hook should pass this bool up to the client. Until the post commit hook, nothing should be sent.

In fact, the willCommit hook doesnt' even need to be sent up to the client. Just the didCommit call.

All this "affecting contexts" crap in webview goes away. Remember, there is only 1 CCLayerTreeHost for 1 webviewimpl, and thus, only one compositor context. All you need to know up at webviewimpl is whether or not the webviewimpl has an accelerated canvas2d. If it does, then the shared graphics context affects the webviewimpl's surface_id.

Remember, accessing the compositor's graphics context is always unsafe on the main thread. So, WebViewImpl should, in its createCompositorGraphicsContext3D() function, cache the surfaceID of the compositor context before returning the pointer. Just have int m_compositorContextSurfaceId on the WebViewIMpl class. IN the didCommitHook, just pass that surface id down to the shared graphics context code along with the bool indicating whether the webview has a 2d canvas or not. 

The actual shared surface manipulation code should be simple:
  - if the bool is true, add the surface id to the shared surface, if not already added previously
  - if the bool is false, remove the surface id from the shared surface, if added previously

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:63
> +    virtual void didCommit() = 0;

This should all go away acept for the didCommit function. That function should be alphebetically ordered wrt the other functions.

It should either take a bool indicating whether we had an accelerated 2d canvas, or we should add a bool accessor for that value to the CCLayerTreeHost. I dont care which way we go.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:139
> +    void willCommit() { m_client->willCommit(); }

this should clear the new m_hasAccelerated2DCanvas flag. And, please name it consistently. 10 lines up from here, there's a beginCommitOnImplThread function. Ths should be beginCommit.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:140
> +    void didCommit() { m_client->didCommit(); }

how does this differ from comitComplete ? Isn't it the same thing?

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:39
> +#if USE(SKIA)

These might need to be USE(SKIA) && PLATFORM(CHROMIUM), check with @twiz or @bsolomon.

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:49
> +        if (allocation.gpuResourceSizeInBytes && m_context->grContext())

Er, if the resourcesize in bytes is nonzero, free? Isn't this backwards?

> Source/WebKit/chromium/src/WebViewImpl.cpp:423
> +    WTF::HashSet<WebCore::GraphicsContext3D*>::iterator affectingContextsIterator(m_affectingGraphicsContexts.begin());

this should all go away, see my notes below.

> Source/WebKit/chromium/src/WebViewImpl.cpp:427
> +        extensions3DChromium->removeAffectedSurfaceCHROMIUM(m_client->surfaceId());

This is all backward. As written, you're telling the compositor context that it affects itself. That is very wrong.

Rather, this should be saying "hey, shared graphics context, you are no longer affecting the COMPOSITOR context's surface." IN code, that looks something like

  SharedSurface::

Except you obviously shouldln't remove it if you've not added it yet.

And, you need to protect against the extension not being present. As written, you'd call this even when the extension isn't there.

AND, more concerningly, you need to deal with the separation of concerns between the WebViewImpl, which is just a go-between here, and the SharedGraphicsContext, which really is the one that is talking to the graphics context. Ideally, what you'd do is make a method pair on the SharedGraphicsContext called "Add/RemoveAffectedSurface(int surfaceID)." WebViewImpl should call those when the didCommit changes.

You need to call removeAffectedSurfaces when we go from hardware mode back to software mode,also. See didActiveAcceleratedCompositing or something like that in this file. When it goes to false, you need to remove the webview's surface_id from the sharedgraphicscontext.

> Source/WebKit/chromium/src/WebViewImpl.cpp:3131
> +{

See my notes above. WebViewIMpl should track
 bool m_surfaceID <- set and updated in createCompositorGraphicsContext
 bool m_hasRegisteredAsAffectingSharedGraphicsContext <- set to true when we add/remove
This function should check the m_hasRegistered and if that is != the new state, call the right add/remove on the SharedGraphicsContext.

The sharedgraphicscontext should track whether the underlying context supports the extension and if it does, call the right wgc3d methods.

> Source/WebKit/chromium/src/WebViewImpl.h:250
> +    virtual void addAffectingContext(WebCore::GraphicsContext3D*);

If you do keep any methods or variables that use the word affect, please use Affected instead of Affecting so that we are consistent with WGC3D's terms.

[Michal Mocny]

*** Bug 77573 has been marked as a duplicate of this bug. ***

[Michal Mocny]

Moving the last comment by twiz@ from the original patch (77573), so we do not forget:

==========

From an offline discussion with bsalomon, we decided that it would be beneficial if this patch set the texture cache size to 0 upon background notification.

If the cache is only purged to 0 when backgrounded, then the GPU memory footprint could still increase as non-raf rendering events are fired.

This change will require the GL_CHROMIUM_gpu_memory_manager to provide foregrounded notifications as well as backgrounded.

[Michal Mocny]

Created attachment 126814 [details]
Patch

[Michal Mocny]

Comment on attachment 126346 [details]
Patch

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

Removed all the affected/affecting context tracking since that extension was removed.

>> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:39
>> +#if USE(SKIA)
> 
> These might need to be USE(SKIA) && PLATFORM(CHROMIUM), check with @twiz or @bsolomon.

@twiz provided this code in his original patch, but I will confirm with him.

>> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:49
>> +        if (allocation.gpuResourceSizeInBytes && m_context->grContext())
> 
> Er, if the resourcesize in bytes is nonzero, free? Isn't this backwards?

Good catch, there was an "== 0" which was a style violation when uploading and I patched it incorrectly

>> Source/WebKit/chromium/src/WebViewImpl.cpp:427
>> +        extensions3DChromium->removeAffectedSurfaceCHROMIUM(m_client->surfaceId());
> 
> This is all backward. As written, you're telling the compositor context that it affects itself. That is very wrong.
> 
> Rather, this should be saying "hey, shared graphics context, you are no longer affecting the COMPOSITOR context's surface." IN code, that looks something like
> 
>   SharedSurface::
> 
> Except you obviously shouldln't remove it if you've not added it yet.
> 
> And, you need to protect against the extension not being present. As written, you'd call this even when the extension isn't there.
> 
> AND, more concerningly, you need to deal with the separation of concerns between the WebViewImpl, which is just a go-between here, and the SharedGraphicsContext, which really is the one that is talking to the graphics context. Ideally, what you'd do is make a method pair on the SharedGraphicsContext called "Add/RemoveAffectedSurface(int surfaceID)." WebViewImpl should call those when the didCommit changes.
> 
> You need to call removeAffectedSurfaces when we go from hardware mode back to software mode,also. See didActiveAcceleratedCompositing or something like that in this file. When it goes to false, you need to remove the webview's surface_id from the sharedgraphicscontext.

Since all of this was removed, I won't go into detail, but it was actually doing what you are describing.  The naming and relationship is confusing and I had pestered @twiz about it too when I first got his patch, but basically the WebViewImpl accepts a list of non-compositor-context (shared contexts in this case) which affect it (thus named Affect-ing-), and later tells them to add/remove _its_ surface id).  Anyway, moot point now.

[WebKit Review Bot]

Comment on attachment 126814 [details]
Patch

Attachment 126814 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11505949

[Nat Duca]

Comment on attachment 126814 [details]
Patch

LGTM. Get an official review from @kbr or have @twiz get you to get a review via skia reviewers.

[Stephen White]

Comment on attachment 126814 [details]
Patch

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

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:38
> +namespace {

Seems a bit weird to have an unnamed namespace inside a named one.  Did you perhaps want this outside the WebCore namespace above?

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:80
> +#if USE(SKIA)
> +    static bool memoryAllocationCallbackInititalized = false;
> +    if (!memoryAllocationCallbackInititalized && context) {
> +        memoryAllocationCallbackInititalized = true;
> +        Extensions3DChromium* extensions3DChromium = static_cast<Extensions3DChromium*>(context->getExtensions());
> +        if (extensions3DChromium->supports("GL_CHROMIUM_gpu_memory_manager"))
> +            extensions3DChromium->setGpuMemoryAllocationChangedCallbackCHROMIUM(adoptPtr(new GrMemoryAllocationChangedCallback(context)));
> +    }
> +#endif

Since the GrContext is actually owned by the GraphicsContext3DPrivate, I'd prefer that this callback was also owned by GraphicsContext3DPrivate as well.  This would future-proof it against use of the GrContext by other users of GraphicsContext3D (e.g., when I add a new context to fix https://bugs.webkit.org/show_bug.cgi?id=78139).

[Michal Mocny]

Created attachment 127638 [details]
Patch

[Michal Mocny]

Comment on attachment 126814 [details]
Patch

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

>> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:80
>> +#endif
> 
> Since the GrContext is actually owned by the GraphicsContext3DPrivate, I'd prefer that this callback was also owned by GraphicsContext3DPrivate as well.  This would future-proof it against use of the GrContext by other users of GraphicsContext3D (e.g., when I add a new context to fix https://bugs.webkit.org/show_bug.cgi?id=78139).

Done. This is an excellent suggestion, it also clean things up a bit.  I hope my change is what you had in mind.

[Stephen White]

Comment on attachment 127638 [details]
Patch

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

> Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:58
> +#if USE(SKIA)
> +class GrMemoryAllocationChangedCallback;
> +#endif

Does this still need to be in the header file?  Or can it be in the .cpp as well?

[Michal Mocny]

Created attachment 127644 [details]
Patch

[Michal Mocny]

Comment on attachment 127638 [details]
Patch

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

>> Source/WebKit/chromium/src/GraphicsContext3DPrivate.h:58
>> +#endif
> 
> Does this still need to be in the header file?  Or can it be in the .cpp as well?

It isn't.  Thanks.

[Stephen White]

Comment on attachment 127644 [details]
Patch

Looks great.  Thanks for the changes.  r=me

[WebKit Review Bot]

Comment on attachment 127644 [details]
Patch

Rejecting attachment 127644 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
urce/WebKit/chromium/third_party/skia/third_party/glu --revision 3230 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
45>At revision 3230.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/11629112

[Michal Mocny]

Created attachment 128788 [details]
Patch

[WebKit Review Bot]

Comment on attachment 128788 [details]
Patch

Clearing flags on attachment: 128788

Committed r108840: <http://trac.webkit.org/changeset/108840>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.