The GraphicsLayerChromium::setChildren will call GraphicsLayer::setChildren, and it will call addChild() for each child, which will end up calling updateChildList() N times. This is potentially(N^2).
Created attachment 97552 [details] Proposed patch
Attachment 97552 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 97554 [details] Proposed patch
Created attachment 97556 [details] Proposed patch
For page load performance test, we find that while load a page that has 3D CSS transformation property, disable h/w accleration is 2x of enable h/w acceleration on chromium both on Linux and Win7. Through analysis, we find the root cause is that with hardware acceleration enabled, when update the compositingLayers, the GraphicsLayerChromium::setChildren will call GraphicsLayer::setChildren, and it will call addChild() for each child, which will end up calling updateChildList() N times. This is potentially(N^2). The patch is to add an flag(m_setChildren)in class 'GraphicsLayerChromium' to avoid the unnecessary call, and the gap between disable h/w and enable h/w becomes < 10%.
I assume this is only a problem for Chromium and not other platforms?
Yeah. We used to have this issue on Windows/CA, but now share code with Mac.
Have checked that qt>k also don't have this issue.
Comment on attachment 97556 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=97556&action=review Thanks for the patch! This solution will fix the problem. I do think that the GraphicsLayerChromium child management code needs some clean up and batching up pushes to the platform layers much like the CA implementation does would be nice. However, I think this is a good intermediate solution. > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:133 > + m_setChildren = true; Maybe using m_inSetChildren (instead of m_setChildren) would be more descriptive? > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:135 > // FIXME: GraphicsLayer::setChildren calls addChild() for each child, which This fixme should be removed now.
Comment on attachment 97556 [details] Proposed patch Per Vangelis' review, it looks like this is close to being ready to land. Please submit a revised one and I'll gladly r+ it. (Also, please set the r? and cq? bits on the new patch.)
Created attachment 98856 [details] new patch update the old patch according to Vangelis's comments
Comment on attachment 98856 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=98856&action=review one additional comments.. Otherwise looks good! > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:98 > + m_inSetChildren = false; This should move up to the initializer list.
Created attachment 98864 [details] new patch update the patch: move the "m_inSetChildren = false" to initializer list.
Comment on attachment 98864 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=98864&action=review > Source/WebCore/ChangeLog:16 > +2011-06-27 Zeng Huiqing <huiqing.zeng@intel.com> Double changelog entry...
Created attachment 98867 [details] newPatch sorry for the double changelog entry, have removed it.
Comment on attachment 98867 [details] newPatch View in context: https://bugs.webkit.org/attachment.cgi?id=98867&action=review > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:136 > // FIXME: GraphicsLayer::setChildren calls addChild() for each child, which > // will end up calling updateChildList() N times. You forgot to remove this FIXME.
Created attachment 98994 [details] newPatch
Created attachment 98996 [details] newPatch new patch: remove the 'FIXME'
Comment on attachment 98996 [details] newPatch Thanks, this last version looks good. r=me
Comment on attachment 98996 [details] newPatch Clearing flags on attachment: 98996 Committed r89980: <http://trac.webkit.org/changeset/89980>
All reviewed patches have been landed. Closing bug.
Thanks for the review.