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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Sep 6 11:15:19 PDT 2012


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





--- Comment #46 from Christopher Cameron <ccameron at chromium.org>  2012-09-06 11:15:33 PST ---
(From update of attachment 162358)
View in context: https://bugs.webkit.org/attachment.cgi?id=162358&action=review

Thanks!

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:425
>> +
> 
> extra newline here

Fixed.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:430
>> +    m_contentsTextureManager->destroyEvictedBackings(purgeCounterAck);
> 
> So this is the change that requires the mutex?  This call can't actually issue any GL calls, so what is it touching?

This line is un-linking CCPrioritizedTextures from their Backings.

The Backings had their resources deleted by the impl thread, but the structures are still hanging around in the m_backings array and are pointed to by CCPrioritizedTextures.

This needs to be done before calling updateLayers because updateLayers will use the existence of a Backing to determine if the layer needs to be re-drawn.

As you noticed, the "ack" is actually not needed for this scheme (and I've removed it).

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTexture.h:110
>> +            : m_texture(id, size, format), m_resourceDeleted(false), m_owner(0) { }
> 
> initialize one thing per line. I think you should define the c'tor and d'tor in the .cpp, not the header

Fixed and moved to the .cpp file.

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:228
>> +        if ((*it)->resourceDeleted()) {
> 
> so much (*it). Can we put that in a temporary with the proper type and a good name so we know what the heck it is?

Put it in a backing pointer (I'd been vacillating  on this because of the surrounding (*it)).

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:273
>> +    bool result = false;
> 
> what does "result" mean? could you give this a more descriptive name - it seems to be indicating if we've evicted anything

Changed to didEvictBackings.

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:277
>> +        for (BackingVector::iterator it = backingsToEvict.begin(); it != backingsToEvict.end(); ++it) {
> 
> instead of the loop, why not just m_evictedBackings.append(m_unevictedBackings); m_unevictedBackings.clear(); ?

This will have to be a loop when we become more discriminating about what we delete (we won't always just be adding all of m_unevictedBackings to m_evictedBackings.

>>>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:297
>>>> +        destroyBacking((*it));
>>> 
>>> this call does a GL call if i understand it correctly - can you not do it inside the mutex? a fairly efficient way to do that would be to make a new vector, .swap() m_evictedBackings into the new vector under the lock, then outside the lock iterate through and actually destroy the backings
>> 
>> Wait sorry, this can't be right - we must have already issued the GL delete before we hit here.  What's this supposed to be doing on the main thread?
> 
> OK sorry, I think I see now - this is just supposed to remove it from this texture manager's set of live backings and unlink the CCPrioritizedTexture
> 
> So now I'm backing to wondering why this has a lock at all.  Why not just pass a vector of deleted backings to the main thread in the beginFrame message and have the main thread unlink these on the main thread prioritized texture manager on its own time?  There's no reason to keep track of this set on the impl thread after the GL textures have been deleted and we've passed the set to the main thread.  Passing a vector can be really efficient if you just .swap() the storage instead of copying element by element.

I tried out that scheme (with the swap, et al) in the patch at
https://bugs.webkit.org/attachment.cgi?id=162173&action=review

I felt really uncomfortable with that patch because there were Backing*s that were in this external list, and that were still in m_backings, and the external list was destroyed by clearEvictedBackings() but could also be destroyed by reduceMemory().  Eric pointed this out in that diff (see the "I'm worried about ... whether backing objects could be destroyed/created by other means while this task is in flight" remark in that diff).

Eric and I convinced ourselves that this couldn't happen (because the list is fetched in scheduledActionBeginFrame, clearEvictedBackings is called in beginFrame, and reduceMemory is called in scheduledActionCommit, and we're pretty sure that you can't have the sequence scheduledActionBeginFrame,scheduledActionCommit,beginFrame -- the beginFrame has to fit in the middle).  But it was a bit scary (and it was harder to add asserts all over that one set was the disjoint union of the other two, etc).

Eric had mentioned cutting the PTM into main/impl parts, and I think that when we do that, the eviction-vector-passing scheme will fall out much more nicely.  I'd like to land this current scheme (because its correctness is solid and it has all sorts of asserts to verify this), and then work on the PTM main/impl split (to get that scheme the same level of clearly-correct-ness) in parallel with the other memory work which is blocked on this functionality.

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:167
>> +
> 
> spurious newlines

Fixed.

>> Source/WebKit/chromium/tests/CCTextureUpdateControllerTest.cpp:90
>> +    TextureForUploadTest() : LayerTextureUpdater::Texture(adoptPtr<CCPrioritizedTexture>(0)), m_deleted(false) { }
> 
> One initialization entry per line.  The WebKit style way to line this up would be:
> 
> TextureForUploadTest()
>     : LayerTextureUpd..())
>     , m_deleted(false)
> {
> }
> 
> You can also use 'nullptr' instead of adoptPtr<T>(0)

Fixed.

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



More information about the webkit-unassigned mailing list