New issue
Advanced search Search tips

Issue 900928 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 9
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug-Regression



Sign in to add a comment

24.1% regression in memory.top_10_mobile at 603125:603179

Project Member Reported by hjd@google.com, Nov 1

Issue description

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

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


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

Android Nexus5 Perf

memory.top_10_mobile - Benchmark documentation link:
  None
Cc: bokan@chromium.org
Owner: bokan@chromium.org
Status: Assigned (was: Untriaged)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/15a8243ee40000

Performance fix for top controls-affected raster by bokan@chromium.org
https://chromium.googlesource.com/chromium/src/+/b2d410710e7ea7c5a33798ff6b5d5191b1951b5e
memory:chrome:all_processes:reported_by_chrome:cc:effective_size: 8.121e+06 → 1.01e+07 (+1.979e+06)

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

Benchmark documentation link:
  None
 Issue 900929  has been merged into this issue.
Components: Blink>Scroll
Project Member

Comment 6 by bugdroid1@chromium.org, Nov 5

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

commit e799978a0188b827aef4ef94bdf31d7b0df5ae7d
Author: David Bokan <bokan@chromium.org>
Date: Mon Nov 05 21:43:35 2018

Rasterize URL bar exposed region only when scrolling

When the URL bar is being actively scrolled out of view, the browser is
exposing a new region of the page at the bottom of the screen. Since the
render/Blink hasn't been resized yet, PictureLayerImpl has some code to
rasterize this new region despite the fact it falls outside the layer's
bounds.

As part of https://crrev.com/77b67445ba6e415e8b41b5b6fd5c016c7195206d, I
changed this code to raster the exact amount the URL bar is hidden.
However, this caused a perf regression as we would now raster the region
in pieces as the URL bar scrolled away, rather than in one go.

So in https://crrev.com/b2d410710e7ea7c5a33798ff6b5d5191b1951b5e I undid
that change and rastered the entire region as before. However, I kept
some the other changes made in the first CL. One of those changes was to
do this only when browser_controls_shrink_blink_size is true (meaning the
renderer has received a resize as a result of the URL bar being shown)
but no longer look if we're in an active scroll. Not looking at whether
we're actively scrolling regresses the metric in the linked bug.

It's more correct to look at both these bits. We should only expand the
raster region when we're going from controls shown to hidden (so look at
browser_controls_shrink_blink_size) but we should also only do this when
we're in a scroll. i.e. If the user isn't hiding the URL bar, there's no
need to raster additional content.

Bug:  900928 
Change-Id: I373ebe8879b832616780996f9afdd16092538fdd
Reviewed-on: https://chromium-review.googlesource.com/c/1315699
Reviewed-by: enne <enne@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605476}
[modify] https://crrev.com/e799978a0188b827aef4ef94bdf31d7b0df5ae7d/cc/layers/picture_layer_impl.cc

Labels: Merge-Request-71
Metric recovered with above CL. Requesting merge to 71. Should be safe since we're restoring old behavior.
Project Member

Comment 8 by sheriffbot@chromium.org, Nov 8

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

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Pls apply appropriate OSs label.
Labels: OS-Android
Labels: -Hotlist-Merge-Review -Merge-Review-71 Merge-Approved-71
Approved for merge to 71, branch 3578.
Project Member

Comment 12 by bugdroid1@chromium.org, Nov 9

Labels: -merge-approved-71 merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eeec698febbe939985a3b28e0e59c7a3d4e8c124

commit eeec698febbe939985a3b28e0e59c7a3d4e8c124
Author: David Bokan <bokan@chromium.org>
Date: Fri Nov 09 15:23:03 2018

Rasterize URL bar exposed region only when scrolling

When the URL bar is being actively scrolled out of view, the browser is
exposing a new region of the page at the bottom of the screen. Since the
render/Blink hasn't been resized yet, PictureLayerImpl has some code to
rasterize this new region despite the fact it falls outside the layer's
bounds.

As part of https://crrev.com/77b67445ba6e415e8b41b5b6fd5c016c7195206d, I
changed this code to raster the exact amount the URL bar is hidden.
However, this caused a perf regression as we would now raster the region
in pieces as the URL bar scrolled away, rather than in one go.

So in https://crrev.com/b2d410710e7ea7c5a33798ff6b5d5191b1951b5e I undid
that change and rastered the entire region as before. However, I kept
some the other changes made in the first CL. One of those changes was to
do this only when browser_controls_shrink_blink_size is true (meaning the
renderer has received a resize as a result of the URL bar being shown)
but no longer look if we're in an active scroll. Not looking at whether
we're actively scrolling regresses the metric in the linked bug.

It's more correct to look at both these bits. We should only expand the
raster region when we're going from controls shown to hidden (so look at
browser_controls_shrink_blink_size) but we should also only do this when
we're in a scroll. i.e. If the user isn't hiding the URL bar, there's no
need to raster additional content.

Bug:  900928 
Change-Id: I373ebe8879b832616780996f9afdd16092538fdd
Reviewed-on: https://chromium-review.googlesource.com/c/1315699
Reviewed-by: enne <enne@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#605476}(cherry picked from commit e799978a0188b827aef4ef94bdf31d7b0df5ae7d)
Reviewed-on: https://chromium-review.googlesource.com/c/1329442
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#610}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/eeec698febbe939985a3b28e0e59c7a3d4e8c124/cc/layers/picture_layer_impl.cc

Labels: Merge-Merged-71-3578
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/eeec698febbe939985a3b28e0e59c7a3d4e8c124

Commit: eeec698febbe939985a3b28e0e59c7a3d4e8c124
Author: bokan@chromium.org
Commiter: bokan@chromium.org
Date: 2018-11-09 15:23:03 +0000 UTC

Rasterize URL bar exposed region only when scrolling

When the URL bar is being actively scrolled out of view, the browser is
exposing a new region of the page at the bottom of the screen. Since the
render/Blink hasn't been resized yet, PictureLayerImpl has some code to
rasterize this new region despite the fact it falls outside the layer's
bounds.

As part of https://crrev.com/77b67445ba6e415e8b41b5b6fd5c016c7195206d, I
changed this code to raster the exact amount the URL bar is hidden.
However, this caused a perf regression as we would now raster the region
in pieces as the URL bar scrolled away, rather than in one go.

So in https://crrev.com/b2d410710e7ea7c5a33798ff6b5d5191b1951b5e I undid
that change and rastered the entire region as before. However, I kept
some the other changes made in the first CL. One of those changes was to
do this only when browser_controls_shrink_blink_size is true (meaning the
renderer has received a resize as a result of the URL bar being shown)
but no longer look if we're in an active scroll. Not looking at whether
we're actively scrolling regresses the metric in the linked bug.

It's more correct to look at both these bits. We should only expand the
raster region when we're going from controls shown to hidden (so look at
browser_controls_shrink_blink_size) but we should also only do this when
we're in a scroll. i.e. If the user isn't hiding the URL bar, there's no
need to raster additional content.

Bug:  900928 
Change-Id: I373ebe8879b832616780996f9afdd16092538fdd
Reviewed-on: https://chromium-review.googlesource.com/c/1315699
Reviewed-by: enne <enne@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#605476}(cherry picked from commit e799978a0188b827aef4ef94bdf31d7b0df5ae7d)
Reviewed-on: https://chromium-review.googlesource.com/c/1329442
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#610}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
Status: Fixed (was: Assigned)

Sign in to add a comment