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

Issue 865295 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

Keyboard shortcut viewer: update is costly

Project Member Reported by sadrul@chromium.org, Jul 19

Issue description

The keyboard-shortcut-viewer app allows doing a search. Attached is a trace for entering the 'n' key in the search box. Look at the 'Service: shortcut_viewer_app' process, at around 2,800.211 timestamp. The task being executed there is KeyboardShortcutView::ShowSearchResults() [1]. It takes over 1 second to complete (~1150ms). Interesting observations:
 . RenderTextHarfBuzz:EnsureLayoutRunList() is called over 6000 times.
 . View::Layout(bounds_changed) is called over 2000 times.
 . The following paint of the view tree takes ~125+ms.

Similar to  issue 865286 , I think views and text layout in ui can be optimized to make this better.

[1] https://cs.chromium.org/chromium/src/ash/components/shortcut_viewer/views/keyboard_shortcut_view.cc?l=359
 
trace_ksv-update-after-key.json.gz
1.3 MB Download
A lot of the time spent during paint is again in RenderTextHarfBuzz:EnsureLayoutRunList(). So there's likely low-hanging fruit(s) here to cache some results (either in views, or in render-text) that would reduce some work during paint. (but the views and text layout would still need optimizations)
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 19

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

commit 384306f75ac2698898541dc17d32aa0486bd47d1
Author: Sadrul Habib Chowdhury <sadrul@chromium.org>
Date: Thu Jul 19 18:03:02 2018

ksv: Change when the startup-time is reported.

Instead of reporting the time to request the Widget to be visible,
report the time when the widget actually shows up on screen. So this
includes the time spent on, for example, View::Paint() etc.

BUG=865295

Change-Id: I040878e6f897b066102608e32ba3cd6d353b01d0
Reviewed-on: https://chromium-review.googlesource.com/1143603
Reviewed-by: Tao Wu <wutao@chromium.org>
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576558}
[modify] https://crrev.com/384306f75ac2698898541dc17d32aa0486bd47d1/ash/components/shortcut_viewer/views/keyboard_shortcut_view.cc
[modify] https://crrev.com/384306f75ac2698898541dc17d32aa0486bd47d1/ash/components/shortcut_viewer/views/keyboard_shortcut_view_unittest.cc

Hi sadrul@, which device is it.
For the search result, we do relayout the discription StyledLabel, because we need to hightlight/bold the matched query.

ps. There is also additional 250ms debounce delay [1]
[1] https://cs.chromium.org/chromium/src/ash/components/shortcut_viewer/views/keyboard_shortcut_view.cc?l=223&rcl=1ce671509c922ab463f3c2c9aff736745ade2850
The time mentioned in OP does not include the debounce-time.

This was on an original pixel-chromebook ('link' board). I have requested for a newer device, since 'link' I believe has been eol'ed.

I will be mildly surprised if the performance is much better on newer devices though.
Labels: M-69 Merge-Request-69
Request to merge cl in #2 to M69, so that we can see improvement.
To clarify: the CL in #2 does not make any change that would affect the user-experience. It just includes additional steps in the compositor pipeline so the reported number better reflects what the user experiences.
Labels: -M-69 -Merge-Request-69
Remove the request. The cl in #2 actually landed in 69.0.3497.0.
Thanks sadrul@!
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 25

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

commit 1c9af66df3f0b677961d6f7149e541ab6c83045a
Author: Sadrul Habib Chowdhury <sadrul@chromium.org>
Date: Sat Aug 25 02:36:50 2018

ksv: Measure how long it takes to update for search result.

Report how long it takes to update the views hierarchy in response
to a search-string. Also report how long it takes before the update
is visible on screen.

BUG=865295

Change-Id: I103aa5cbd86be3189ab44121e72d375e6d742b4b
Reviewed-on: https://chromium-review.googlesource.com/1188755
Reviewed-by: Tao Wu <wutao@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586111}
[modify] https://crrev.com/1c9af66df3f0b677961d6f7149e541ab6c83045a/ash/components/shortcut_viewer/views/keyboard_shortcut_view.cc
[modify] https://crrev.com/1c9af66df3f0b677961d6f7149e541ab6c83045a/tools/metrics/histograms/histograms.xml

Cc: -dfried@chromium.org

Sign in to add a comment