[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 #19 from Christopher Cameron <ccameron at chromium.org>  2012-08-30 23:45:57 PST ---
(From update of attachment 161601)
View in context: https://bugs.webkit.org/attachment.cgi?id=161601&action=review

Thanks for taking a look at this!  

Yes, the SharedState is not the best name for the state -- it should denote StateThatCanBeMutatedByTheImplThreadWhileTheMainThreadIsNotBlocked (in fewer characters).

The ack # is a mechanism for making sure that the impl thread's layer tree doesn't contain any references to deleted textures.  The use of an ack # is, as you said, to avoid passing a list of the deleted resources between the two threads (and dealing with the lifetime management issues that arise there (resource IDs can be recycled, etc).  In a future (farther-future) patch, I hope to create a mechanism fir the impl thread's tree to prune out evicted textures so that it can be drawn without requiring a re-commit (but that's a substantial change, and it, too, will need an ack to be able to say "visible stuff was evicted from this tree, don't draw until a commit brings it back").

>> Source/WebCore/ChangeLog:20
>> +        Allow multiple in-flight texture purges. In CCThreadProxy, make the
> 
> What does "in-flight" mean?

A texture purge is "in-flight" if the main thread has not acknowledged the texture purge at a commit yet (so the impl thread's tree may still have references to evicted textures).

One of the features is that you can have the impl thread delete some textures, then delete some more, before a commit happens.  Before this change, the impl thread could only delete textures once per commit call (which makes sense if you're only able to delete all textures).

>> Source/WebCore/ChangeLog:36
>> +        this.
> 
> This is great, I'm really glad you are doing this.  Please break these changes out into a separate standalone patch.  This will be a lot easier to review + land quickly on its own and make this patch smaller+simpler to boot.  As is, this patch is a little too large.

Yes, this is got unreasonably big (it came in from earlier feedback) -- I'll split out the asserts (and related tests) into a separate patch.

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:413
>> +    for (BackingSet::iterator it = m_unevictedBackings.begin(); it != m_unevictedBackings.end(); ++it) {
> 
> Any time you have a mutex guarding some shared data and you need to access it, your goal should always be to get the lock, do whatever accesses/mutations you need to do to the shared data, then release the lock as quickly as possible.  In particular, you wanna do as little as possible under the lock that doesn't need to be protected by the lock.  This minimizes the chance that you'll get unnecessary lock contention and makes it much easier to reason about potential deadlocks.
> 
> In this case, it appears that SharedState::m_mutex protects only m_evictedBackings.  Could you iterate through m_unevictedBackings and do the deleteResource() calls outside the lock, then take the lock and append m_unevictedBackings to m_evictedBackings, then release the lock and clear m_unevictedBackings and return?

Good point -- I'd vascillated a bit on whether or not to create a temporary vector of things to evict (and, in fact, in my next patch, which sets a priority cutoff, I did exactly that).  I'll update this to reduce the scope of the mutex held.

Yes, m_unevictedBackings is not protected by the mutex.  More accurately (than my comment), SharedState is "stuff that can be mutated by the impl thread while the main thread is not blocked" (I also used Async as a shorthand for this in other locations).  I'd like to keep this stuff in a separate structure, but SharedState may not be the right name.

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:415
>> +        (*it)->deleteResource(resourceProvider);
> 
> I'm particularly worried about making this call with the mutex held - this makes a GL call, right? That could end up blocking on the GPU process for a potentially long time.

Good point.

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:85
>> +    bool reduceMemoryOnImplThreadAsync(CCResourceProvider*);
> 
> What does the "async" part of this mean? does it perform some of its work in a non-blocking fashion? after this function returns, is the memory reduced or not?

This was another shorthand for "can be called by the impl thread while the main thread is not blocked".  I'd like to denote this property in the function name -- do we have a standard shorthand for this?

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:91
>> +    // Delete all textures and frm the impl thread (at thread exit and
> 
> typo "frm"
> 
> whatcha mean by "thread exit" ? the thread outlives all compositor data structures

Oops, "thread exit" is a mentalmangling of layerTreeHostClosedOnImplThread.

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:106
>> +
> 
> spurious newline

Axed.

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:174
>> +        BackingSet m_unevictedBackings;
> 
> I can't find any evidence of this being protected from concurrent access by the mutex - is it really shared state?

True, it's not -- it's state that can be messed with by the impl thread while the main thread is running.  I should have a clearer definition (and a better word than "shared").

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

Will do.

>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:595
>> +        CCProxy::implThread()->postTask(createCCThreadTask(this, &CCThreadProxy::beginFrameCompleteOnImplThread, &completion, queue.release(), request->contentsTexturesPurgeAck));
> 
> ...but this acks the # on the request. there might have been any number of textures evicted after the request was posted to the main thread but before we hit the destroyContents...() call.  What does having the ack # tell us that we don't already know - i.e. that we know the main thread's beginFrame ran after we sent the beginFrame message to the main thread?  We never have more than 1 pending beginFrame so that is unambiguous

This is being unnecessarily conservative about what it acknowledges -- it can get a tighter result by getting the purge count to ack from the texture manager in the destroyContentsTexturesEvictedByImplThread call (this is just following vestigial structure).

That said, the ack # is necessary to handle cases where the impl thread purged some textures multiple times.  Consider the case where one purge happens just before the beginFrame, and a second purge happens just after the main thread called destroyContentsTexturesEvictedByImplThread.  We don't want to let the acknowledgement that our render tree is clear of references to the textures purged in the first purge to obscure the fact that our render tree still has references to textures that were purged in the second purge.

>> Source/WebKit/chromium/tests/CCTextureUpdateControllerTest.cpp:703
>> +// ASYNC DELETION TESTS
> 
> This comment doesn't add much - the next line says that it's a texture deletion test just as well

Torched.

>> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:84
>> +        {
> 
> no need for this extra scope

Elimidated.

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