Issue metadata
Sign in to add a comment
|
167.7% regression in smoothness.sync_scroll.key_mobile_sites_smooth at 470727:470763 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
May 18 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8979237362059748160
,
May 18 2017
=== Auto-CCing suspected CL author pdr@chromium.org === Hi pdr@chromium.org, the bisect results pointed to your CL, please take a look at the results. === BISECT JOB RESULTS === Perf regression found with culprit Suspected Commit Author : pdr Commit : 6280cc1d5a1dccfbe23ca438eed862e245f18614 Date : Wed May 10 23:22:47 2017 Subject: Add a cache of LayerImpl's viewport layer type Bisect Details Configuration: android_nexus6_perf_bisect Benchmark : smoothness.sync_scroll.key_mobile_sites_smooth Metric : first_gesture_scroll_update_latency/http___mlb.com_ Change : 130.79% | 9.73577777778 -> 22.469 Revision Result N chromium@470726 9.73578 +- 13.1391 9 good chromium@470736 8.061 +- 3.7517 6 good chromium@470741 8.05317 +- 2.7114 6 good chromium@470742 8.33867 +- 1.7809 6 good chromium@470743 22.7198 +- 1.89165 6 bad <-- chromium@470745 22.667 +- 1.75584 6 bad chromium@470763 22.469 +- 2.2869 6 bad To Run This Test src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=http...mlb.com. smoothness.sync_scroll.key_mobile_sites_smooth Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8979237362059748160 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=6104227425812480 | O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq | X | for more information addressing perf regression bugs. For feedback, | / \ | file a bug with component Speed>Bisection. Thank you!
,
May 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/11da65335711376455faffa6746036518e2d76a9 commit 11da65335711376455faffa6746036518e2d76a9 Author: Philip Rogers <pdr@chromium.org> Date: Wed May 31 03:38:55 2017 Remove unnecessary LayerById lookups in DidUpdateScrollOffset This patch removes unnecessary LayerById lookups in DidUpdateScrollOffset in an attempt to improve both understanding and performance. In addition: 1) An call to InnerViewportContainerLayer has been removed from DidUpdateScrollState. This contains a LayerById lookup and is false in the common case. This null-check removal is safe because it will be null-checked later in DidUpdateScrollState. 2) The scrollbar vertical adjustment code has been moved so it only applies to viewport scrolls. Non-viewport scrolls do not have ViewportBoundsDelta. Bug: 724193 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: Ia1bc76769781243b2e486d0f1bbbae59bbe07ad3 Reviewed-on: https://chromium-review.googlesource.com/517902 Reviewed-by: enne <enne@chromium.org> Commit-Queue: Philip Rogers <pdr@chromium.org> Cr-Commit-Position: refs/heads/master@{#475774} [modify] https://crrev.com/11da65335711376455faffa6746036518e2d76a9/cc/layers/layer_impl.h [modify] https://crrev.com/11da65335711376455faffa6746036518e2d76a9/cc/trees/layer_tree_impl.cc
,
Jun 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fb6fd3402373e1e7204bff1060e2adad9ed2f5f0 commit fb6fd3402373e1e7204bff1060e2adad9ed2f5f0 Author: pdr <pdr@chromium.org> Date: Mon Jun 05 18:49:13 2017 Remove LayerTreeImpl::IsViewportLayerId This function is no longer used. BUG: 724193 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: I6789dfbae724121ed72a19caa74a527e4b1d9712 Reviewed-on: https://chromium-review.googlesource.com/524265 Reviewed-by: enne <enne@chromium.org> Commit-Queue: Philip Rogers <pdr@chromium.org> Cr-Commit-Position: refs/heads/master@{#477043} [modify] https://crrev.com/fb6fd3402373e1e7204bff1060e2adad9ed2f5f0/cc/trees/layer_tree_impl.cc [modify] https://crrev.com/fb6fd3402373e1e7204bff1060e2adad9ed2f5f0/cc/trees/layer_tree_impl.h
,
Jun 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/57c9f0fa2ca884f44b783e3b7aaf8ce528fe4886 commit 57c9f0fa2ca884f44b783e3b7aaf8ce528fe4886 Author: pdr <pdr@chromium.org> Date: Tue Jun 06 03:39:48 2017 Only do work in SetViewportLayersFromIds if ids change We still have a small regression from [1] that is proving hard to track down. This is a speculative patch to squash the remaining regression by early-outing in SetViewportLayersFromIds if values do not change. Another patch [2] dropped the perf regression by half by removing layer map lookups so it seems plausible that these lookups are expensive. [1] https://codereview.chromium.org/2867793002 [2] https://chromium-review.googlesource.com/517902 BUG: 724193 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: I78ae46e4fee8b7e21994de92d3fb3899dfc5c869 Reviewed-on: https://chromium-review.googlesource.com/524578 Reviewed-by: enne <enne@chromium.org> Commit-Queue: Philip Rogers <pdr@chromium.org> Cr-Commit-Position: refs/heads/master@{#477181} [modify] https://crrev.com/57c9f0fa2ca884f44b783e3b7aaf8ce528fe4886/cc/trees/layer_tree_impl.cc [modify] https://crrev.com/57c9f0fa2ca884f44b783e3b7aaf8ce528fe4886/cc/trees/layer_tree_impl.h
,
Jun 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bf2f6fafd87c796737895702f3b7813916c50aa8 commit bf2f6fafd87c796737895702f3b7813916c50aa8 Author: pdr <pdr@chromium.org> Date: Wed Jun 07 23:25:43 2017 Add a scrollable bit to Layers This patch adds a scrollable bit to Layer and LayerImpl in preparation for removing scroll_clip_layer. When a Layer/LayerImpl is "scrollable", it should have an associated scroll node with bounds equal to the Layer's bounds. Other Layers can create or have associated scroll nodes but their bounds do not need to correspond to the scroll node's bounds. This link between scroll nodes and layers is required for hit testing which happens on layers. For SPV1, the scrolling bounds will be set on Layer which update the associated scroll node's bounds. For SPV2, a base scrolling layer should always be created in paint-order with bounds equal to the scroll node's bounds. BUG: 724193 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: I77b5cbb3426b95d7fc8075e73a4489ed465bc1d9 Reviewed-on: https://chromium-review.googlesource.com/524773 Reviewed-by: enne <enne@chromium.org> Commit-Queue: Philip Rogers <pdr@chromium.org> Cr-Commit-Position: refs/heads/master@{#477778} [modify] https://crrev.com/bf2f6fafd87c796737895702f3b7813916c50aa8/cc/layers/layer.cc [modify] https://crrev.com/bf2f6fafd87c796737895702f3b7813916c50aa8/cc/layers/layer.h [modify] https://crrev.com/bf2f6fafd87c796737895702f3b7813916c50aa8/cc/layers/layer_impl.cc [modify] https://crrev.com/bf2f6fafd87c796737895702f3b7813916c50aa8/cc/layers/layer_impl.h [modify] https://crrev.com/bf2f6fafd87c796737895702f3b7813916c50aa8/cc/layers/layer_unittest.cc [modify] https://crrev.com/bf2f6fafd87c796737895702f3b7813916c50aa8/cc/layers/scrollbar_layer_unittest.cc [modify] https://crrev.com/bf2f6fafd87c796737895702f3b7813916c50aa8/cc/trees/layer_tree_impl.cc
,
Jul 27 2017
Explictly assigning. A CL you landed tripped one of the speed metrics we measure in the lab. If this is the first time this has happened to one of your CLs, or if it's been a while, please read: https://chromium.googlesource.com/chromium/src/+/master/docs/speed/addressing_performance_regressions.md We're looking for one of the following: 1. Justification via explanation 2. Plan to revert or fix 3. Angry rage throwing of equipment at my head Just be aware that I'm trained in trumpet playing and First Aid and am not afraid to use it. Note: This was a bulk edit message and not very personal.
,
Sep 15 2017
pdr: looks like this went about halfway back down, but fully recovered. However it's just one page on one device. Should we mark fixed?
,
Sep 15 2017
Yeah, I think that makes sense. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by tdres...@chromium.org
, May 18 2017