Bug 137454 - [Mac] We are spending a lot of time loading fonts when loading weather.com
Summary: [Mac] We are spending a lot of time loading fonts when loading weather.com
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 137539
  Show dependency treegraph
 
Reported: 2014-10-06 11:57 PDT by Chris Dumez
Modified: 2014-10-08 15:17 PDT (History)
10 users (show)

See Also:


Attachments
Patch (3.80 KB, patch)
2014-10-06 12:30 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (3.81 KB, patch)
2014-10-06 14:03 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.88 KB, patch)
2014-10-07 20:36 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.96 KB, patch)
2014-10-07 22:54 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2014-10-06 11:57:03 PDT
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.
Comment 1 Chris Dumez 2014-10-06 11:57:13 PDT
<rdar://problem/18545368>
Comment 2 Chris Dumez 2014-10-06 12:30:13 PDT
Created attachment 239348 [details]
Patch
Comment 3 Chris Dumez 2014-10-06 14:03:41 PDT
Created attachment 239354 [details]
Patch
Comment 4 Enrica Casucci 2014-10-06 14:28:14 PDT
Comment on attachment 239354 [details]
Patch

This is great! Looks good to me.
Comment 5 Geoffrey Garen 2014-10-06 14:28:54 PDT
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,...".
Comment 6 Myles C. Maxfield 2014-10-06 14:32:04 PDT
Is it possible to do this for iOS too? I'm trying to get rid of WebFontCache altogether.
Comment 7 Myles C. Maxfield 2014-10-06 14:34:41 PDT
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.
Comment 8 Myles C. Maxfield 2014-10-06 14:39:08 PDT
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.
Comment 9 Chris Dumez 2014-10-06 14:40:46 PDT
(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).
Comment 10 Chris Dumez 2014-10-06 14:41:18 PDT
(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.
Comment 11 Chris Dumez 2014-10-06 14:42:28 PDT
(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.
Comment 12 Chris Dumez 2014-10-06 14:52:56 PDT
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).
Comment 13 Myles C. Maxfield 2014-10-06 14:58:20 PDT
(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
Comment 14 Alexey Proskuryakov 2014-10-06 17:08:49 PDT
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.
Comment 15 Chris Dumez 2014-10-06 17:12:22 PDT
(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.
Comment 16 Chris Dumez 2014-10-07 17:18:40 PDT
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).
Comment 17 Chris Dumez 2014-10-07 20:36:23 PDT
Created attachment 239450 [details]
Patch
Comment 18 Chris Dumez 2014-10-07 20:43:16 PDT
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.
Comment 19 Darin Adler 2014-10-07 21:18:27 PDT
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.
Comment 20 Chris Dumez 2014-10-07 22:54:49 PDT
Created attachment 239457 [details]
Patch
Comment 21 WebKit Commit Bot 2014-10-08 09:20:38 PDT
Comment on attachment 239457 [details]
Patch

Clearing flags on attachment: 239457

Committed r174456: <http://trac.webkit.org/changeset/174456>
Comment 22 WebKit Commit Bot 2014-10-08 09:20:46 PDT
All reviewed patches have been landed.  Closing bug.