Issue metadata
Sign in to add a comment
|
Paint storm on composited elements |
|||||||||||||||||||||||||||
Issue descriptionhttps://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.
,
Feb 22 2018
,
Feb 22 2018
,
Feb 22 2018
My guess is this is related to SlimmingPaintV175 (https://bugs.chromium.org/p/chromium/issues/detail?id=771643#c101).
,
Feb 22 2018
,
Feb 23 2018
It might be, bisecting now. (In the future it would be very helpful to us if you could bisect also.)
,
Feb 23 2018
Appears to be broken in 66.0.3343.3. Working in beta 65.0.3325.88.
,
Feb 23 2018
,
Feb 23 2018
,
Feb 24 2018
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.
,
Feb 26 2018
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...!!
,
Feb 26 2018
,
Feb 26 2018
,
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
,
Feb 26 2018
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?
,
Feb 26 2018
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.
,
Feb 26 2018
Screencast of the issue: https://www.youtube.com/watch?v=O16I9hYaYKU
,
Feb 26 2018
,
Feb 28 2018
It was indeed SPv175, which we turned off two days ago. Therefore not blocking M66. We'll fix it for M67.
,
Feb 28 2018
This line of code is the culprit: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/LayoutObject.cpp?type=cs&q=SlowSetPaintingLayerNeedsRepaint&sq=package:chromium&l=2012 I think we need to fast-path transform/opacity updates here.
,
Feb 28 2018
,
Mar 7 2018
The call to ObjectPaintInvalidator(*this).slowSetPaintingLayerNeedsRepaint() was added in this CL: https://codereview.chromium.org/2307623002
,
Mar 8 2018
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.
,
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
,
Mar 9 2018
@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.
,
Mar 12 2018
The NextAction date has arrived: 2018-03-12
,
Apr 29 2018
This doesn't appear to have been fixed, and the regression is now in stable.
,
Apr 30 2018
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.
,
May 1 2018
It's still paint storming in Canary (68) on OSX. Am I missing something?
,
May 1 2018
chrishtr@, could you verify Canary behavior?
,
May 1 2018
,
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
,
May 2 2018
Removing the Needs-Bisect as bisect is already updated in C#16. Thanks...!!
,
May 2 2018
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...!!
,
May 2 2018
I also see the paint storming on a pixelbook, but only during the zooming part of the animation. Trace attached.
,
May 2 2018
Also reproduced on Linux. The invalidation seems to come from CompositedLayerRasterInvalidator and the reason is "paint property change". I'll take a look.
,
May 2 2018
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?
,
May 2 2018
*** 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.
,
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
,
May 5 2018
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.
,
May 5 2018
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
,
May 5 2018
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?
,
May 6 2018
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.
,
May 6 2018
Approving merge to M67 branch 3396 based on comment #40 and #44. Pls merge by 4:00 PM PT, Monday. Thank you.
,
May 6 2018
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
,
May 6 2018
,
May 7 2018
Confirmed in Canary! Thanks for fixing this. |
||||||||||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||||||||||
Comment 1 by jakearchibald@chromium.org
, Feb 22 2018