Bug 140138 – Optimize GraphicsLayer::removeAllChildren()

• Component: New Bugs
• Status: NEW

[Zan Dobersek]

Optimize GraphicsLayer::removeAllChildren()

[Zan Dobersek]

Created attachment 244083 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 244083 [details]
Patch

This breaks Mac/iOS because removeFromParent is no longer called, so call to noteSublayersChanged() no longer happens.

[Zan Dobersek]

(In reply to comment #2)
> Comment on attachment 244083 [details]
> Patch
> 
> This breaks Mac/iOS because removeFromParent is no longer called, so call to
> noteSublayersChanged() no longer happens.

Did not spot that one.

[Zan Dobersek]

Created attachment 244153 [details]
Patch

[Chris Dumez]

Comment on attachment 244153 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:78
> +    WEBCORE_EXPORT virtual void removeAllChildren() override;

Please add __ZN7WebCore15GraphicsLayerCA17removeAllChildrenEv to WebCore.exp.in then run Tools/Scripts/sort-export-file. This should fix mac build

[Zan Dobersek]

Created attachment 245884 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 245884 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:456
> +    noteSublayersChanged();
> +    GraphicsLayer::removeAllChildren();

I think this should be flipped.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:191
> +    didChangeChildren();
> +    GraphicsLayer::removeAllChildren();

Ditto.

[Zan Dobersek]

Created attachment 245897 [details]
Patch

[Build Bot]

Comment on attachment 245897 [details]
Patch

Attachment 245897 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5014396556279808

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 245901 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Zan Dobersek]

Comment on attachment 245884 [details]
Patch

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

>> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:456
>> +    GraphicsLayer::removeAllChildren();
> 
> I think this should be flipped.

This (and the previous iteration as well) caused a bunch of assertions on Mac -- noteSublayersChanged() has to be called for parent once for each child that is being removed.

[Zan Dobersek]

Comment on attachment 245884 [details]
Patch

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

>> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:456
>> +    GraphicsLayer::removeAllChildren();
> 
> I think this should be flipped.

This (and the previous iteration as well) caused a bunch of assertions on Mac -- noteSublayersChanged() has to be called for parent once for each child that is being removed.

[Zan Dobersek]

Created attachment 246037 [details]
Patch

[Darin Adler]

Comment on attachment 246037 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:465
> +    for (size_t i = 0; i < m_children.size(); ++i)
> +        noteSublayersChanged();

Do we really need to call noteSublayersChanged over and over again? How is that useful?

[Simon Fraser (smfr)]

Comment on attachment 246037 [details]
Patch

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

>> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:465
>> +        noteSublayersChanged();
> 
> Do we really need to call noteSublayersChanged over and over again? How is that useful?

No, we only need to call it once.

[Zan Dobersek]

Comment on attachment 246037 [details]
Patch

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

>>> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:465
>>> +        noteSublayersChanged();
>> 
>> Do we really need to call noteSublayersChanged over and over again? How is that useful?
> 
> No, we only need to call it once.

As I wrote in comment #11, Mac EWS rejected the patch that only called noteSublayersChanged() (comment #9 and comment #10).

I'm able to confirm the problem on Mac, with lots of crashes regardless of whether noteSublayersChanged() is called before or after GraphicsLayer::removeAllChildren(). I know nothing about CA and why the crashes occur, but keeping the previous behaviour of notifying about changed sublayers for each removed layer works without a problem. I'll attach backtraces from a debug build.

[Zan Dobersek]

Comment on attachment 246037 [details]
Patch

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

>>> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:465
>>> +        noteSublayersChanged();
>> 
>> Do we really need to call noteSublayersChanged over and over again? How is that useful?
> 
> No, we only need to call it once.

As I wrote in comment #11, Mac EWS rejected the patch that only called noteSublayersChanged() (comment #9 and comment #10).

I'm able to confirm the problem on Mac, with lots of crashes regardless of whether noteSublayersChanged() is called before or after GraphicsLayer::removeAllChildren(). I know nothing about CA and why the crashes occur, but keeping the previous behaviour of notifying about changed sublayers for each removed layer works without a problem. I'll attach backtraces from a debug build.