Bug 87996 - [chromium] Set default memory allocation limit bytes when GL_CHROMIUM_gpu_memory_manager is not supported by graphics context.
Summary: [chromium] Set default memory allocation limit bytes when GL_CHROMIUM_gpu_mem...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michal Mocny
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-31 11:36 PDT by Michal Mocny
Modified: 2012-06-05 21:45 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.15 KB, patch)
2012-05-31 11:38 PDT, Michal Mocny
no flags Details | Formatted Diff | Diff
Patch (5.19 KB, patch)
2012-06-05 07:56 PDT, Michal Mocny
no flags Details | Formatted Diff | Diff
Patch (9.11 KB, patch)
2012-06-05 08:42 PDT, Michal Mocny
no flags Details | Formatted Diff | Diff
Patch (10.26 KB, patch)
2012-06-05 09:48 PDT, Michal Mocny
no flags Details | Formatted Diff | Diff
Patch (10.19 KB, patch)
2012-06-05 10:41 PDT, Michal Mocny
no flags Details | Formatted Diff | Diff
Patch (9.76 KB, patch)
2012-06-05 11:20 PDT, Michal Mocny
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michal Mocny 2012-05-31 11:36:59 PDT
[chromium] Set default memory allocation limit bytes when GL_CHROMIUM_gpu_memory_manager is not supported by graphics context.
Comment 1 Michal Mocny 2012-05-31 11:38:49 PDT
Created attachment 145121 [details]
Patch
Comment 2 Michal Mocny 2012-05-31 11:42:03 PDT
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.
Comment 3 Nat Duca 2012-06-04 11:17:20 PDT
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
Comment 4 Michal Mocny 2012-06-04 11:48:28 PDT
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.
Comment 5 Nat Duca 2012-06-04 19:32:35 PDT
Have the single threaded memory allocation callback call through an apapter class that DebugSetsImplThread.
Comment 6 Michal Mocny 2012-06-05 07:56:22 PDT
Created attachment 145795 [details]
Patch
Comment 7 Dana Jansens 2012-06-05 08:10:13 PDT
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?
Comment 8 Dana Jansens 2012-06-05 08:11:02 PDT
(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
Comment 9 Michal Mocny 2012-06-05 08:42:29 PDT
Created attachment 145807 [details]
Patch
Comment 10 Michal Mocny 2012-06-05 08:43:56 PDT
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.
Comment 11 Michal Mocny 2012-06-05 09:48:21 PDT
Created attachment 145824 [details]
Patch
Comment 12 Michal Mocny 2012-06-05 09:49:32 PDT
Unittests were failing because LRCTest was not initializing WebCompositor, but thats all fixed up now.
Comment 13 Dana Jansens 2012-06-05 09:56:01 PDT
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
Comment 14 Michal Mocny 2012-06-05 10:41:23 PDT
Created attachment 145835 [details]
Patch
Comment 15 Michal Mocny 2012-06-05 10:41:57 PDT
Comment on attachment 145835 [details]
Patch

Rebasing.
Comment 16 Nat Duca 2012-06-05 10:51:15 PDT
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
Comment 17 Michal Mocny 2012-06-05 10:57:54 PDT
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.
Comment 18 James Robinson 2012-06-05 11:14:42 PDT
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.
Comment 19 Michal Mocny 2012-06-05 11:20:01 PDT
Created attachment 145844 [details]
Patch
Comment 20 James Robinson 2012-06-05 11:22:56 PDT
(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?
Comment 21 Michal Mocny 2012-06-05 11:27:52 PDT
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.
Comment 22 Michal Mocny 2012-06-05 11:29:36 PDT
(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.
Comment 23 James Robinson 2012-06-05 13:03:40 PDT
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.
Comment 24 WebKit Review Bot 2012-06-05 21:45:11 PDT
Comment on attachment 145844 [details]
Patch

Clearing flags on attachment: 145844

Committed r119554: <http://trac.webkit.org/changeset/119554>
Comment 25 WebKit Review Bot 2012-06-05 21:45:17 PDT
All reviewed patches have been landed.  Closing bug.