Bug 196846 - [Cocoa] FontPlatformData objects aren't cached at all when using font-family:system-ui
Summary: [Cocoa] FontPlatformData objects aren't cached at all when using font-family:...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-11 19:38 PDT by Myles C. Maxfield
Modified: 2019-04-18 09:08 PDT (History)
10 users (show)

See Also:


Attachments
WIP (9.03 KB, patch)
2019-04-11 19:39 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-highsierra (2.34 MB, application/zip)
2019-04-11 21:33 PDT, Build Bot
no flags Details
WIP (11.26 KB, patch)
2019-04-12 19:04 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (11.67 KB, patch)
2019-04-14 11:05 PDT, Myles C. Maxfield
simon.fraser: review+
Details | Formatted Diff | Diff
Patch for committing (15.15 KB, patch)
2019-04-15 15:20 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2019-04-11 19:38:30 PDT
[Cocoa] FontPlatformData objects aren't cached at all when using font-family:system-ui
Comment 1 Myles C. Maxfield 2019-04-11 19:39:17 PDT
Created attachment 367278 [details]
WIP
Comment 2 Myles C. Maxfield 2019-04-11 19:39:37 PDT
<rdar://problem/49499971>
Comment 3 Build Bot 2019-04-11 19:41:13 PDT
Attachment 367278 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Build Bot 2019-04-11 21:33:27 PDT
Comment on attachment 367278 [details]
WIP

Attachment 367278 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11849345

New failing tests:
webgl/2.0.0/conformance/context/context-release-upon-reload.html
Comment 5 Build Bot 2019-04-11 21:33:28 PDT
Created attachment 367285 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 6 Myles C. Maxfield 2019-04-11 23:13:54 PDT
Test failure is unrelated
Comment 7 Myles C. Maxfield 2019-04-12 19:04:47 PDT
Created attachment 367372 [details]
WIP
Comment 8 Myles C. Maxfield 2019-04-14 10:59:23 PDT
16.8x increase in performance on the benchmark!!
Comment 9 Myles C. Maxfield 2019-04-14 11:05:08 PDT
Created attachment 367402 [details]
Patch
Comment 10 Darin Adler 2019-04-15 10:54:30 PDT
Comment on attachment 367402 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/FontFamilySpecificationCoreText.cpp:53
> +    static inline bool safeCFEqual(CFTypeRef a, CFTypeRef b)
> +    {
> +        return (!a && !b) || (a && b && CFEqual(a, b));
> +    }

Seems like this could be a helper somewhere else to be shared with other call sites instead of here in the class. I would search for other calls to CFEqual in WebKit to see what we can share with.

> Source/WebCore/platform/graphics/cocoa/FontFamilySpecificationCoreText.cpp:69
> +        return (fontDescriptor ? CFHash(fontDescriptor.get()) : 0) ^ fontDescriptionKey.computeHash();

To combine two hashes to make a single result like this, which is a suboptimal idiom, but sometimes necessary, I suggest using pairIntHash from HashFunctions.h rather than exclusive or.

Also seems like we could put "safeCFHash" in the same place as "safeCFEqual" and make this nicer.
Comment 11 Darin Adler 2019-04-15 10:58:56 PDT
Comment on attachment 367402 [details]
Patch

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

> Source/WebCore/platform/graphics/cocoa/FontFamilySpecificationCoreText.cpp:38
> +struct FontFamilySpecificationKey {

Sure would be nice if this struct could be a. lot simpler. So many lines of code! Need to fix our hashing machinery so this idiom is not so complex.

> Source/WebCore/platform/graphics/cocoa/FontFamilySpecificationCoreText.cpp:48
> +    explicit FontFamilySpecificationKey(WTF::HashTableDeletedValueType hashTableDeletedValueType)
> +        : fontDescriptionKey(hashTableDeletedValueType)
> +    { }

Argument name here is peculiar. Should not include the word "type" in the name of an argument. I think I would call it "deletedValue".

> Source/WebCore/platform/graphics/cocoa/FontFamilySpecificationCoreText.cpp:87
> +    static unsigned hash(const FontFamilySpecificationKey& key)
> +    {
> +        return key.computeHash();
> +    }
> +
> +    static bool equal(const FontFamilySpecificationKey& a, const FontFamilySpecificationKey& b)
> +    {
> +        return a == b;
> +    }
> +
> +    static const bool safeToCompareToEmptyOrDeleted = true;

Style-wise seems it would be nicer to use one-liners like in HashFunctions.h for these functions.
Comment 12 Myles C. Maxfield 2019-04-15 15:20:01 PDT
Created attachment 367465 [details]
Patch for committing
Comment 13 WebKit Commit Bot 2019-04-15 17:51:10 PDT
Comment on attachment 367465 [details]
Patch for committing

Clearing flags on attachment: 367465

Committed r244315: <https://trac.webkit.org/changeset/244315>
Comment 14 Darin Adler 2019-04-17 10:04:10 PDT
Comment on attachment 367465 [details]
Patch for committing

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

> Source/WTF/wtf/RetainPtr.h:374
> +    return (!a && !b) || (a && b && CFEqual(a, b));

I have an idea for an alternate implementation and I wonder which is more efficient in the most important cases. Here’s my alternative:

    return (a == b) || (a && b && CFEqual(a, b));
Comment 15 Simon Fraser (smfr) 2019-04-17 11:07:52 PDT
(In reply to Darin Adler from comment #14)
> Comment on attachment 367465 [details]
> Patch for committing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367465&action=review
> 
> > Source/WTF/wtf/RetainPtr.h:374
> > +    return (!a && !b) || (a && b && CFEqual(a, b));
> 
> I have an idea for an alternate implementation and I wonder which is more
> efficient in the most important cases. Here’s my alternative:
> 
>     return (a == b) || (a && b && CFEqual(a, b));

This looks like a CF equivalent of WTF::arePointingToEqualData()
Comment 16 Myles C. Maxfield 2019-04-17 11:24:45 PDT
Comment on attachment 367465 [details]
Patch for committing

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

>>> Source/WTF/wtf/RetainPtr.h:374
>>> +    return (!a && !b) || (a && b && CFEqual(a, b));
>> 
>> I have an idea for an alternate implementation and I wonder which is more efficient in the most important cases. Here’s my alternative:
>> 
>>     return (a == b) || (a && b && CFEqual(a, b));
> 
> This looks like a CF equivalent of WTF::arePointingToEqualData()

How confident are we that CFEqual doesn't do the shallow comparison already? If so, then this idea would be faster if the CFEqual call hasn't already been loaded or isn't in the instruction cache. CFEqual is a pretty common operation, so I'm not sure whether or not it would be better.
Comment 18 Darin Adler 2019-04-18 09:08:28 PDT
Comment on attachment 367465 [details]
Patch for committing

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

>>>> Source/WTF/wtf/RetainPtr.h:374
>>>> +    return (!a && !b) || (a && b && CFEqual(a, b));
>>> 
>>> I have an idea for an alternate implementation and I wonder which is more efficient in the most important cases. Here’s my alternative:
>>> 
>>>     return (a == b) || (a && b && CFEqual(a, b));
>> 
>> This looks like a CF equivalent of WTF::arePointingToEqualData()
> 
> How confident are we that CFEqual doesn't do the shallow comparison already? If so, then this idea would be faster if the CFEqual call hasn't already been loaded or isn't in the instruction cache. CFEqual is a pretty common operation, so I'm not sure whether or not it would be better.

Yes, CFEqual does do the shallow comparison; I didn’t mean to imply that it didn’t. But I don’t understand your logic about which version is slightly better. I’m pretty sure that my alternative generates smaller code with one fewer branch, for example.