119ms startup performance regression in RenderText::GetCursorBounds() |
|||||
Issue descriptionData 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
,
Nov 2 2017
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
,
Nov 2 2017
,
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
,
Nov 7 2017
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()
,
Jan 5 2018
Bulk adding Performance-Browser label for bugs identified with the UMA Sampling Profiler. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by tapted@chromium.org
, Nov 2 2017Components: UI>GFX UI>Internationalization