Bug 58830

Summary: [chromium] Decouple LayerChromium/CCLayerImpl trees
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: James Robinson <jamesr>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, enne, jamesr, kbr, nduca, vangelis, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 58799, 58840    
Attachments:
Description Flags
work in progress
none
with some unit tests
none
Patch
none
Patch
none
Patch
none
merge fixed, compiles+passes tests
none
merged for 86208
none
Patch kbr: review+

Description James Robinson 2011-04-18 14:44:12 PDT
We need to decouple the ownership of the LayerChromium and CCLayerImpl trees so the CCLayerImpl tree can live on the compositor thread and be safe to concurrent modifications to LayerChromiums.
Comment 1 James Robinson 2011-04-19 19:09:58 PDT
Created attachment 90293 [details]
work in progress
Comment 2 James Robinson 2011-04-19 19:12:27 PDT
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.
Comment 3 James Robinson 2011-04-28 18:22:46 PDT
Created attachment 91611 [details]
with some unit tests
Comment 4 James Robinson 2011-05-02 18:00:27 PDT
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 5 Vangelis Kokkevis 2011-05-05 16:54:19 PDT
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?
Comment 6 James Robinson 2011-05-05 18:10:12 PDT
(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).
Comment 7 Vangelis Kokkevis 2011-05-06 14:11:27 PDT
(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?
Comment 8 James Robinson 2011-05-06 14:36:09 PDT
Created attachment 92641 [details]
Patch
Comment 9 James Robinson 2011-05-06 14:37:33 PDT
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
Comment 10 James Robinson 2011-05-06 18:16:11 PDT
Created attachment 92672 [details]
Patch
Comment 11 James Robinson 2011-05-10 16:13:54 PDT
Created attachment 93035 [details]
Patch
Comment 12 James Robinson 2011-05-10 16:15:23 PDT
Merged the patch up for http://trac.webkit.org/changeset/86189
Comment 13 James Robinson 2011-05-10 16:22:30 PDT
Comment on attachment 93035 [details]
Patch

clearing review - I didn't merge this properly and it won't compile
Comment 14 WebKit Review Bot 2011-05-10 16:30:32 PDT
Comment on attachment 93035 [details]
Patch

Attachment 93035 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8687104
Comment 15 James Robinson 2011-05-10 16:33:50 PDT
Created attachment 93045 [details]
merge fixed, compiles+passes tests
Comment 16 James Robinson 2011-05-11 13:34:42 PDT
Created attachment 93171 [details]
merged for 86208
Comment 17 Adrienne Walker 2011-05-11 18:09:28 PDT
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?
Comment 18 James Robinson 2011-05-11 18:12:21 PDT
(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.
Comment 19 James Robinson 2011-05-13 19:14:12 PDT
Created attachment 93542 [details]
Patch
Comment 20 Vangelis Kokkevis 2011-05-16 18:29:32 PDT
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 21 Kenneth Russell 2011-05-16 19:27:15 PDT
Comment on attachment 93542 [details]
Patch

rs=me
Comment 22 James Robinson 2011-05-16 19:51:30 PDT
Committed r86652: <http://trac.webkit.org/changeset/86652>