New issue
Advanced search Search tips

Issue 724193 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

167.7% regression in smoothness.sync_scroll.key_mobile_sites_smooth at 470727:470763

Project Member Reported by tdres...@chromium.org, May 18 2017

Issue description

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

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDg6o3sowkM


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

android-nexus6
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, May 18 2017

Cc: pdr@chromium.org
Owner: pdr@chromium.org

=== 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!
Project Member

Comment 4 by bugdroid1@chromium.org, 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

Project Member

Comment 5 by bugdroid1@chromium.org, 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

Project Member

Comment 6 by bugdroid1@chromium.org, 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

Project Member

Comment 7 by bugdroid1@chromium.org, 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

Status: Assigned (was: Untriaged)
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.
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?

Comment 10 by pdr@chromium.org, Sep 15 2017

Status: Fixed (was: Assigned)
Yeah, I think that makes sense.

Sign in to add a comment