[chromium] Allow impl-thread deletion of only some resources
Created attachment 160669 [details] Patch
Towards allowing the impl thread to delete just some resources (as opposed to deleting all resources). Do impl-thread deletion through CCPrioritizedTextureManager instead of CCResourceProvider. In subsequent changes the impl thread will be able to set a threshold for what to delete). Add a mutex to protect the state shared between the two threads. Add a set of textures that have been deleted by the impl thread, but have not had their related structures clear by the main thread yet. Allow multiple in-flight texture purges. In CCThreadProxy, make the acknowledgement of texture deletion be an integer counter instead of a bool, because we may have multiple in-flight purges. Remove many instances where texture purges were avoided when there was already an outstanding purge. Add the ability to clear only the references to deleted textures from the texture upload queue.
Comment on attachment 160669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160669&action=review > Source/WebKit/chromium/ChangeLog:13 > + Remove the test releaseContentsTextureShouldTriggerCommit because it > + is no longer valid -- a releaseContentsTexture will only trigger a > + commit if some resources are deleted. I do wonder about testing for this. What about testing these two things? Commit happens on delete, doesn't happen on no delete.
(In reply to comment #3) > (From update of attachment 160669 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160669&action=review > > > Source/WebKit/chromium/ChangeLog:13 > > + Remove the test releaseContentsTextureShouldTriggerCommit because it > > + is no longer valid -- a releaseContentsTexture will only trigger a > > + commit if some resources are deleted. > > I do wonder about testing for this. What about testing these two things? Commit happens on delete, doesn't happen on no delete. The commit logic has moved out of CCLayerTreeHostImpl and moved into CCProxy. There does exist a test, CCLayerTreeHostTestEvictTextures, which checks the commits that come in and pairs them with evictions. Now that you mention it though, I did not include the update in CCSingleThreadProxy (so I wonder how that passed). I'll look into making a test that fails if the commit doesn't happen (or updating the CCLayerTreeHostTestEvictTextures test to fail appropriately when the commit doesn't happen).
Comment on attachment 160669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160669&action=review > 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? > 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? > 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. > 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? > 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? > 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. > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateQueue.cpp:69 > + if (!upload.texture->backingWasAsynchronouslyDeleted()) Isn't this racy if the m_backingsMutex isn't held?
Comment on attachment 160669 [details] Patch 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.
Comment on attachment 160669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160669&action=review >>> 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. I'm not sure about moving things into CCTexture. The CCTexture structure should be agnostic about where it is used, and not tied to the texture manager. It's meant to be used in all use cases of textures, not just contents.
Comment on attachment 160669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160669&action=review >>>> 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. > > I'm not sure about moving things into CCTexture. The CCTexture structure should be agnostic about where it is used, and not tied to the texture manager. It's meant to be used in all use cases of textures, not just contents. Yeah, it doesn't feel right. I'll change it to composition to control access to the interface.
Created attachment 161077 [details] Patch
(In reply to comment #9) > Created an attachment (id=161077) [details] > Patch I've updated the patch. The big change is that I moved all PTM state that can be accessed by multiple threads concurrently into a smaller sub-class. I'm much happier with this scheme. This sub-class will get a bit bigger when I give the main thread a way to inform it of the backings' priorities (or at least orderings) so that the impl thread can make informed asynchronous eviction decisions. I also added roughly twelve thousand asserts of what-thread-can-call-what in the PTM, and then created big diffs in some tests to obey these requirements.
Comment on attachment 161077 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161077&action=review I like this patch way better than the other approach in bug 94751. Thanks for taking the time to add all the asserts. It makes reading the code and understanding where functions can be called a lot easier. I've got a bunch of small comments, but the patch on the whole looks good to me. Can you add some new unit tests for the update queue purging logic? I think that's probably the biggest untested piece of this patch. > Source/WebCore/ChangeLog:17 > + structures clear by the main thread yet. Add helper functions to delete clear => cleared > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:206 > + return m_contentsTextureManager->reduceMemoryOnImplThreadAsync(resourceProvider); > + return false; Looks like you didn't mean to have this here. > Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTexture.cpp:153 > + ASSERT(CCProxy::isImplThread()); Also assert not deleted? > Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:328 > + m_sharedState.addBackingOnImplThread(backing); I find it a little strange that createBacking manages the shared state list, but destroyBacking doesn't and assumes you've called it beforehand. Is it possible to make them more symmetrical, possibly by moving the m_sharedState stuff into destroyBacking? > Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:-314 > - if (resourceProvider) > - resourceProvider->deleteResource(backing->id()); Can you assert on m_deleted here? > Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:150 > + // This state that can be concurrently accessed by the main and impl thread, > + // and needs protection by a mutex. > + class SharedState { Thanks for this. It makes the shared access pattern so much clearer. > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:604 > + // doesn't know of. Rquest a recommit, which will inform the main thread typo > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:663 > + // The tree now references no purged textures. Allow it to draw again. > m_layerTreeHostImpl->resetContentsTexturesPurged(); I hope we can make this less restrictive in the future. When we're clearing all the textures like we do in this patch, it makes sense to disallow drawing. In the future, maybe we can be more smart and check if any purged / unavailable resources are needed to put up a frame. > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:196 > + // Counter incremented every time the impl thread deletes textures, sent to the > + // main thread with each commit. > + int m_contentsTexturesPurgeCount; > + > + // Value that the main thread sends as an acknowledgement to the impl thread. > + // If this does not equal m_contentsTexturesPurgeCount then the impl layer tree > + // may reference textures that have been purged. > + int m_contentsTexturesPurgeAck; This is elegant. > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:174 > +class TiledLayerChromiumTestOnImplThread : public TiledLayerChromiumTest { > +public: > + TiledLayerChromiumTestOnImplThread() { } > + virtual ~TiledLayerChromiumTestOnImplThread() { } > + > +private: > + DebugScopedSetImplThreadAndMainThreadBlocked m_implThreadAndMainThreadBlocked; > +}; This is a little weird to my eyes, mostly just because I think of TiledLayerChromium as a "main thread" class that only gets used on the compositor thread during tree sync/pushPropertiesTo. I think only the updateAndPush part of these tests (that calls updateTextures and pushPropertiesTo) should need DebugScopedSetImplThreadAndMainThreadBlocked and not the whole test.
Comment on attachment 161077 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161077&action=review The best test against race conditions (like the existing one that I found) is to insert a synchronous-impl-thread-evict-textures call between every line of CCThreadProxy::beginFrame (say, on frame 10 trigger the one after line 1, on frame 20 trigger the one after line 2, etc). I'd like to add a test to this effect in a follow-on CL (it will be necessarily messy in order to be effective, I suspect). >> Source/WebCore/ChangeLog:17 >> + structures clear by the main thread yet. Add helper functions to delete > > clear => cleared Fixed. >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:206 >> + return false; > > Looks like you didn't mean to have this here. Ah, I meant to have a "if (m_rendererInitialized)" before that. Fixed. >> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTexture.cpp:153 >> + ASSERT(CCProxy::isImplThread()); > > Also assert not deleted? This isn't true with this change -- the impl thread's tree can have invalid resource IDs in it, so its canDraw() returns false until it knows its most recent commit has only-valid textures in it. I plan to change this in a follow-up change in which I allow the frame to be drawn immediately after some of its textures are discarded (only if we can be sure that the textures discarded are sufficiently offscreen). >> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:328 >> + m_sharedState.addBackingOnImplThread(backing); > > I find it a little strange that createBacking manages the shared state list, but destroyBacking doesn't and assumes you've called it beforehand. Is it possible to make them more symmetrical, possibly by moving the m_sharedState stuff into destroyBacking? So the messy part of this is that backings can be destroyed in two ways -- either "resource and backing at the same time" or "just the backing because the resource was destroyed earlier". I've made this symmetrical, but by moving the m_sharedState call out of createBacking and to the caller (like destroy). >> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:-314 >> - resourceProvider->deleteResource(backing->id()); > > Can you assert on m_deleted here? Done. >> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:150 >> + class SharedState { > > Thanks for this. It makes the shared access pattern so much clearer. Thanks! The structure will probably get a bit bigger in subsequent changes, as I add priority information to the impl thread. > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:357 > + m_contentsTexturesPurgeCount += 1; I found a latent bug that needed a fix here. If we had already created m_currentTextureUpdateControllerOnImplThread, it may contain references to now-invalid textures. We need to clear its references to invalid textures here. >> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:604 >> + // doesn't know of. Rquest a recommit, which will inform the main thread > > typo Fixed. > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:663 > m_layerTreeHostImpl->resetContentsTexturesPurged(); Yup, that's the plan! >> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:196 >> + int m_contentsTexturesPurgeAck; > > This is elegant. Thank you! >> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:174 >> +}; > > This is a little weird to my eyes, mostly just because I think of TiledLayerChromium as a "main thread" class that only gets used on the compositor thread during tree sync/pushPropertiesTo. I think only the updateAndPush part of these tests (that calls updateTextures and pushPropertiesTo) should need DebugScopedSetImplThreadAndMainThreadBlocked and not the whole test. You're right -- treating them as an impl-thread test was getting to be a mess. I created wrappers for the the impl class and impl functions and then did a lot of replace-alls on the file. The result is simpler than the original.
Created attachment 161370 [details] Patch
Sorry, I didn't add the texture queue unit test -- ignore that patch.
Created attachment 161379 [details] Patch
New patch uploaded which includes new unit test.
Created attachment 161601 [details] Patch
Comment on attachment 161601 [details] Patch 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
Comment on attachment 161601 [details] Patch 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.
Created attachment 161808 [details] Patch
Created attachment 161809 [details] Patch
Patch updated, resolved against bug 95596 (which was split off from this bug). There was some offline discussion about separating CCPrioritizedTextureManager::EvictionState into a stand-alone class. I'd prefer to keep the classes coupled until we have a clearer picture of the final state of the code (with the associated priority changes and impl-tree pruning). It may be that we will be able to fold this back into CCPrioritizedTextureManager, or it may be that we'll separate it into its own proper class -- I think this is the best step to avoid churn.
ptal
Comment on attachment 161809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161809&action=review I'm just back from vacation. The other comments seems to address any races/dead-locks I can think of but I'll take another pass to see if I can think of anything else. I've added some other general comments for now. When moved entirely to the the impl-thread, do you anticipate this will still have value, or is this mainly a hold-over? There might be other threading issues, but I don't see this in particular being a problem. On that note, while I kind of like all the isImplThread() ASSERTs etc., previously passing the ResourceProvider* was the means of indicating that resources could allocated/destroyed. This implicitly indicated isImplThread() && isMainThreadBlocked() but didn't enforce it explicitly (so that this class could be used on any thread, and be kept as a single-threaded data-structure rather than being so coupled to other classes). > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:62 > virtual void postAnimationEventsToMainThreadOnImplThread(PassOwnPtr<CCAnimationEventsVector>, double wallClockTime) = 0; I'm really curious as to why we can post an event for animations above but not similarly do so for texture management below. Why can we not post tasks to achieve the end result here rather than resorting to adding a mutex? I doubt this is the only case where this type of communication will be needed, and I'm hoping that adding a mutex will not become the norm for solving such problems. So I just want to be sure we have considered all other options here. > Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:80 > void reduceMemory(CCResourceProvider*); I find the naming confusing here given: - reduceMemory above must be called on the impl thread (with main thread blocked) since it takes CCResourceProvider*. - reduceMemoryOnImplThread below doesn't reduce memory to the limit, but rather clears all memory. I would recommend: - renaming to clearAllMemoryOnImplThread - reserve "onImplThread" only for functions that could occur when the main thread is not blocked (if the main thread is blocked it is more like "onBothThreads" or "duringCommit"). > Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:160 > + void addBackingOnImplThread(CCPrioritizedTexture::Backing*); See my above comment about "onImplThread".
Comment on attachment 161809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161809&action=review Thanks! > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:62 > virtual void postAnimationEventsToMainThreadOnImplThread(PassOwnPtr<CCAnimationEventsVector>, double wallClockTime) = 0; Yes -- we could communicate the list of destroyed backings to the main thread at commit time -- we would call getAndClearEvictedBackings on the impl thread and communicate them via scheduledActionBeginFrame. I had originally started with this scheme, and then abandoned it because the implementation was getting complicated -- copying the set of backings to and from the main thread was involved. In the intervening iteration, it looks like it's not terribly complicated. That said, there was some discussion of wanting the PTM to have a mutex for moving towards impl-side painting. I'm very open to changing back to the scheme of passing the set of evicted textures to the main thread, but I'm getting a bit worried about vascilating too much before checking anything in. > Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:80 > void reduceMemory(CCResourceProvider*); I wanted to name this as reduceMemoryOnImplThread because that's what the function will do in a subsequent change (I renamed a few things from "clear all" to "reduce").
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:62 > > virtual void postAnimationEventsToMainThreadOnImplThread(PassOwnPtr<CCAnimationEventsVector>, double wallClockTime) = 0; > > Yes -- we could communicate the list of destroyed backings to the main thread at commit time -- we would call getAndClearEvictedBackings on the impl thread and communicate them via scheduledActionBeginFrame. I had originally started with this scheme, and then abandoned it because the implementation was getting complicated -- copying the set of backings to and from the main thread was involved. I was thinking more along the lines of notifying the main thread so it could do the memory reduction and then push the updates across like it already does. Currently this would require a full commit though, and AFAIU there is a problem with that for invisible tabs? If we created custom impl-side objects for each texture we could also just push the ids across in a custom 'mini-commit'. > In the intervening iteration, it looks like it's not terribly complicated. That said, there was some discussion of wanting the PTM to have a mutex for moving towards impl-side painting. I'm very open to changing back to the scheme of passing the set of evicted textures to the main thread, but I'm getting a bit worried about vascilating too much before checking anything in. Looks like you have buy-in from all the feedback. All I'd say is that the impl-side painting threading issues are quite different than the issues this is solving, so the stuff behind the mutex would be very different I think. Anyway, I'm a bit late to the party to request huge changes, so as long as reviewers are okay with it then that's fine with me. > I wanted to name this as reduceMemoryOnImplThread because that's what the function will do in a subsequent change (I renamed a few things from "clear all" to "reduce"). Ah okay, if follow up CLs will fix then np, and consider removing "OnImplThread" for main-thread-blocked functions as well (if you agree with that bit - I'm not sure of the convention for that elsewhere).
(In reply to comment #26) > I was thinking more along the lines of notifying the main thread so it could do the memory reduction and then push the updates across like it already does. Currently this would require a full commit though, and AFAIU there is a problem with that for invisible tabs? If we created custom impl-side objects for each texture we could also just push the ids across in a custom 'mini-commit'. Ah, one of the goals is to be able to adjust memory without going back to the main thread (so we can very quickly adjust memory without getting blocked on JS, etc). There are some things that are preventing this from always being the case ATM (visibility messages being synchronous, etc). > > In the intervening iteration, it looks like it's not terribly complicated. That said, there was some discussion of wanting the PTM to have a mutex for moving towards impl-side painting. I'm very open to changing back to the scheme of passing the set of evicted textures to the main thread, but I'm getting a bit worried about vascilating too much before checking anything in. > > Looks like you have buy-in from all the feedback. All I'd say is that the impl-side painting threading issues are quite different than the issues this is solving, so the stuff behind the mutex would be very different I think. Anyway, I'm a bit late to the party to request huge changes, so as long as reviewers are okay with it then that's fine with me. Yes, you're right about the different mutex structure for the different purposes. I'm strongly considering re-examining the "send the set to the main thread" scheme (to avoid the lock), but I'll land this first and then tweak it. > > I wanted to name this as reduceMemoryOnImplThread because that's what the function will do in a subsequent change (I renamed a few things from "clear all" to "reduce"). > > Ah okay, if follow up CLs will fix then np, and consider removing "OnImplThread" for main-thread-blocked functions as well (if you agree with that bit - I'm not sure of the convention for that elsewhere). Oh -- I like the convention (and I think this CL follows it -- reduceMemoryOnImplThread doesn't block main thread but reduceMemory does).
> > I was thinking more along the lines of notifying the main thread so it could do the memory reduction and then push the updates across like it already does. Currently this would require a full commit though, and AFAIU there is a problem with that for invisible tabs? If we created custom impl-side objects for each texture we could also just push the ids across in a custom 'mini-commit'. > > Ah, one of the goals is to be able to adjust memory without going back to the main thread (so we can very quickly adjust memory without getting blocked on JS, etc). There are some things that are preventing this from always being the case ATM (visibility messages being synchronous, etc). > I see, okay that rules out the commit then. > > > In the intervening iteration, it looks like it's not terribly complicated. That said, there was some discussion of wanting the PTM to have a mutex for moving towards impl-side painting. I'm very open to changing back to the scheme of passing the set of evicted textures to the main thread, but I'm getting a bit worried about vascilating too much before checking anything in. > > > > Looks like you have buy-in from all the feedback. All I'd say is that the impl-side painting threading issues are quite different than the issues this is solving, so the stuff behind the mutex would be very different I think. Anyway, I'm a bit late to the party to request huge changes, so as long as reviewers are okay with it then that's fine with me. > > Yes, you're right about the different mutex structure for the different purposes. I'm strongly considering re-examining the "send the set to the main thread" scheme (to avoid the lock), but I'll land this first and then tweak it. I'm wonder if it could even just send the deleted-priority-threshold in the task? Anyway, that would require strict timing which might end up getting complicated, but I would like it a lot if it could be done. > > > > I wanted to name this as reduceMemoryOnImplThread because that's what the function will do in a subsequent change (I renamed a few things from "clear all" to "reduce"). > > > > Ah okay, if follow up CLs will fix then np, and consider removing "OnImplThread" for main-thread-blocked functions as well (if you agree with that bit - I'm not sure of the convention for that elsewhere). > > Oh -- I like the convention (and I think this CL follows it -- reduceMemoryOnImplThread doesn't block main thread but reduceMemory does). See: addBackingOnImplThread / removeBackingOnImplThread, but whatev' for that.
Created attachment 162173 [details] Patch
After thinking more about Eric's remarks, I've removed the mutex and shared state from the PTM and am instead passing the evicted texture list to the main thread. This has a smaller diff and feels more natural.
Comment on attachment 162173 [details] Patch 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?
Comment on attachment 162173 [details] Patch As a last thought, I wonder if we could avoid having two backing sets, and instead copy a custom vector of structs like: struct ImplBacking { Backing* backing, int priority; bool evicted; }; Then we could just process that list before doing any other backing management in the commit (which all happens during updating textures). Thought for food.
(In reply to comment #32) > 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. So there does exist a bug here -- a reduceMemory() call can destroy a backing that has been handed to the main thread, but the main thread hasn't deleted yet. I don't see a way around that without basically going back to the scheme of having a list of evicted textures in the PTM which is protected by a mutex (so, going back to the old scheme). > (From update of attachment 162173 [details]) > As a last thought, I wonder if we could avoid having two backing sets, and instead copy a custom vector of structs like: > struct ImplBacking > { > Backing* backing, > int priority; > bool evicted; > }; > Then we could just process that list before doing any other backing management in the commit (which all happens during updating textures). Thought for food. From this it sounds like we're converging to a place where the m_backings vector is controlled entirely by the impl thread. This makes sense to me -- that way we wouldn't have to have the extra BackingSet for the impl thread. The change to do this would be fairly simple; - have prioritizeTextures() not touch the backings array because it's called on the main thread - add a prioritizeBackings() function to be called during commit, which pushes the texture priorities to their backings, and sorts the backings array - add a "int m_priority" to CCPrioritizedTexture::Backing The mutex-protected list of evicted textures would still be needed, though.
(In reply to comment #33) > (In reply to comment #32) > > > 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. > > So there does exist a bug here -- a reduceMemory() call can destroy a backing that has been handed to the main thread, but the main thread hasn't deleted yet. I don't see a way around that without basically going back to the scheme of having a list of evicted textures in the PTM which is protected by a mutex (so, going back to the old scheme). Can this actually happen though? reduceMemory() can only occur during a commit when the main thread is blocked, and it appears as though there can not be any other list of evicted backings in flight from when the task is posted through to when the commit is finished. So it seem to me we just need to process that list during the commit and we should be fine. > > > (From update of attachment 162173 [details] [details]) > > As a last thought, I wonder if we could avoid having two backing sets, and instead copy a custom vector of structs like: > > struct ImplBacking > > { > > Backing* backing, > > int priority; > > bool evicted; > > }; > > Then we could just process that list before doing any other backing management in the commit (which all happens during updating textures). Thought for food. > > From this it sounds like we're converging to a place where the m_backings vector is controlled entirely by the impl thread. This makes sense to me -- that way we wouldn't have to have the extra BackingSet for the impl thread. The change to do this would be fairly simple; > - have prioritizeTextures() not touch the backings array because it's called on the main thread > - add a prioritizeBackings() function to be called during commit, which pushes the texture priorities to their backings, and sorts the backings array > - add a "int m_priority" to CCPrioritizedTexture::Backing > > The mutex-protected list of evicted textures would still be needed, though. I might be wrong, but could mutex be avoided like this? During Commit: - Process eviction list and delete those backings - Manage lifetime of other backings via updating textures - Create a new eviction list for impl thread to use until next commit. Main Thread: - Prioritize textures and backings but never manage backing life-times. - It should be impossible to manage backing life-times anyway since this requires the CCResourceProvider (commit only) Impl Thread: - Mark it's own list of backings as being evicted. Do we even need an event? Or maybe I'm missing something.
Created attachment 162335 [details] Patch
(In reply to comment #34) > (In reply to comment #33) > > So there does exist a bug here -- a reduceMemory() call can destroy a backing that has been handed to the main thread, but the main thread hasn't deleted yet. I don't see a way around that without basically going back to the scheme of having a list of evicted textures in the PTM which is protected by a mutex (so, going back to the old scheme). > > Can this actually happen though? reduceMemory() can only occur during a commit when the main thread is blocked, and it appears as though there can not be any other list of evicted backings in flight from when the task is posted through to when the commit is finished. So it seem to me we just need to process that list during the commit and we should be fine. > > > The mutex-protected list of evicted textures would still be needed, though. > > I might be wrong, but could mutex be avoided like this? > <snip> I think that you are right, but it's not very clear. I feel very confident about correctness and performance of the mutex scheme, and I'd prefer to check that in first. I've cleaned up the CL to have a smaller surface area (removing the separated shared state). Is it okay in its current form? [Btw, I'm having trouble with the patch not applying to TOT... I may upload a new version in a bit -- it won't differ substantially from the most current patch]
Created attachment 162345 [details] Patch
Created attachment 162358 [details] Patch
> > I might be wrong, but could mutex be avoided like this? > > <snip> > > I think that you are right, but it's not very clear. I feel very confident about correctness and performance of the mutex scheme, and I'd prefer to check that in first. > > I've cleaned up the CL to have a smaller surface area (removing the separated shared state). Is it okay in its current form? > > [Btw, I'm having trouble with the patch not applying to TOT... I may upload a new version in a bit -- it won't differ substantially from the most current patch] I think the above approach is more similar to other places where the impl side holds the canonical information and needs to get it back to the main thread (eg. scroll deltas or animation events). But yeah, since this has been put together and reviewed, I don't want to hold it from landing if there is a need for it.
Comment on attachment 162358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162358&action=review Overall this seems nice. I think there are a few things that need more attention - I really want to make sure that the threading stuff is solid and robust for future changes. The ack passing seems needlessly complicated and error prone. It seems like what you are trying to determine in CCThreadProxy is the answer to the following question: "Have I evicted any textures from the impl thread since the last time I sent a beginFrame message to the main thread?" If that's the case then we know when we get a beginFrameComplete on the impl thread it's out of date. That seems like it's much easier to track directly in CCThreadProxy with a bool rather than passing an int back+forth across threads. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:425 > + extra newline here > 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? > 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 > 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? > 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 > Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:277 > + if (!backingsToEvict.isEmpty()) { > + result = true; > + 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(); ? > 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 > Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:167 > + > + spurious newlines > 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)
I know this isn't intended to be complete yet, but the big missing piece in my mind is how you intend to delete 'only some resources', as the name of this CL implies. Currently we only push raw texture ids over to the impl thread. How do you intend to delete some of those raw IDs in all layers that contain them? Would it make sense to start a parallel CL for that, or possibly even do that part first? I'd be a lot more happy if this CL actually enabled some new functionality, after all the other bits were in place.
Comment on attachment 162358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162358&action=review >> 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?
Comment on attachment 162358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162358&action=review >>> 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.
(In reply to comment #36) > (In reply to comment #34) > > (In reply to comment #33) > > > So there does exist a bug here -- a reduceMemory() call can destroy a backing that has been handed to the main thread, but the main thread hasn't deleted yet. I don't see a way around that without basically going back to the scheme of having a list of evicted textures in the PTM which is protected by a mutex (so, going back to the old scheme). > > > > Can this actually happen though? reduceMemory() can only occur during a commit when the main thread is blocked, and it appears as though there can not be any other list of evicted backings in flight from when the task is posted through to when the commit is finished. So it seem to me we just need to process that list during the commit and we should be fine. > > > > > The mutex-protected list of evicted textures would still be needed, though. > > > > I might be wrong, but could mutex be avoided like this? > > <snip> > > I think that you are right, but it's not very clear. I feel very confident about correctness and performance of the mutex scheme, and I'd prefer to check that in first. Ah yes, I see Eric's suggested pretty much the exact same thing earlier on. Serves me right for skimming the thread. The message passing method is how we handle nearly all shared state in the compositor (and for that matter in most of chromium) and is IMO a lot easier to reason about. The thing that makes the mutex based version really hard to reason about is you can know that can get an internally consistent picture from any thread, but it's really hard to reason about *which* picture you got. Hence the awkward ack passing scheme in your patch. If you instead embrace message passing then it gets easier to think about things in terms of what each thread has told the other thread about. Once thread A passes the data into a message to thread B, thread A never has to worry about it any more - it's gone.
> 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. James, can you confirm that there can *never* be two beginFrame messages at once? I grokked that to be the case but it's very important for using a task here. If the main thread has a chance to delete/create backings while another list of pointers is pending in the message queue, those pointers may no longer be unique by the time they are processed. Backings are only created/destroyed during commits, so the only important bit is that there isn't another pending beginFrame task waiting (that contains a list of these pointers) while the commit is taking place. As another possibility, we could actually even avoid posting a task at all, by having all synchronization occur during the commit. The commit could process the impl thread's deletion list at the beginning, and then create a fresh deletion list for the impl thread to use at the end.
Comment on attachment 162358 [details] Patch 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.
(In reply to comment #45) > > 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. > > James, can you confirm that there can *never* be two beginFrame messages at once? Yes.
(In reply to comment #41) > I know this isn't intended to be complete yet, but the big missing piece in my mind is how you intend to delete 'only some resources', as the name of this CL implies. That would fit in at CCPrioritizedTextureManager::reduceMemoryOnImplThread, where we'd put only some Backing*s into backingsToEvict. The reduceMemoryOnImplThread will take a priority cutoff (and maybe a hard limit too). Before adding that, I'll need to push priorities over to the Backing*s themselves. > Currently we only push raw texture ids over to the impl thread. How do you intend to delete some of those raw IDs in all layers that contain them? We just let the now-invalid raw IDs languish in the impl tree. We disable drawing from the tree until we get a commit that was done when the main thread knew about all of the evictions (which guarantees that the impl tree no longer has invalid IDs). I'd like to not require a commit, and have the impl tree be able to prune out the invalid part (and be able to determine "can I draw with the resources that are left?"), but that is a more long-term project.
(In reply to comment #48) > (In reply to comment #41) > > I know this isn't intended to be complete yet, but the big missing piece in my mind is how you intend to delete 'only some resources', as the name of this CL implies. > > That would fit in at CCPrioritizedTextureManager::reduceMemoryOnImplThread, where we'd put only some Backing*s into backingsToEvict. The reduceMemoryOnImplThread will take a priority cutoff (and maybe a hard limit too). Before adding that, I'll need to push priorities over to the Backing*s themselves. > > > Currently we only push raw texture ids over to the impl thread. How do you intend to delete some of those raw IDs in all layers that contain them? > > We just let the now-invalid raw IDs languish in the impl tree. We disable drawing from the tree until we get a commit that was done when the main thread knew about all of the evictions (which guarantees that the impl tree no longer has invalid IDs). > > I'd like to not require a commit, and have the impl tree be able to prune out the invalid part (and be able to determine "can I draw with the resources that are left?"), but that is a more long-term project. I see. This all sounds good. I was worried we would need the long term project before this would be beneficial. The intermediate sounds good! (waiting for a commit to be able to draw). Thanks for the clarification.
This has been cut into a bunch of separate sub-bugs Bug 95596 - [chromium] Add main versus impl thread asserts Bug 96018 - [chromium] Do not delete CCPrioritizedTexture::Backing structures on the main thread Bug 96114 - [chromium] Make prioritized texture manager not touch backings array on the main thread Bug 96463 - [chromium] Evict textures through the texture manager instead of the resource provider Bug 96555 - [chromium] Add clearing of evicted textures from texture upload queues (not landed yet) Closing this one as a duplicate of bug 96463. *** This bug has been marked as a duplicate of bug 96463 ***