[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 #6 from Christopher Cameron <ccameron at chromium.org>  2012-08-27 12:33:08 PST ---
(From update of attachment 160669)
View in context: https://bugs.webkit.org/attachment.cgi?id=160669&action=review

Thanks!  I'm going to try some ways to make the logic if "where is the mutex necessary" less ambiguous and I'll re-upload when I think I have a decent scheme.  LMK if you have any thoughts on the ideas I put out.

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTexture.cpp:120
>> +        return m_backing->resourceDeleted();
> 
> Isn't this racy if the m_backingsMutex in the manager isn't held?

This can only be called on the impl thread while the main thread is inactive.  I need to add an assertion of this.

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:230
>> +            resourceProvider->deleteResource((*it)->id());
> 
> What if resourceProvider is NULL? Or, if the semantics have changed to not allow NULL anymore, mind adding an assert to make that clear?

The semantics have changed to disallow NULL.  I'll add an assert (and in the other places that it comes in as an argument).

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:271
>> +                resourcesToDelete.append((*it)->id());
> 
> It would be good to reset the resource ID here, making it harder to accidentally use a stale resource id.

Good idea.  I moved the deletion tracking to the CCTexture structure.  As a (not absolutely necessary) consequence of this change, I delete the resource via the resourceProvider while the m_backingsMutex is held (we already call into the resourceProvider while the mutex is held while creating resources, so no further harm is done).

I will also need to add an assert to the id() accessor to make sure that it isn't called on the main thread.  Because of all of this instrumentation, it may be best to use composition of a CCTexture instead of inheritance.

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:352
>> +    ASSERT(backing->resourceDeleted() || !backing->owner() || !backing->owner()->isAbovePriorityCutoff());
> 
> since backing->resourceDeleted() has to be true (since you're asserting below), !backing->owner() || !backing->owner()->isAbovePriorityCutoff() is never tested. Should it be tested separately? Is it not true anymore?

Good point.  It is true only when destroyBacking() is called through reduceMemory.  I've re-added resourceProvider as an argument and made the logic more clear.

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:143
>> +    BackingSet m_backingsFreedByImplThread;
> 
> TBH a vector is just as good, and cheaper memory and time complexity-wise. You'll never add the same backing to this set twice, correct?

I used a set because of the following sequence of calls
1. deleteBackingsResourcesOnImplThread (adds a Backing* to the set)
2. reduceMemory (deletes that Backing*)
3. destroyBackingsFreedByImplThread (needs to be sure that it doesn't still have a pointer to that Backing*)
I need to be able to clear out some entries from the set in step 2.

>> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:151
>> +    Mutex m_backingsMutex;
> 
> I think we should be clear what is protected by this mutex and what is not. It looks to me that what is actually protected by the mutex is:
> - m_backings
> - m_backingsFreedByImplThread
> - the contents of each Backing in m_backings (in particular the m_resourceDeleted flag and the resource ID).
> (basically anything that is accessed by deleteBackingsResourcesOnImplThread while the mutex is held).
> 
> Among others, what is not protected by this is:
> - m_textures
> - texture->backing() for each texture in m_texture.
> It's important to note, so that we don't try to take the mutex in various operations and expect to be safe - we need the higher level notion of blocked main thread to ensure safety.

I've added the m_resourceDeleted flag and the resource ID to the comment, and clarified the comment.  I'll add a way to assert that the main thread is known to be blocked during an operation.

It may be that I should add a separate member class for the state that needs to be accessed through the mutex -- I'll see if that ends up feeling more reasonable.

>> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateQueue.cpp:69
>> +        if (!upload.texture->backingWasAsynchronouslyDeleted())
> 
> Isn't this racy if the m_backingsMutex isn't held?

This can only be called on the impl thread while the main thread is inactive.  I need to add an assertion of this.

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