Closed
Bug 409342
Opened 17 years ago
Closed 17 years ago
improve font matching performance on mac
Categories
(Core :: Graphics, defect, P3)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jtd, Assigned: jtd)
References
Details
(Keywords: perf)
Attachments
(5 files, 1 obsolete file)
46.31 KB,
patch
|
pavlov
:
review+
pavlov
:
superreview+
beltzner
:
approval1.9b3+
|
Details | Diff | Splinter Review |
61.67 KB,
image/png
|
Details | |
3.17 KB,
patch
|
pavlov
:
review+
pavlov
:
superreview+
mtschrep
:
approval1.9b3+
|
Details | Diff | Splinter Review |
14.16 KB,
patch
|
Details | Diff | Splinter Review | |
6.09 KB,
patch
|
pavlov
:
review+
pavlov
:
superreview+
|
Details | Diff | Splinter Review |
This is a follow-on bug to bug 396137, the port of Windows font matching code to the Mac. Right now, the two things I think should be improved are: 1. Cache pref font lists rather than generating them each time a character is not found in the font group font list. As noted in comment 28 of bug 396137, profiling with the 396137 patch shows that 80% of time spent within InitTextRun for the fifty slowest pages in the talos page set is spent creating pref font lists. 2. Set up a background task of some flavor that loads in cmap information little-by-little to avoid being forced to load all cmap information at startup or on the first miss (i.e. when a character is not found either the style fonts or in the fallback pref fonts). Suggested priority: P3
Flags: blocking1.9?
These will be cross-platform optimizations for both Windows and Mac, right? Also, are we going to have Linux use this font matching code too? I hope the answer is yes...
Assignee | ||
Comment 2•17 years ago
|
||
(In reply to comment #1) > These will be cross-platform optimizations for both Windows and Mac, right? Strictly speaking, no. The Windows code already caches the pref font lists in arrays of nsRefPtr<FontEntry> objects. The FontEntry objects are different on Mac and Windows so this code doesn't work easily cross-platform. Right now the FontEntry class isn't particularly Windows dependent but Stuart has talked about adding more Windows-specific functionality there. To make a cross-platform version we'd need to work out something that fits into those plans. Masayuki has a patch to unify these across platforms but I'm not sure whether this is going in or not. We could implement the incremental reading of cmap data cross-platform, it's currently done all at startup on Windows. > Also, are we going to have Linux use this font matching code too? I hope the > answer is yes... The CmapFontMatcher code uses gfxAtsuiFont objects but doesn't make any ATSUI calls. So with a little work to move some functionality into gfxFont instead we could probably figure out a way of making that cross-platform. We also need cross-platform FontEntry objects to abstract out the methods for accessing cmaps. As for the API's that maintain the system-wide font list under Linux, I don't really know much about that code. You'd need to know about any sort of "font funkiness" with typical sets of fonts on Linux, which flavor of cmap's they tend to use, yadda, yadda.
Oh, hmm. That makes sense. I was thinking about cached character-to-glyph mappings for the pref fonts, but I guess that's a different bug.
(In reply to comment #1) > Also, are we going to have Linux use this font matching code too? I hope the > answer is yes... I think the current answer is 'no', based on discussions with behdad; however, if font selection ends up being a big bottleneck on linux we may need to. The reasoning is that to be fully linux-compliant or whatever we need to use fontconfig for font selection, so that it can apply the user's fontconfig preferences and any custom configuration/matching bits. That said, if the port wouldn't be too hard, it would be useful to benchmark this code on linux, but that's a separate bug too.
Assignee | ||
Updated•17 years ago
|
Severity: normal → major
Status: NEW → ASSIGNED
Assignee | ||
Updated•17 years ago
|
Priority: -- → P3
Comment 5•17 years ago
|
||
+ing since this can make a major difference to Tp
Flags: blocking1.9? → blocking1.9+
Assignee | ||
Comment 6•17 years ago
|
||
I'm adding a dependency on 404310, because to be able to cache pref fonts in an efficient manner I need to make changes to the structure of the hashtables in gfxQuartzFontCache. These will change with the work that's required for 404310.
Depends on: 404310
Assignee | ||
Comment 7•17 years ago
|
||
Instead of building the pref font lists for *each* character, cache the list of font families per lang group (this never changes unless the pref settings are changed), and make gfxAtsuiFont objects only when the font contains a codepoint in its cmap. Also, cache away CJK preferences based on accept-language and system default script.
Attachment #300294 -
Flags: superreview?(pavlov)
Attachment #300294 -
Flags: review?(pavlov)
Updated•17 years ago
|
Attachment #300294 -
Flags: superreview?(pavlov)
Attachment #300294 -
Flags: superreview+
Attachment #300294 -
Flags: review?(pavlov)
Attachment #300294 -
Flags: review+
Comment 8•17 years ago
|
||
We want this for b3?
Assignee | ||
Comment 9•17 years ago
|
||
Comment on attachment 300294 [details] [diff] [review] patch, v.0.1, cache pref font lists more efficiently This is a Mac-only fix, it should give us a good improvement in Tp loadtime performance.
Attachment #300294 -
Flags: approval1.9b3?
Comment 10•17 years ago
|
||
we should take this, imho.
Comment 11•17 years ago
|
||
Comment on attachment 300294 [details] [diff] [review] patch, v.0.1, cache pref font lists more efficiently a=beltzner for beta 3
Attachment #300294 -
Flags: approval1.9b3? → approval1.9b3+
Comment 12•17 years ago
|
||
With this patch applied I get poor display of Japanese characters on pages with utf-8 encoding (google) or/and stylesheets that specify only roman fonts, like http://mozillazine.jp/ http://www.google.com/search?q=mozilla%20japan&ie=utf-8&oe=utf-8
Comment 13•17 years ago
|
||
screenshot for comment 12 both 10.4 and 10.5 Camino build, 2.0a1pre (1.9b3pre 2008013017) OS is English
Comment 14•17 years ago
|
||
That is the same issue that was originally fixed in bug 35755.
Comment 15•17 years ago
|
||
er, I meant bug 357555. sorry for the bugspam.
Assignee | ||
Comment 16•17 years ago
|
||
Argh, my bad. Include eFontPrefLang_CJKSet as a CJK lang and handle pref lang array correctly.
Attachment #300572 -
Flags: superreview?(pavlov)
Attachment #300572 -
Flags: review?(pavlov)
Attachment #300572 -
Flags: approval1.9b3?
Comment 17•17 years ago
|
||
Comment on attachment 300572 [details] [diff] [review] patch, fixup screwup handling CJK lang prefs assuming this passes review let's take this
Attachment #300572 -
Flags: approval1.9b3? → approval1.9b3+
Comment 18•17 years ago
|
||
(In reply to comment #16) > Created an attachment (id=300572) [details] yup, that fixes the issue on 10.5.1 on my local build
Updated•17 years ago
|
Attachment #300572 -
Flags: superreview?(pavlov)
Attachment #300572 -
Flags: superreview+
Attachment #300572 -
Flags: review?(pavlov)
Attachment #300572 -
Flags: review+
Assignee | ||
Comment 19•17 years ago
|
||
For pages that hit the pref fonts, this adds a nice speedup since in most cases the last pref font hit will be the same for other characters in the same unicode range. The best case fast path can only be used by the first pref font in the list, in other cases need to check through the pref font list in order.
Assignee | ||
Comment 20•17 years ago
|
||
Comment on attachment 300773 [details] [diff] [review] patch, cache the last pref font looked up This is a simple speedup tweak with no change in behavior. It will only affect pages that often fall through to pref fonts, like Chinese pages that specify Arial for everything.
Attachment #300773 -
Flags: superreview?(pavlov)
Attachment #300773 -
Flags: review?(vladimir)
Attachment #300773 -
Flags: review?(pavlov)
Assignee | ||
Comment 21•17 years ago
|
||
Uses real-time clock to count cycles for font matching process within InitTextRun. Dumps the text run with style and font matching info, along with cycle counts per character and the stage at which the a font was matched within gfxAtsuiFontGroup::FindFontForChar. The code appended to the count means: jn joiner fg font group pu private use char pr pref font pv prev font sy system font lookup nm not matched Handy for figuring out what fonts are matching to particular text runs and why.
Attachment #300773 -
Flags: review?(vladimir) → review+
Comment 22•17 years ago
|
||
Comment on attachment 300773 [details] [diff] [review] patch, cache the last pref font looked up please expand the comment: // check to see if this is the last pref family looked up just to be a little more specific on what is going on and in what conditions we are likely to hit that case.
Attachment #300773 -
Flags: review?(pavlov) → review+
Assignee | ||
Comment 23•17 years ago
|
||
created bug 416923 to deal with moving gfxSparseBitSet over to xpcom
Assignee | ||
Comment 24•17 years ago
|
||
additional comment description added: // if a pref font is used, it's likely to be used again in the same text run. // the style doesn't change so the face lookup can be cached rather than calling // GetOrMakeFont repeatedly. speeds up FindFontForChar lookup times for subsequent // pref font lookups
Attachment #300773 -
Attachment is obsolete: true
Attachment #302732 -
Flags: superreview?(pavlov)
Attachment #302732 -
Flags: review?(pavlov)
Attachment #300773 -
Flags: superreview?(pavlov)
Updated•17 years ago
|
Attachment #302732 -
Flags: superreview?(pavlov)
Attachment #302732 -
Flags: superreview+
Attachment #302732 -
Flags: review?(pavlov)
Attachment #302732 -
Flags: review+
Assignee | ||
Comment 25•17 years ago
|
||
patch for caching last pref font checked in. With this the tasks for item 1 in the description are pretty much wrapped up (always room for performance improvements, naturally!). Font cmaps are still loaded lazily, this was task 2, so the first page to cause a system-wide font search will take a performance hit. Several patches have already been checked in for this bug, so I'm going to move this task out to a separate bug, bug 416946.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•