New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 814711 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-03-12
OS: Mac
Pri: 1
Type: Bug-Regression

Blocking:
issue 803867



Sign in to add a comment

Paint storm on composited elements

Project Member Reported by jakearchibald@chromium.org, Feb 22 2018

Issue description

https://codepen.io/jaffathecake/full/gvzGZL/

The above runs 60fps on stable (64.0.3282.167) without main-thread painting. However in Canary (66.0.3352.0) there are loads of main-thread paints per frame.

This is a very recent regression. Past couple of days I think.

Spotted on mac.
 
 Issue 814725  has been merged into this issue.
Components: Internals>Compositing>Animation
Cc: chrishtr@chromium.org
Cc: wangxianzhu@chromium.org
My guess is this is related to SlimmingPaintV175 (https://bugs.chromium.org/p/chromium/issues/detail?id=771643#c101).
Labels: -Type-Bug Type-Bug-Regression
It might be, bisecting now.

(In the future it would be very helpful to us if you could bisect also.)
Appears to be broken in 66.0.3343.3. Working in beta 65.0.3325.88.
Labels: -Pri-3 FoundIn-66 ReleaseBlock-Stable Target-66 needs-bi Pri-1
Labels: -needs-bi Needs-Bisect
I usually would do the bisecting, but I'm on the road right now. I gave up when it was 45mins to download the first build.
Cc: krajshree@chromium.org
Labels: Needs-Feedback
Tested the issue on mac 10.12.6 using chrome stable #64.0.3282.167 and latest canary #66.0.3353.0.

Attached screen casts for reference.

Following are the steps followed to reproduce the issue.
------------
1. Navigated to https://codepen.io/jaffathecake/full/gvzGZL/
2. Opened performance in dev tools.
3. Observed that 60 fps was observed in both chrome version #64.0.3282.167 and latest canary #66.0.3353.0.

Note: The behaviour is same in both the chrome reported version #64.0.3282.167 and chrome version #66.0.3353.0.

jakearchibald@ - Could you please check the attached screen casts and please let us know if anything missed from our side. Also please provide a screen cast if possible for better understanding of the issue.

Thanks...!!
814711@64.0.3282.167.mp4
3.0 MB View Download
814711@66.0.3353.0.mp4
6.0 MB View Download
Labels: Triaged-ET
NextAction: 2018-03-12
Project Member

Comment 14 by sheriffbot@chromium.org, Feb 26 2018

This issue is marked as a release blocker with no milestone associated. Please add an appropriate milestone.

All release blocking issues should have milestones associated to it, so that the issue can tracked and the fixes can be pushed promptly.

This issue is marked as a release blocker with no OS labels associated. Please add an appropriate OS label.

All release blocking issues should have OS labels associated to it, so that the issue can tracked and promptly verified, once it gets fixed.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Owner: chrishtr@chromium.org
Status: Assigned (was: Untriaged)
chrishtr@chromium.org: are you still bisecting? I seem to remember someone (some bot, or some testing team?) would pick up bugs with Needs-Bisect label and start some bisect, that doesn't seem to be happening here. Does something else needs to happen to trigger that? Or does that only work for telemetry failures?
I bisected the issue, and it points to https://chromium.googlesource.com/chromium/src/+log/f30697974420401bfe6a9205b11b93039f838a36..3dfc0ccdfbb1e7e4150e5a3a2eb7e9ad640c4e4c.

The paint related stuff looks like:

https://chromium.googlesource.com/chromium/src/+/0266b394e77b7bf79e9e8c29f39b0e5890bed106 Merge GPU and SW memory limits
https://chromium.googlesource.com/chromium/src/+/0a9a5c311a1d3a298f952e495510bd6fe3faa2f6 Enable SlimmingPaintV175 by default

I'd bet money on it being the latter.

krajshree@: The issue is the main-thread painting. --show-paint-rects will make the difference obvious, as will about:tracing or devtools timeline.
Screencast of the issue: https://www.youtube.com/watch?v=O16I9hYaYKU
Components: -Internals>Compositing>Animation
Labels: -ReleaseBlock-Stable -Target-66 Target-67
It was indeed SPv175, which we turned off two days ago. Therefore not blocking
M66. We'll fix it for M67.
Blocking: 803867
The call to ObjectPaintInvalidator(*this).slowSetPaintingLayerNeedsRepaint() was
added in this CL: 

https://codereview.chromium.org/2307623002
It appears that all tests pass except for
paint/invalidation/compositing/chunk-reorder.html when the code is removed,
since I think paint is now invalidated for transforms etc in other places. (?)

Will try a CL which leaves the code in place for z-index change.
Project Member

Comment 24 by bugdroid1@chromium.org, Mar 9 2018

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

commit a632d48d0225c3e181681ef514583dc567fa4e06
Author: Chris Harrelson <chrishtr@chromium.org>
Date: Fri Mar 09 01:32:27 2018

[SPv175] Reduce over-invalidation of repaint during style update on paint property change.

In SPv1 mode, LayoutObject::AdjustStyleDifference will cause subtree paint invalidation
(including PaintLayer repaint) on change of paint properties or z-index, but only
if there is no direct compositing reason for the object. The reason for the LayoutObject
paint invalidations was to cause raster invalidation as a side-effect. Painted
display list output of individual LayoutObjects does not change as a result of these
updates.

In SPv175 mode, we don't need to invalidate paint of individual LayoutObjects just
becaue of these changes, since raster invalidation happens via a new PaintChunk
mechanism. However, we still need to cause PaintLayer repaint if any paint property
tree nodes are added or removed, or z-index changes, to cause re-drawing of PaintChunks
with the correct paint properties and order.

There is already invalidation of PaintLayer repaint for paint property node change,
in PaintPropertyTreeBuilder::UpdateForSelf and ::UpdateForChildren (it was added
recently, after the code being removed in this CL was committed). Therefore we
only need to invalidate repaint for z-index change. Further, this is only needed
if the LayoutObject already had a PaintLayer, because z-index has no effect on
objects without a PaintLayer, and there is already invalidation happening on
PaintLayer addition or removal.

Tested by:

LayoutTests/paint/invalidation/compositing/chunk-reorder.html
svg/transforms/change-transform-to-none-shape.html

Bug:814711

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I208b03b9e6b1a6635dab9585b8884ed0e2f8688d
Reviewed-on: https://chromium-review.googlesource.com/954352
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541981}
[modify] https://crrev.com/a632d48d0225c3e181681ef514583dc567fa4e06/third_party/WebKit/Source/core/layout/LayoutObject.cpp
[modify] https://crrev.com/a632d48d0225c3e181681ef514583dc567fa4e06/third_party/WebKit/Source/core/paint/PaintLayer.cpp
[modify] https://crrev.com/a632d48d0225c3e181681ef514583dc567fa4e06/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/a632d48d0225c3e181681ef514583dc567fa4e06/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h

Status: Fixed (was: Assigned)
@jakearchibald: thanks again for filing this bug. Please keep them coming, especially
performance regressions. We will be turning SPv175 back on within the next week;
--enable-slimming-paint-v175 forces it on.
The NextAction date has arrived: 2018-03-12
Status: Untriaged (was: Fixed)
This doesn't appear to have been fixed, and the regression is now in stable.
Labels: -Needs-Feedback
Status: Fixed (was: Untriaged)
As I understand it, this is fixed in M-67 because Slimming Paint v1.75 is default on. But for M-66 it is not and it is not intended to be fixed. See comment #19.
It's still paint storming in Canary (68) on OSX. Am I missing something?
Status: Assigned (was: Fixed)
chrishtr@, could you verify Canary behavior?
Labels: ReleaseBlock-Stable
Project Member

Comment 32 by sheriffbot@chromium.org, May 1 2018

This issue is marked as a release blocker with no milestone associated. Please add an appropriate milestone.

All release blocking issues should have milestones associated to it, so that the issue can tracked and the fixes can be pushed promptly.

This issue is marked as a release blocker with no OS labels associated. Please add an appropriate OS label.

All release blocking issues should have OS labels associated to it, so that the issue can tracked and promptly verified, once it gets fixed.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Needs-Bisect Needs-Triage-M66
Removing the Needs-Bisect as bisect is already updated in C#16.

Thanks...!!
Labels: M-66 Needs-Feedback OS-Mac
Able to reproduce the issue on mac 10.13.3 using chrome stable #66.0.3359.139, latest beta #67.0.3396.18. Issue is not seen in OS-Win and OS-Linux.
Tried testing the issue on mac 10.13.3 using chrome version #68.0.3417.0 as per comment #0 and #17.
Observed loads of main-thread paints per frame in chrome which seems to be the issue.

chrishtr@ - Could you please check the attached screen cast and please help us in confirming the fix.

Thanks...!!
814711.mp4
3.0 MB View Download
I also see the paint storming on a pixelbook, but only during the zooming part of the animation. Trace attached.
trace_Wed_May_02_2018_14.25.06.json.gz
9.7 MB Download
Labels: -M-66 M-67
Owner: wangxianzhu@chromium.org
Also reproduced on Linux. The invalidation seems to come from CompositedLayerRasterInvalidator and the reason is "paint property change". I'll take a look. 
Cc: trchen@chromium.org
The raster invalidation is because of transform change:

Transform (from changed:
"translation(1.13687e-13,-1.13687e-13,0), scale(1.00044,1.00044,1), skew(0,0,0), quaternion(0,0,5.54865e-17,1), perspective(0,0,0,1)"
->
"translation(5.68434e-14,-1.13687e-13,0), scale(1.00044,1.00044,1), skew(0,0,0), quaternion(0,0,8.32297e-17,1), perspective(0,0,0,1)"

At the time, the chunk has only one extra transform from the layer state:
"translation(0,0,0), scale(1.00044,1.00044,1), skew(0,0,0), quaternion(0,0,0,1), perspective(0,0,0,1)"

The changed transform seems a result of computation error in GeometryMapper::SourceToDestinationProjection().

I think we can do both of the following:
1. ignore tiny differences when comparing chunk_to_layer_transform;
2. let Geometry::SourceToDestinationProjection() return the child matrix directly if the source is a child of the destination.

Is there an existing function for 1?
*** Bulk Edit ***
M67 Stable promotion is coming soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. 

If fix is already merged to M67 and nothing else is pending, pls mark the bug as fixed. Thank you.
Project Member

Comment 39 by bugdroid1@chromium.org, May 3 2018

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

commit 8ab29f09a7bbf904ba1567a26ff28b9cf4d6953a
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Thu May 03 18:29:42 2018

[SPv175+] Don't invalidate chunk for tiny transform changes

This ignores the computation error introduced when mapping to and from
the common planar of the layer and the chunk while the relative
transform between the layer and the chunk doesn't actually change.

Bug:  814711 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Ie97b425eb3de601107328513b611b3f8676afa73
Reviewed-on: https://chromium-review.googlesource.com/1040537
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555820}
[modify] https://crrev.com/8ab29f09a7bbf904ba1567a26ff28b9cf4d6953a/third_party/blink/renderer/platform/graphics/compositing/composited_layer_raster_invalidator.cc
[modify] https://crrev.com/8ab29f09a7bbf904ba1567a26ff28b9cf4d6953a/third_party/blink/renderer/platform/graphics/compositing/composited_layer_raster_invalidator_test.cc

Labels: -Needs-Feedback Merge-Request-67
Verified #c39 CL in recent canaries. The CL is small and the method is straightforward for the issue, so it's quite safe to merge. 
Project Member

Comment 41 by sheriffbot@chromium.org, May 5 2018

Labels: -Merge-Request-67 Merge-Review-67 Hotlist-Merge-Review
This bug requires manual review: M67 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 42 Deleted

Thank you  wangxianzhu@ for canary verification at #40. This was regressed in M66 which is already in stable. Why it is critical to merge CL listed at #39 to M67?


It ever occurred in M66 when we enabled SlimmingPaintV175 for several days, then we decided not to ship SlimmingPaintV175 in M66 and disabled it. After M66 branch we and enabled it again in M67. So the regression is a M67 regression and is not in M66 beta and stable.
Labels: -Merge-Review-67 Merge-Approved-67
Approving merge to M67 branch 3396 based on comment #40 and #44. Pls merge by 4:00 PM PT, Monday. Thank you.
Project Member

Comment 46 by bugdroid1@chromium.org, May 6 2018

Labels: -merge-approved-67 merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/351ffc04b77bfe8bb120097eafc56f2e62fee6a8

commit 351ffc04b77bfe8bb120097eafc56f2e62fee6a8
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Sun May 06 17:55:32 2018

[SPv175+] Don't invalidate chunk for tiny transform changes

This ignores the computation error introduced when mapping to and from
the common planar of the layer and the chunk while the relative
transform between the layer and the chunk doesn't actually change.

(In M67 branch: also cherry-picked
 https://chromium-review.googlesource.com/c/chromium/src/+/1011826)

TBR=wangxianzhu@chromium.org

(cherry picked from commit 8ab29f09a7bbf904ba1567a26ff28b9cf4d6953a)

Bug:  814711 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Ie97b425eb3de601107328513b611b3f8676afa73
Reviewed-on: https://chromium-review.googlesource.com/1040537
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#555820}
Reviewed-on: https://chromium-review.googlesource.com/1046150
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#494}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/351ffc04b77bfe8bb120097eafc56f2e62fee6a8/third_party/blink/renderer/platform/graphics/compositing/composited_layer_raster_invalidator.cc
[modify] https://crrev.com/351ffc04b77bfe8bb120097eafc56f2e62fee6a8/third_party/blink/renderer/platform/graphics/compositing/composited_layer_raster_invalidator.h
[modify] https://crrev.com/351ffc04b77bfe8bb120097eafc56f2e62fee6a8/third_party/blink/renderer/platform/graphics/compositing/composited_layer_raster_invalidator_test.cc
[modify] https://crrev.com/351ffc04b77bfe8bb120097eafc56f2e62fee6a8/third_party/blink/renderer/platform/graphics/compositing/paint_chunks_to_cc_layer.cc

Status: Fixed (was: Assigned)
Confirmed in Canary! Thanks for fixing this.

Sign in to add a comment