[chromium] Replace RefPtr with OwnPtr for CCLayerImpl tree structure
Created attachment 126614 [details] Patch
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
Created attachment 126837 [details] Patch
(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.
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?
(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.
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.
Created attachment 127246 [details] Patch
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...
Created attachment 127271 [details] Patch
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
(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
(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?
(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.
(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.
Created attachment 128835 [details] Patch
(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).
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
(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?
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.
(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
(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().
Created attachment 128839 [details] Patch
Comment on attachment 128839 [details] Patch R=me awesome! Thanks for persisting with this.
Comment on attachment 128839 [details] Patch Clearing c-q for now, ASAN seems to have an issue with one of the unit tests..
(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.
Committed r108886: <http://trac.webkit.org/changeset/108886>
(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.
Committed r108902: <http://trac.webkit.org/changeset/108902>