[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 #18 from James Robinson <jamesr at chromium.org>  2012-08-30 21:58:58 PST ---
(From update of attachment 161601)
View in context: https://bugs.webkit.org/attachment.cgi?id=161601&action=review

I'm a little unclear about the shared state and ack count stuff.  Is the idea that we want the main thread to observe the side effects of purges that happen after we generate the beginFrame message, or that we want to avoid copying the list of deleted resource IDs?

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

What does "in-flight" mean?

> Source/WebCore/ChangeLog:36
> +        Fix a dozen places where CCThreadProxy and CCSingleThreadProxy hold
> +        the main thread blocked, but don't notify the debug structures of
> +        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.

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:413
> +    MutexLocker lock(m_mutex);
> +    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?

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

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

> 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

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

spurious newline

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

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

++

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:566
> +    m_layerTreeHost->destroyContentsTexturesEvictedByImplThread();

this destroys all textures that have been evicted at the time this call is made....

> 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

> 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

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:84
> +        {

no need for this extra scope

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