streamline MacOSFontEntry::HasFontTable

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jtd, Assigned: jtd)

Tracking

Trunk
mozilla43
Unspecified
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8656416 [details] [diff] [review]
patch, streamline MacOSFontEntry::HasFontTable

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
Last Resolved: 3 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.