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.
Created attachment 188676 [details] Patch
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?
(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.
This means that you'll do three passes over the layer tree instead of two?
(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?
(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.
(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.
Having perf numbers would be good.
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)
Created attachment 189578 [details] Added explanation in changelog, otherwise same patch
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/ .
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.