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

Issue 877580 link

Starred by 3 users

Issue metadata

Status: Started
Owner:
Cc:
EstimatedDays: ----
NextAction: 2018-10-31
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

19.7%-18484.6% regression in rendering.mobile at 581037:585096

Project Member Reported by m...@chromium.org, Aug 24

Issue description

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

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


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

Android Nexus5 Perf
Android Nexus6 WebView Perf

rendering.mobile - Benchmark documentation link:
  https://bit.ly/rendering-benchmarks
Cc: acondor@chromium.org
Owner: acondor@chromium.org
Status: Assigned (was: Untriaged)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/16e874c5640000

Remove default source from MotionEventSynthetizerImpl by acondor@chromium.org
https://chromium.googlesource.com/chromium/src/+/14e88d0ddbdd58d346bc9c3bb0629af3fdf56f0f
16.95 → 2144 (+2127)

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions
Owner: sadrul@chromium.org
After several perf regression reports (see issues  855626 ,  874363 ), we are back to the original implementation of MotionEventSynthetizer. Assigning to sadrul, owner of the cases, to advice how to handle this. I believe the metrics shouldn't take into account the time for synthesizing the events.
Cc: majidvp@google.com
 Issue 877171  has been merged into this issue.
Cc: nzolghadr@chromium.org
It's not clear to me what the timeline of the regressions is. Is there a reason the CL in comment #3 should not be reverted?
I experimented different ways of overcoming perf regressions. Solutions for one caused others. This last CL reverts back to the behavior prior to https://crrev.com/c/1089856. My point is that the metrics are very sensitive on how the events are generated, which shouldn't be the case IMO. None of the CLs had behavior changes, just an extra argument for the event creation, which originally was a constant.
Cc: sadrul@chromium.org sahel@chromium.org
Owner: nzolghadr@chromium.org
Changing the source of the event would affect how the event is processed later in the pipeline. So it makes sense that changing the source would affect the performance numbers. It looks like before any of the changes, the source of the events created was unspecified. After the changes, the source is now 0, which I believe is treated as an unknown source [1]. What is the source set to when no source is specified during MotionEvent.obtain()? Should the default-source have been SOURCE_TOUCHSCREEN, instead of SOURCE_CLASS_POINTER in the original CL [2]?

I am going to hand this off to nzolghadr@ to triage.

[1] https://developer.android.com/reference/android/view/InputDevice#SOURCE_UNKNOWN
[2] https://chromium-review.googlesource.com/c/chromium/src/+/1167207/6/content/public/android/java/src/org/chromium/content/browser/MotionEventSynthesizerImpl.java#20
Oh, sorry, I missed that!

Yea, this is pretty weird then. Restoring the old behaviour of using 0 as the default source should have restored the metrics too?
FWIW, here's the full history of changes:
https://crrev.com/c/1089856
https://crrev.com/c/1167207
https://crrev.com/c/1176379

Each of them caused different metrics to regress.
Project Member

Comment 12 by bugdroid1@chromium.org, Sep 6

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

commit 6429116387660e52bed97d4eb7470665f999dd95
Author: Aldo Culquicondor <acondor@chromium.org>
Date: Thu Sep 06 13:52:41 2018

Add source to hover events for VR

Hover events don't get processed if the source is not set.
This change shouldn't affect scrolling performance metrics.

Bug:  880879 , 877580
Change-Id: I07ae28fcfdebfee77eab57e5659d1150745b6e9a
Reviewed-on: https://chromium-review.googlesource.com/1207651
Reviewed-by: Jinsuk Kim <jinsukkim@chromium.org>
Commit-Queue: Aldo Culquicondor <acondor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589151}
[modify] https://crrev.com/6429116387660e52bed97d4eb7470665f999dd95/content/public/android/java/src/org/chromium/content/browser/MotionEventSynthesizerImpl.java

Project Member

Comment 13 by bugdroid1@chromium.org, Sep 10

Labels: merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7b47e1342c67e0980c6206449a009897ef3ec3d1

commit 7b47e1342c67e0980c6206449a009897ef3ec3d1
Author: Aldo Culquicondor <acondor@chromium.org>
Date: Mon Sep 10 14:56:30 2018

Add source to hover events for VR

Hover events don't get processed if the source is not set.
This change shouldn't affect scrolling performance metrics.

Bug:  880879 , 877580
Change-Id: I07ae28fcfdebfee77eab57e5659d1150745b6e9a
Reviewed-on: https://chromium-review.googlesource.com/1207651
Reviewed-by: Jinsuk Kim <jinsukkim@chromium.org>
Commit-Queue: Aldo Culquicondor <acondor@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#589151}(cherry picked from commit 6429116387660e52bed97d4eb7470665f999dd95)
Reviewed-on: https://chromium-review.googlesource.com/1216522
Reviewed-by: Aldo Culquicondor <acondor@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#214}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/7b47e1342c67e0980c6206449a009897ef3ec3d1/content/public/android/java/src/org/chromium/content/browser/MotionEventSynthesizerImpl.java

NextAction: 2018-10-31
Status: Started (was: Assigned)
nzolghadr: Is this issue resolved now?
Aldo I didn't realize this was assigned to me. I see a bunch of code landing and reverting for this. Is this something I need to look into still?
Yes, Navid. The issue is that any changes in the MotionEventSynthetizer causes different perf regressions. See comment 11 for prior history of changes, which ended up fully reverting any source changes. The last commit only fixes the source for hover, which is not perf tested, IIRC.
The NextAction date has arrived: 2018-10-31

Sign in to add a comment