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

Issue 778587 link

Starred by 3 users

Issue metadata

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



Sign in to add a comment

252.9% regression in thread_times.tough_scrolling_cases at 511369:511403

Project Member Reported by kraynov@chromium.org, Oct 26 2017

Issue description

See the link to graphs below.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Oct 26 2017

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

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


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

android-webview-nexus6
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Oct 26 2017

Cc: bokan@chromium.org
Owner: bokan@chromium.org
Status: Assigned (was: Untriaged)

=== Auto-CCing suspected CL author bokan@chromium.org ===

Hi bokan@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 : David Bokan
  Commit : dbab215523c8d77a7b54fcf3776357b079c36899
  Date   : Tue Oct 24 14:24:10 2017
  Subject: Fix telemetry gesture speeds

Bisect Details
  Configuration: android_webview_nexus6_aosp_perf_bisect
  Benchmark    : thread_times.tough_scrolling_cases
  Metric       : thread_raster_cpu_time_per_frame/text_constant_full_page_raster_20000_pixels_per_second
  Change       : 274.89% | 0.843449282846 -> 3.16200321883

Revision                                 Result                     N
chromium@511368                          0.843449 +- 0.147427       6      good
chromium@511386                          0.925228 +- 0.250727       6      good
chromium@511395                          0.991017 +- 0.210565       6      good
chromium@511397                          0.850726 +- 0.0942913      6      good
chromium@511397,catapult@f9691b256a      0.906883 +- 0.19888        6      good
chromium@511397,catapult@dbab215523      3.13416 +- 0.178554        6      bad       <--
chromium@511397,catapult@2f4cf37a9f      3.16219 +- 0.0875242       6      bad
chromium@511398                          3.11457 +- 0.147957        6      bad
chromium@511399                          3.13444 +- 0.0975286       6      bad
chromium@511403                          3.162 +- 0.175306          6      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-webview --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=text.constant.full.page.raster.20000.pixels.per.second 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/8964678053482373328


For feedback, file a bug with component Speed>Bisection
Project Member

Comment 4 by 42576172...@developer.gserviceaccount.com, Oct 26 2017

 Issue 778588  has been merged into this issue.

Comment 5 by bokan@chromium.org, Oct 27 2017

Status: Started (was: Assigned)
This isn't a real regression and a change was expected. However, it looks like the telemetry gestures are wrong in this test (yet again, sigh). I'm working on a fix which I'll attribute to this bug.
Project Member

Comment 6 by 42576172...@developer.gserviceaccount.com, Oct 31 2017

Cc: briander...@chromium.org
 Issue 779810  has been merged into this issue.
Project Member

Comment 7 by 42576172...@developer.gserviceaccount.com, Oct 31 2017

 Issue 779795  has been merged into this issue.
Project Member

Comment 8 by 42576172...@developer.gserviceaccount.com, Oct 31 2017

 Issue 779810  has been merged into this issue.
Project Member

Comment 9 by 42576172...@developer.gserviceaccount.com, Oct 31 2017

 Issue 779813  has been merged into this issue.
Project Member

Comment 10 by 42576172...@developer.gserviceaccount.com, Oct 31 2017

 Issue 779816  has been merged into this issue.
Project Member

Comment 11 by 42576172...@developer.gserviceaccount.com, Oct 31 2017

Cc: npm@chromium.org
 Issue 779812  has been merged into this issue.
Project Member

Comment 12 by 42576172...@developer.gserviceaccount.com, Oct 31 2017

Cc: yukishiino@chromium.org
 Issue 779811  has been merged into this issue.
Project Member

Comment 13 by 42576172...@developer.gserviceaccount.com, Oct 31 2017

 Issue 780199  has been merged into this issue.
 Issue 780200  has been merged into this issue.
Project Member

Comment 15 by bugdroid1@chromium.org, Nov 13 2017

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

commit cbeb064ce8a422528be2e2e6eca1942333628f01
Author: David Bokan <bokan@chromium.org>
Date: Mon Nov 13 17:00:01 2017

Fix telemetry gesture wheel scrolling on Android

On Android, wheel events are sent with the pixels per tick and number
of ticks. Currently, the synthetic_smooth_move_gesture puts the
desired delta into the wheel event which assumes each wheel tick scrolls
1 pixel. On Android, the default today is each tick is 64px. This means
the scroll distance and speed end up being 64X the requested amount!

This patch fixes the Android gesture target to set the event values
correctly by setting the correct number of ticks given the desired
delta (and truncating any remainder).

However, since we try to send an event every 16ms, this means any
gesture that scrolls slower than a wheel tick every 16ms wont scroll
at all since we truncate the delta to the wheel tick size. So this
patch also changes the dispatch logic to avoid accumulating and sending
an event that doesn't have any ticks. This means for slower scrolls, we
may go multiple frames without sending a scroll.

I've also added tests in telemetry for this change:
https://chromium-review.googlesource.com/c/catapult/+/751684
This patch adds methods setPageScaleFactor and setBrowserControlsShown
that are used from those tests.

Bug:  778587 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ib40aee2b3f9f38f68482b30e27b95f7fe84e1ef9
Reviewed-on: https://chromium-review.googlesource.com/749980
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515967}
[modify] https://crrev.com/cbeb064ce8a422528be2e2e6eca1942333628f01/content/browser/android/content_view_core.cc
[modify] https://crrev.com/cbeb064ce8a422528be2e2e6eca1942333628f01/content/browser/android/content_view_core.h
[modify] https://crrev.com/cbeb064ce8a422528be2e2e6eca1942333628f01/content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc
[modify] https://crrev.com/cbeb064ce8a422528be2e2e6eca1942333628f01/content/browser/renderer_host/input/synthetic_gesture_target.h
[modify] https://crrev.com/cbeb064ce8a422528be2e2e6eca1942333628f01/content/browser/renderer_host/input/synthetic_gesture_target_android.cc
[modify] https://crrev.com/cbeb064ce8a422528be2e2e6eca1942333628f01/content/browser/renderer_host/input/synthetic_gesture_target_base.cc
[modify] https://crrev.com/cbeb064ce8a422528be2e2e6eca1942333628f01/content/browser/renderer_host/input/synthetic_gesture_target_base.h
[modify] https://crrev.com/cbeb064ce8a422528be2e2e6eca1942333628f01/content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc
[modify] https://crrev.com/cbeb064ce8a422528be2e2e6eca1942333628f01/content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc
[modify] https://crrev.com/cbeb064ce8a422528be2e2e6eca1942333628f01/content/browser/renderer_host/render_widget_host_view_android.cc
[modify] https://crrev.com/cbeb064ce8a422528be2e2e6eca1942333628f01/content/browser/renderer_host/render_widget_host_view_android.h
[modify] https://crrev.com/cbeb064ce8a422528be2e2e6eca1942333628f01/content/browser/renderer_host/render_widget_host_view_base.cc
[modify] https://crrev.com/cbeb064ce8a422528be2e2e6eca1942333628f01/content/browser/renderer_host/render_widget_host_view_base.h
[modify] https://crrev.com/cbeb064ce8a422528be2e2e6eca1942333628f01/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
[modify] https://crrev.com/cbeb064ce8a422528be2e2e6eca1942333628f01/content/renderer/gpu/gpu_benchmarking_extension.cc
[modify] https://crrev.com/cbeb064ce8a422528be2e2e6eca1942333628f01/content/renderer/gpu/gpu_benchmarking_extension.h
[modify] https://crrev.com/cbeb064ce8a422528be2e2e6eca1942333628f01/third_party/WebKit/LayoutTests/fast/scroll-behavior/wheel-and-touch-scroll-use-count.html

Project Member

Comment 16 by 42576172...@developer.gserviceaccount.com, Nov 13 2017

Cc: tdres...@chromium.org
 Issue 784432  has been merged into this issue.

Comment 17 by bokan@chromium.org, Nov 15 2017

Status: Fixed (was: Started)
The patch above fixes tests that use mouse wheels (e.g. text_hover_x_pixels_per_second)

Otherwise, the changes to all the benchmarks are expected.
Project Member

Comment 18 by bugdroid1@chromium.org, Nov 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/7e4796650c837c13363eceb011d0c8bd8fef72dc

commit 7e4796650c837c13363eceb011d0c8bd8fef72dc
Author: David Bokan <bokan@chromium.org>
Date: Fri Nov 17 18:46:47 2017

Test that gestures scroll the correct distance

Add tests to ensure smooth scrolling with a given distance scrolls the
correct distance under various scenarios. These tests go together with
fixes introduced in:
https://chromium-review.googlesource.com/c/chromium/src/+/749980

Bug:  chromium:778587 
Change-Id: I3f2eb12c8430f38501398e140588e3e47eb4970a
Reviewed-on: https://chromium-review.googlesource.com/751684
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: Ned Nguyen <nednguyen@google.com>

[modify] https://crrev.com/7e4796650c837c13363eceb011d0c8bd8fef72dc/telemetry/telemetry/internal/actions/scroll_unittest.py

Project Member

Comment 19 by bugdroid1@chromium.org, Nov 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/d0e5c9c00f4e0d4e654efd9eb9f3d8fa025a8a4e

commit d0e5c9c00f4e0d4e654efd9eb9f3d8fa025a8a4e
Author: Ned Nguyen <nednguyen@google.com>
Date: Wed Nov 22 12:19:47 2017

Revert "Test that gestures scroll the correct distance"

This reverts commit 7e4796650c837c13363eceb011d0c8bd8fef72dc.

Reason for revert: test is failing on Mac, blocking catapult roll: https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/595740

Original change's description:
> Test that gestures scroll the correct distance
> 
> Add tests to ensure smooth scrolling with a given distance scrolls the
> correct distance under various scenarios. These tests go together with
> fixes introduced in:
> https://chromium-review.googlesource.com/c/chromium/src/+/749980
> 
> Bug:  chromium:778587 
> Change-Id: I3f2eb12c8430f38501398e140588e3e47eb4970a
> Reviewed-on: https://chromium-review.googlesource.com/751684
> Commit-Queue: David Bokan <bokan@chromium.org>
> Reviewed-by: Ned Nguyen <nednguyen@google.com>

TBR=bokan@chromium.org,nednguyen@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  chromium:778587 
Change-Id: I9054f57abde61d62f7805c48b2c53791bcfc3c15
Reviewed-on: https://chromium-review.googlesource.com/785330
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Commit-Queue: Ned Nguyen <nednguyen@google.com>

[modify] https://crrev.com/d0e5c9c00f4e0d4e654efd9eb9f3d8fa025a8a4e/telemetry/telemetry/internal/actions/scroll_unittest.py

Project Member

Comment 20 by bugdroid1@chromium.org, Nov 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/b4ea163be204edca1db758b3bf90071879da78cd

commit b4ea163be204edca1db758b3bf90071879da78cd
Author: David Bokan <bokan@chromium.org>
Date: Wed Nov 22 17:47:59 2017

Reland "Test that gestures scroll the correct distance"

This patch was originally landed in
7e4796650c837c13363eceb011d0c8bd8fef72dc
and then reverted in d0e5c9c00f4e0d4e654efd9eb9f3d8fa025a8a4e due to
failing on the Mac bot when rolled into the Chromium tree.

The issue in the original patch was that the tests were sending touch
events to a Mac browser which causes a crash. This CL relands that patch
by skipping touch tests on Mac.

Bug:  chromium:778587 
Change-Id: Ibb08baae2b70c42e092fc877f252c7073b0f8493
Reviewed-on: https://chromium-review.googlesource.com/785550
Reviewed-by: Ned Nguyen <nednguyen@google.com>
Commit-Queue: David Bokan <bokan@chromium.org>

[modify] https://crrev.com/b4ea163be204edca1db758b3bf90071879da78cd/telemetry/telemetry/internal/actions/scroll_unittest.py

Project Member

Comment 21 by bugdroid1@chromium.org, Nov 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/86c17b98f5364a8a3f903df3d946ea7afe4d729b

commit 86c17b98f5364a8a3f903df3d946ea7afe4d729b
Author: Juan Antonio Navarro Pérez <perezju@chromium.org>
Date: Thu Nov 23 10:24:00 2017

Revert "Reland "Test that gestures scroll the correct distance""

This reverts commit b4ea163be204edca1db758b3bf90071879da78cd.

Reason for revert: Still failing on catapult roller, see e.g.:
https://chromium-review.googlesource.com/c/chromium/src/+/786139

Original change's description:
> Reland "Test that gestures scroll the correct distance"
> 
> This patch was originally landed in
> 7e4796650c837c13363eceb011d0c8bd8fef72dc
> and then reverted in d0e5c9c00f4e0d4e654efd9eb9f3d8fa025a8a4e due to
> failing on the Mac bot when rolled into the Chromium tree.
> 
> The issue in the original patch was that the tests were sending touch
> events to a Mac browser which causes a crash. This CL relands that patch
> by skipping touch tests on Mac.
> 
> Bug:  chromium:778587 
> Change-Id: Ibb08baae2b70c42e092fc877f252c7073b0f8493
> Reviewed-on: https://chromium-review.googlesource.com/785550
> Reviewed-by: Ned Nguyen <nednguyen@google.com>
> Commit-Queue: David Bokan <bokan@chromium.org>

TBR=bokan@chromium.org,nednguyen@google.com

Change-Id: Ic246e622f44ea1f07e98e486e40bddf901e1d3c1
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  chromium:778587 
Reviewed-on: https://chromium-review.googlesource.com/787450
Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org>
Commit-Queue: Juan Antonio Navarro Pérez <perezju@chromium.org>

[modify] https://crrev.com/86c17b98f5364a8a3f903df3d946ea7afe4d729b/telemetry/telemetry/internal/actions/scroll_unittest.py

Project Member

Comment 22 by bugdroid1@chromium.org, Nov 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/884db2336247b7b412e913516ebe7bbe2768c477

commit 884db2336247b7b412e913516ebe7bbe2768c477
Author: David Bokan <bokan@chromium.org>
Date: Tue Nov 28 00:56:08 2017

Re-reland "Test that gestures scroll the correct distance"

The original patch in 7e4796650c837c13363eceb011d0c8bd8fef72dc was once
again reverted in 86c17b98f5364a8a3f903df3d946ea7afe4d729b. This time
the newly added test broke on Windows and Linux during the catapult
roll.

This test failure actually uncovered an issue in animated wheel
scrolling, the fix for which is in https://crrev.com/c/788134.
Additionally, I discovered that animated scrolls can lose some delta
when the visual viewport reaches the layout viewport extent. At that
point the layout viewport starts animating, however, the delta that
contributed to the visual viewport's scroll animation curve is lost.

This CL changes the testWheelScrollDistanceWhileZoomed test to first
scroll the visual viewport to the end so that the entire delta is
applied to the layout viewport.

Bug:  chromium:778587 
Change-Id: I50520fd1c43e607d1e79ce22b6a0a8ba9fee154e
Reviewed-on: https://chromium-review.googlesource.com/788490
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: Ned Nguyen <nednguyen@google.com>

[modify] https://crrev.com/884db2336247b7b412e913516ebe7bbe2768c477/telemetry/telemetry/internal/actions/scroll_unittest.py

 Issue 778605  has been merged into this issue.
 Issue 780203  has been merged into this issue.

Comment 25 by bokan@chromium.org, Jan 11 2018

 Issue 780206  has been merged into this issue.

Sign in to add a comment