Closed
Bug 1201403
Opened 9 years ago
Closed 9 years ago
streamline MacOSFontEntry::HasFontTable
Categories
(Core :: Graphics: Text, defect)
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: jtd, Assigned: jtd)
Details
Attachments
(1 file)
2.35 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Use CGFontCopyTableTags to store the set of available tables in a simple hashset.
Attachment #8656416 -
Flags: review?(jfkthame)
Comment 2•9 years ago
|
||
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 3•9 years ago
|
||
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+
Assignee | ||
Comment 4•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8175a26ffebf
Comment 6•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/219979d9d8ba
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•