Issue metadata
Sign in to add a comment
|
24.1% regression in memory.top_10_mobile at 603125:603179 |
||||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Nov 1
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/15a8243ee40000
,
Nov 2
📍 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
,
Nov 2
Issue 900929 has been merged into this issue.
,
Nov 2
,
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
,
Nov 8
Metric recovered with above CL. Requesting merge to 71. Should be safe since we're restoring old behavior.
,
Nov 8
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
,
Nov 8
Pls apply appropriate OSs label.
,
Nov 8
,
Nov 8
Approved for merge to 71, branch 3578.
,
Nov 9
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
,
Nov 9
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}
,
Nov 9
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Nov 1