Fallback path in RenderText on mac is slow |
||||||
Issue descriptionI took perf and looks like the fallback path is slow on mac. If you remove the non ascii text in views example multiline example, it runs reasonably fast. There are two mac specific functions that are slower than linux/chromeos. * GetFallbackFont (https://cs.chromium.org/chromium/src/ui/gfx/font_fallback_mac.mm?rcl=0&l=24). It was ~100x (*) slower than linux/chromeos. * CreateSkiaTypeface (https://cs.chromium.org/chromium/src/ui/gfx/render_text_mac.mm?rcl=0&l=70). It was ~10x (*) slower on mac than linux/chromeos. Looks like SkCreateTypefaceFromCTFont is slow. Caveat: My mac is much slower than desktop, so actual number would probably be 50~30x, 5~3x, respectively. Maybe Font should have fallback list, or RT can cache it for the font assigned. tapted@, can you look into this?
,
Jun 15 2016
,
Jun 15 2016
( Issue 608990 has some traces as well)
,
Dec 9 2016
Looking at this if it's ok with you Sarah.
,
Dec 9 2016
#4 You have my blessing
,
May 1 2017
Un-assigning myself. Link to some related investigation I did - https://docs.google.com/document/d/13XJ34-Z7L8KHaMGjFBRxz_M-kFDG-U4g0B6TZoqeD00/edit.
,
May 1 2017
OK. This has been on the back of my mind for a while, so I'm going to tackle it. There seem to be a number of problems with font fallbacks on Mac. Both in Blink and in toolkit-views UI (--secondary-ui-md). Currently on Mac we are using two private APIs for font fallbacks. Blink uses +[NSFont findFontLike:...] and gfx::RenderTextHarfbuzz uses CTFontCopyDefaultCascadeListForLanguages(). But there's a public API: CTFontCreateForString() - https://developer.apple.com/reference/coretext/1509506-ctfontcreateforstring?language=objc I suspect findFontLike and CTFontCopyDefaultCascadeListForLanguages have been rotting, and don't share code with CTFontCreateForString. CTFontCreateForString has a different API to what gfx::RenderText currently wants. gfx::RenderTextHarfbuzz doesn't pass a text "sample" to the fallback engine: instead it just iterates over the fallback list calculating runs and picking the one that has the fewest missing glyphs. But this can result in different fallbacks when compared with Safari or native UI. So my first step will be to introduce CTFontCreateForString() into gfx::RenderTextHarfbuzz for Mac. This will overhaul the fallback path, offloading more stuff to the system font engine and hopefully be more efficient and get better results. If it goes smoothly, we should consider using CTFontCreateForString() for Blink as well. Related bugs Issue 696867 - Mac: Harfbuzz fallback fonts don't always match native/Blink (e.g. for Hebrew) Issue 255122 - Blink SPI use: +[NSFont findFontLike:...] Issue 439039 - Implement gfx::GetFallbackFontFamilies() for Mac Possibly related bugs (Blink, mostly, and some maybe obsolete/fixed already) Issue 687886 - Big size font render is wrong Issue 665725 - Failing fallback from Roboto to system font on MacOS 10.12.1 Issue 660807 - Font fallback fails on Devangari and Gothic scripts Issue 458919 - Vietnamese fonts garbled, not displayed properly Issue 28426 - Gujarati font is not legible
,
May 2 2017
Thanks for the analysis, there is definitely room for improvement in Blink's font fallback code, particularly on Mac. IIRC I looked briefly at issue 255122 and replacing the API usage, but looking at stack traces with mac OS symbols, it seemed like NSFont findFontLike has been implemented using CoreText behind the scenes, so I lowered priority. But it would not hurt to verify my brief analysis. One of the big issues we have in Blink fallback is finding the right fonts for grapheme clusters that have a chaing of unusual combining marks, e.g. A plus COMBINING SQUARE BELOW, optionally plus additional combining marks, which should be Menlo on Mac. So I would suggest to really use at least the whole grapheme cluster, or small strings, not just characters to perform fallback. We should refactor Blink's fallback towards this, currently it's designed to do per character fallback only.
,
Jun 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/62cbc10d4dbfeed614eb23c3d996793814a35334 commit 62cbc10d4dbfeed614eb23c3d996793814a35334 Author: tapted <tapted@chromium.org> Date: Wed Jun 21 01:26:13 2017 Use CTFontCreateForString for RenderTextHarfbuzz on Mac The approach ends up pretty close to what's already done for Windows. On Mac, CTFontCreateForString() gives better fallbacks (i.e. they more often match what native UI does) and requires RenderTextHarfbuzz to do less work. However, it doesn't guarantee that the returned font can display every glyph in the provided string. So still use CTFontCopyDefaultCascadeListForLanguages (which is now a public API - hooray) to provide a last-resort list of fallback fonts. BUG= 696867 , 619684 Review-Url: https://codereview.chromium.org/2927953003 Cr-Commit-Position: refs/heads/master@{#481062} [modify] https://crrev.com/62cbc10d4dbfeed614eb23c3d996793814a35334/ui/gfx/font_fallback.h [modify] https://crrev.com/62cbc10d4dbfeed614eb23c3d996793814a35334/ui/gfx/font_fallback_mac.mm [modify] https://crrev.com/62cbc10d4dbfeed614eb23c3d996793814a35334/ui/gfx/font_fallback_mac_unittest.cc [modify] https://crrev.com/62cbc10d4dbfeed614eb23c3d996793814a35334/ui/gfx/font_fallback_win.cc [modify] https://crrev.com/62cbc10d4dbfeed614eb23c3d996793814a35334/ui/gfx/font_fallback_win.h [modify] https://crrev.com/62cbc10d4dbfeed614eb23c3d996793814a35334/ui/gfx/render_text_harfbuzz.cc
,
Jun 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dca011e454153e08602ead192cda84abc2946a64 commit dca011e454153e08602ead192cda84abc2946a64 Author: tapted <tapted@chromium.org> Date: Wed Jun 21 02:58:07 2017 Revert of Use CTFontCreateForString for RenderTextHarfbuzz on Mac (patchset #5 id:80001 of https://codereview.chromium.org/2927953003/ ) Reason for revert: Causes views_unittest LabelSelectionTest.MouseDragSingleLineRTL to fail on 10.10 and 10.11 Original issue's description: > Use CTFontCreateForString for RenderTextHarfbuzz on Mac > > The approach ends up pretty close to what's already done for Windows. > > On Mac, CTFontCreateForString() gives better fallbacks (i.e. they more > often match what native UI does) and requires RenderTextHarfbuzz to do > less work. However, it doesn't guarantee that the returned font can > display every glyph in the provided string. > > So still use CTFontCopyDefaultCascadeListForLanguages (which is now a > public API - hooray) to provide a last-resort list of fallback fonts. > > BUG= 696867 , 619684 > > Review-Url: https://codereview.chromium.org/2927953003 > Cr-Commit-Position: refs/heads/master@{#481062} > Committed: https://chromium.googlesource.com/chromium/src/+/62cbc10d4dbfeed614eb23c3d996793814a35334 TBR=asvitkine@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 696867 , 619684 Review-Url: https://codereview.chromium.org/2950013002 Cr-Commit-Position: refs/heads/master@{#481095} [modify] https://crrev.com/dca011e454153e08602ead192cda84abc2946a64/ui/gfx/font_fallback.h [modify] https://crrev.com/dca011e454153e08602ead192cda84abc2946a64/ui/gfx/font_fallback_mac.mm [modify] https://crrev.com/dca011e454153e08602ead192cda84abc2946a64/ui/gfx/font_fallback_mac_unittest.cc [modify] https://crrev.com/dca011e454153e08602ead192cda84abc2946a64/ui/gfx/font_fallback_win.cc [modify] https://crrev.com/dca011e454153e08602ead192cda84abc2946a64/ui/gfx/font_fallback_win.h [modify] https://crrev.com/dca011e454153e08602ead192cda84abc2946a64/ui/gfx/render_text_harfbuzz.cc
,
Jun 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7d814671483a3cde6b0c9e7c616ceb1eebfd3f17 commit 7d814671483a3cde6b0c9e7c616ceb1eebfd3f17 Author: tapted <tapted@chromium.org> Date: Fri Jun 23 01:37:09 2017 Use CTFontCreateForString for RenderTextHarfbuzz on Mac The approach ends up pretty close to what's already done for Windows. On Mac, CTFontCreateForString() gives better fallbacks (i.e. they more often match what native UI does) and requires RenderTextHarfbuzz to do less work. However, it doesn't guarantee that the returned font can display every glyph in the provided string. So still use CTFontCopyDefaultCascadeListForLanguages (which is now a public API - hooray) to provide a last-resort list of fallback fonts. This CL causes a font used for Hebrew text in a test to change, which tickled a pre-existing bug/corner case in the test for some macOS versions. BUG= 696867 , 619684 , 735346 Previously landed in r481062, but tickled some OS-specific tests due to a valid discrepancy between rounding directions. Review-Url: https://codereview.chromium.org/2927953003 Cr-Commit-Position: refs/heads/master@{#481764} [modify] https://crrev.com/7d814671483a3cde6b0c9e7c616ceb1eebfd3f17/ui/gfx/font_fallback.h [modify] https://crrev.com/7d814671483a3cde6b0c9e7c616ceb1eebfd3f17/ui/gfx/font_fallback_mac.mm [modify] https://crrev.com/7d814671483a3cde6b0c9e7c616ceb1eebfd3f17/ui/gfx/font_fallback_mac_unittest.cc [modify] https://crrev.com/7d814671483a3cde6b0c9e7c616ceb1eebfd3f17/ui/gfx/font_fallback_win.cc [modify] https://crrev.com/7d814671483a3cde6b0c9e7c616ceb1eebfd3f17/ui/gfx/font_fallback_win.h [modify] https://crrev.com/7d814671483a3cde6b0c9e7c616ceb1eebfd3f17/ui/gfx/render_text_harfbuzz.cc [modify] https://crrev.com/7d814671483a3cde6b0c9e7c616ceb1eebfd3f17/ui/views/controls/label_unittest.cc
,
Jul 17 2017
The fallback path no longer seems to be a bottleneck. Repeating the analysis of views_examples_with_content_exe turns up different things. For RenderTextMac, about 40% of time is spent in calls to Label::GetLinesForWidth which creates a render text instance then discards it. For RenderTextHarfbuzz, one issue seems to that Label::GetHeightForWidth(int w) always calls render_text_->SetDisplayRect(gfx::Rect(0, 0, w, 0)). That clobbers any existing layout and the result can't be reused for Label::CalculatePreferredSize(). A third layout gets triggered by views::Textfield::UpdateCursorViewPosition(). Caching text runs in the gfx::Font is probably the best way to address this - I don't see an easy way to collapse those 3 computations into one. There will still be some corner-casey strings with missing glyphs that trigger slower paths, but they should be uncommon. To poke the fallback paths some more we'd probably want to first restructure the code so that the different fallback paths get different symbols, but caching runs would overhaul this anyway. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by tapted@chromium.org
, Jun 15 2016SkCreateTypefaceFromCTFont uses its own cache: SkFontHost_mac.cpp: /* This function is visible on the outside. It first searches the cache, and if * not found, returns a new entry (after adding it to the cache). */ SkTypeface* SkCreateTypefaceFromCTFont(CTFontRef fontRef, CFTypeRef resourceRef) { SkTypeface* face = SkTypefaceCache::FindByProcAndRef(find_by_CTFontRef, (void*)fontRef); if (face) { return face; } CFRetain(fontRef); /* etc */ The problem might instead be that using gfx::Font::Derive in gfx::Internal::CreateSkiaTypeface(..) is creating a new NSFont object each time and breaking Skia's ability to cache it. (This probably also means there's a memory leak, as each NSFont will get a cache entry and be retained forever). The gfx::Fonts are probably already cached in ui::ResourceBundle, but gfx can't depend on that. We could instead deprecate changes to RenderText "style" that require new fonts, and require callers to provide a font instead. E.g., it currently takes // Text styles and adornments. // TODO(msw): Merge with gfx::Font::FontStyle. enum TextStyle { ITALIC = 0, STRIKE, DIAGONAL_STRIKE, UNDERLINE, NUM_TEXT_STYLES, }; (and, separately, font weight) If we remove `ITALIC` and call that enum `TextAdornment` or similar, then we wouldn't need to derive new fonts when applying styles, and calling code could fetch cached fonts from the ResourceBundle. This is probably a requirement before bothering to cache GetFallbackFont() on gfx::Font too, since the derived fonts won't have it. On the Harfbuzz side, CreateSkiaTypeface calls SkTypeface::MakeFromName, which calls into some "legacy" methods - SkFontMgr::legacyCreateTypeface To maximize the benefit, we probably want to cache the SkTypeface on the gfx::Font as well. That should improve performance on all platforms. It doesn't look like Windows caches the result of legacyCreateTypeface -- it makes calls to logfont_for_name(char[]) and SkCreateTypefaceFromLOGFONT in its onLegacyCreateTypeface. OTOH, SkFontMgr_fontconfig's onLegacyCreateTypeface (which I think is what CrOS/Linux will use) *does* have its own cache, which might be why we get good performance numbers there. I'm happy to pursue removing "Italic" and "Weight" from the things that RenderText sets, and require a gfx::Font instead - does that sound like a good plan?