New issue
Advanced search Search tips

Issue 783350 link

Starred by 1 user
Status: Fixed
Owner:
kylixrd@chromium.org
Closed: Nov 2017
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Startup performance regression in LocationBarView::RefreshFindBarIcon()

Project Member Reported by wittman@chromium.org, Nov 9 2017 
Data from the UMA Sampling Profiler shows that changes to LocationBarView::RefreshFindBarIcon() regressed browser process UI thread startup on 64 bit Chrome on Windows by 8ms. This occurred in the 64.0.3262.0 release.

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

Flame graph difference of 64.0.3261.0 vs. 64.0.3262.0: https://uma.googleplex.com/p/chrome/callstacks?sid=c6a10b83941cc3b5bbea62a5789c0deb

The regression looks to be due to the code now forcing the load of the IDS_OMNIBOX_CLEAR_ALL string resource at startup, where it wasn't previously. If this can be lazily loaded the regression would be avoided.

Comment 1 by kylixrd@chromium.org, Nov 10 2017 

It looks like the real culprit here is the call to GetFindBarController() which creates the FindBar subsystem on demand. There is a HasFindBarController() function on Browser that should be called in RefreshFindBarIcon() to avoid creating FindBar subsystem just to see if it's visible. If the FindBar isn't available, there is no way it can be visible.

CL forthcoming.
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 17 2017 

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

commit 2e00483a0db227fbe68803f18083570fa507a398
Author: Allen Bauer <kylixrd@chromium.org>
Date: Fri Nov 17 17:17:01 2017

Added check for existance of the FindBarController before causing it to be created on demand. This should prevent a startup performance regression.

Bug:  783350 
Change-Id: Idbf4243e22db833fba8123c6b9e828c8c953ff12
Reviewed-on: https://chromium-review.googlesource.com/764030
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517441}
[add] https://crrev.com/2e00483a0db227fbe68803f18083570fa507a398/chrome/browser/ui/find_bar/find_bar_controller_browsertest.cc
[modify] https://crrev.com/2e00483a0db227fbe68803f18083570fa507a398/chrome/browser/ui/views/location_bar/location_bar_view.cc
[modify] https://crrev.com/2e00483a0db227fbe68803f18083570fa507a398/chrome/test/BUILD.gn

Comment 3 by kylixrd@chromium.org, Nov 17 2017

Status: Fixed (was: Assigned)

Comment 4 by wittman@chromium.org, Jan 5 2018

Labels: Performance-Browser
Bulk adding Performance-Browser label for bugs identified with the UMA Sampling Profiler.

Comment 5 by wittman@chromium.org, Jan 5 2018 

For posterity: I happened to be looking at this again and discovered that the 8ms cited corresponded to only one of several calls to HasFindBarController(). The total regression (and resulting fix) actually totaled 52ms.
►

Sign in to add a comment