New issue
Advanced search Search tips

Issue 923890 link

Starred by 1 user

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Omnibox metric not measured correctly (CharTypedToRepaintLatency)

Project Member Reported by sadrul@chromium.org, Yesterday (38 hours ago)

Issue description

The Omnibox.CharTypedToRepaintLatency metric currently reports the time between from:
 (a) the IME instructing chrome to do an insertion in DoInsertChar() [1], until
 (b) the time the ui is notified of the display-compositor having processed the visual update in OnCompositingEnded() [2] (but not necessarily displayed on screen yet).

There are several issues with (b):
 i) The presentation-feedback provides a better timestamp for measuring the metric, which gives the actual timestamp of when the text update goes on screen. The ui::Compositor receives the presentation-timestamp in [3]. It would be better if that timestamp was plumbed back into OmniboxViewViews and used to measure the metric.
 ii) The usage of TimeTicks::Now() in the existing metric means any changes to IPC, or the order of tasks, would affect the metric, even if that doesn't mean an actual change in the time the display-compositor processed the visual update.

To make the metric more meaningful and actionable, I suggest switching to using presentation-timestamp for measuring the metric.

[1] https://cs.chromium.org/chromium/src/chrome/browser/ui/views/omnibox/omnibox_view_views.cc?type=cs&sq=package:chromium&g=0&l=1330
[2] https://cs.chromium.org/chromium/src/chrome/browser/ui/views/omnibox/omnibox_view_views.cc?type=cs&sq=package:chromium&g=0&l=1721
[3] https://cs.chromium.org/chromium/src/ui/compositor/compositor.cc?type=cs&sq=package:chromium&g=0&l=588
 

Comment 1 by k...@chromium.org, Today (13 hours ago)

Cc: mpear...@chromium.org
Some comments regarding the suggestion:

i) The Omnibox team would prefer to measure shorter stacks (to know more easily where the regression occurred, and because there's less variation) close to the Omnibox code (because this is what we might have changed and can remedy.)

ii) A static change e.g. IPC would simply be bisected, and other random differences would be amortized among the billions of data points.

Comment 2 by sadrul@chromium.org, Today (13 hours ago)

Using base::TimeTicks::Now() is not going to give you the right time though. You should at least plumb through the timestamps from when the display-compositor is done processing the visual update, instead of getting the timestamp from when the task is run in the UI thread. For example, with the switch to out-of-process display-compositor (OOP-D), the current time would go up, but that doesn't mean the metric has regressed.

Comment 3 by mpear...@chromium.org, Today (11 hours ago)

Cc: sullivan@chromium.org
Status: Untriaged (was: Available)
CC sullivan@, who've been talking about improving this metric in (I think) exactly the way you suggest.  I'm not sure on the progress on that however, or if there's already a tracking bug.

Comment 4 by sullivan@chromium.org, Today (11 hours ago)

I made some changes to get the timestamp from the input event plumbed through to the page load metrics observer, so that we can measure input->fcp and metrics like that.

Doc: https://docs.google.com/document/d/1sAaPCsvxF19l1fgGcxW-SouxVwwcQ4rsz4md4jjq2Gs/edit
CL: https://chromium-review.googlesource.com/1178624

Sign in to add a comment