Bug 137454 – [Mac] We are spending a lot of time loading fonts when loading weather.com

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Chris Dumez]

We are spending a lot of time loading fonts when loading weather.com: ~4.2% of WebProcess's cpu time in FontCache::getCachedFrontData().
In particular, we are spending a lot of time doing font auto-activation because we don't have the Open Sans fonts installed and weather.com is trying to load those.

[Chris Dumez]

<rdar://problem/18545368>

[Chris Dumez]

Created attachment 239348 [details]
Patch

[Chris Dumez]

Created attachment 239354 [details]
Patch

[Enrica Casucci]

Comment on attachment 239354 [details]
Patch

This is great! Looks good to me.

[Geoffrey Garen]

Comment on attachment 239354 [details]
Patch

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

Code looks right to me -- but how serious is this auto-activation at a different size problem? Should we keep a set of sets, where the value in the first set is a set of sizes?

Also, I be this would be faster if you used HashSet<RetainPtr<NSString>> instead of NSMutableSet. Actually, I think you should make that change before committing, since this patch is about performance.

> Source/WebCore/platform/mac/WebFontCache.mm:303
> +    // FIXME: Font auto-activation also takes a font size argument so in theory, this could break

Should be "...font size argument. So, in theory,...".

[Myles C. Maxfield]

Is it possible to do this for iOS too? I'm trying to get rid of WebFontCache altogether.

[Myles C. Maxfield]

Comment on attachment 239354 [details]
Patch

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

>> Source/WebCore/platform/mac/WebFontCache.mm:303
>> +    // FIXME: Font auto-activation also takes a font size argument so in theory, this could break
> 
> Should be "...font size argument. So, in theory,...".

Font files themselves don't contain particular sizes. The size argument is regarding the return value of fontWithName. I recommend you remove this entire comment.

[Myles C. Maxfield]

I know Enrica already r+'ed this, but I think it should be r-'ed. Please investigate doing this in a non-mac-specific way.

[Chris Dumez]

(In reply to comment #5)
> (From update of attachment 239354 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239354&action=review
> 
> Code looks right to me -- but how serious is this auto-activation at a different size problem? Should we keep a set of sets, where the value in the first set is a set of sizes?

I discussed this over with Gavin. It seemed unlikely to happen in the real world to us. Considering the performance implication, this is much better than completely disabling the font auto-activation. I will file a bug against NSFont so that we could add an API telling us if auto-activation failed because of the size only. Most of the time, however, it will fail simply because SpotLight couldn't find a matching font file.

Note that having a cache that would take use [Family, size] as key (as you suggest) would not help much performance wise. The reason we end up doing auto-activation 250 times for only 10 font families is because the page is requesting the font in various sizes (and italics, weight, but mostly sizes).

[Chris Dumez]

(In reply to comment #8)
> I know Enrica already r+'ed this, but I think it should be r-'ed. Please investigate doing this in a non-mac-specific way.

What do you mean? auto-activation *is* mac-specific.

[Chris Dumez]

(In reply to comment #7)
> (From update of attachment 239354 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239354&action=review
> 
> >> Source/WebCore/platform/mac/WebFontCache.mm:303
> >> +    // FIXME: Font auto-activation also takes a font size argument so in theory, this could break
> > 
> > Should be "...font size argument. So, in theory,...".
> 
> Font files themselves don't contain particular sizes. The size argument is regarding the return value of fontWithName. I recommend you remove this entire comment.

Oh OK, this is good to know, thanks. It is safer than I thought then, good.

[Chris Dumez]

After discussing with Miles, it seems feasible to address the issue at higher level and cache if we already asked for a given font family in the past. Apparently, the fact that the size is different for each request does not matter. However, I need to ask for the font again if other font traits differ (e.g. italics).

[Myles C. Maxfield]

(In reply to comment #9)
> (In reply to comment #5)
> > (From update of attachment 239354 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=239354&action=review
> > 
> > Code looks right to me -- but how serious is this auto-activation at a different size problem? Should we keep a set of sets, where the value in the first set is a set of sizes?
> 
> I discussed this over with Gavin. It seemed unlikely to happen in the real world to us. Considering the performance implication, this is much better than completely disabling the font auto-activation. I will file a bug against NSFont so that we could add an API telling us if auto-activation failed because of the size only. Most of the time, however, it will fail simply because SpotLight couldn't find a matching font file.
> 
> Note that having a cache that would take use [Family, size] as key (as you suggest) would not help much performance wise. The reason we end up doing auto-activation 250 times for only 10 font families is because the page is requesting the font in various sizes (and italics, weight, but mostly sizes).

By the way, we eventually want to move font selection to 100% CoreText functions and move away from NSFont completely (thereby allowing us to unify the mac + iOS font caches). Please see https://bugs.webkit.org/show_bug.cgi?id=134752 and https://bugs.webkit.org/show_bug.cgi?id=132159

[Alexey Proskuryakov]

Comment on attachment 239354 [details]
Patch

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

> Source/WebCore/platform/mac/WebFontCache.mm:283
> +    static NSMutableSet *set = [[NSMutableSet alloc] init];

Is this code main thread only? Please add an assertion to check for that.

[Chris Dumez]

(In reply to comment #14)
> (From update of attachment 239354 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239354&action=review
> 
> > Source/WebCore/platform/mac/WebFontCache.mm:283
> > +    static NSMutableSet *set = [[NSMutableSet alloc] init];
> 
> Is this code main thread only? Please add an assertion to check for that.

Ok, I will. The patch will likely change a lot, I am trying to handle the caching at FontCache.cpp level instead.

[Chris Dumez]

Ok, so I implemented this at higher level (FontCache.cpp) by basically asking the platform for the font only if this did not ask for that Family/weight/italics combination before (notice that size is not in there). However, this causes 60 font auto-activations to happens (because of the different weight / italics combinations) instead of 10 with my lower-level patch (and 250 without any change). Font auto activation only takes the font family as argument so implementing this at higher level is suboptimal. This also adds complexity to FontCache.cpp.

After discussing this over with Myles, I think I will refine my earlier patch instead (use a HashSet as suggested and add a assertion for threading).

[Chris Dumez]

Created attachment 239450 [details]
Patch

[Chris Dumez]

Here is a new iteration of the patch. I am setting the review flag again because the changes are non-trivial even though the approach stays the same.

The following changes were made:
- Use a HashSet<AtomicString> instead of an NSSet of NSStrings as suggested by Geoffrey. To avoid having to convert NSStrings into AtomicStrings (especially considering the string is initially an AtomicString), I moved the cache up to FontCacheMac.mm, where the font family is still an AtomicString, as suggested by Myles.
- I added a fontWithFamily() overload to WebFontCache taking an additional |shouldAutoActivateFontIfNeeded| argument as the cache is now at the call site. I kept the old API around in case other clients are relying on it.
- I added an assertion making sure the new cache is always used from the same thread, as suggested by Alexey.

[Darin Adler]

Comment on attachment 239450 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:104
> +    if (knownFamilies.get().size() == maxCacheSize)

I’d also assert that size is <= maxCacheSize.

[Chris Dumez]

Created attachment 239457 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 239457 [details]
Patch

Clearing flags on attachment: 239457

Committed r174456: <http://trac.webkit.org/changeset/174456>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This broke Mavericks build: https://build.webkit.org/builders/Apple%20Mavericks%20Debug%20%28Build%29/builds/11204/steps/compile-webkit/logs/errors

[Chris Dumez]

(In reply to comment #23)
> This broke Mavericks build: https://build.webkit.org/builders/Apple%20Mavericks%20Debug%20%28Build%29/builds/11204/steps/compile-webkit/logs/errors

Fixed in https://trac.webkit.org/r174457, thanks.