New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 715956 link

Starred by 3 users

Issue metadata

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



Sign in to add a comment

3.5% regression in thread_times.key_silk_cases at 467051:467113

Project Member Reported by rmcilroy@chromium.org, Apr 27 2017

Issue description

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

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


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

android-nexus5
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Apr 28 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 : 23d381fd827c8880b85fe5794627e98af07454cc
  Date   : Tue Apr 25 21:06:31 2017
  Subject: Move LayerImpl's bounds_delta to property trees

Bisect Details
  Configuration: android_nexus5_perf_bisect
  Benchmark    : thread_times.key_silk_cases
  Metric       : thread_renderer_compositor_cpu_time_per_frame/thread_renderer_compositor_cpu_time_per_frame
  Change       : 3.90% | 4.02589287185 -> 4.18286949066

Revision             Result                    N
chromium@467050      4.02589 +- 0.0307398      6      good
chromium@467082      4.04778 +- 0.0416012      6      good
chromium@467098      4.05036 +- 0.0434244      6      good
chromium@467106      4.05254 +- 0.0276748      6      good
chromium@467110      4.043 +- 0.0358903        6      good
chromium@467111      4.21129 +- 0.0437818      6      bad       <--
chromium@467112      4.17334 +- 0.0502871      6      bad
chromium@467113      4.18287 +- 0.0458589      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 thread_times.key_silk_cases

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8981168544607101216

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5882781176954880


| 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!

Comment 4 by pdr@chromium.org, May 3 2017

Cc: jaydasika@chromium.org weiliangc@chromium.org enne@chromium.org wkorman@chromium.org
 Issue 717701  has been merged into this issue.

Comment 5 by pdr@chromium.org, May 4 2017

My patch moved viewport-specific delta values from all layerimpls to a fixed number of viewport delta values on the scroll tree. This caused a regression because it's expensive to compute if a given layer is one of the special viewport layers.
Project Member

Comment 6 by bugdroid1@chromium.org, May 10 2017

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

commit 6280cc1d5a1dccfbe23ca438eed862e245f18614
Author: pdr <pdr@chromium.org>
Date: Wed May 10 23:22:47 2017

Add a cache of LayerImpl's viewport layer type

This patch caches the viewport layer type of LayerImpl to fix a performance
regression in LayerImpl::ViewportBoundsDelta due to layer lookups.

Big changes in this patch:
* A viewport_layer_type_ member has been added to LayerImpl along with
getters and setters.
* The viewport layer type is updated when LayerTreeImpl's viewport layer
id changes, or when LayerImpl's scroll_clip_layer_id changes. Both of
these values affect the viewport layer type.
* DCHECKs have been added that the viewport layer type does not change.
Making this a constructor parameter to LayerImpl would be ideal but is
very difficult.
* DCHECKs have been added in LTI::IsViewportLayerId that verify
LayerTreeImpl's viewport layer ids are always in sync with LayerImpl's
viewport layer types.

BUG= 715956 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2867793002
Cr-Commit-Position: refs/heads/master@{#470743}

[modify] https://crrev.com/6280cc1d5a1dccfbe23ca438eed862e245f18614/cc/layers/layer_impl.cc
[modify] https://crrev.com/6280cc1d5a1dccfbe23ca438eed862e245f18614/cc/layers/layer_impl.h
[modify] https://crrev.com/6280cc1d5a1dccfbe23ca438eed862e245f18614/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/6280cc1d5a1dccfbe23ca438eed862e245f18614/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/6280cc1d5a1dccfbe23ca438eed862e245f18614/cc/trees/layer_tree_impl.h
[modify] https://crrev.com/6280cc1d5a1dccfbe23ca438eed862e245f18614/cc/trees/layer_tree_impl_unittest.cc

Comment 7 by pdr@chromium.org, May 11 2017

Status: Fixed (was: Untriaged)
The perf graphs recovered and even improved a little. I also verified that CalcDrawPropsTest.TenTen recovered. Marking this as fixed.

Sign in to add a comment