Bug 199283 – Prewarm font cache with more fonts

• Component: WebKit Misc.
• Status: NEW

[Per Arne Vollan]

This is a confirmed improvement in page load time.

[Per Arne Vollan]

Created attachment 373046 [details]
Patch

[Antti Koivisto]

Comment on attachment 373046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373046&action=review

> Source/WebCore/page/ProcessWarming.cpp:82
> +    static NeverDestroyed<Vector<String>> families = std::initializer_list<String> {
> +        "-apple-system"_s,
> +        "-webkit-standard"_s,
> +        "sans-serif"_s,
> +        "system-ui"_s,
> +        "Arial"_s,
> +        "Avenir"_s,
> +        "Helvetica"_s,
> +        "Helvetica Neue"_s,
> +        "Hiragino Sans GB"_s,
> +        "Lucida Grande"_s,
> +        "PingFang"_s,
> +        "SF Pro Text"_s,
> +        "SF Pro Icons"_s,
> +        "STHeiti"_s,
> +        "Segoe UI"_s,
> +        "Times"_s,
> +        "Times New Roman"_s
> +    };

There is substantial power and memory cost in warming fonts. The code here runs for every web process and should only warm things that are very likely needed. I don't think this list fits the criteria. With language specific fonts, it looks more like a benchmark hack.

I'm not opposed to globally prewarming more fonts but each entry needs to be justified based on real world data, not artificial benchmark scenarios.

[Per Arne Vollan]

Created attachment 373118 [details]
Patch

[Per Arne Vollan]

(In reply to Antti Koivisto from comment #2)
> Comment on attachment 373046 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373046&action=review
> 
> > Source/WebCore/page/ProcessWarming.cpp:82
> > +    static NeverDestroyed<Vector<String>> families = std::initializer_list<String> {
> > +        "-apple-system"_s,
> > +        "-webkit-standard"_s,
> > +        "sans-serif"_s,
> > +        "system-ui"_s,
> > +        "Arial"_s,
> > +        "Avenir"_s,
> > +        "Helvetica"_s,
> > +        "Helvetica Neue"_s,
> > +        "Hiragino Sans GB"_s,
> > +        "Lucida Grande"_s,
> > +        "PingFang"_s,
> > +        "SF Pro Text"_s,
> > +        "SF Pro Icons"_s,
> > +        "STHeiti"_s,
> > +        "Segoe UI"_s,
> > +        "Times"_s,
> > +        "Times New Roman"_s
> > +    };
> 
> There is substantial power and memory cost in warming fonts. The code here
> runs for every web process and should only warm things that are very likely
> needed. I don't think this list fits the criteria. With language specific
> fonts, it looks more like a benchmark hack.
> 
> I'm not opposed to globally prewarming more fonts but each entry needs to be
> justified based on real world data, not artificial benchmark scenarios.

Yes, that makes sense. I have uploaded a new patch with a font list consisting of some of the fonts used by the 10 most popular web sites on Alexa top sites.

Thanks for reviewing!

[Antti Koivisto]

Is there a radar for this?

[Radar WebKit Bug Importer]

<rdar://problem/52333302>

[Per Arne Vollan]

(In reply to Antti Koivisto from comment #5)
> Is there a radar for this?

I just had one created by the importer.

[Sam Weinig]

Comment on attachment 373118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=373118&action=review

> Source/WebCore/page/ProcessWarming.cpp:71
> +        "-apple-system"_s,
> +        "sans-serif"_s,
> +        "system-ui"_s,
> +        "Arial"_s,
> +        "Helvetica"_s,
> +        "Helvetica Neue"_s,
> +        "Segoe UI"_s,

It seems wrong to have a very platform specific list like this in non-platform code. This list should probably be moved to WebCore's FontCache so that platforms can override it.

[Per Arne Vollan]

Created attachment 374141 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 374141 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=374141&action=review

> Source/WebCore/page/ProcessWarming.cpp:-77
> -    // Cache system UI font fallbacks. Almost every web process needs these.
> -    // Initializing one size is sufficient to warm CoreText caches.

Really? Most web content doesn't use system-ui. Where do the uses come from?

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1644
> +    prewarmInfo.fontNamesRequiringSystemFallback = fontFamiliesForPrewarming();

I don't think this is right. We only take the "system fallback" path for the system font. Arial, Helvetica, Helvetica Neue, Times, and Times New Roman would never hit that path. OTOH, it could be that running these CoreText functions ahead-of-time is still helpful regarding the codepath those fonts ::do:: go through. Do we still get the benefit if we move this list to the "seenFamilies" member?

"SF Pro Text" shouldn't match any fonts. Are we sure this one is really valuable?

[Per Arne Vollan]

Created attachment 374228 [details]
Patch

[Per Arne Vollan]

Created attachment 374237 [details]
Patch

[Per Arne Vollan]

Created attachment 374238 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 374238 [details]
Patch

Clearing flags on attachment: 374238

Committed r247498: <https://trac.webkit.org/changeset/247498>

[Per Arne Vollan]

Created attachment 374427 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 374427 [details]
Patch

Clearing flags on attachment: 374427

Committed r247626: <https://trac.webkit.org/changeset/247626>