Bug 87996 – [chromium] Set default memory allocation limit bytes when GL_CHROMIUM_gpu_memory_manager is not supported by graphics context.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Michal Mocny]

[chromium] Set default memory allocation limit bytes when GL_CHROMIUM_gpu_memory_manager is not supported by graphics context.

[Michal Mocny]

Created attachment 145121 [details]
Patch

[Michal Mocny]

Nat, I've gotten bit by the "graphics context callback are called on the wrong thread" issue here again.

I got around the issue by introducing a new DebugScopedSetMainThreadFromEither which will set main thread and reset to whatever thread it was on during initialization, instead of forcing reset to impl.

The real solution would be to make graphics context callbacks run on impl.  An alternative workaround would be to remove all ASSERT(is*Thread) from the allocation plumbing.  Please advise, thanks.

[Nat Duca]

Comment on attachment 145121 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:318
> +        m_client->setContentsMemoryAllocationLimitBytes(TextureManager::highLimitBytes(viewportSize()));

How about calling postSetContentsMemoryAllocationBlahballbahbah? Then you can assert impl thread...

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.h:144
>  

This smells

[Michal Mocny]

Comment on attachment 145121 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:318
>> +        m_client->setContentsMemoryAllocationLimitBytes(TextureManager::highLimitBytes(viewportSize()));
> 
> How about calling postSetContentsMemoryAllocationBlahballbahbah? Then you can assert impl thread...

This is possible, but it seems so ugly that I want to make sure we are on the same page:

- I would need to add two different postSetContentsMemoryAllocationBlahballbahbah to CCLayerTreeHostImpl
-- postSetContentsMemoryAllocationOnImpl, used here (initialize)
-- postSetContentsMemoryAllocationOnMainIfSingleThreadedOtherwiseOnImpl, used by wgc memory allocation callback
- CCLTHImpl would forward over to two equivalent versions in CCProxy
- Then finally CCSingleThreadProxy::postSetContentsMemoryAllocationOnMainIfSingleThreadedOtherwiseOnImpl will know its on the wrong thread, and can DebugScopedImplThread

Another possible alternative is that LRC callback could test if we are running with threaded compositing and DebugScopedImplThread if not.  This is a lot cleaner, but I am not sure if it is possible.

[Nat Duca]

Have the single threaded memory allocation callback call through an apapter class that DebugSetsImplThread.

[Michal Mocny]

Created attachment 145795 [details]
Patch

[Dana Jansens]

Comment on attachment 145795 [details]
Patch

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

does it make sense to have a unit test that verifies the allocation is non-zero in this scenario?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:208
> +        if (!CCProxy::hasImplThread())
> +            onGpuMemoryAllocationChangedOnMain(allocation);
> +        else
> +            onGpuMemoryAllocationChangedOnImpl(allocation);

Why not just put a DebugScopedSetImplThread here and avoid the 2 extra functions?

[Dana Jansens]

(In reply to comment #7)
> Why not just put a DebugScopedSetImplThread here and avoid the 2 extra functions?

Oh I see, previous comments.. heh :) Yay bikesheds

[Michal Mocny]

Created attachment 145807 [details]
Patch

[Michal Mocny]

I've changed this to use two adaptors so that we don't have to test the if statement inside the callback every time, just once during initialization.

I've also added the unit test as Dana suggested, but unit tests are currently failing since some of the thread assertions have changed.  Will fix that and post update.

[Michal Mocny]

Created attachment 145824 [details]
Patch

[Michal Mocny]

Unittests were failing because LRCTest was not initializing WebCompositor, but thats all fixed up now.

[Dana Jansens]

Comment on attachment 145824 [details]
Patch

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

Looks like it needs a rebase, I like the separate SingleThread class, seems cleaner than the extra methods in the last patch to me.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:200
> +

nit: extra whitespace

[Michal Mocny]

Created attachment 145835 [details]
Patch

[Michal Mocny]

Comment on attachment 145835 [details]
Patch

Rebasing.

[Nat Duca]

Comment on attachment 145835 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:67
> +#include "cc/CCSingleThreadProxy.h"

Why this and not CCPRoxy?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:221
> +class LayerRendererGpuMemoryAllocationChangedCallbackAdapterSingleThread : public LayerRendererGpuMemoryAllocationChangedCallbackAdapter {

Why not just make the existing adapter have a bool saying "m_bindToImplThread" that you set in the single threaded case?

> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:129
> +        WebKit::WebCompositor::initialize(0);

Eep, whats this?

> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:339
> +class NonGpuMemoryManagingFakeContext : public FakeWebGraphicsContext3D {

ContextThatDoesNotSupportMemoryManagmentExtensions

[Michal Mocny]

Comment on attachment 145835 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:67
>> +#include "cc/CCSingleThreadProxy.h"
> 
> Why this and not CCPRoxy?

This was needed for ScopedSet*Thread.  This is not the only place where CCSingleThreadProxy.h is included just for those, should I move them into CCProxy.h?

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:221
>> +class LayerRendererGpuMemoryAllocationChangedCallbackAdapterSingleThread : public LayerRendererGpuMemoryAllocationChangedCallbackAdapter {
> 
> Why not just make the existing adapter have a bool saying "m_bindToImplThread" that you set in the single threaded case?

Sure, I'll do that.

>> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:129
>> +        WebKit::WebCompositor::initialize(0);
> 
> Eep, whats this?

This initializes the main/impl threads and calls CCProxy::setMainThread (among other things) which you need before you can ASSERT(CCProxy::isMainThread()) etc.

I copied this from the behaviour of other unittests: CCLayerTreeHostTest.cpp, Canvas2DLayerChromiumTest.cpp, etc etc

>> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:339
>> +class NonGpuMemoryManagingFakeContext : public FakeWebGraphicsContext3D {
> 
> ContextThatDoesNotSupportMemoryManagmentExtensions

sure.

[James Robinson]

Comment on attachment 145835 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:67
>> +#include "cc/CCSingleThreadProxy.h"
> 
> Why this and not CCPRoxy?

Woah, why does LRC need to know about the proxy at all?  LayerRendererChromium should call out to the embedder via CCRendererClient and the client implementation (normally CCLayerTreeHostImpl) should be dealing with any proxy interactions

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:347
> +        if (CCProxy::hasImplThread())

LayerRendererChromium is only accessed from the thread owning the GC3D, so it never should have to worry about whether it's in threaded mode or not.  Its client (CCLTHI) may have to make some threading decisions, but that's not LRC's concern.

[Michal Mocny]

Created attachment 145844 [details]
Patch

[James Robinson]

(In reply to comment #2)
> Nat, I've gotten bit by the "graphics context callback are called on the wrong thread" issue here again.
> 

Mind expanding on what this issue is?  Is it just that our debug thread tracking isn't set to "impl" when running GC3D callbacks, or are we actually running code on the wrong thread?  In which mode do you have an issue?

[Michal Mocny]

Comment on attachment 145835 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:67
>>>> +#include "cc/CCSingleThreadProxy.h"
>>> 
>>> Why this and not CCPRoxy?
>> 
>> This was needed for ScopedSet*Thread.  This is not the only place where CCSingleThreadProxy.h is included just for those, should I move them into CCProxy.h?
> 
> Woah, why does LRC need to know about the proxy at all?  LayerRendererChromium should call out to the embedder via CCRendererClient and the client implementation (normally CCLayerTreeHostImpl) should be dealing with any proxy interactions

CCProxy has been used in LRC for a while now.  Grepping the file, its only used to make a few decisions depending on CCProxy::hasImplThread and assert which thread we are on.

[Michal Mocny]

(In reply to comment #20)
> (In reply to comment #2)
> > Nat, I've gotten bit by the "graphics context callback are called on the wrong thread" issue here again.
> > 
> 
> Mind expanding on what this issue is?  Is it just that our debug thread tracking isn't set to "impl" when running GC3D callbacks, or are we actually running code on the wrong thread?  In which mode do you have an issue?

The former, our debug thread tracking isn't set to impl when running the callback.

[James Robinson]

Comment on attachment 145844 [details]
Patch

R=me. It's a bit sad to me that these ASSERT()s are getting in the way - they're supposed to just be helpful - but this seems like a pretty reasonable way to deal with them.

[WebKit Review Bot]

Comment on attachment 145844 [details]
Patch

Clearing flags on attachment: 145844

Committed r119554: <http://trac.webkit.org/changeset/119554>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.