[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 #11 from Adrienne Walker <enne at google.com>  2012-08-29 12:33:46 PST ---
(From update of attachment 161077)
View in context: https://bugs.webkit.org/attachment.cgi?id=161077&action=review

I like this patch way better than the other approach in bug 94751.  Thanks for taking the time to add all the asserts.  It makes reading the code and understanding where functions can be called a lot easier.  I've got a bunch of small comments, but the patch on the whole looks good to me.

Can you add some new unit tests for the update queue purging logic? I think that's probably the biggest untested piece of this patch.

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

clear => cleared

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:206
> +        return m_contentsTextureManager->reduceMemoryOnImplThreadAsync(resourceProvider);
> +    return false;

Looks like you didn't mean to have this here.

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

Also assert not deleted?

> 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?

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:-314
> -    if (resourceProvider)
> -        resourceProvider->deleteResource(backing->id());

Can you assert on m_deleted here?

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:150
> +    // This state that can be concurrently accessed by the main and impl thread,
> +    // and needs protection by a mutex.
> +    class SharedState {

Thanks for this.  It makes the shared access pattern so much clearer.

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

typo

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:663
> +        // The tree now references no purged textures. Allow it to draw again.
>          m_layerTreeHostImpl->resetContentsTexturesPurged();

I hope we can make this less restrictive in the future.  When we're clearing all the textures like we do in this patch, it makes sense to disallow drawing.  In the future, maybe we can be more smart and check if any purged / unavailable resources are needed to put up a frame.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:196
> +    // Counter incremented every time the impl thread deletes textures, sent to the
> +    // main thread with each commit.
> +    int m_contentsTexturesPurgeCount;
> +
> +    // Value that the main thread sends as an acknowledgement to the impl thread.
> +    // If this does not equal m_contentsTexturesPurgeCount then the impl layer tree
> +    // may reference textures that have been purged.
> +    int m_contentsTexturesPurgeAck;

This is elegant.

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:174
> +class TiledLayerChromiumTestOnImplThread : public TiledLayerChromiumTest {
> +public:
> +    TiledLayerChromiumTestOnImplThread() { }
> +    virtual ~TiledLayerChromiumTestOnImplThread() { }
> +
> +private:
> +     DebugScopedSetImplThreadAndMainThreadBlocked m_implThreadAndMainThreadBlocked;
> +};

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.

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