Bug 215688 – [Cocoa] Prepare for migrating to CTFontHasTable() once it's faster than CTFontCopyAvailableTables()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[Cocoa] CTFontHasTable() is faster than iterating over every table in the font

[Myles C. Maxfield]

Created attachment 406917 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/57131142>

[Myles C. Maxfield]

Created attachment 408077 [details]
Patch

[Darin Adler]

Comment on attachment 408077 [details]
Patch

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

> Source/WTF/wtf/PlatformUse.h:309
> +#define USE_CTFONTHASTABLE 1

Is this really a USE and not a HAVE?

USE for policy. HAVE for capability. I assume we’d use this whenever we have a working implementation.

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:65
>  static bool fontHasVerticalGlyphs(CTFontRef ctFont)

Seems like the argument can just be named font.

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:71
>      RetainPtr<CFArrayRef> tableTags = adoptCF(CTFontCopyAvailableTables(ctFont, kCTFontTableOptionNoOptions));

auto

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:76
> +        CTFontTableTag tag = static_cast<CTFontTableTag>(reinterpret_cast<uintptr_t>(CFArrayGetValueAtIndex(tableTags.get(), index)));

auto

Is the static_cast really needed? I would think that conversion between integer types would work without it. In fact we could even compare uintptr_t values with CTFontTableTag constants without having to make the types match. I think using auto and taking out the static_cast would be good.

> Source/WebCore/platform/graphics/opentype/OpenTypeCG.cpp:42
>  bool fontHasMathTable(CTFontRef ctFont)

Could this share code with the other some day?

A function that takes a CTFontRef and an argument list of tables and checks for any of them? Would be nice to not have this twice.

> Source/WebCore/platform/graphics/opentype/OpenTypeCG.cpp:47
>      RetainPtr<CFArrayRef> tableTags = adoptCF(CTFontCopyAvailableTables(ctFont, kCTFontTableOptionNoOptions));

Ditto.

> Source/WebCore/platform/graphics/opentype/OpenTypeCG.cpp:52
> +        CTFontTableTag tag = static_cast<CTFontTableTag>(reinterpret_cast<uintptr_t>(CFArrayGetValueAtIndex(tableTags.get(), index)));

Ditto.

[Myles C. Maxfield]

Created attachment 408089 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 408090 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 408091 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 408096 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 408100 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 408145 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 408146 [details]
Patch for committing

[Darin Adler]

Comment on attachment 408146 [details]
Patch for committing

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

> Source/WebCore/platform/graphics/Font.cpp:488
> +#if USE(CG)
> +bool fontHasTable(CTFontRef ctFont, unsigned tableTag)

In the past sometimes we have literally named our function "CTFontHasTable" and just compile it when on a system that doesn’t have it. Risky though.

> Source/WebCore/platform/graphics/Font.cpp:498
> +        auto tag = static_cast<CTFontTableTag>(reinterpret_cast<uintptr_t>(CFArrayGetValueAtIndex(tableTags.get(), index)));

I don’t think the static_cast to CTFontTableTag here is needed.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:495
> +        bool foundFvar = CTFontHasTable(font, kCTFontTableFvar);
> +        foundStat = CTFontHasTable(font, kCTFontTableSTAT);
> +        aatShaping = CTFontHasTable(font, kCTFontTableMorx) || CTFontHasTable(font, kCTFontTableMort);
> +        openTypeShaping = CTFontHasTable(font, kCTFontTableGPOS) || CTFontHasTable(font, kCTFontTableGSUB);
> +        foundTrak = CTFontHasTable(font, kCTFontTableTrak);

Are you sure that 7 calls to this function are fast enough?

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:67
> +    return fontHasTable(font, kCTFontTableVhea) || fontHasTable(font, kCTFontTableVORG);

Is it OK that this now is twice as slow on older systems?

[Myles C. Maxfield]

Created attachment 408154 [details]
Patch for committing

[Myles C. Maxfield]

Comment on attachment 408146 [details]
Patch for committing

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

>> Source/WebCore/platform/graphics/Font.cpp:498
>> +        auto tag = static_cast<CTFontTableTag>(reinterpret_cast<uintptr_t>(CFArrayGetValueAtIndex(tableTags.get(), index)));
> 
> I don’t think the static_cast to CTFontTableTag here is needed.

See the obsoleted patches in this bug. I tried taking it out but the builds turned red.

[Myles C. Maxfield]

Comment on attachment 408146 [details]
Patch for committing

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

>> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:67
>> +    return fontHasTable(font, kCTFontTableVhea) || fontHasTable(font, kCTFontTableVORG);
> 
> Is it OK that this now is twice as slow on older systems?

I don't have a benchmark (yet), but the idea is that asking a font if it has a particular table is O(1) instead of O(n) to iterate through all tables. Also, there doesn't have to be a CFArray allocation.

I can make a tiny microbenchmark to verify this intuition.

[Myles C. Maxfield]

Heyyyyy guess what? From creating a microbenchmark, it turns out CTFontHasTable() is slower than the loop! I've filed <rdar://problem/68438759>

I'll modify this patch to always run the loop, but I'll keep the design where this stuff is centralized so that when that radar is fixed we can have a drop-in replacement. I guess I can make a second variation of the loop which looks for two tag names, also.

[Myles C. Maxfield]

Created attachment 408158 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 408159 [details]
Patch for committing

[EWS]

Committed r266692: <https://trac.webkit.org/changeset/266692>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408159 [details].