New issue
Advanced search Search tips

Issue 872172 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

5.1%-22.1% regression in rendering.mobile at 580989:581036

Project Member Reported by toyoshim@chromium.org, Aug 8

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=872172

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


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

Android Nexus6 WebView Perf
Cc: chrishtr@chromium.org
Owner: chrishtr@chromium.org
Status: Assigned (was: Untriaged)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/12e74778640000

Only invalidate property trees and compositing for PaintLayers when needed, on style change. by chrishtr@chromium.org
https://chromium.googlesource.com/chromium/src/+/ba8c87a1a32815e6061b7c38d31ddfbbaac7a124
15.25 → 18.49 (+3.244)

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions
The regressions appear to all be on mobile WebViews.
My first theory after perusing the patch is that in cases where
PaintLayer::AttemptDirectCompositingUpdate returned true, which
previously avoided SetNeedsCompositingInputsUpdate() in PaintLayer::StyleDidChange.

Now we may be able to have SetNeedsCompositingInputsUpdate happen in some of these
cases. I think the code in LayoutObject::StyleDidChange in my patch might trigger 
in this case.

If so, this is a real regression since the whole point of AttemptDirectCompositingUpdate
is to provide a fast path for composited transform and opacity animations.
 Issue 872596  has been merged into this issue.
Cc: sadrul@chromium.org vmi...@chromium.org lanwei@chromium.org
 Issue 873382  has been merged into this issue.
Components: Blink>Compositing
Cc: tdres...@chromium.org
 Issue 874874  has been merged into this issue.
Project Member

Comment 10 by bugdroid1@chromium.org, Aug 21

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

commit 82efd6c5c86089eda2d54a0783e429ff96a36825
Author: Chris Harrelson <chrishtr@chromium.org>
Date: Tue Aug 21 20:45:52 2018

Reduce over-invalidation of compositing inputs.

In particular, for cases when the PaintLayer direct compositing update fast-path
applies.

Bug:  872172 

Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: If059f2f76d9973c2a19d2f326f495e4891813d6e
Reviewed-on: https://chromium-review.googlesource.com/1171797
Reviewed-by: vmpstr <vmpstr@chromium.org>
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584892}
[modify] https://crrev.com/82efd6c5c86089eda2d54a0783e429ff96a36825/third_party/blink/renderer/core/layout/layout_object.cc
[modify] https://crrev.com/82efd6c5c86089eda2d54a0783e429ff96a36825/third_party/blink/renderer/core/paint/paint_layer.cc

Cc: jsaul@google.com leon....@intel.com hjd@google.com gogerald@google.com cduvall@chromium.org
 Issue 875981  has been merged into this issue.
Cc: -cduvall@chromium.org
Cc: -jsaul@google.com
I think this was caused by an over-invalidation in LayoutView when it
recomputes overflow. If overflow did not actually change, compositing inputs
don't need to be dirtied.
Labels: -Pri-2 RegressedIn-70 ReleaseBlock-Stable Target-70 Pri-1
Fixed in:

https://chromium.googlesource.com/chromium/src/+/d1042cc805130a5ed959f06593787dd0021106dc

Will request merge after verifying bots recover.
Project Member

Comment 18 by sheriffbot@chromium.org, Sep 3

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: OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
Labels: Merge-Request-70
Recovered.
Project Member

Comment 21 by sheriffbot@chromium.org, Sep 5

Labels: -Merge-Request-70 Hotlist-Merge-Approved Merge-Approved-70
Your change meets the bar and is auto-approved for M70. Please go ahead and merge the CL to branch 3538 manually. Please contact milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

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

Comment 23 by sheriffbot@chromium.org, Sep 10

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

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: -Merge-Approved-70
already merged. 

Sign in to add a comment