Issue metadata
Sign in to add a comment
|
19.7%-18484.6% regression in rendering.mobile at 581037:585096 |
||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Aug 24
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/16e874c5640000
,
Aug 26
📍 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
,
Aug 27
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.
,
Aug 27
,
Aug 27
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?
,
Aug 27
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.
,
Aug 27
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
,
Aug 27
Minor correction: The source of the events was originally 0 [1]. [1] https://crrev.com/c/1089856/7/content/public/android/java/src/org/chromium/content/browser/MotionEventSynthesizerImpl.java#b103
,
Aug 27
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?
,
Aug 27
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.
,
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
,
Sep 10
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
,
Oct 10
nzolghadr: Is this issue resolved now?
,
Oct 10
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?
,
Oct 12
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.
,
Oct 31
The NextAction date has arrived: 2018-10-31 |
|||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Aug 24