[Webkit-unassigned] [Bug 95057] [chromium] Allow impl-thread deletion of only some resources

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=95057





--- Comment #12 from Christopher Cameron <ccameron at chromium.org>  2012-08-29 17:25:55 PST ---
(From update of attachment 161077)
View in context: https://bugs.webkit.org/attachment.cgi?id=161077&action=review

The best test against race conditions (like the existing one that I found) is to insert a synchronous-impl-thread-evict-textures call between every line of CCThreadProxy::beginFrame (say, on frame 10 trigger the one after line 1, on frame 20 trigger the one after line 2, etc).  I'd like to add a test to this effect in a follow-on CL (it will be necessarily messy in order to be effective, I suspect).

>> Source/WebCore/ChangeLog:17
>> +        structures clear by the main thread yet. Add helper functions to delete
> 
> clear => cleared

Fixed.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:206
>> +    return false;
> 
> Looks like you didn't mean to have this here.

Ah, I meant to have a "if (m_rendererInitialized)" before that.  Fixed.

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTexture.cpp:153
>> +    ASSERT(CCProxy::isImplThread());
> 
> Also assert not deleted?

This isn't true with this change -- the impl thread's tree can have invalid resource IDs in it, so its canDraw() returns false until it knows its most recent commit has only-valid textures in it.

I plan to change this in a follow-up change in which I allow the frame to be drawn immediately after some of its textures are discarded (only if we can be sure that the textures discarded are sufficiently offscreen).

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:328
>> +    m_sharedState.addBackingOnImplThread(backing);
> 
> I find it a little strange that createBacking manages the shared state list, but destroyBacking doesn't and assumes you've called it beforehand.  Is it possible to make them more symmetrical, possibly by moving the m_sharedState stuff into destroyBacking?

So the messy part of this is that backings can be destroyed in two ways -- either "resource and backing at the same time" or "just the backing because the resource was destroyed earlier".

I've made this symmetrical, but by moving the m_sharedState call out of createBacking and to the caller (like destroy).

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:-314
>> -        resourceProvider->deleteResource(backing->id());
> 
> Can you assert on m_deleted here?

Done.

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:150
>> +    class SharedState {
> 
> Thanks for this.  It makes the shared access pattern so much clearer.

Thanks!  The structure will probably get a bit bigger in subsequent changes, as I add priority information to the impl thread.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:357
> +        m_contentsTexturesPurgeCount += 1;

I found a latent bug that needed a fix here.

If we had already created m_currentTextureUpdateControllerOnImplThread, it may contain references to now-invalid textures.  We need to clear its references to invalid textures here.

>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:604
>> +        // doesn't know of. Rquest a recommit, which will inform the main thread
> 
> typo

Fixed.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:663
>          m_layerTreeHostImpl->resetContentsTexturesPurged();

Yup, that's the plan!

>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:196
>> +    int m_contentsTexturesPurgeAck;
> 
> This is elegant.

Thank you!

>> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:174
>> +};
> 
> This is a little weird to my eyes, mostly just because I think of TiledLayerChromium as a "main thread" class that only gets used on the compositor thread during tree sync/pushPropertiesTo.  I think only the updateAndPush part of these tests (that calls updateTextures and pushPropertiesTo) should need DebugScopedSetImplThreadAndMainThreadBlocked and not the whole test.

You're right -- treating them as an impl-thread test was getting to be a mess.

I created wrappers for the the impl class and impl functions and then did a lot of replace-alls on the file.  The result is simpler than the original.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.