Closed Bug 1201403 Opened 6 years ago Closed 6 years ago

streamline MacOSFontEntry::HasFontTable

Categories

(Core :: Graphics: Text, defect)

Unspecified
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: jtd, Assigned: jtd)

Details

Attachments

(1 file)

Profiling textbench tests under 10.10, roughly 10% of the time spent was in MacOSFontEntry::HasFontTable. This is a query to the font as to whether a given table exists or not in the font. The implementation is copying the table data as a way to test whether a table exists. It would be better to use the available CoreGraphics call to list tables in the font and stick those into a hash table.
Use CGFontCopyTableTags to store the set of available tables in a simple hashset.
Attachment #8656416 - Flags: review?(jfkthame)
Interesting. Profiling on 10.9, I see less than 1% under HasFontTable. I've always understood that table data is not actually copied by the CGFontCopyTableForTag call that it uses; it just bumps the refcount of an existing CFData and returns it.

Sounds like 10.10 may have changed in such a way that this is no longer true, and CGFontCopyTableForTag has become much more expensive. :(
Comment on attachment 8656416 [details] [diff] [review]
patch, streamline MacOSFontEntry::HasFontTable

Review of attachment 8656416 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxMacPlatformFontList.mm
@@ +380,2 @@
>  
> +       CGFontRef fontRef = GetFontRef();

nit: indentation is wrong on these two lines

@@ +381,5 @@
> +       CGFontRef fontRef = GetFontRef();
> +        if (!fontRef) {
> +            return false;
> +        }
> +        CFArrayRef tags = CGFontCopyTableTags(fontRef);

nit: for consistency with nearby code, it'd be nice to use the :: prefix on the CG call here. Same for two CF calls below.
Attachment #8656416 - Flags: review?(jfkthame) → review+
https://hg.mozilla.org/mozilla-central/rev/219979d9d8ba
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.