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

Issue 780670 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

119ms startup performance regression in RenderText::GetCursorBounds()

Project Member Reported by wittman@chromium.org, Nov 2 2017

Issue description

Data from the UMA Sampling Profiler shows that RenderText::GetCursorBounds() during pre-message loop start regressed browser process UI thread startup on 64 bit Chrome on Windows by 87ms. This occurred in the 64.0.3252.0 release.

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

Flame graph difference of 64.0.3251.0 vs. 64.0.3252.0: https://uma.googleplex.com/p/chrome/callstacks?sid=59bf7d6acf16768a3b13d47d3f4f9926

 
Cc: js...@chromium.org osh...@chromium.org andrewhayden@chromium.org msw@chromium.org
Components: UI>GFX UI>Internationalization
Yikes - that's pretty slow. That 87ms is all in base::i18n::BreakIterator::Init(..) - there are probably some disk reads in there.

I think something like https://chromium-review.googlesource.com/#/c/chromium/src/+/750461 will "fix" the performance regression. But it looks like The code in BreakIterator::createInstance is *always* picking the "slow path", and recompiling the break rules every time we need an iterator. It's probably not 87ms once the rule dictionary is cached in memory, but there's prob

But there are strategies for avoiding this. We can update base::i18n::BreakIterator either to initialize ICU's BreakIterator "service" so that BreakIterator::createInstance can use the fast path, or possibly just have one UBreakIterator stashed in the ui::ResourceBundle that we call ubrk_safeClone and ubrk_setText on whenever we need a new base::i18n::BreakIterator.

But it doesn't look like we're making any attempts to *disable* ICU's BreakIterator service. It just reaches code at


static inline UBool
hasService(void)
{
    return !gInitOnce.isReset() && getService() != NULL;
}

getService() lazily creates a service, but `!gInitOnce.isReset()` is always false, so it never tries.


Seems we need to trigger a call to BreakIterator::registerInstance at least once (and thereafter it auto-create if the locale changes? - that third_party/icu code is weird). Of course.. calling BreakIterator::registerInstance() during startup isn't going to remove the 87ms *startup* performance regression. But we should be able to lazily register something.


I actually get dozens of calls to BreakIterator::makeInstance when I start chrome - 1199 to be precise fact.  They seem to come from bookmarks code:

    frame #12: 0x000000011bc90997 libbase_i18n.dylib`base::i18n::BreakIterator::Init(this=0x000070000e516b60) at break_iterator.cc:73
    frame #13: 0x0000000101af6aa5 libchrome_dll.dylib`query_parser::QueryParser::ParseQueryImpl(this=0x000070000e516e90, query=0x000070000e516e78, matching_algorithm=DEFAULT, root=0x000070000e516c38) at query_parser.cc:412
    frame #14: 0x0000000101af7f7d libchrome_dll.dylib`query_parser::QueryParser::ParseQueryWords(this=0x000070000e516e90, query=0x000070000e516e78, matching_algorithm=DEFAULT, words=0x000070000e516e98 size=0) at query_parser.cc:351
    frame #15: 0x00000001083536fd libchrome_dll.dylib`bookmarks::TitledUrlIndex::ExtractQueryWords(this=0x000000013652b3c0, query=0x000070000e516f88) at titled_url_index.cc:229
    frame #16: 0x0000000108353232 libchrome_dll.dylib`bookmarks::TitledUrlIndex::Add(this=0x000000013652b3c0, node=0x000000015ce178d0) at titled_url_index.cc:60
    frame #17: 0x000000010833e4e7 libchrome_dll.dylib`bookmarks::(anonymous namespace)::AddBookmarksToIndex(details=0x000000013652b8a0, node=0x000000015ce178d0) at bookmark_storage.cc:47
    frame #18: 0x000000010833e533 libchrome_dll.dylib`bookmarks::(anonymous namespace)::AddBookmarksToIndex(details=0x000000013652b8a0, node=0x000000013652b4e0) at bookmark_storage.cc:50
    frame #19: 0x000000010833cb12 libchrome_dll.dylib`bookmarks::(anonymous namespace)::LoadCallback(


Does anyone own i18n::BreakIterator? (has a cache been considered?)
Summary: 119ms startup performance regression in RenderText::GetCursorBounds() (was: 87ms startup performance regression in RenderText::GetCursorBounds())
Looks like this is actually even worse than first reported -- I was only looking at one of four regression cases for this function. Including the other three the total pre-message loop start regression is actually 119ms(!): https://uma.googleplex.com/p/chrome/callstacks?sid=6d14d896e5d42eab3be702d284b45416
Labels: UMA-Sampling-Profiler
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 3 2017

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

commit 999fb54f66a703c9593d5a51a031ef5f7c26fc60
Author: Trent Apted <tapted@chromium.org>
Date: Fri Nov 03 21:18:07 2017

Optimize RenderText::GetCursorBounds() for cursor_position 0.

views::Textfield positions the cursor even when not focused. When doing
so, the cursor position is almost always at position 0. And whether the
text is RTL or LTR or mixed, the cursor will always be at the very start
or very end of the text area bounds.

Skip creating a costly i81n::BreakIterator, or calculating any glyph
bounds in this case, since it doesn't impact the cursor position.

Test coverage: 
 - TextfieldTest.GetCompositionCharacterBoundsTest,
 - TextfieldTest.TextCursorDisplay[InRTL]Test
 - RenderTextHarfBuzzTest.MoveCursorLeftRightIn{Ltr,LtrRtl,LtrRtlLtr,RtlLtr,Rtl,RtlLtr,RtlLtrRtl}

Bug:  780670 
Change-Id: I3a45a5d87ea4779324b77ad6148d192bdbcfce2e
Reviewed-on: https://chromium-review.googlesource.com/750461
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513912}
[modify] https://crrev.com/999fb54f66a703c9593d5a51a031ef5f7c26fc60/ui/gfx/render_text.cc

Status: Fixed (was: Assigned)
Fixed -> https://uma.googleplex.com/p/chrome/callstacks?sid=e2e940591ae082541662b434d930e71a

There's probably still some low-hanging fruit though. The juiciest seems to be a call to FontList::Derive taking 33ms in the omnibox constructor, at

OmniboxResultView::OmniboxResultView(OmniboxPopupContentsView* model,
                                     int model_index,
                                     const gfx::FontList& font_list)
    : model_(model),
      model_index_(model_index),
      is_hovered_(false),
      font_list_(font_list),
      font_height_(std::max(
          font_list.GetHeight(),
          font_list.DeriveWithWeight(gfx::Font::Weight::BOLD).GetHeight())),

If this got the font list from the resource bundle rather than deriving a new one, that 33ms would either disappear or at least be cached so it doesn't happen each time.

It's probably also safe to assume that all BOLD fonts have an identical height to their non-bold counterpart (it's the average width that could change)....

time spent in ResourceBundle::PNGContainsFallbackMarker seems like it could be encoded in resource bundle metadata too..

Had already filed  Issue 780714  for the unnecessary and slow call to UTF8ToUTF16()
before.png
457 KB View Download
after.png
444 KB View Download
Labels: Performance-Browser
Bulk adding Performance-Browser label for bugs identified with the UMA Sampling Profiler.

Sign in to add a comment