[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 #31 from Eric Penner <epenner at chromium.org>  2012-09-05 11:48:09 PST ---
(From update of attachment 162173)
View in context: https://bugs.webkit.org/attachment.cgi?id=162173&action=review

I'm a big fan of this approach, which I hadn't thought of.

The big assumption if I understand correctly is that backing life-time is managed exclusively during commits? Possibly it's worth adding a comment to that effect on the various backing lists, and make it as difficult as possible to break that assumption throughout the code.

It seems like this could almost evolve into an impl-side PTM, where that impl-PTM gets a full list of textures to work with, and then passes back the eviction list. And the PTM itself is otherwise unaffected. No need for that now, but I certainly like that direction.

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:61
> +    ASSERT(m_backings.isEmpty());

I figured this second ASSERT would be trouble for unit tests, but great if this is now always empty. The TiledLayerChromiumTests share a PTM now, so if it's an issue for those tests we can purge before destroying the PTM in the test destructor.

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:208
>          backing = createBacking(texture->size(), texture->format(), resourceProvider);

Could go into the createBacking function?

I'm wondering if this could instead be a complete copy that occurs at some point, rather than being managed incrementally. In that case could it be a simple vector instead of a set?

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:-222
> -        destroyBacking((*it), resourceProvider);

Could we possibly put below logic into destroyBacking itself? And add back the CCResourceProvider* parameter? Having the CCResourceProvider* enforces that backing object life-time is managed entire during the commit, and makes create/destroy symmetric.

I'm also wondering if there should be a special step that deals with all evicted backings first, since all of those are definitely gone?

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:160
> +    BackingSet m_unevictedBackings;

Could you add a set of ASSERTs in ::assertInvariants() that verifies there is no errors in these lists?  eg. there shouldn't be duplicates between evicted/unevicted, and there shouldn't be any backing in either of these that isn't also in m_backings and vice versa.  Ideally also make sure we evict some textures in one ore more PrioritizedTextureTests.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:503
>      m_mainThreadProxy->postTask(createCCThreadTask(this, &CCThreadProxy::beginFrame));

The only thing I'm worried about here is whether backing objects could be destroyed/created by other means while this task is in flight. If so these pointers don't uniquely identify backing objects any more.

We only create/recycle/destroy backings during commits, so it seems safe at first glance. The ASSERT(!m_pendingBeginFrameRequest); at the beginning says there shouldn't ever be two of these in flight... Any other scenario you can think of where these backings could be out of date?

Do we still need an eviction counter with this approach? It seems like when the main thread processes the eviction list during the commit, it is guaranteed to be the one-and-only up-to-date eviction list?

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