Bug 109992 – Minor refactor of RenderLayerCompositor computeCompositingRequirements

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

[Shawn Singh]

We would like to separate updateBacking() and computeCompositingRequirements().

Doing this will allow us to add something like "refineCompositingRequirements()" as a separate walk from originally computing compositing requirements, before we actually go through the trouble of allocating/deallocating backings.  This way we can attempt to compute more intelligent compositing in a multi-pass manner, and such refinements can be optionally skipped based on flags/macros if needed, without modifying the core compositing algorithm.

One detail about this patch - the CompositingChangeRepaint enum was not really ever being used for any other value than "repaintNow", so this enum is removed.  If we want to keep this enum as-is, then we would need to add another property to RenderLayer objects to represent this value for each layer.  Otherwise we would have no way to separate updateBacking from computeCompositingRequirements.

[Shawn Singh]

Created attachment 188676 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 188676 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Minor refactor of RenderLayerCompositor::computeCompositingRequirements
> +        https://bugs.webkit.org/show_bug.cgi?id=109992
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        No new tests needed, no change in behavior expected.

You need to provide more explanation of the change here. Why are you making it?

[Shawn Singh]

(In reply to comment #2)
> (From update of attachment 188676 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188676&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Minor refactor of RenderLayerCompositor::computeCompositingRequirements
> > +        https://bugs.webkit.org/show_bug.cgi?id=109992
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        No new tests needed, no change in behavior expected.
> 
> You need to provide more explanation of the change here. Why are you making it?

I'll add an explanation in the ChangeLog in the next patch.

We would like to have a pattern in RenderLayerCompositor as follows:
 1. computeCompositingRequirements()
 2. refineCompositingRequirements()  // more explanation below
 3. updateLayerBackings()

Currently updateLayerBackings() is effectively integrated into computeCompositingRequirements, so we cannot add refineCompositingRequirements() yet.

The reason we would like to add refineCompositingRequirements() is to add additional intelligence to RenderLayerCompositor.  We would like to add logic that avoids an explosion of composited layers due to overlap.  This can be done as a postprocess to computeCompositingRequirements, recognizing layers that need compositing only because of overlap, and grouping them into one backing.  Even if we add this logic directly into computeCompositingRequirements, we should not incur the overhead of creating backings that may get squashed together, anyway.  Also, refineCompositingRequirements() can be skipped if desired, and this way we won't be invasive to the existing algorithm that is becoming mature and seems to work correctly.

[Simon Fraser (smfr)]

This means that you'll do three passes over the layer tree instead of two?

[Shawn Singh]

(In reply to comment #4)
> This means that you'll do three passes over the layer tree instead of two?

In the current code, I'm only seeing one layer pass in computeCompositingRequirements (not counting rebuildCompositingLayerTree), right?

After this patch, there would be two passes.  The new pass should be lightweight, since there is no RenderGeometryMap tracking / overlap testing.  The original pass no longer updates backings, either, so it is marginally lighter.

Then, with refineCompositingRequirements(), yes there would be three passes.  It should be possible to do this post-process "squashing" without additional RenderGeometryMap / overlap testing as well.

I'm working under the assumption that walking the layer tree on it's own is not expensive - it's the computations we incur during the walk that are expensive.  Do you feel that's a reasonable assumption?

[Simon Fraser (smfr)]

(In reply to comment #5)
> I'm working under the assumption that walking the layer tree on it's own is not expensive - it's the computations we incur during the walk that are expensive.  Do you feel that's a reasonable assumption?

No, some pages have very extensive layer tree, e.g. a facebook page with as much content as the server will provide (keep scrolling to the bottom), or a therverge.com review article with lots of comments.

[Shawn Singh]

(In reply to comment #6)
> (In reply to comment #5)
> > I'm working under the assumption that walking the layer tree on it's own is not expensive - it's the computations we incur during the walk that are expensive.  Do you feel that's a reasonable assumption?
> 
> No, some pages have very extensive layer tree, e.g. a facebook page with as much content as the server will provide (keep scrolling to the bottom), or a therverge.com review article with lots of comments.

OK, fair enough.  =)  On the other hand, we also still have the issue of layer explosion for some sites, too, like gmail, so that still needs to be addressed.

This refactor would still be necessary to do any sort of "layer squashing" feature, though, regardless of whether we implement that in the existing pass, or if we implement it as an additional pass through the layer tree.  The reason it is necessary is because we would not know which layers really need backings until the recursion/walk is complete.

Are you OK with moving forward with this patch then?  I'll try to get some performance numbers to indicate that this additional walk will not add cost for large RenderLayer trees.

[Simon Fraser (smfr)]

Having perf numbers would be good.

[Shawn Singh]

OK, I have some numbers.  

Conclusion: It's no surprise that adding a second pass does increase overhead slightly for 2 out of the 3 cases that I tried, by at most about 80-90 microseconds.  In my personal opinion the extra cost is reasonable and not a problem even on these complicated pages.  Overlap testing still clearly dominates (the verge I think had low overlap complexity), and there are likely many other ways we can consider improving the performance of RenderLayerCompositor in further work.


Summary of results, measuring average milliseconds for each invocation of computeCompositingRequirements:

Digg.com
  ~1700 RenderLayers.
  6.22 ms before,  6.19 ms after

theverge.com article
  http://www.theverge.com/2013/2/20/3999860/lenovo-thinkpad-tablet-2-review 
  ~540 RenderLayers
  0.30 ms before, 0.348 ms after

Gmail
  ~2000 RenderLayers
  5.06 ms before, 5.14 ms after



Raw data for your interest:  this measured total milliseconds all occurrences combined for each trial.

Digg.com:
  approx 1700 RenderLayers

  before patch:
  computeCompositingRequirements trial 1:  1193.632 ms  192 occurrences   (avg: 6.22 ms)
  computeCompositingRequirements trial 2:  1267.236 ms  204 occurrences   (avg: 6.22 ms)

  after patch:
  computeCompositingRequirements trial 3:  1363.162 ms  235 occurrences   (avg: 5.80 ms)
  updateLayerBackings trial 3: 74.941 ms   235 occurrences                (avg: 0.32 ms)

  computeCompositingRequirements trial 4:  1423.607 ms  243 occurrences   (avg: 5.86 ms)
  updateLayerBackings trial 4:  81.32 ms   243 occurrences                (avg: 0.33 ms)


http://www.theverge.com/2013/2/20/3999860/lenovo-thinkpad-tablet-2-review:
  approx 540 RenderLayers

  computeCompositingRequirements trial 1:  391.057ms   1303 occurrences   (avg: 0.30 ms)
  computeCompositingRequirements trial 2:  246.431 ms  818 occurrences    (avg: 0.30 ms)

  computeCompositingRequirements trial 3:  224.162 ms  879 occurrences    (avg: 0.255 ms)
  updateLayerBackings trial 3: 79.818 ms   879 occurrences                (avg: 0.090 ms)
  computeCompositingRequirements trial 4: 210.066 ms    816 occurrences   (avg: 0.257 ms)
  updateLayerBackings trial 4:  74.568 ms 816 occurrences                 (avg: 0.091 ms)

gmail:
  approx 2000 RenderLayers
  computeCompositingRequirements trial 1: 5839.237 ms  1155 occurrences   (avg: 5.06 ms)
  computeCompositingRequirements trial 2: 3295.727 ms  655 occurrences    (avg: 5.03 ms)

  computeCompositingRequirements trial 3:  2752.019 ms 557 occurrences    (avg: 4.94 ms)
  updateLayerBackings trial 3: 140.781 ms  557 occurrences                (avg: 0.253 ms)
  computeCompositingRequirements trial 4:  2846.227 ms  579 occurrences   (avg: 4.91 ms)
  updateLayerBackings trial 4:  133.181 ms 579 occurrences                (avg: 0.23 ms)

[Shawn Singh]

Created attachment 189578 [details]
Added explanation in changelog, otherwise same patch

[Shawn Singh]

This was primarily to support layer squashing in WebKit;  but I will not be pushing for that anymore.   

Simon, I'm happy to discuss how layer squashing might work or cc you on the Blink version of the layer squashing work - please just let me know if you would like to be included.   The layer squashing not-very-clean prototype I had written can be found here:  https://codereview.appspot.com/7625051/ .

[Zan Dobersek]

Comment on attachment 189578 [details]
Added explanation in changelog, otherwise same patch

The work on the bug has ceased, removing the r? flag on this patch.