NEW 140138
Optimize GraphicsLayer::removeAllChildren()
https://bugs.webkit.org/show_bug.cgi?id=140138
Summary Optimize GraphicsLayer::removeAllChildren()
Zan Dobersek
Reported 2015-01-06 11:52:16 PST
Optimize GraphicsLayer::removeAllChildren()
Attachments
Patch (1.81 KB, patch)
2015-01-06 11:59 PST, Zan Dobersek
no flags
Patch (6.34 KB, patch)
2015-01-07 06:31 PST, Zan Dobersek
no flags
Patch (6.92 KB, patch)
2015-02-02 10:14 PST, Zan Dobersek
no flags
Patch (6.94 KB, patch)
2015-02-02 13:41 PST, Zan Dobersek
no flags
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (247.49 KB, application/zip)
2015-02-02 14:31 PST, Build Bot
no flags
Patch (7.01 KB, patch)
2015-02-04 10:16 PST, Zan Dobersek
simon.fraser: review-
Zan Dobersek
Comment 1 2015-01-06 11:59:30 PST
Simon Fraser (smfr)
Comment 2 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.
Zan Dobersek
Comment 3 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.
Zan Dobersek
Comment 4 2015-01-07 06:31:00 PST
Chris Dumez
Comment 5 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
Zan Dobersek
Comment 6 2015-02-02 10:14:52 PST
Simon Fraser (smfr)
Comment 7 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.
Zan Dobersek
Comment 8 2015-02-02 13:41:20 PST
Build Bot
Comment 9 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.
Build Bot
Comment 10 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
Zan Dobersek
Comment 11 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.
Zan Dobersek
Comment 12 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.
Zan Dobersek
Comment 13 2015-02-04 10:16:40 PST
Darin Adler
Comment 14 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?
Simon Fraser (smfr)
Comment 15 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.
Zan Dobersek
Comment 16 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.
Zan Dobersek
Comment 17 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.
Note You need to log in before you can comment on or make changes to this bug.