Bug 58830

Created attachment 90293 [details] work in progress

Initial patch for reference. This passes the compositor/ tests but is not really an improvement by itself and won't really be much better until bug 58833 is resolved. After that happens we can get rid of the back/forward pointers between LayerChromium and CCLayerImpls which will clean up the code a good bit. I also need to add explicit unit tests for TreeSynchronizer and make sure that the context lost case works.

Created attachment 91611 [details] with some unit tests

Comment on attachment 91611 [details] with some unit tests I think this is OK to land now and need it in order to proceed on bug https://bugs.webkit.org/show_bug.cgi?id=58840, so requesting review.

Comment on attachment 91611 [details] with some unit tests View in context: https://bugs.webkit.org/attachment.cgi?id=91611&action=review Looks good overall other than the tree-syncing code which seems very inefficient. I would suggest adding some dirty flags to at least be able to know whether the subtree tree geometry changes between draws. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:714 > + const TransformationMatrix& layerDrawMatrix = layer->renderSurface() ? layer->renderSurface()->m_drawTransform : layer->drawTransform(); You'll need to sync as this code is now gone. > Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:54 > + CCLayerImplMap::iterator it = map.find(layer->id()); This seems rather inefficient. Every time you draw you essentially destroy all connectivity information on the CCLayerImpl tree and copy all properties over. I think without some notion of dirty flags this will end up being quite expensive. Even before adding flags it would be faster to simply go through all the layers in the map and simply reset their parent and clear their child list instead of painfully doing it recursively. Or even throw the ccLayerImpl's out and start from scratch. > Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:85 > + if (oldCCLayerImplRoot) Why not keep the old map around rather than rebuilding it? > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:157 > LayerChromium* m_owner; The m_owner pointer is used freely in this class . What happens if the LayerChromium is destroyed. Isn't this going to end up being a dangling pointer?

(In reply to comment #5) > (From update of attachment 91611 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=91611&action=review > > Looks good overall other than the tree-syncing code which seems very inefficient. I would suggest adding some dirty flags to at least be able to know whether the subtree tree geometry changes between draws. > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:714 > > + const TransformationMatrix& layerDrawMatrix = layer->renderSurface() ? layer->renderSurface()->m_drawTransform : layer->drawTransform(); > > You'll need to sync as this code is now gone. Will do. > > > Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:54 > > + CCLayerImplMap::iterator it = map.find(layer->id()); > > This seems rather inefficient. Every time you draw you essentially destroy all connectivity information on the CCLayerImpl tree and copy all properties over. I think without some notion of dirty flags this will end up being quite expensive. Even before adding flags it would be faster to simply go through all the layers in the map and simply reset their parent and clear their child list instead of painfully doing it recursively. Or even throw the ccLayerImpl's out and start from scratch. The property behavior is not different from what we currently have. We can add dirty flags for that if/when it turns out to be a performance issue. I don't think that rebuilding the tree structure is particularly expensive since we have on the order of a few dozen layers. This particular tree sync is of the same complexity (although a bit cheaper) than RenderLayerCompositor::rebuildCompositingLayerTree, which we currently call on every frame when scrolling and (due to a bug) on every frame when WebGL/canvas2d are used. On my desktop the tree sync operation takes an average of 0.022ms/frame on poster circle, 0.37ms/frame on snowstack. If we see that it's too expensive we can optimize, but adding geometry dirty flags makes the logic a lot more complicated and I'd rather not spent time optimizing that when the other parts of the compositor need so much more attention. I'll add a TRACE_EVENT to make it easier to keep tabs on the cost of this step. > > > Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:85 > > + if (oldCCLayerImplRoot) > > Why not keep the old map around rather than rebuilding it? The map holds a reference to each CCLayerImpl. We have to drop the map at the end of the sync in order to delete the CCLayerImpls that are not referenced in the new tree. > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:157 > > LayerChromium* m_owner; > > The m_owner pointer is used freely in this class . What happens if the LayerChromium is destroyed. Isn't this going to end up being a dangling pointer? Hence the FIXME/TODOs :). It is a bit fragile for now. The current code depends on the trees being synchronized before any functions using m_owner are called on CCLayerImpls and that no LayerChromiums are destroyed until the draw cycle is complete. Both of these invariants are true today, but we definitely need to get rid of the m_owner pointer ASAP in order to do that. https://bugs.webkit.org/show_bug.cgi?id=58833 is the main blocker there, but I can't make much progress on that until this one lands (lovely chicken-and-egg problem).

(In reply to comment #6) > > > > > Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:54 > > > + CCLayerImplMap::iterator it = map.find(layer->id()); > > > > This seems rather inefficient. Every time you draw you essentially destroy all connectivity information on the CCLayerImpl tree and copy all properties over. I think without some notion of dirty flags this will end up being quite expensive. Even before adding flags it would be faster to simply go through all the layers in the map and simply reset their parent and clear their child list instead of painfully doing it recursively. Or even throw the ccLayerImpl's out and start from scratch. > > The property behavior is not different from what we currently have. We can add dirty flags for that if/when it turns out to be a performance issue. > > I don't think that rebuilding the tree structure is particularly expensive since we have on the order of a few dozen layers. This particular tree sync is of the same complexity (although a bit cheaper) than RenderLayerCompositor::rebuildCompositingLayerTree, which we currently call on every frame when scrolling and (due to a bug) on every frame when WebGL/canvas2d are used. On my desktop the tree sync operation takes an average of 0.022ms/frame on poster circle, 0.37ms/frame on snowstack. If we see that it's too expensive we can optimize, but adding geometry dirty flags makes the logic a lot more complicated and I'd rather not spent time optimizing that when the other parts of the compositor need so much more attention. > > I'll add a TRACE_EVENT to make it easier to keep tabs on the cost of this step. > I guess you're right. As long as we can keep tabs on it for now and mark it it will be ok. Maybe at least consider blindly nuking the parent pointer and children lists instead of removing them one by one?

Created attachment 92641 [details] Patch

Changes in this patch: - merges up to ToT - gets rid of unnecessary parent/child pointer adjustments during sync - adds proper changelog for the unit tests

Created attachment 92672 [details] Patch

Created attachment 93035 [details] Patch

Merged the patch up for http://trac.webkit.org/changeset/86189

Comment on attachment 93035 [details] Patch clearing review - I didn't merge this properly and it won't compile

Comment on attachment 93035 [details] Patch Attachment 93035 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8687104

Created attachment 93045 [details] merge fixed, compiles+passes tests

Created attachment 93171 [details] merged for 86208

Comment on attachment 93171 [details] merged for 86208 Yay, more testing. I'd love to see some tests that include mask and replica layers, because those always break. Also, you have simpleTreeFromEmpty, so maybe a simpleTree*To*Empty where you destroy all the layers would be a good test as well. Can the layer being passed into synchronizeTrees ever be NULL?

(In reply to comment #17) > (From update of attachment 93171 [details]) > Yay, more testing. I'd love to see some tests that include mask and replica layers, because those always break. Also, you have simpleTreeFromEmpty, so maybe a simpleTree*To*Empty where you destroy all the layers would be a good test as well. > > Can the layer being passed into synchronizeTrees ever be NULL? The LayerChromium* can never be NULL but the CCLayerImpl* can be (and not infrequently is) NULL. Those all sound like good tests, I'll add some.

Created attachment 93542 [details] Patch

Comment on attachment 93542 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93542&action=review Looks good > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:69 > , m_debugID(s_nextLayerDebugID++) Looks like m_debugID's can now be replaced by the m_layerID's ? > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:51 > // When this class gets subclasses, remember to add 'virtual' here. The comment can be removed now?

Comment on attachment 93542 [details] Patch rs=me

Committed r86652: <http://trac.webkit.org/changeset/86652>