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

Issue 861855 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

First Input Delay reports invalid values in > 0.0006% of cases.

Project Member Reported by tdres...@chromium.org, Jul 9

Issue description

We see a variety of values which are invalid, including a large number of FIDs which are MAX_INT.


 
Cc: nzolghadr@chromium.org majidvp@chromium.org bokan@chromium.org
It appears that this is due to invalid timestamps. More details on invalid timestamps here: https://bugs.chromium.org/p/chromium/issues/detail?id=650338#c1.

I plan to enable fixing invalid timestamps across all platforms.

Its possible we should really be throwing out data in these cases, instead of fixing the timestamps. That's a bit more plumbing though. I'll do the simple thing first, and merge reasonably aggressively, and then dig into a slightly better long term solution.
One thing that made me hesitant to apply a  timestamp fix for all platforms was the fact that the current solution 
requires getting TimeTicks::Now() to  detect invalid timestamps.

While we heavily use TimeTicks::Now() and it is expected to be cheap but it felt overkill to use it on every event 
for platforms where only a very small fraction of events need correction (i.e., none linux platforms).


Checking the event timestamp does not really require TimeTicks::Now() but a recent value 
can suffice. I suspect a simple change that can make this check very cheap is to have a reasonably
recent now_ticks value instead of TimeTicks::Now(). That value can updated much more infrequently 
(e.g., once every second) and avoid reading ticks per event.
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 10

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

commit aacf8c02f2478af54fae69058cfecd846fd95138
Author: Tim Dresser <tdresser@chromium.org>
Date: Tue Jul 10 20:13:26 2018

Fix bogus event timestamps across all platforms.

First Input Delay was reporting bogus values in > 0.0006% of cases.
The data suggests that this is due to invalid event timestamps.

Bug: 861855
Change-Id: Ib3e6bb22348546f60dc6b98c6955e8620e87672b
Reviewed-on: https://chromium-review.googlesource.com/1129843
Commit-Queue: Timothy Dresser <tdresser@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573878}
[modify] https://crrev.com/aacf8c02f2478af54fae69058cfecd846fd95138/ui/events/event_utils.cc

Labels: Merge-Request-68 OS-Android OS-Chrome OS-Mac OS-Windows
Requesting merge to M68.

Why: this is causing us to have invalid data for a small number of cases in the public facing Chrome User Experience Report.

Risk: This is a couple of line fix, aligning behavior on non-X11 with the behavior currently on X11.


(Note that this issue does still show up on Linux, but it's much less prevalent. I've temporarily removed Linux from the listed OS's to make it clear that the merge has no impact on Linux)

Project Member

Comment 5 by sheriffbot@chromium.org, Jul 11

Labels: -Merge-Request-68 Hotlist-Merge-Review Merge-Review-68
This bug requires manual review: We are only 12 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
comment+4 does not explicitly say how safe this change is for M68 stable.
Re #6, sorry, I'm not sure what the recommended approach is of communicating how risky a change is. 

From my perspective, this is about as safe as a merge can be: this path is already taken on Linux, and the number of lines of code changed is minimal.
Labels: -Merge-Review-68 Merge-Approved-68
Thanks for more details. Approving merge to M68. Branch:3440
Project Member

Comment 9 by bugdroid1@chromium.org, Jul 13

Labels: -merge-approved-68 merge-merged-3440
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a8f0c2cc9414984cf4dccfb4364a2bf4fd79cf0c

commit a8f0c2cc9414984cf4dccfb4364a2bf4fd79cf0c
Author: Tim Dresser <tdresser@chromium.org>
Date: Fri Jul 13 18:15:22 2018

Fix bogus event timestamps across all platforms.

First Input Delay was reporting bogus values in > 0.0006% of cases.
The data suggests that this is due to invalid event timestamps.

Bug: 861855
Change-Id: Ib3e6bb22348546f60dc6b98c6955e8620e87672b
Reviewed-on: https://chromium-review.googlesource.com/1129843
Commit-Queue: Timothy Dresser <tdresser@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#573878}(cherry picked from commit aacf8c02f2478af54fae69058cfecd846fd95138)
Reviewed-on: https://chromium-review.googlesource.com/1136615
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#666}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/a8f0c2cc9414984cf4dccfb4364a2bf4fd79cf0c/ui/events/event_utils.cc

Cc: tdres...@chromium.org
Owner: charliea@chromium.org
Status: Assigned (was: Started)
Cc: charliea@chromium.org
Owner: tdres...@chromium.org
Whoops, wrong bug.
Ping! This bug has been flagged as stale by Chrome Speed Metrics bug triage, and it has an owner. Any update on this issue?
Our choices are:
1) Use the timestamp from the browser for these metrics.
2) Accept that these numbers are clamped, and a small fraction of the data is bogus.

Clamping will, I think, prevent issues with downstream users seeing completely bogus data.

Navid, any thoughts? Whatever we do should be aligned with the events timestamp as exposed to JS. If we think that the issues with event hardware timestamps are sufficient for us to not be able to use them for our metrics, we shouldn't expose them to webdevs either.
I'm not following the question. Isn't "using the timestamps that browser received the events" as the timestamp of the event what we do today? Like here:
https://cs.chromium.org/chromium/src/ui/views/win/hwnd_message_handler.cc?type=cs&q=handlepointereventtypetouch&sq=package:chromium&g=0&l=2970

On Windows we use the time the browser received the event. On other platforms we use the timestamp from the driver. The drivers lie, but infrequently.

Sign in to add a comment