Bug 137539 – [Mac] Spending too much time mapping desired font families to available ones

• Component: Platform
• Status: RESOLVED FIXED

[Chris Dumez]

While profiling the load of weather.com, I noticed that we are spending quite a bit of time trying to map the font family requested to a font that is available on the system. The process involves:
1. Doing a linear search of all the installed font families and do a case-insensitive string comparison for each of them until we find a match
2. If we don't find a match, do another linear search of the fonts postscript names this time

This process is costly and the fonts requested by weather.com are not available, causing us to do 2 linear searches and a lot of string comparisons (accounting for ~2% of the WebProcess CPU time for the page load). As a result, we end up spending ~90ms in internalFontWithFamily() when loading weather.com.

Based on my testing, if we kept a cache of the mapping between desired font families and available font families, we could cut the time spent in internalFontWithFamily() in half (~45ms).

[Chris Dumez]

<rdar://problem/18545368>

[Chris Dumez]

Created attachment 239498 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 239498 [details]
Patch

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

> Source/WebCore/platform/mac/WebFontCache.mm:124
> +    static NSMutableDictionary *dictionary = [[NSMutableDictionary alloc] init];

There's no way you can do this with WTF types instead? :\

> Source/WebCore/platform/mac/WebFontCache.mm:160
> +        CFNotificationCenterAddObserver(CFNotificationCenterGetLocalCenter(), nullptr, fontCacheRegisteredFontsChangedNotificationCallback, kCTFontManagerRegisteredFontsChangedNotification, nullptr, CFNotificationSuspensionBehaviorDeliverImmediately);

We already do this in FontCache, and I'm wary of setting up too many independent handlers. Can you plumb the call from FontCache?

[Myles C. Maxfield]

Unofficial r=me, given my comments

[Chris Dumez]

(In reply to comment #3)
> (From update of attachment 239498 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239498&action=review
> 
> > Source/WebCore/platform/mac/WebFontCache.mm:124
> > +    static NSMutableDictionary *dictionary = [[NSMutableDictionary alloc] init];
> 
> There's no way you can do this with WTF types instead? :\

I discussed this with Geoffrey earlier. The issue is that if I used a WTF::HashMap, I'd have to convert the key from NSString* to WTF::String, which is currently cheap (involves copying the raw string data).

> 
> > Source/WebCore/platform/mac/WebFontCache.mm:160
> > +        CFNotificationCenterAddObserver(CFNotificationCenterGetLocalCenter(), nullptr, fontCacheRegisteredFontsChangedNotificationCallback, kCTFontManagerRegisteredFontsChangedNotification, nullptr, CFNotificationSuspensionBehaviorDeliverImmediately);
> 
> We already do this in FontCache, and I'm wary of setting up too many independent handlers. Can you plumb the call from FontCache?

Yes, there is already a handler in FontCacheMac.mm and I have thought of reusing that one. The issue is that I will then need to add a new invalidateCache() ObjC API to WebFontCache. Would that be fine? How about third party using that API, wouldn't that break them if they did not call invalidateCache() when needed?

[Chris Dumez]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 239498 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=239498&action=review
> > 
> > > Source/WebCore/platform/mac/WebFontCache.mm:124
> > > +    static NSMutableDictionary *dictionary = [[NSMutableDictionary alloc] init];
> > 
> > There's no way you can do this with WTF types instead? :\
> 
> I discussed this with Geoffrey earlier. The issue is that if I used a WTF::HashMap, I'd have to convert the key from NSString* to WTF::String, which is currently cheap (involves copying the raw string data).

*not* cheap, sorry.

[Chris Dumez]

Comment on attachment 239498 [details]
Patch

After discussing this offline with Myles, we decided to have FontCacheMac invalidate WebFontCache since it already listens for font registration changes. New patch iteration incoming.

[Chris Dumez]

Created attachment 239513 [details]
Patch

[Darin Adler]

Comment on attachment 239513 [details]
Patch

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

OK to land as is. A few ideas for improvements.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:57
> +    [WebFontCache invalidate];

I think it’s a little indirect that WebFontCache is only properly invalidated if WebCore::FontCache is allocated. I suppose that in practice this is not a problem, but it’s sort of an indirect dependency that could possibly be made slightly more direct. I could imagine adding a call to fontCache() to the desiredFamilyToAvailableFamilyDictionary() function, for example, with a comment explaining the reason and the side effect.

> Source/WebCore/platform/mac/WebFontCache.mm:132
> +    if ([desiredFamilyToAvailableFamilyDictionary() count] == maxCacheSize) {

Might be faster to put the dictionary into a local variable rather than calling the function at least two times.

> Source/WebCore/platform/mac/WebFontCache.mm:134
> +        NSEnumerator *keyEnumerator = [desiredFamilyToAvailableFamilyDictionary() keyEnumerator];
> +        [desiredFamilyToAvailableFamilyDictionary() removeObjectForKey: [keyEnumerator nextObject]];

Fast enumeration would probably be more efficient and I think reads a little better.

    for (NSString *key in desiredFamilyToAvailableFamilyDictionary()) {
        [desiredFamilyToAvailableFamilyDictionary() removeObjectForKey:key];
        break;
    }

> Source/WebCore/platform/mac/WebFontCache.mm:139
> +    if (availableFamily)
> +        [desiredFamilyToAvailableFamilyDictionary() setObject:availableFamily forKey:desiredFamily];
> +    else
> +        [desiredFamilyToAvailableFamilyDictionary() setObject:@"" forKey:desiredFamily];

Seems a little wordy this way. Might read better like this:

    [desiredFamilyToAvailableFamilyDictionary() setObject:availableFamily ?: @"" forKey:desiredFamily];

Or like this:

    NSString *value = availableFamily ? availableFamily : @"";
    [desiredFamilyToAvailableFamilyDictionary() setObject:value forKey:desiredFamily];

Could also consider representing null in the dictionary as NSNull rather than with an empty string. Downside is you’d have to use id rather than NSString * when getting values out of the dictionary. Upside is that NSNull is a more logical direct mapping to/from nil than the empty string is.

> Source/WebCore/platform/mac/WebFontCache.mm:194
>      NSFontManager *fontManager = [NSFontManager sharedFontManager];

I’d put this down where it’s used in the non-cached case; seems a little ugly to do this even in the cached case.

And on reflection, I think this function would be more readable broken into three pieces:

1) A function that maps a desired family name to an available family name, without caching.
2) A function that finds a desired family name to an available family name, with caching, that calls (1) in the "not yet in cache" case.
3) A function that finds an NSFont, which calls (2).

I won’t insist on this refactoring, but I do think it could make the code easier to read.

> Source/WebCore/platform/mac/WebFontCache.mm:200
> +            // Already tried mapping the desired font to an available one in the past and failed, so
> +            // don't try again.

It's good to have a comment here, but this one seems a little too wordy. The idea that we cache failures as well as successes is a normal part of the memoization pattern; I wish there was a more straightforward way of separating “it was cached” from “the cached value happens to be null” that makes this look less like a special case, and more like “decoding empty string and turning it into nil”.

> Source/WebCore/platform/mac/WebFontCache.mm:208
> +        NSEnumerator *e = [[fontManager availableFontFamilies] objectEnumerator];
> +        while ((availableFamily = [e nextObject])) {

Could use fast enumeration:

    for (availableFamily in [fontManager availableFontFamilies]) {

> Source/WebCore/platform/mac/WebFontCache.mm:219
> +            NSEnumerator *availableFonts = [[fontManager availableFonts] objectEnumerator];
> +            NSString *availableFont;
> +            NSFont *nameMatchedFont = nil;
> +            NSFontTraitMask desiredTraitsForNameMatch = desiredTraits | (desiredWeight >= 7 ? NSBoldFontMask : 0);
> +            while ((availableFont = [availableFonts nextObject])) {

Could use fast enumeration:

    for (NSString *availableFont in [fontManager availableFonts]) {

[Chris Dumez]

> And on reflection, I think this function would be more readable broken into three pieces:
> 
> 1) A function that maps a desired family name to an available family name, without caching.
> 2) A function that finds a desired family name to an available family name, with caching, that calls (1) in the "not yet in cache" case.
> 3) A function that finds an NSFont, which calls (2).
> 
> I won’t insist on this refactoring, but I do think it could make the code easier to read.

This sounds like a nice proposal, I will propose this refactoring in a follow-up patch to keep the patch size small.

I will include your other smaller suggestions in this patch before landing though.

[Myles C. Maxfield]

Comment on attachment 239513 [details]
Patch

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

>> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:57
>> +    [WebFontCache invalidate];
> 
> I think it’s a little indirect that WebFontCache is only properly invalidated if WebCore::FontCache is allocated. I suppose that in practice this is not a problem, but it’s sort of an indirect dependency that could possibly be made slightly more direct. I could imagine adding a call to fontCache() to the desiredFamilyToAvailableFamilyDictionary() function, for example, with a comment explaining the reason and the side effect.

I thought that I should document here that cdumez chose this design at my request given my work regarding WebFontCache (https://bugs.webkit.org/show_bug.cgi?id=134752)

[Chris Dumez]

(In reply to comment #9)
> (From update of attachment 239513 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239513&action=review
> 
> OK to land as is. A few ideas for improvements.
> 
> > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:57
> > +    [WebFontCache invalidate];
> 
> I think it’s a little indirect that WebFontCache is only properly invalidated if WebCore::FontCache is allocated. I suppose that in practice this is not a problem, but it’s sort of an indirect dependency that could possibly be made slightly more direct. I could imagine adding a call to fontCache() to the desiredFamilyToAvailableFamilyDictionary() function, for example, with a comment explaining the reason and the side effect.

It is actually if FontCache::getCachedFontPlatformData() is called (not if FontCache is allocated). It is in getCachedFontPlatformData() that we construct the FontPlatformDataCache and call platformInit() to register the callback. My understanding after talking with Myles is that this is relatively safe because callers should go via FontCache instead of accessing WebFontCache directly. There was an exception to that in WebKit but Myles is taking care of fixing it and have that code use FontCache instead (https://bugs.webkit.org/show_bug.cgi?id=137550).

[Chris Dumez]

Created attachment 239550 [details]
Patch

[Chris Dumez]

Comment on attachment 239513 [details]
Patch

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

>> Source/WebCore/platform/mac/WebFontCache.mm:132
>> +    if ([desiredFamilyToAvailableFamilyDictionary() count] == maxCacheSize) {
> 
> Might be faster to put the dictionary into a local variable rather than calling the function at least two times.

Done.

>> Source/WebCore/platform/mac/WebFontCache.mm:134
>> +        [desiredFamilyToAvailableFamilyDictionary() removeObjectForKey: [keyEnumerator nextObject]];
> 
> Fast enumeration would probably be more efficient and I think reads a little better.
> 
>     for (NSString *key in desiredFamilyToAvailableFamilyDictionary()) {
>         [desiredFamilyToAvailableFamilyDictionary() removeObjectForKey:key];
>         break;
>     }

Done.

>> Source/WebCore/platform/mac/WebFontCache.mm:139
>> +        [desiredFamilyToAvailableFamilyDictionary() setObject:@"" forKey:desiredFamily];
> 
> Seems a little wordy this way. Might read better like this:
> 
>     [desiredFamilyToAvailableFamilyDictionary() setObject:availableFamily ?: @"" forKey:desiredFamily];
> 
> Or like this:
> 
>     NSString *value = availableFamily ? availableFamily : @"";
>     [desiredFamilyToAvailableFamilyDictionary() setObject:value forKey:desiredFamily];
> 
> Could also consider representing null in the dictionary as NSNull rather than with an empty string. Downside is you’d have to use id rather than NSString * when getting values out of the dictionary. Upside is that NSNull is a more logical direct mapping to/from nil than the empty string is.

Done. Went with the second proposal and NSNull.

>> Source/WebCore/platform/mac/WebFontCache.mm:194
>>      NSFontManager *fontManager = [NSFontManager sharedFontManager];
> 
> I’d put this down where it’s used in the non-cached case; seems a little ugly to do this even in the cached case.
> 
> And on reflection, I think this function would be more readable broken into three pieces:
> 
> 1) A function that maps a desired family name to an available family name, without caching.
> 2) A function that finds a desired family name to an available family name, with caching, that calls (1) in the "not yet in cache" case.
> 3) A function that finds an NSFont, which calls (2).
> 
> I won’t insist on this refactoring, but I do think it could make the code easier to read.

Putting this off this later since Myles plans to kill WebFontCache.

>> Source/WebCore/platform/mac/WebFontCache.mm:200
>> +            // don't try again.
> 
> It's good to have a comment here, but this one seems a little too wordy. The idea that we cache failures as well as successes is a normal part of the memoization pattern; I wish there was a more straightforward way of separating “it was cached” from “the cached value happens to be null” that makes this look less like a special case, and more like “decoding empty string and turning it into nil”.

Reduced the comment size and used NSNull instead of empty NString.

>> Source/WebCore/platform/mac/WebFontCache.mm:208
>> +        while ((availableFamily = [e nextObject])) {
> 
> Could use fast enumeration:
> 
>     for (availableFamily in [fontManager availableFontFamilies]) {

Done.

>> Source/WebCore/platform/mac/WebFontCache.mm:219
>> +            while ((availableFont = [availableFonts nextObject])) {
> 
> Could use fast enumeration:
> 
>     for (NSString *availableFont in [fontManager availableFonts]) {

Done.

[Chris Dumez]

Created attachment 239551 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 239551 [details]
Patch

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

> Source/WebCore/platform/mac/WebFontCache.mm:133
> +    if ([familyMapping count] == maxCacheSize) {

Might as well make this <= since ASSERTS only get hit in debug builds. Your call, though; it's probably fine either way.

[WebKit Commit Bot]

Comment on attachment 239551 [details]
Patch

Clearing flags on attachment: 239551

Committed r174516: <http://trac.webkit.org/changeset/174516>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.