Issue metadata
Sign in to add a comment
|
3.5% regression in thread_times.key_silk_cases at 467051:467113 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Apr 27 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8981168544607101216
,
Apr 28 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 : 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!
,
May 3 2017
Issue 717701 has been merged into this issue.
,
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.
,
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
,
May 11 2017
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 |
|||||||||||||||||||||
Comment 1 by rmcilroy@chromium.org
, Apr 27 2017