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

Issue 776736 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug-Regression



Sign in to add a comment

8.1%-23.5% regression in system_health.common_desktop at 508286:508346

Project Member Reported by pmeenan@chromium.org, Oct 20 2017

Issue description

See the link to graphs below.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Oct 20 2017

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

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


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

chromium-rel-mac11
chromium-rel-mac11-pro
chromium-rel-mac12
Project Member

Comment 2 by 42576172...@developer.gserviceaccount.com, Oct 20 2017

๐Ÿ“ Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/12e0d927780000
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Oct 20 2017

๐Ÿ˜ฟ Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/12e0d927780000
Project Member

Comment 4 by 42576172...@developer.gserviceaccount.com, Oct 20 2017

๐Ÿ“ Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/14e9b2b7780000
Project Member

Comment 5 by 42576172...@developer.gserviceaccount.com, Oct 21 2017

Cc: chrishtr@chromium.org schenney@chromium.org
Owner: schenney@chromium.org
Status: Assigned (was: Untriaged)
๐Ÿ“ Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/14e9b2b7780000

Prevent squashing of border-radius layers
By schenney@chromium.org ยท Thu Oct 12 13:11:52 2017
chromium @ 8ff2371257ac4f24d77da4a62569840a53c49b47

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions
I'll look into whether we can use a smaller hammer to fix the rendering error.
Components: Blink>Compositing
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 25 2017

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

commit 2faf4aa8f809aee54bd228d0fb375bbd67b4d2be
Author: Stephen Chenney <schenney@chromium.org>
Date: Wed Oct 25 18:58:49 2017

Tighten the restriction on squashing for border radius

The fix for https://bugs.chromium.org/p/chromium/issues/detail?id=761298
prevents sqaushing of anything that has a border radius, but in reality
we only need to prevent squashing when the layer actually clips a
child composited layer. The composited part is not known at the
time we compute squashing disallowed reasons, so we can't use that. But
we can make sure that overflow does clip.

R=chrishtr@chromium.org
BUG= 776736 , 774544 

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I0ec1b8d8e2c0ebfef5004f313d49ae41f14e647e
Reviewed-on: https://chromium-review.googlesource.com/734140
Commit-Queue: Stephen Chenney <schenney@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511540}
[modify] https://crrev.com/2faf4aa8f809aee54bd228d0fb375bbd67b4d2be/third_party/WebKit/Source/core/paint/compositing/CompositingLayerAssigner.cpp

Project Member

Comment 9 by bugdroid1@chromium.org, Nov 3 2017

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

commit c24e45d72fd1003e4b49341650d9c40b709752c2
Author: Stephen Chenney <schenney@chromium.org>
Date: Fri Nov 03 19:25:48 2017

Use sibling HasInlineTransform to enable more squashing

Traverse previous siblings of a layer looking for inline transforms
as a way to enable more squashing when border radius is present.
This targets the squashing restriction at the specific case that
generated the original bug, and does not cause issues in other
tests (that is, next siblings do not cause the original bug and
only inline transforms cause the original bug).

Telemetry tests on MacOS show a 3% or more improvement on time
to first paint for load tests.

TBR=chrishtr@chromium.org

Bug:  776736 , 774544 , 780244
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I1f9973e5c52d40a47c05dd2d8310bb66de3e0dfa
Reviewed-on: https://chromium-review.googlesource.com/743694
Commit-Queue: Stephen Chenney <schenney@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513869}
[modify] https://crrev.com/c24e45d72fd1003e4b49341650d9c40b709752c2/third_party/WebKit/Source/core/paint/PaintLayer.cpp
[modify] https://crrev.com/c24e45d72fd1003e4b49341650d9c40b709752c2/third_party/WebKit/Source/core/paint/PaintLayer.h
[modify] https://crrev.com/c24e45d72fd1003e4b49341650d9c40b709752c2/third_party/WebKit/Source/core/paint/compositing/CompositingLayerAssigner.cpp
[modify] https://crrev.com/c24e45d72fd1003e4b49341650d9c40b709752c2/third_party/WebKit/Source/core/paint/compositing/CompositingRequirementsUpdater.cpp

Labels: Merge-Request-63
The patch from comment #9, that really improved performance, appears first in 64.0.3258.0

The patch from comment #8, which is minimally helpful, appears first in 64.0.3250.0

The patch that regressed things horribly appeared first in 63.0.3239.0.

We're seeing major paint update time regressions in UMA data, https://bugs.chromium.org/p/chromium/issues/detail?id=782353

So requesting merge of both patches to M-63 after a little time to bake on Canary.
Project Member

Comment 11 by sheriffbot@chromium.org, Nov 8 2017

Labels: -Merge-Request-63 Merge-Review-63 Hotlist-Merge-Review
This bug requires manual review: M63 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), gkihumba@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: benhenry@chromium.org
Pls apply appropriate OSs label. 
Please do not merge c24e45d72fd1003e4b49341650d9c40b709752c2, this commit
is still under discussion.
We're not merging anything yet, but are asking for approval in the event the team decides it's safe, and to bring it to release manager's attention.
Labels: OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
A decision has been made to revert this and two earlier patches, and one of those patches should be reverted on the M-63 branch.

I propose we revert them all on trunk and then merge back the ones we need for M-63.
Thank you schenney@. Please update the bug once revert looks good to merge to M63 after change is well baked/verified in canary and safe to merge to M63.
Project Member

Comment 18 by bugdroid1@chromium.org, Nov 15 2017

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

commit 90bb5240fe42935776901f33bc7c515e8c24da1c
Author: Stephen Chenney <schenney@chromium.org>
Date: Wed Nov 15 06:17:32 2017

Revert "Use sibling HasInlineTransform to enable more squashing"

This reverts commit c24e45d72fd1003e4b49341650d9c40b709752c2.

Reason for revert: This does not fix all cases. The plan is to revert all the patches trying to fix the underlying border radius and squashing issue and fix it correctly.

Original change's description:
> Use sibling HasInlineTransform to enable more squashing
> 
> Traverse previous siblings of a layer looking for inline transforms
> as a way to enable more squashing when border radius is present.
> This targets the squashing restriction at the specific case that
> generated the original bug, and does not cause issues in other
> tests (that is, next siblings do not cause the original bug and
> only inline transforms cause the original bug).
> 
> Telemetry tests on MacOS show a 3% or more improvement on time
> to first paint for load tests.
> 
> TBR=chrishtr@chromium.org
> 
> Bug:  776736 , 774544 , 780244
> Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
> Change-Id: I1f9973e5c52d40a47c05dd2d8310bb66de3e0dfa
> Reviewed-on: https://chromium-review.googlesource.com/743694
> Commit-Queue: Stephen Chenney <schenney@chromium.org>
> Reviewed-by: Philip Rogers <pdr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#513869}

TBR=pdr@chromium.org,chrishtr@chromium.org,schenney@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  776736 ,  774544 , 780244
Change-Id: I3e620a3c12c0f383bb3f57189afb8e3f05e9f960
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Reviewed-on: https://chromium-review.googlesource.com/770230
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516598}
[modify] https://crrev.com/90bb5240fe42935776901f33bc7c515e8c24da1c/third_party/WebKit/Source/core/paint/PaintLayer.cpp
[modify] https://crrev.com/90bb5240fe42935776901f33bc7c515e8c24da1c/third_party/WebKit/Source/core/paint/PaintLayer.h
[modify] https://crrev.com/90bb5240fe42935776901f33bc7c515e8c24da1c/third_party/WebKit/Source/core/paint/compositing/CompositingLayerAssigner.cpp
[modify] https://crrev.com/90bb5240fe42935776901f33bc7c515e8c24da1c/third_party/WebKit/Source/core/paint/compositing/CompositingRequirementsUpdater.cpp

Project Member

Comment 19 by bugdroid1@chromium.org, Nov 15 2017

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

commit 152e7cf991c83b8ebac89db3f67cfe80511d47e5
Author: Stephen Chenney <schenney@chromium.org>
Date: Wed Nov 15 18:58:04 2017

Revert "Tighten the restriction on squashing for border radius"

This reverts commit 2faf4aa8f809aee54bd228d0fb375bbd67b4d2be.

Reason for revert: <INSERT REASONING HERE>

Original change's description:
> Tighten the restriction on squashing for border radius
> 
> The fix for https://bugs.chromium.org/p/chromium/issues/detail?id=761298
> prevents sqaushing of anything that has a border radius, but in reality
> we only need to prevent squashing when the layer actually clips a
> child composited layer. The composited part is not known at the
> time we compute squashing disallowed reasons, so we can't use that. But
> we can make sure that overflow does clip.
> 
> R=โ€‹chrishtr@chromium.org
> BUG= 776736 , 774544 
> 
> Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
> Change-Id: I0ec1b8d8e2c0ebfef5004f313d49ae41f14e647e
> Reviewed-on: https://chromium-review.googlesource.com/734140
> Commit-Queue: Stephen Chenney <schenney@chromium.org>
> Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#511540}

TBR=chrishtr@chromium.org,schenney@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  776736 ,  774544 
Change-Id: I9092431dd162191b5ee92f64efb45aba164c382e
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Reviewed-on: https://chromium-review.googlesource.com/771990
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Commit-Queue: Stephen Chenney <schenney@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516768}
[modify] https://crrev.com/152e7cf991c83b8ebac89db3f67cfe80511d47e5/third_party/WebKit/Source/core/paint/compositing/CompositingLayerAssigner.cpp

What is the merge review for? There are many things going on here and it is confusing. Regrading M63, please be explicit about:
* What CL you are planning to merge into M63 and why?
* What CL you are planning to revert from M63 and why?
Sorry for the confusion. The Merge-Review is not for anything here - these are patches that needed to be reverted in order to revert the patch of interest.

Head on over to https://bugs.chromium.org/p/chromium/issues/detail?id=761298 for the correct merge request.
Labels: -Hotlist-Merge-Review -Merge-Review-63
schenney: Thanks for all your hard work here! Looks like  bug 761298  was merged. Is there further work to do on this bug? Or should we mark Fixed?
Status: Fixed (was: Assigned)
Yep, time to mark Fixed.

Sign in to add a comment