Bug 62837 – [chromium]Optimize GraphicsLayerChromium::setChildren

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zeng huiqing]

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).

[zeng huiqing]

Created attachment 97552 [details]
Proposed patch

[WebKit Review Bot]

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.

[zeng huiqing]

Created attachment 97554 [details]
Proposed patch

[zeng huiqing]

Created attachment 97556 [details]
Proposed patch

[zeng huiqing]

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%.

[Eric Seidel (no email)]

I assume this is only a problem for Chromium and not other platforms?

[Simon Fraser (smfr)]

Yeah. We used to have this issue on Windows/CA, but now share code with Mac.

[zeng huiqing]

Have checked that qt&gtk also don't have this issue.

[Vangelis Kokkevis]

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.

[Kenneth Russell]

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.)

[zeng huiqing]

Created attachment 98856 [details]
new patch

update the old patch according to Vangelis's comments

[Vangelis Kokkevis]

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.

[zeng huiqing]

Created attachment 98864 [details]
new patch

update the patch: move the "m_inSetChildren = false" to initializer list.

[Vangelis Kokkevis]

Comment on attachment 98864 [details]
new patch

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

> Source/WebCore/ChangeLog:16
> [phone]  Zeng Huiqing  <[e-mail]>

Double changelog entry...

[zeng huiqing]

Created attachment 98867 [details]
newPatch

sorry for the double changelog entry, have removed it.

[Kenneth Russell]

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.

[zeng huiqing]

Created attachment 98994 [details]
newPatch

[zeng huiqing]

Created attachment 98996 [details]
newPatch 

new patch: remove the 'FIXME'

[Kenneth Russell]

Comment on attachment 98996 [details]
newPatch 

Thanks, this last version looks good. r=me

[WebKit Review Bot]

Comment on attachment 98996 [details]
newPatch 

Clearing flags on attachment: 98996

Committed r89980: <http://trac.webkit.org/changeset/89980>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[zeng huiqing]

Thanks for the review.