First Input Delay reports invalid values in > 0.0006% of cases. |
|||||||
Issue descriptionWe see a variety of values which are invalid, including a large number of FIDs which are MAX_INT.
,
Jul 9
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.
,
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
,
Jul 11
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)
,
Jul 11
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
,
Jul 12
comment+4 does not explicitly say how safe this change is for M68 stable.
,
Jul 13
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.
,
Jul 13
Thanks for more details. Approving merge to M68. Branch:3440
,
Jul 13
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
,
Aug 14
,
Aug 14
Whoops, wrong bug.
,
Nov 26
Ping! This bug has been flagged as stale by Chrome Speed Metrics bug triage, and it has an owner. Any update on this issue?
,
Nov 26
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.
,
Nov 26
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
,
Nov 27
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 |
|||||||
Comment 1 by tdres...@chromium.org
, Jul 9