Bug 78404 – [chromium] Replace RefPtr with OwnPtr for CCLayerImpl tree structure

• Component: New Bugs
• Status: RESOLVED FIXED

[Tien-Ren Chen]

[chromium] Replace RefPtr with OwnPtr for CCLayerImpl tree structure

[Tien-Ren Chen]

Created attachment 126614 [details]
Patch

[James Robinson]

Comment on attachment 126614 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126614&action=review

I like everything except the TreeSynchronizer change.  TreeSynchronizer shouldn't be instantiable, it should just be pure functions.

> Source/WebCore/platform/graphics/chromium/TreeSynchronizer.h:51
> +    TreeSynchronizer() { }

why did you change this? i much prefer having tree syncing be a pure function, this is weird

[Tien-Ren Chen]

Created attachment 126837 [details]
Patch

[Tien-Ren Chen]

(In reply to comment #2)
> (From update of attachment 126614 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126614&action=review
> 
> I like everything except the TreeSynchronizer change.  TreeSynchronizer shouldn't be instantiable, it should just be pure functions.
> 
> > Source/WebCore/platform/graphics/chromium/TreeSynchronizer.h:51
> > +    TreeSynchronizer() { }
> 
> why did you change this? i much prefer having tree syncing be a pure function, this is weird

I was thinking that I need some way to allow a layer to have pointers referring to other layers (for example, m_horizontalScrollbarLayer). I could do that by passing the CCLayerImplMap to LayerChromium::pushPropertiesTo. Instead of passing the map directly, it is better to encapsulate it with TreeSynchronizer so pushPropertiesTo can access the synchronizer state through its public methods. (That's why reuseOrCreateCCLayerImpl was declared public.)

Another reason is having to pass the state in every recursive call is a pain... I find it much better to write recursive calls in functor style. More clean and less error-prone to me. Well that's more of personal preference so I won't insist it.

I uploaded an alternative implementation in pure function style. Either way is fine to me.

[Tien-Ren Chen]

Now I think of it, we still need some way to do inter-layer referencing. The scrolling layer need to know its two scrollbar layers, and the scrollbar layer also needs to know its scrolling layer.

I have two proposed implementation:

A. Keep a ID-->Layer HashMap in CCLayerTreeHostImpl, so the CCLayerImpl only remembers the layer ID, look up the layer pointer on demand.

B. Add another pass in TreeSynchronizer to resolve the pointers after the layer tree has been constructed.

Personally I prefer the second implementation, any comments?

[James Robinson]

(In reply to comment #5)
> Now I think of it, we still need some way to do inter-layer referencing. The scrolling layer need to know its two scrollbar layers, and the scrollbar layer also needs to know its scrolling layer.

We will.  We should address is _separately_ from changing the CCLayerImpl tree ownership.

[James Robinson]

Comment on attachment 126837 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126837&action=review

Unfortunately we can't change the main thread LayerList type away from RefPtr<> at this time.  This may be a show-stopper, at the least I can't think of a simple way to resolve this with the way we share templated code.  Perhaps there is a way with more template indirection but at that point the whole thing becomes less appealing.

> Source/WebCore/platform/graphics/chromium/TreeSynchronizer.h:49
> +    typedef HashMap<int, OwnPtr<CCLayerImpl> > CCLayerImplMap;
>  
> -    typedef HashMap<int, RefPtr<CCLayerImpl> > CCLayerImplMap;
> +    TreeSynchronizer(); // Not instantiable.

no reason to switch the order of these two lines

> Source/WebCore/platform/graphics/chromium/TreeSynchronizer.h:-50
> -    // Declared as static member functions so they can access functions on LayerChromium as a friend class.

I found this comment helpful - did you not?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:206
> +    typedef Vector<LayerChromium*> LayerList;

Woah - I don't think we can do this at all.  We need the LayerLists on the main thread side (such as CCLayerTreeHost::m_updateList) to retain references to their layers because calling into WebKit to paint will sometimes cause the layer tree to get mutated and we can't risk a dangling pointer.

[Tien-Ren Chen]

Created attachment 127246 [details]
Patch

[Tien-Ren Chen]

The new patch addresses the problem with LayerChromium reference holding.

It is done by more template indirection. I agree it is a bit nasty. The biggest problem with different LayerList type in main and CC thread is the difference in accessing the raw pointer. I have to write something like:

inline static LayerChromium* getRawPtr(const RefPtr<LayerChromium>& ptr) { return ptr.get(); }
inline static CCLayerImpl* getRawPtr(CCLayerImpl* ptr) { return ptr; }

I wish WTF::Vector has something similar to WTF::HashMap::peek...

[Tien-Ren Chen]

Created attachment 127271 [details]
Patch

[James Robinson]

Comment on attachment 127271 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127271&action=review

I think this is looking quite good.  I would like to see the mask/replica issue resolved and I have a style quibble with the way many of the tests have been transformed, but otherwise this looks great and I'd like to make this change ASAP.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:-327
> -    if (m_maskLayer == maskLayer)
> -        return;
> -

does this mean that we'll always call noteLayerPropertyChangedForSubtree() even if we're setting the mask layer to the same thing it used to be? that'll pretty much completely disable scissoring and any other damage tracking based optimizations for pages that have masks or replicas :/

is there any way to preserve this?  maybe use IDs instead of pointers if we need to?

> Source/WebKit/chromium/tests/CCLayerImplTest.cpp:69
> +    OwnPtr<CCLayerImpl> opChild = CCLayerImpl::create(2);

it's fairly unusual to have an "op" prefix.  we use the "pop" prefix for PassOwnPtr<> when we put the result immediately into a local and never use the prefixed variable again. another issue with this construction is that the .release() call nulls out opChild, but it's still sitting in the scope tempting someone to write code that incorrectly tries to use it

here's another way to write this that would be more consistent with other WebKit code and a bit less error-prone:

OwnPtr<CCLayerImpl> root = CCLayerImpl::create(1);
root->addChild(CCLayerImpl::create(2));
child->addChild(CCLayerImpl::create(3));
CCLayerImpl* child = root->children()[0];
CCLayerImpl* grandChild = child->children()[0];

etc.

same goes for rest of the tests

[Tien-Ren Chen]

(In reply to comment #11)
> (From update of attachment 127271 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127271&action=review
> 
> I think this is looking quite good.  I would like to see the mask/replica issue resolved and I have a style quibble with the way many of the tests have been transformed, but otherwise this looks great and I'd like to make this change ASAP.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:-327
> > -    if (m_maskLayer == maskLayer)
> > -        return;
> > -
> 
> does this mean that we'll always call noteLayerPropertyChangedForSubtree() even if we're setting the mask layer to the same thing it used to be? that'll pretty much completely disable scissoring and any other damage tracking based optimizations for pages that have masks or replicas :/
> 
> is there any way to preserve this?  maybe use IDs instead of pointers if we need to?

By using PassOwnPtr/OwnPtr we automatically prevent this from happening. Comparing two own pointers should always return false because an object can't be owned by two at the same time. (wtf emits compile error if you try to compare)
 
> > Source/WebKit/chromium/tests/CCLayerImplTest.cpp:69
> > +    OwnPtr<CCLayerImpl> opChild = CCLayerImpl::create(2);
> 
> it's fairly unusual to have an "op" prefix.  we use the "pop" prefix for PassOwnPtr<> when we put the result immediately into a local and never use the prefixed variable again. another issue with this construction is that the .release() call nulls out opChild, but it's still sitting in the scope tempting someone to write code that incorrectly tries to use it
> 
> here's another way to write this that would be more consistent with other WebKit code and a bit less error-prone:
> 
> OwnPtr<CCLayerImpl> root = CCLayerImpl::create(1);
> root->addChild(CCLayerImpl::create(2));
> child->addChild(CCLayerImpl::create(3));
> CCLayerImpl* child = root->children()[0];
> CCLayerImpl* grandChild = child->children()[0];
> 
> etc.
> 
> same goes for rest of the tests

Got it

[Shawn Singh]

(In reply to comment #12)
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:-327
> > > -    if (m_maskLayer == maskLayer)
> > > -        return;
> > > -
> > 
> > does this mean that we'll always call noteLayerPropertyChangedForSubtree() even if we're setting the mask layer to the same thing it used to be? that'll pretty much completely disable scissoring and any other damage tracking based optimizations for pages that have masks or replicas :/
> > 
> > is there any way to preserve this?  maybe use IDs instead of pointers if we need to?
> 
> By using PassOwnPtr/OwnPtr we automatically prevent this from happening. Comparing two own pointers should always return false because an object can't be owned by two at the same time. (wtf emits compile error if you try to compare)
> 


I was just "driving by", and this detail is still unclear to me.  Tien-Ren@ can you please clarify?  Do you mean that somehow we still retain the benefit of the early-exit?  If so, how does it work - am I missing something obvious?

[James Robinson]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 127271 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=127271&action=review
> > 
> > I think this is looking quite good.  I would like to see the mask/replica issue resolved and I have a style quibble with the way many of the tests have been transformed, but otherwise this looks great and I'd like to make this change ASAP.
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:-327
> > > -    if (m_maskLayer == maskLayer)
> > > -        return;
> > > -
> > 
> > does this mean that we'll always call noteLayerPropertyChangedForSubtree() even if we're setting the mask layer to the same thing it used to be? that'll pretty much completely disable scissoring and any other damage tracking based optimizations for pages that have masks or replicas :/
> > 
> > is there any way to preserve this?  maybe use IDs instead of pointers if we need to?
> 
> By using PassOwnPtr/OwnPtr we automatically prevent this from happening. Comparing two own pointers should always return false because an object can't be owned by two at the same time. (wtf emits compile error if you try to compare)
> 

I know the pointer value will not match.  That's not the point. Logically, the mask and replica layers will very frequently be the same from one tree sync to the next.  It's important for performance that we know if the mask/replicas are they same as they were on the previous frame or not.  If we don't track this, then we have to assume that it's changed and redraw the layer completely after every sync which would suck.

One potential solution since we can't rely on pointer identity with this patch would be to compare layer IDs to that on the previous frame.

[Tien-Ren Chen]

(In reply to comment #14)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (From update of attachment 127271 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=127271&action=review
> > > 
> > > I think this is looking quite good.  I would like to see the mask/replica issue resolved and I have a style quibble with the way many of the tests have been transformed, but otherwise this looks great and I'd like to make this change ASAP.
> > > 
> > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:-327
> > > > -    if (m_maskLayer == maskLayer)
> > > > -        return;
> > > > -
> > > 
> > > does this mean that we'll always call noteLayerPropertyChangedForSubtree() even if we're setting the mask layer to the same thing it used to be? that'll pretty much completely disable scissoring and any other damage tracking based optimizations for pages that have masks or replicas :/
> > > 
> > > is there any way to preserve this?  maybe use IDs instead of pointers if we need to?
> > 
> > By using PassOwnPtr/OwnPtr we automatically prevent this from happening. Comparing two own pointers should always return false because an object can't be owned by two at the same time. (wtf emits compile error if you try to compare)
> > 
> 
> I know the pointer value will not match.  That's not the point. Logically, the mask and replica layers will very frequently be the same from one tree sync to the next.  It's important for performance that we know if the mask/replicas are they same as they were on the previous frame or not.  If we don't track this, then we have to assume that it's changed and redraw the layer completely after every sync which would suck.
> 
> One potential solution since we can't rely on pointer identity with this patch would be to compare layer IDs to that on the previous frame.

Ah, I see what you mean. That's indeed a tricky problem. I think layer ID is the way to go. Will solve this in next patch.

[Tien-Ren Chen]

Created attachment 128835 [details]
Patch

[Tien-Ren Chen]

(In reply to comment #11)
> > Source/WebKit/chromium/tests/CCLayerImplTest.cpp:69
> > +    OwnPtr<CCLayerImpl> opChild = CCLayerImpl::create(2);
> 
> it's fairly unusual to have an "op" prefix.  we use the "pop" prefix for PassOwnPtr<> when we put the result immediately into a local and never use the prefixed variable again. another issue with this construction is that the .release() call nulls out opChild, but it's still sitting in the scope tempting someone to write code that incorrectly tries to use it
> 
> here's another way to write this that would be more consistent with other WebKit code and a bit less error-prone:
> 
> OwnPtr<CCLayerImpl> root = CCLayerImpl::create(1);
> root->addChild(CCLayerImpl::create(2));
> child->addChild(CCLayerImpl::create(3));
> CCLayerImpl* child = root->children()[0];
> CCLayerImpl* grandChild = child->children()[0];
> 
> etc.
> 
> same goes for rest of the tests

I find this results in behavior change in some case, for example when the layer properties need to be configured before added to the tree. In those case I'll use leakPtr() then adoptPtr() again. I think this approach is not too error-prone as the dirty part is limited in between leakPtr/adoptPtr pair only (which is usually short).

[James Robinson]

Comment on attachment 128835 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128835&action=review

I like this patch and think it's really close but we definitely do not want to add a bunch of leakPtr() calls to the test. leakPtr() pretty much always indicates a bug or a boundary with an API that isn't 100% compatible with the WTF smart pointer types.  For instance, today we use leakPtr() in one place to pass a value between threads and in another place to interface with the WebKit API which has a different set of smart pointers.  Otherwise we should never use it.  I commented on two specific instances with what I think would be a better approach, I believe that one or the other can be extended to the rest of the tests.

> Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:46
> +    CCLayerImpl* ccLayerImpl = popCCLayerImpl.get();

the normal idiom for functions that take a PassOwnPtr<> parameter is to immediately put the parameter in a local OwnPtr<> and then never use the POP<> parameter again.  I do not think the code you've written here is incorrect, but I think the idiomatic way would be equally correct and less error-prone.

IOW, have ccLayerImpl be an OwnPtr<CCLayerImpl>, leave the null-check-and-return alone, and move the map.set() call to the end of the function and have it take ccLayerImpl.release()

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:46
> +    , m_maskLayerID(-1)
> +    , m_replicaLayerID(-1)

nit we use "xxxId" in other places (like m_layerId below). I don't have strong feelings on which is better but we should be consistent, and "xxxId" seems to be winning.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerIteratorPosition.h:1
> +/*

i think you have a bad merge here - this file was deleted upstream in http://trac.webkit.org/changeset/108678

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:347
> +

nit: don't need the newline here

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:323
> +    CCLayerImpl* child2 = CCLayerImpl::create(3).leakPtr();

This is unnecessarily confusing - leakPtr() pretty much always means that there's a leak and requires special care, in this case the code looks buggy unless you look down to the adoptPtr() call 9 lines below.  I think what you really want here is to have child2 be an OwnPtr<> and .release() it in the addChild() call.

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:359
> +    CCLayerImpl* child2 = CCLayerImpl::create(3).leakPtr();

again the leakPtr() is unnecessarily scary. In this instance, it's probably simpler to add child2 to root's children here and then grab a raw pointer to it via .get() since you need to dereference it later on in the test

[Shawn Singh]

(In reply to comment #17)
> I find this results in behavior change in some case, for example when the layer properties need to be configured before added to the tree. In those case I'll use leakPtr() then adoptPtr() again. I think this approach is not too error-prone as the dirty part is limited in between leakPtr/adoptPtr pair only (which is usually short).

Would PassOwnPtr somehow be useful in this situation?

[James Robinson]

http://www.webkit.org/coding/RefPtr.html is a useful (albeit somewhat data) guide for when to use XXXPtr<> and PassXXXPtr<>.  It's written for RefPtr/PassRefPtr, but most of it applies to OwnPtr/PassOwnPtr.  In particular, PassRefPtr/PassOwnPtr should never be locals and there are some nice general guidelines about how they interact with RefPtr/OwnPtr.

[Tien-Ren Chen]

(In reply to comment #18)
> (From update of attachment 128835 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128835&action=review
> 
> I like this patch and think it's really close but we definitely do not want to add a bunch of leakPtr() calls to the test. leakPtr() pretty much always indicates a bug or a boundary with an API that isn't 100% compatible with the WTF smart pointer types.  For instance, today we use leakPtr() in one place to pass a value between threads and in another place to interface with the WebKit API which has a different set of smart pointers.  Otherwise we should never use it.  I commented on two specific instances with what I think would be a better approach, I believe that one or the other can be extended to the rest of the tests.

Ok I'll eliminate the leakPtr().

> > Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:46
> > +    CCLayerImpl* ccLayerImpl = popCCLayerImpl.get();
> 
> the normal idiom for functions that take a PassOwnPtr<> parameter is to immediately put the parameter in a local OwnPtr<> and then never use the POP<> parameter again.  I do not think the code you've written here is incorrect, but I think the idiomatic way would be equally correct and less error-prone.
> 
> IOW, have ccLayerImpl be an OwnPtr<CCLayerImpl>, leave the null-check-and-return alone, and move the map.set() call to the end of the function and have it take ccLayerImpl.release()

Wow this is brilliant! I love this solution. done

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:46
> > +    , m_maskLayerID(-1)
> > +    , m_replicaLayerID(-1)
> 
> nit we use "xxxId" in other places (like m_layerId below). I don't have strong feelings on which is better but we should be consistent, and "xxxId" seems to be winning.
done

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerIteratorPosition.h:1
> > +/*
> 
> i think you have a bad merge here - this file was deleted upstream in http://trac.webkit.org/changeset/108678
done

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:347
> > +
> 
> nit: don't need the newline here
done

> > Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:323
> > +    CCLayerImpl* child2 = CCLayerImpl::create(3).leakPtr();
> 
> This is unnecessarily confusing - leakPtr() pretty much always means that there's a leak and requires special care, in this case the code looks buggy unless you look down to the adoptPtr() call 9 lines below.  I think what you really want here is to have child2 be an OwnPtr<> and .release() it in the addChild() call.

Then again we have the problem that child2 will be null after the release() call...
I'll limit the scope of child2 in cases like this, then declare another raw pointer child2 later on if used. Sounds better?

> 
> > Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:359
> > +    CCLayerImpl* child2 = CCLayerImpl::create(3).leakPtr();
> 
> again the leakPtr() is unnecessarily scary. In this instance, it's probably simpler to add child2 to root's children here and then grab a raw pointer to it via .get() since you need to dereference it later on in the test

ditto

[James Robinson]

(In reply to comment #21) 
> > > Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:323
> > > +    CCLayerImpl* child2 = CCLayerImpl::create(3).leakPtr();
> > 
> > This is unnecessarily confusing - leakPtr() pretty much always means that there's a leak and requires special care, in this case the code looks buggy unless you look down to the adoptPtr() call 9 lines below.  I think what you really want here is to have child2 be an OwnPtr<> and .release() it in the addChild() call.
> 
> Then again we have the problem that child2 will be null after the release() call...
> I'll limit the scope of child2 in cases like this, then declare another raw pointer child2 later on if used. Sounds better?
> 

I think that sounds like the right approach.  It still may be awkward in some cases, but IMO awkward-looking code without leakPtr() is generally either obvious to correct or very reliably fails.  It's difficult to write a subtle memory leak or race without leakPtr().

[Tien-Ren Chen]

Created attachment 128839 [details]
Patch

[James Robinson]

Comment on attachment 128839 [details]
Patch

R=me awesome! Thanks for persisting with this.

[James Robinson]

Comment on attachment 128839 [details]
Patch

Clearing c-q for now, ASAN seems to have an issue with one of the unit tests..

[James Robinson]

(In reply to comment #25)
> (From update of attachment 128839 [details])
> Clearing c-q for now, ASAN seems to have an issue with one of the unit tests..

Ah, ASAN caught an interesting bug in removeFromParent():

82	void CCLayerImpl::removeFromParent()
83	{
84	    if (!m_parent)
85	        return;
86	    for (size_t i = 0; i < m_parent->m_children.size(); ++i) {
87	        if (m_parent->m_children[i].get() == this) {
88	            m_parent->m_children.remove(i);
89	            break;
90	        }
91	    }
92	    m_parent = 0;
93      }

Line 88 was deleting 'this' and line 92 was attempting to set a member variable.  Shuffling the order around fixes this and results in a clean ASAN run of webkit_unit_tests.  I'll run the layout tests under ASAN, then land with this fix.

[James Robinson]

Committed r108886: <http://trac.webkit.org/changeset/108886>

[Tien-Ren Chen]

(In reply to comment #26)
> (In reply to comment #25)
> > (From update of attachment 128839 [details] [details])
> > Clearing c-q for now, ASAN seems to have an issue with one of the unit tests..
> 
> Ah, ASAN caught an interesting bug in removeFromParent():
> 
> 82    void CCLayerImpl::removeFromParent()
> 83    {
> 84        if (!m_parent)
> 85            return;
> 86        for (size_t i = 0; i < m_parent->m_children.size(); ++i) {
> 87            if (m_parent->m_children[i].get() == this) {
> 88                m_parent->m_children.remove(i);
> 89                break;
> 90            }
> 91        }
> 92        m_parent = 0;
> 93      }
> 
> Line 88 was deleting 'this' and line 92 was attempting to set a member variable.  Shuffling the order around fixes this and results in a clean ASAN run of webkit_unit_tests.  I'll run the layout tests under ASAN, then land with this fix.

Now I think of it, why bother who is my mommy when I'm going to die anyway?
I think we can extend this function to return a PassOwnPtr to allow re-parenting a layer, so the PassOwnPtr can also extend the lifetime of 'this' until it returns.

[Adrienne Walker]

Committed r108902: <http://trac.webkit.org/changeset/108902>