New issue
Advanced search Search tips

Issue 860789 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug-Regression



Sign in to add a comment

32ms startup performance regression in OmniboxTextView::CalculatePreferredSize()

Project Member Reported by wittman@chromium.org, Jul 6

Issue description

Data from the UMA Sampling Profiler shows that OmniboxTextView::CalculatePreferredSize() regressed browser process UI thread startup on 64-bit Chrome on Windows by 32ms. This occurred in the 69.0.3480.0 Canary release.

The responsible CL appears to be https://chromium.googlesource.com/chromium/src.git/+/45bf0faac15aab71a2e0ea6eb090ff2fdad01017

Execution profile difference of Canary 69.0.3479.0 vs. 69.0.3480.0: https://uma.googleplex.com/p/chrome/callstacks?sid=18626d20573158a0d3b9987bca7afdb0

 
So, it looks like what's happening here is:
- The omnibox popup is being created at startup
- Each of the result cells sets " - " as text on its separator view
- The linked change caches OmniboxTextView preferred sizes when text is changed
... but I didn't realize the separator view was one, so the cached size is never used. This part might not be totally relevant to this issue, but I'll address it regardless

Normally I would say "maybe we don't want to construct the Omnibox popup at startup at all", but it looks like doing the text shaping *doubles* construction time. There's some stuff about creating glyph caches in the flame graph, so my theory is that.

Before getting someone who understands Windows text layout involved, I'm curious if 
https://chromium-review.googlesource.com/c/chromium/src/+/1123456 fixes the regression, since it will cause only 1 shape vs. one for each match cell.

Otherwise, I think we would need to weigh this vs. savings while typing in the Omnibox from not shaping every time when results aren't changing.
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 9

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

commit 6aac6ccf30deb69d888247a853b9087610606bd4
Author: Leonard Grey <lgrey@chromium.org>
Date: Mon Jul 09 14:50:17 2018

Views: Use cached size for Omnibox match separator

Missed that the separator was an OmniboxTextView.

Bug: 860789, 712268
Change-Id: I670fa25754f23a5aaaa9201f548d9b735f92ecf3
Reviewed-on: https://chromium-review.googlesource.com/1128187
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573304}
[modify] https://crrev.com/6aac6ccf30deb69d888247a853b9087610606bd4/chrome/browser/ui/views/omnibox/omnibox_match_cell_view.cc

wittman@ are we still seeing this as a regression or did it cancel out in layouts? I probably don't want to pull the change either way, but we might be able to split the difference somehow on creating the popup.
The regression is still there. The change above landed in 69.0.3487.0 and the diff between 69.0.3486.0 and 69.0.3487.0 shows no improvement: https://uma.googleplex.com/p/chrome/callstacks?sid=b726db3d43124a46971bbb314ca42ba1

Sign in to add a comment