Bug 140138

Summary: Optimize GraphicsLayer::removeAllChildren()
Product: WebKit Reporter: Zan Dobersek <zan>
Component: New BugsAssignee: Zan Dobersek <zan>
Status: NEW ---    
Severity: Normal CC: buildbot, cdumez, cmarcelo, commit-queue, kondapallykalyan, luiz, noam, rniwa, sergio, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews106 for mac-mavericks-wk2
none
Patch simon.fraser: review-

Description Zan Dobersek 2015-01-06 11:52:16 PST
Optimize GraphicsLayer::removeAllChildren()
Comment 1 Zan Dobersek 2015-01-06 11:59:30 PST
Created attachment 244083 [details]
Patch
Comment 2 Simon Fraser (smfr) 2015-01-06 12:16:28 PST
Comment on attachment 244083 [details]
Patch

This breaks Mac/iOS because removeFromParent is no longer called, so call to noteSublayersChanged() no longer happens.
Comment 3 Zan Dobersek 2015-01-07 05:54:07 PST
(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.
Comment 4 Zan Dobersek 2015-01-07 06:31:00 PST
Created attachment 244153 [details]
Patch
Comment 5 Chris Dumez 2015-01-27 20:48:42 PST
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
Comment 6 Zan Dobersek 2015-02-02 10:14:52 PST
Created attachment 245884 [details]
Patch
Comment 7 Simon Fraser (smfr) 2015-02-02 10:41:29 PST
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.
Comment 8 Zan Dobersek 2015-02-02 13:41:20 PST
Created attachment 245897 [details]
Patch
Comment 9 Build Bot 2015-02-02 14:31:32 PST
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.
Comment 10 Build Bot 2015-02-02 14:31:36 PST
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
Comment 11 Zan Dobersek 2015-02-04 09:59:54 PST
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.
Comment 12 Zan Dobersek 2015-02-04 09:59:58 PST
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.
Comment 13 Zan Dobersek 2015-02-04 10:16:40 PST
Created attachment 246037 [details]
Patch
Comment 14 Darin Adler 2015-02-07 07:54:31 PST
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?
Comment 15 Simon Fraser (smfr) 2015-02-07 08:52:41 PST
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.
Comment 16 Zan Dobersek 2015-02-10 08:43:45 PST
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.
Comment 17 Zan Dobersek 2015-02-10 08:43:47 PST
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.