New issue
Advanced search Search tips

Issue 747104 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

7.1%-11.1% regression in thread_times.tough_scrolling_cases at 485730:485894

Project Member Reported by benhenry@google.com, Jul 20 2017

Issue description

I'm skeptical we will find anything here.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Jul 20 2017

All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=747104

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=ad85f4fb5f11bbfb6485b08fb32d3e2fbf8e434394321dec093fb68bb2dbbe16


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

android-nexus5X
android-nexus7v2
Project Member

Comment 3 by sheriffbot@chromium.org, Jul 21 2017

Labels: Hotlist-Google
Project Member

Comment 4 by 42576172...@developer.gserviceaccount.com, Jul 21 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 : Philip Rogers
  Commit : 106da9bd377ce42b00dc3d00fba01a298e8d1690
  Date   : Wed Jul 12 04:10:18 2017
  Subject: Remove LayerImpl scroll offset synchronization

Bisect Details
  Configuration: android_nexus7_perf_bisect
  Benchmark    : thread_times.tough_scrolling_cases
  Metric       : thread_raster_cpu_time_per_frame/thread_raster_cpu_time_per_frame
  Change       : 11.77% | 4.75021573596 -> 5.30949657479

Revision             Result                    N
chromium@485814      4.75022 +- 0.0719824      6      good
chromium@485834      4.79881 +- 0.0880165      6      good
chromium@485837      4.79822 +- 0.116237       6      good
chromium@485838      5.31864 +- 0.156496       6      bad       <--
chromium@485839      5.31566 +- 0.0829913      6      bad
chromium@485844      5.30963 +- 0.104585       6      bad
chromium@485854      5.27951 +- 0.045112       6      bad
chromium@485894      5.3095 +- 0.0912149       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.tough_scrolling_cases

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8973518954338299264


For feedback, file a bug with component Speed>Bisection
Status: Assigned (was: Untriaged)
Updating status. If you want some tools or context, please take a look at: https://chromium.googlesource.com/chromium/src/+/master/docs/speed/addressing_performance_regressions.md

Comment 6 by pdr@chromium.org, Jul 27 2017

To spite benhenry's pessimism in the first post, we have a patch in the CQ :)
https://chromium-review.googlesource.com/c/583565/
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 28 2017

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

commit 79fe848764520df822d23d9a234a7710a7ebfe83
Author: pdr <pdr@chromium.org>
Date: Fri Jul 28 03:03:35 2017

Update transform tree scroll offset from PushScrollUpdatesFromMainThread

This patch updates PushScrollUpdatesFromMainThread to update the transform
tree scroll offset (via DidUpdateScrollOffset) when the main thread offset
differs from the current offset.

This fixes a performance regression in  https://crbug.com/747104 . Before [1],
the transform tree scroll offsets were always updated with the current scroll
offset at the end of FinishCommitOnImplThread. After [1], some changes to
the current scroll offset could be missed which would cause an additional
update in a future frame.

[1] https://chromium.googlesource.com/chromium/src/+/106da9bd377ce42b00dc3d00fba01a298e8d1690

Bug:  747104 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Iae9ffbcdfbc89a2a7a78d5e0d3d574469898d84d
Reviewed-on: https://chromium-review.googlesource.com/583565
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490228}
[modify] https://crrev.com/79fe848764520df822d23d9a234a7710a7ebfe83/cc/trees/layer_tree_host_unittest_scroll.cc
[modify] https://crrev.com/79fe848764520df822d23d9a234a7710a7ebfe83/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/79fe848764520df822d23d9a234a7710a7ebfe83/cc/trees/property_tree.cc

Is spitefulness is next to cleanliness?

Thanks.

Comment 9 by pdr@chromium.org, Jul 29 2017

Status: Fixed (was: Assigned)
woohoo! Graphs dropped back by the amount I originally regressed.

Sign in to add a comment