New issue
Advanced search Search tips

Issue 840504 link

Starred by 1 user

Issue metadata

Status: Duplicate
Merged: issue 838111
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Large (>1000%) regression on smoothness.top_25_smooth in 553446:553476

Project Member Reported by ajuma@chromium.org, May 7 2018

Issue description

mean_pixels_checkerboarded regressed by ~1100% on Nexus 5, and by ~5000% on Nexus 5X.

See https://chromeperf.appspot.com/report?sid=ba1d3593d56488ae9a98d30b1d6bf0d4d6a2ecddeaab3f81e99c3429e39df814&start_rev=550917&end_rev=555854

This seems to correspond to a regression in UMA metric Compositing.RenderPass.AppendQuadData.CheckerboardedNoRecordingContentArea on Android between April 27 and April 28.

There's similar UMA regression in Dev channel between May 2 and May 3, suggesting that the CL that caused the regression on ToT has been merged to Dev.

Going to kick off bisects on the perf bots.
 
Cc: chrishtr@chromium.org pdr@chromium.org
Owner: pdr@chromium.org
Status: Assigned (was: Untriaged)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/14d937a7c40000

[RLS] Do not apply scroll when clipping fixed-position visual rects by pdr@chromium.org
https://chromium.googlesource.com/chromium/src/+/e931d2aecb4d72946962e57e7e1586668071b504

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

Comment 4 by ajuma@chromium.org, May 8 2018

Components: Blink>Scroll
Labels: -Pri-3 ReleaseBlock-Stable M-67 Pri-1
The CL identified in #3 also fits the regression ranges for the Canary and Dev UMA regressions: https://uma.googleplex.com/p/chrome/timeline_v2?sid=e10da5c4b8819fb191b65b284d13ff42
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/14be8fa7c40000

[RLS] Do not apply scroll when clipping fixed-position visual rects by pdr@chromium.org
https://chromium.googlesource.com/chromium/src/+/e931d2aecb4d72946962e57e7e1586668071b504

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

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

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

commit cfebac612b10fdf961dca9df2bb28581577a612a
Author: Philip Rogers <pdr@chromium.org>
Date: Wed May 09 02:29:02 2018

[RLS] Ignore fixed when applying root scroll offset for interest rect

This patch fixes a regression from [1] where
MapToVisualRectInAncestorSpaceInternal was changed to not apply a
counterscroll offset for fixed position descendants. Interest rect
calculations incorrectly relied on this behavior.

MapToVisualRectInAncestorSpace is exclusive of the clip and scroll on
the ancestor object. To account for this, the interest rect logic would
call MapToVisualRectInAncestorSpace and then apply the root clip and
scroll offset, but this is not correct if there are fixed-position
children.

This patch updates the callsite to MapToVisualRectInAncestorSpace to
map to nullptr instead of the root view which will include the layout
view's clip and scroll.

[1] http://crrev.com/e931d2aecb

Bug:  838111 ,  831380 ,  840504 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I14b023279ef58e0123febae367a2ea0c67733c50
Reviewed-on: https://chromium-review.googlesource.com/1047193
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557066}
[modify] https://crrev.com/cfebac612b10fdf961dca9df2bb28581577a612a/third_party/blink/renderer/core/layout/layout_object.h
[modify] https://crrev.com/cfebac612b10fdf961dca9df2bb28581577a612a/third_party/blink/renderer/core/layout/layout_view.cc
[modify] https://crrev.com/cfebac612b10fdf961dca9df2bb28581577a612a/third_party/blink/renderer/core/layout/layout_view.h
[modify] https://crrev.com/cfebac612b10fdf961dca9df2bb28581577a612a/third_party/blink/renderer/core/paint/compositing/composited_layer_mapping.cc
[modify] https://crrev.com/cfebac612b10fdf961dca9df2bb28581577a612a/third_party/blink/renderer/core/paint/compositing/composited_layer_mapping_test.cc

Comment 7 by pdr@chromium.org, May 9 2018

Status: Fixed (was: Assigned)
The Nexus 5X graph has recovered and I expect the Nexus 5 to do so as well. I confirmed this patch fixes the regression locally on mac.
Labels: Merge-TBD
[Auto-generated comment by a script] We noticed that this issue is targeted for M-67; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-67 label, otherwise remove Merge-TBD label. Thanks.

Comment 9 by cmasso@google.com, May 9 2018

Is the fix in M67 already?
Note: I filed bug 841779 to file up on monitoring of this metric.

Comment 11 by pdr@chromium.org, May 10 2018

Mergedinto: 831380
Status: Duplicate (was: Fixed)
This should merge into M67. I'll be requesting a merge today.

I'm going to merge this bug into 831380 which is tracking the correctness fallout of this regression.

Comment 12 by pdr@chromium.org, May 10 2018

Mergedinto: -831380 838111
Oops, should have been 838111.
Project Member

Comment 13 by bugdroid1@chromium.org, May 10 2018

Labels: merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/657835337eb4fc280366c4ddbcc987a1b8db431b

commit 657835337eb4fc280366c4ddbcc987a1b8db431b
Author: Philip Rogers <pdr@chromium.org>
Date: Thu May 10 16:29:23 2018

[RLS] Ignore fixed when applying root scroll offset for interest rect

This patch fixes a regression from [1] where
MapToVisualRectInAncestorSpaceInternal was changed to not apply a
counterscroll offset for fixed position descendants. Interest rect
calculations incorrectly relied on this behavior.

MapToVisualRectInAncestorSpace is exclusive of the clip and scroll on
the ancestor object. To account for this, the interest rect logic would
call MapToVisualRectInAncestorSpace and then apply the root clip and
scroll offset, but this is not correct if there are fixed-position
children.

This patch updates the callsite to MapToVisualRectInAncestorSpace to
map to nullptr instead of the root view which will include the layout
view's clip and scroll.

[1] http://crrev.com/e931d2aecb

Bug:  838111 ,  831380 ,  840504 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I14b023279ef58e0123febae367a2ea0c67733c50
Reviewed-on: https://chromium-review.googlesource.com/1047193
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#557066}(cherry picked from commit cfebac612b10fdf961dca9df2bb28581577a612a)
Reviewed-on: https://chromium-review.googlesource.com/1054110
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#545}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/657835337eb4fc280366c4ddbcc987a1b8db431b/third_party/blink/renderer/core/layout/layout_object.h
[modify] https://crrev.com/657835337eb4fc280366c4ddbcc987a1b8db431b/third_party/blink/renderer/core/layout/layout_view.cc
[modify] https://crrev.com/657835337eb4fc280366c4ddbcc987a1b8db431b/third_party/blink/renderer/core/layout/layout_view.h
[modify] https://crrev.com/657835337eb4fc280366c4ddbcc987a1b8db431b/third_party/blink/renderer/core/paint/compositing/composited_layer_mapping.cc
[modify] https://crrev.com/657835337eb4fc280366c4ddbcc987a1b8db431b/third_party/blink/renderer/core/paint/compositing/composited_layer_mapping_test.cc

Comment 14 by cmasso@google.com, May 16 2018

Labels: -Merge-TBD

Sign in to add a comment