All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=814227

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=d5a76edb49ef39c233f14caf18b2655afec7340d3044c2523a3ac71ef1b43c25


Bot(s) for this bug's original alert(s):

android-nexus5X
android-webview-nexus5X
android-webview-nexus6
chromium-rel-win7-gpu-ati
win-high-dpi

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/178561b7840000

📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/178561b7840000

[SPv175] Enable SlimmingPaintV175 by default by wangxianzhu@chromium.org
https://chromium.googlesource.com/chromium/src/+/0a9a5c311a1d3a298f952e495510bd6fe3faa2f6

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

 Issue 814237  has been merged into this issue.

We create 129 PaintOps (152 skia paint methods) for the test when SPv175 is enabled. Otherwise we create only 85 PaintOps (108 skia paint method).

Here are several patterns of the extra operations:
1. Extra restore and save:
 SPv175:
   save
    concat
    drawing1
   restore
   save
    concat (same as the previous concat)
    drawing2
   restore
   ...
 SPv1:
   save
    concat
    drawing1
    drawing2
   restore

2. We always output two pairs of saveLayer/restore for each effect node
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graphics/compositing/PaintChunksToCcLayer.cpp?rcl=9e35840d7795775ae97b5eae59a36227f0967176&l=298)

The extra restore seems caused by effect nodes' null OutputClip() which causes us to always pop all clip nodes.

The extra restore and save maybe related to multicol. In SPv1 a single saveLayer applies to all fragments, while in SPv175 each fragment has its own effect node.

For (2), sorry I added that lame todo. :(
However I can't recall why we really have to apply filter separately from other effects. I believe Skia does apply blending and opacity in the expected order (enclosing the filter). Do you mind to try combining both saveLayer to see if anything fails?

WIP CLs:

1. Don't exit clip states for effects without OutputClip in common cases: https://chromium-review.googlesource.com/c/chromium/src/+/930464

2. Don't issue no-op saveLayer operations for effect nodes:
https://chromium-review.googlesource.com/c/chromium/src/+/929878

There are still some layout test failures to investigate.

The problem is about the following situations:
1.       |Chunk1|Chunk2|Chunk3|
  Clip:  |======|======|
Effect:         |======|======|

2.       |Chunk1|Chunk2|Chunk3|
  Clip:  |======|======|======|
Effect:         |======|

In SwitchToEffect for Chunk2, for now we just exit Clip at the beginning of Chunk2, apply Effect, and apply Clip again, which breaks the Clip into 2 or 3:

Situation1:
  Chunk1:
    Save Clip
  Chunk2:
    Restore Clip
    SaveLayer Effect
      Save Clip
  Chunk3:
      Restore Clip
    Restore Effect
which seems good.

Situation2:
  Chunk1:
    Save Clip
  Chunk2:
    Restore Clip
    SaveLayer Effect
      Save Clip
  Chunk3:
      Restore Clip
    Restore Effect
    Save Clip
which should be simplified to
  Chunk1:
    Save Clip
  Chunk2:
    SaveLayer Effect
  Chunk3:
    Restore Effect

It seems that a linear algorithm is not enough. One thought is to save the operations for active effects into a stack, and when an effect ends, output the top of the stack, with extra operations stripped, to cc_layer_.

Thought of two methods:

Method 1
1. Start a new cc::DisplayItemList for each effect and put it into state_stack_ if
  - the effect's OutputClip is null, and
  - the top of state_stack_ is kClip.
2. When a clip ends, and the top of state_stack_ is kEffect
  add the clip into a pending_reset_clip_ list of the top the state_stack_.
3. When an effect ends, issue sequence Restore/Restore.../SaveLayer/Save/Clip/Save/Clip/... (each pair of Restore and Save/Clip is for each item in pending_reset_clip_ list) into the previous cc::DisplayItemList in state_stack_ and append all operations in the top cc::DisplayItemList into previous cc::DisplayItemList.

The biggest drawback of the method is that we may copy some operations multiple times, even if there is no interleaving clip and effect.

Method 2 is modified from Method 1, but requires DisplayItemList supports insert. Instead of buffering operations in temporary cc::DisplayItemLists, we just first push operations under effect just like the effect won't interleave with any clip. In 2 when we find interleaving clip, we insert Restore and Save/Clip around the effect's SaveLayer sequence. In this method, we just move operations in rare interleaving effect/clip cases.

I'm thinking of more details.

I see. For the problem 1 "extra clip save/restore for effect without output clip", I think a less intrusive fix is possible.

Essentially what CL 930464 does is to detect whether whatever the current clip is a common clip ancestor of all chunks in such effect, and apply the clip as a group if possible. However doing so has a side effect of changing anti-aliasing behavior. 

For example, the presence of a clip-escaping child affects whether a common clip can be applied as a group:

<div style="overflow:hidden; width:100px; height:100px;">
  <div style="position:relative;">foo</div>
  <div style="opacity:0.5;">
    <div style="width:200px; height:200px; background:red;">
      <div style="width:200px; height:200px; background:green;">The rendering of this element can be affected</div>
    </div>
    <div style="position:absolute;">by the presence of this element</div>
  </div>
</div>

Or worse, the clip state of the paint chunk right before entering the effect (which can be something totally unrelated) can also affect the common clip will be applied as a group:

<div style="overflow:hidden; width:100px; height:100px;">
  <div style="position:relative;">The presence of this element also affect</div>
  <div style="opacity:0.5;">
    <div style="width:200px; height:200px; background:red;">
      <div style="width:200px; height:200px; background:green;">the rendering of this element</div>
    </div>
  </div>
</div>

Moreover, I don't think SPv175 would generate less efficient drawing commands than SPv1, because PaintLayerPainter doesn't perform such clip-grouping conversion anyway. I think the performance regression is specific to SVG, because SVG never escape clips, and when SVGPaintContext applies effects, the effect is indeed enclosed by whatever clip inherited from ancestor.

I think a less intrusive fix would be changing PaintPropertyTreeBuilder, so that the output clip of a SVG effect is always set to the current clip of the in-flow context. Less code needs to be added, and is behaviorally more inline with SPv1.

Thanks for the examples. https://chromium-review.googlesource.com/c/chromium/src/+/934842 is trying the SVG OutputClip method you proposed.

I'm not sure if I saw real anti-alias differences in the examples. If I change the opacity from 0.5 to 0.501, both tests will show no rendering difference under the opacity. This looks to me like some error of alpha blending for opacity 0.5 instead of anti-alias. Do you have examples showing real anti-alias differences?

Also we still have another regression. For

<div style="overflow:hidden; width:100px; height:100px;">
  <div style="overflow:hidden; width:100px; height:100px;">
    <div style="overflow:hidden; width:100px; height:100px;">
      <div style="position:relative;">Foo</div>
      <div style="opacity:0.5;">
        <div style="width:200px; height:200px; background:green;"></div>
      </div>
    </div>
  </div>
</div>

SPv175 outputs 12 operations more than SPv1 because in SPv1 the clips are combined. Will experiment combining multiple consecutive clips into one to see performance impact.

Just found that we output more clip operations in SPv175 for the following case:

<!DOCTYPE html>
<div style="overflow: hidden; border-radius: 20px; width: 200px; height: 200px">
  <div style="width: 300px; height: 300px; background: blue"></div>
</div>

We created InnerBorderRadiusClip and OverflowClip:
    InnerBorderRadiusClip (LayoutBlockFlow DIV)
      OverflowClip (LayoutBlockFlow DIV)
and for InnerBorderRadiusClip we output:
  Save
    ClipRect
    ClipRRect
and for OverflowClip we output
    Save
      ClipRect

SPv1 outputs the first sequence only. Perhaps we should omit OverflowClip if we have outputted InnerBorderRadiusClip. Or can we combine them into one?

Another question: Can we just output ClipRRect without the preceding ClipRect?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b202297fb6e880cadbfaf52678f095186757ac7d

commit b202297fb6e880cadbfaf52678f095186757ac7d
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Fri Feb 23 21:00:28 2018

[SPv175+] Optimize cc operations for blink effect nodes

Previously we always output 2 saveLayers for each effect node, one
for non-filter effect another for filter effect. Now don't output
no-op saveLayer operations.

Bug:  814227 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I4462c387dfaf3fa246dcf6df6ebca84f30a5d2a2
Reviewed-on: https://chromium-review.googlesource.com/929878
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538894}
[modify] https://crrev.com/b202297fb6e880cadbfaf52678f095186757ac7d/third_party/WebKit/LayoutTests/css3/filters/root-renderer-with-opacity-filter-expected.html
[modify] https://crrev.com/b202297fb6e880cadbfaf52678f095186757ac7d/third_party/WebKit/LayoutTests/css3/filters/root-renderer-with-opacity-filter.html
[modify] https://crrev.com/b202297fb6e880cadbfaf52678f095186757ac7d/third_party/WebKit/LayoutTests/http/tests/devtools/layers/layer-canvas-log-expected.txt
[modify] https://crrev.com/b202297fb6e880cadbfaf52678f095186757ac7d/third_party/WebKit/Source/platform/graphics/compositing/PaintChunksToCcLayer.cpp
[modify] https://crrev.com/b202297fb6e880cadbfaf52678f095186757ac7d/third_party/WebKit/Source/platform/graphics/compositing/PaintChunksToCcLayerTest.cpp

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3c07cd368caae990955af6f1eaea5d078425bb89

commit 3c07cd368caae990955af6f1eaea5d078425bb89
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Sat Feb 24 17:33:02 2018

[SPv175] Use the current clip as OutputClip of effect nodes for SVG children

This avoids unnecessary existing and re-enter/exiting clips within the
effect nodes.

Bug:  814227 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Iba12228c06663a4764a3c612cba2451ddc15b378
Reviewed-on: https://chromium-review.googlesource.com/934842
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539015}
[modify] https://crrev.com/3c07cd368caae990955af6f1eaea5d078425bb89/third_party/WebKit/LayoutTests/http/tests/devtools/layers/layer-canvas-log-expected.txt
[modify] https://crrev.com/3c07cd368caae990955af6f1eaea5d078425bb89/third_party/WebKit/LayoutTests/platform/linux/svg/batik/text/smallFonts-expected.png
[delete] https://crrev.com/314af2260fdc8f686c403c00a7844a429ee7ccaa/third_party/WebKit/LayoutTests/platform/mac-mac10.12/svg/batik/text/smallFonts-expected.png
[modify] https://crrev.com/3c07cd368caae990955af6f1eaea5d078425bb89/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/3c07cd368caae990955af6f1eaea5d078425bb89/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c422bd5ac77511a7f9c52cd3da627cd13f6f3691

commit c422bd5ac77511a7f9c52cd3da627cd13f6f3691
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Mon Feb 26 02:12:39 2018

[SPv175+] Don't output OverflowClip if InnerBorderRadiusClip already has the same rect

Previously we output three clip operations in SPv175 for
  <div style="overflow: hidden; border-radius: 5px; width: 100px; height: 100px">...</div>
which is one more than SPv1.

By omitting OverflowClip, now we output the same clip operations in
SPv175 and Spv1.

Bug:  814227 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I8839d3af5180b017e20681c54ad94316632c08ab
Reviewed-on: https://chromium-review.googlesource.com/934923
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539057}
[modify] https://crrev.com/c422bd5ac77511a7f9c52cd3da627cd13f6f3691/third_party/WebKit/LayoutTests/platform/linux/fast/borders/border-inner-bleed-expected.png
[modify] https://crrev.com/c422bd5ac77511a7f9c52cd3da627cd13f6f3691/third_party/WebKit/LayoutTests/platform/mac-mac10.12/fast/borders/border-inner-bleed-expected.png
[modify] https://crrev.com/c422bd5ac77511a7f9c52cd3da627cd13f6f3691/third_party/WebKit/LayoutTests/platform/win/fast/borders/border-inner-bleed-expected.png
[modify] https://crrev.com/c422bd5ac77511a7f9c52cd3da627cd13f6f3691/third_party/WebKit/Source/core/paint/BoxClipperBase.cpp
[modify] https://crrev.com/c422bd5ac77511a7f9c52cd3da627cd13f6f3691/third_party/WebKit/Source/core/paint/FindPropertiesNeedingUpdate.h
[modify] https://crrev.com/c422bd5ac77511a7f9c52cd3da627cd13f6f3691/third_party/WebKit/Source/core/paint/FragmentData.cpp
[modify] https://crrev.com/c422bd5ac77511a7f9c52cd3da627cd13f6f3691/third_party/WebKit/Source/core/paint/ObjectPaintProperties.h
[modify] https://crrev.com/c422bd5ac77511a7f9c52cd3da627cd13f6f3691/third_party/WebKit/Source/core/paint/PaintLayerClipper.cpp
[modify] https://crrev.com/c422bd5ac77511a7f9c52cd3da627cd13f6f3691/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/c422bd5ac77511a7f9c52cd3da627cd13f6f3691/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
[modify] https://crrev.com/c422bd5ac77511a7f9c52cd3da627cd13f6f3691/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.cpp
[modify] https://crrev.com/c422bd5ac77511a7f9c52cd3da627cd13f6f3691/third_party/WebKit/Source/core/paint/PaintPropertyTreeUpdateTests.cpp
[modify] https://crrev.com/c422bd5ac77511a7f9c52cd3da627cd13f6f3691/third_party/WebKit/Source/core/paint/SVGContainerPainter.cpp
[modify] https://crrev.com/c422bd5ac77511a7f9c52cd3da627cd13f6f3691/third_party/WebKit/Source/core/paint/SVGForeignObjectPainter.cpp
[modify] https://crrev.com/c422bd5ac77511a7f9c52cd3da627cd13f6f3691/third_party/WebKit/Source/core/paint/ViewPainterTest.cpp

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9626ac98ec0cfee6c72e812302576b6aa28bca6c

commit 9626ac98ec0cfee6c72e812302576b6aa28bca6c
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Mon Feb 26 02:37:49 2018

[SPv175+] Combine series of rectangular clips in the same transform space

Output one Save/ClipRect sequence for each series of rectangular clips
instead of output the sequence for each clip.

This along with other optimizations will fix SPv175 regression of
raster time, picture memory, etc.

Bug:  814227 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I8596213ce3356b9d9c9b003c64a4bed5773aea50
Reviewed-on: https://chromium-review.googlesource.com/936683
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539060}
[modify] https://crrev.com/9626ac98ec0cfee6c72e812302576b6aa28bca6c/third_party/WebKit/Source/platform/graphics/LoggingCanvas.cpp
[modify] https://crrev.com/9626ac98ec0cfee6c72e812302576b6aa28bca6c/third_party/WebKit/Source/platform/graphics/LoggingCanvas.h
[modify] https://crrev.com/9626ac98ec0cfee6c72e812302576b6aa28bca6c/third_party/WebKit/Source/platform/graphics/compositing/PaintChunksToCcLayer.cpp
[modify] https://crrev.com/9626ac98ec0cfee6c72e812302576b6aa28bca6c/third_party/WebKit/Source/platform/graphics/compositing/PaintChunksToCcLayerTest.cpp

Update:
- On ct.skia.org (the second sheet in https://docs.google.com/spreadsheets/d/1kc20YKhBRY_Mmr6daVqgB_u8k5KEaEI385lk2Cf4Rw8/edit#gid=0)
  - raster_time regression reduced from about 10% to 5%
  - viewport_picture_size reduced from above 3% to 2.7% (not sure if this is just a noise).

- Regression of Analog_Clock.svg restored by about a half of the regression.

Compared the operations for Analog_Clock.svg in SPv1 and SPv175. The differences were much fewer, but still some, for example:

  SPv1                    SPv175
  save                    save                                
    concat                  concat
    save
      concat
      drawPath              drawPath
    restore               
  restore                 restore
  save                    saveLayer-
    concat                  concat
    save                    
      concat
      saveLayer+
        save                save
          clipRect            ClipRect
          saveLayer+          saveLayer-
          restore             restore
        restore             restore
      restore
    restore
  restore                 restore

saveLayer+: with bounds
saveLayer-: without bounds

The differences are:
1. SPv175 combines consecutive transforms;
2. SPv175's saveLayers have no bounds;
3. SPv175 outputs transforms for an effect after the saveLayer while SPv1 outputs transforms before the saveLayer.

SPv175 seems more efficient about number of operations.

TODO:
- Figure out which of the above differences (likely 2 and 3) are causing regression of Analog_Clock.svg.
- Figure out the causes of ct.skia.org raster_time and viewport_picture_size regressions.

FYI, the test doesn't show regression on powerful machines because the actual busy time in a frame is much smaller than 1/60s. However, we can show the regression of raster_time by applying the following patch:

diff --git a/tools/perf/benchmarks/rasterize_and_record_micro.py b/tools/perf/benchmarks/rasterize_and_record_micro.py
index 5737d350a645..653c3aa13648 100644
--- a/tools/perf/benchmarks/rasterize_and_record_micro.py
+++ b/tools/perf/benchmarks/rasterize_and_record_micro.py
@@ -57,6 +57,13 @@ class RasterizeAndRecordMicroTop25(_RasterizeAndRecordMicro):
     return 'rasterize_and_record_micro.top_25'
 
 
+class RasterizeAndRecordMicroToughFilters(_RasterizeAndRecordMicro):
+  page_set = page_sets.ToughFiltersCasesPageSet
+  @classmethod
+  def Name(cls):
+    return 'rasterize_and_record_micro.tough_filters_cases'
+
+
 @benchmark.Owner(
     emails=['vmpstr@chromium.org', 'wkorman@chromium.org'],
     component='Internals>Compositing>Rasterization')

and running:
tools/perf/run_benchmark rasterize_and_record_micro.tough_filters_cases --story-filter=Analog_Clock_SVG [--extra-browser-args=--disable-features=SlimmingPaintV175]

Let's close this bug, and wait for new bugs filed by pinpoint after we enabled SPv175 again.

Re: comment #13

Try https://jsbin.com/miguhevihu/quiet on a device with 1.0 scale factor. A red edge bleeding can be seen, due to anti-aliased clipping not applied as a group.

If the LCA of descendant clip is applied to non-clipping effects as an optimization, I believe the red bleeding edge would disappear as a side effect grouping. In general I agree it is better for render quality, but then the presence of a clip-escaping sibling can change the edge bleeding behavior, which is certainly not desirable.