Bug 145180 – [Mac] Font lookups are flakey due to caching

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Chris Dumez]

Font lookups are flakey due to caching in fontWithFamily().

Radar: <rdar://problem/21012406>

[Chris Dumez]

Created attachment 253395 [details]
Patch

[Darin Adler]

Comment on attachment 253395 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        This patch now uses a std::pair as key containing both the "desired
> +        family" and the "desired traits" for correctness. I also updated the
> +        cache to use WTF types instead of NS types.

So now looking up a name in the cache requires converting an AtomicString to an NSString every time? That doesn’t seem like an improvement!

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:159
> +    familyMapping.add(std::make_pair(desiredFamily, desiredTraits), value);

Old code did the equivalent of HashMap::set, not HashMap::add. Is this a desirable change?

[Chris Dumez]

Comment on attachment 253395 [details]
Patch

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

>> Source/WebCore/ChangeLog:16
>> +        cache to use WTF types instead of NS types.
> 
> So now looking up a name in the cache requires converting an AtomicString to an NSString every time? That doesn’t seem like an improvement!

Converting an AtomicString into a NSString is relatively cheap. It ends up calling StringImpl::createCFString() which constructs a CFStringRef (later casted to a NSString*) *without* copying the characters.
The cost of converting an AtomicString into a NSString* is thus really small compared to calling [fontManager availableFontFamilies] + [fontManager availableFonts] and doing linear searches on those 2 lists.

There is a slight overhead when saving a mapping into the cache though because we need to construct an AtomicString for the NSString* of the availableFamily.

However, we already get the desiredFamily as an AtomicString so we no longer need to convert it to an NSString* when the cache is successful (I will re-upload an iteration with this optimization). Therefore, it is a win there and also HashMap usually performs better than NSDictionary.

>> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:159
>> +    familyMapping.add(std::make_pair(desiredFamily, desiredTraits), value);
> 
> Old code did the equivalent of HashMap::set, not HashMap::add. Is this a desirable change?

We only call this function if the key is not already present so add() is slightly faster here.

[Chris Dumez]

Created attachment 253400 [details]
Patch

[Chris Dumez]

Created attachment 253401 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 253401 [details]
Patch

Clearing flags on attachment: 253401

Committed r184599: <http://trac.webkit.org/changeset/184599>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.