Slight performance degradation in RenderTextHarfbuzz layout
Reported by
alshaba...@yandex-team.ru,
Dec 22 2017
|
||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 YaBrowser/17.11.1.844 Yowser/2.5 Safari/537.36 Steps to reproduce the problem: Run this code before and after https://chromium.googlesource.com/chromium/src/+/141af5c41b4e6a777e5c6d53575f124bc838865e gfx::RenderTextHarfBuzz render_text; int height = 0; render_text.SetText(base::ASCIIToUTF16("aaaa bbbb cccc, dddd eeee ffff")); auto now = base::TimeTicks::Now(); height = render_text.GetStringSize().height(); auto after = base::TimeTicks::Now(); LOG(ERROR) << (after - now); LOG(ERROR) << height; What is the expected behavior? The last printed number (i.e the time to do layout) didn't change much. What went wrong? On my Windows box in release build it used to be 0.09 +- 0.02 ms. And after the patch it became 0.21 +- 0.03 ms. Did this work before? N/A Chrome version: 62.0.3202.94 Channel: n/a OS Version: OS X 10.12.6 Flash Version: Shockwave Flash 28.0 r0 The issue here is that the string used to be broken into 3 runs: before ", ", ", " itself, and after ", ". But after the patch it breaks into 11 runs around each ' '.
,
Dec 26 2017
,
Dec 28 2017
,
Jan 3 2018
There are exceptions, but UI strings tend not to have very many words in them. If this represents a regression of ~20 microseconds per word, I don't think that's a concern. r480350 is actually working towards a world where we share all this typesetting with what's done in Blink. In Blink, there a cache for each font that maps "word" to a TextRun. However, in UI, we only cache entire RenderText objects, so whenever text wrapping might change, the text run for each word has to be recalculated. Moving to a cache per-word when they Blink/UI typesetting code stacks are merged should see a massive performance boost.
,
Jan 3 2018
,
Jan 23 2018
I see, thanks! This type of caching should indeed be amazing.
,
Aug 1
tapted@ were there concrete plans to share code with Blink or is c#4 a "would be nice to do this later" type thing?
,
Aug 1
See also issue 870007 .
,
Aug 2
ccameron has since added a cache, so I think the performance concerns here are now moot -> Wontfix. re: #c7: No concrete plans :/. But definitely a nice to have. Or even a must-have. Nobody is a maintainer for ui/gfx/render_text* and it's dumb that we maintain two stacks on top of harfbuzz/skia that each have code for breaking runs and doing font typeface fallbacks (and run/glyph caching, and other things). I think we're getting closer to nuking RenderTextMac, which means a shared layer on top of Harfbuzz is more straightforward. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by lgrey@chromium.org
, Dec 22 2017