Bug 315135 – [Skia] SkFontMgr_fontconfig::onMatchFamilyStyle is absurdly slow

• Component: WebKitGTK
• Status: NEW

[Jeff Fortin]

Created attachment 479721 [details]
Demonstration video

This is a performance bug found as a derivative of investigating https://gitlab.gnome.org/GNOME/epiphany/-/work_items/2880, separately from that Epiphany-specific adblocking / content filters performance issue.

See attached video demonstrating the issue on my desktop workstation, with Epiphany 50.4 and WebKitGTK 2.52.3 from Fedora Workstation 44's RPM repositories.

To reproduce, simply:
epiphany --incognito-mode https://www.wikipedia.org

(with or without adblocking filters enabled)

On my desktop workstation (Intel Xeon W3520 × 8),
it takes 20 to 25 seconds for that page to start loading.

On my laptop (Intel Core i5 8250U × 8),
it takes 12 seconds for that page to start loading.

[Jeff Fortin]

Created attachment 479723 [details]
Sysprof capture (normal non-incognito launch)

Here is a Sysprof capture launching Epiphany 50.4 "normally" (not* incognito mode), configured to have all content filters (adblocking, cookie banners blocking, intelligent tracking protection, etc.) disabled.

*: in incognito mode, for some reason, Epiphany would process the content filters even if you disable them, so you'd end up with similar results as the Sysprof captures I'm attaching to https://gitlab.gnome.org/GNOME/epiphany/-/work_items/2880.

[Jeff Fortin]

Created attachment 479724 [details]
Sysprof capture flamegraph

In the flamegraph, we can see here that other than the bunch of webkit web processes being created on startup for the bunch of tabs, the largest tower of function calls has to do with SkFontMgr spamming fontconfig.

[Jeff Fortin]

Created attachment 479725 [details]
Sysprof marks

I don't know if this one shows anything meaningful.

[Michael Catanzaro]

Just reading the flame graph, it seems SkFontMgr_fontconfig::onMatchFamilyStyle is absurdly slow when calling FcFontMatch() and especially remove_weak(), which itself has a warning "This can be quite expensive." OK, but surely it's not intended to be *this* expensive. 5 seconds is pretty extreme.

One of the comments in the Skia code looked familiar. Turns out I copied this slow Skia code into WebKit's FontCacheFreeType.cpp a decade ago, in 165476@main. Then in https://bugs.freedesktop.org/show_bug.cgi?id=19375 the Fontconfig developers added an API that we could use to avoid the need for it. Then many years later, I finally got around to using it and discovered it didn't work, https://gitlab.freedesktop.org/fontconfig/fontconfig/-/work_items/294. Finally I was able to actually use the new API in 241000@main, but I never contributed this change back to Skia. Later on, WebKitGTK switched from Cario to Skia, and therefore from FontCacheFreeType.cpp to FontCacheSkia.cpp. So now that we are using Skia directly, and we are right back to square one.

Anyway, key observation: the slow function SkFontMgr_fontconfig::onMatchFamilyStyle looks extremely similar to FontCacheFreeType::strongAliasesForFamily, which is obsolete since 241000@main. It's very likely that the slow code can be replaced by using FcPatternGetWithBinding(), assuming Skia is willing to depend on Fontconfig 2.14, which it should be because that is pretty old now. It would require some investigation, though, because the WebKit code FontCache::createFontPlatformData in FontCacheFreeType.cpp does font matching and then chooses a family that has a strong family binding, whereas SkFontMgr_fontconfig::onMatchFamilyStyle actually does the opposite. And the strategies are also not identical: FontCacheFreeType.cpp ignores all weak family bindings, while Skia conditionally allows them depending on order. (Is that important? I'm not sure, because I don't understand Fontconfig. Probably not?) Also, the Skia code is structured such that the easiest way to implement the change would be to modify the FcPattern directly using FcPatternRemove(), which WebKit does not do. So it's not so simple as just copying the WebKit changes into Skia. But goal should be to remove is_weak() and rewrite remove_weak().

One thing I don't understand: Chrome and Firefox presumably both also use SkFontMgr_fontconfig, so why are they not comparably slow?

P.S. The FCLocker in SkFontMgr_fontconfig.cpp is also obsolete and can be deleted for a small win.

[Michael Catanzaro]

P.S.S. If you have made any customizations to your Fontconfig configuration, there's a high chance that's relevant.

[Michael Catanzaro]

P.S.S.S. remove_weak is designed to allow removing FC_FAMILY or FC_POSTSCRIPT_NAME or actually any Fontconfig "object" (a better term might be "property"), but it's only ever used with FC_FAMILY, so that too can be simplified.

[Michael Catanzaro]

Created attachment 479743 [details]
Test patch

Actually, this was easier than I expected. Try this test patch! Who knows: it might even be correct? Hard to say, because I don't claim to understand what I'm doing.

I think it is a behavior change though. The original code permitted weak matches prior to the first strong match, whereas the new code is stricter about requiring only a strong match. This should result in more failed matches and more fallback down CSS font lists. Arguably that is more correct, but whether this will lead to better results or worse results is debatable.

I think I've previously decided not to sign the Google CLA -- don't remember why -- so I probably won't contribute this back to Skia. But anybody else is should feel welcome to do so.

[Michael Catanzaro]

Created attachment 479744 [details]
Alternative patch

Here's an alternative patch, with less-related changes, deleting unnecessary code in this file.

Looks like skfontstyle_from_fcpattern() and fcpattern_from_skfontstyle() could both be simplified more, but I did not try.

[Adrian Perez]

I think part of the issue is that we may be hitting a fallback to synchronous file copy
of compiled content filter rules -- the Fontconfig/Skia issue still dominates the profile,
but seems worth looking into, so I have filed bug #315236 for that.

[Michael Catanzaro]

One more thing: it wouldn't be hard to change my patch to preserve the original behavior of accepting weak matches that are ordered before the first strong match. Maybe that would be a safer approach, because it would avoid changing which font gets selected. I don't understand why that behavior is actually desirable, though. Fontconfig is really confusing.

[Michael Catanzaro]

Created attachment 479750 [details]
Alternative patch