Based on discussions with stuart and roc, we should try porting the font matching code used on Windows to the Mac. This will eliminate the need to use ATSUI to do font matching. We'll still use ATSUI for glyph shaping and positioning.

Related font issues: Bug 362093 - support Windows-style font linking (e.g. Tahoma automagically links to CJK fonts for non-Latin glyphs) Bug 366664 Redesign gfxFont/gfxPlatform/Font Name Resolver for FontEntry

Gfx:Mac is dead ;)

Planning to do this in three steps: 1. Separate the code in gfxAtsuiFontGroup::InitTextRun into two routines, one that does per-character font matching and the other that sets up and executes the ATSUI layout callback which we use to grab glyphs and positions for the characters to be rendered. After this step, there should be no change in our rendering behavior. This is a fallback point if we run into problems in the steps that follow. 2a. Port over Stuart's code to scan fonts on the system to set up a list of supported scripts and character sets. Scanning all fonts at startup may be too slow, may need to add lazy evaluation of font data/cmap tables. [issues: differences in cmap table formats, Microsoft vs. Apple format differences, need to separate out cmap utility routines] 2b. Write code to produce a set of ranges, one per font, for the characters in a text run, something like the work done in UniscribeItem::ComputeRanges. This will replace the ATSUI font-matching routine in step (1). At this point, there may be differences in the fallback font matching. 3. Add code to fix the bugs blocked by this one (e.g. add code to turn off complex script features in OpenType fonts, since AAT doesn't handle these).

(In reply to comment #3) > 2a. Port over Stuart's code to scan fonts on the system to set up a list of > supported scripts and character sets. Scanning all fonts at startup may be too > slow, may need to add lazy evaluation of font data/cmap tables. FWIW, startup time took a massive hit doing font name resolution (see bug 364785, and though that bug is marked fixed, Ts didn't actually decrease), so it would be good if we could avoid increasing startup time even more :|

Ran some timing tests within the code in gfxQuartzFontCache::InitFontList: http://mxr.mozilla.org/mozilla/source/gfx/thebes/src/gfxQuartzFontCache.mm#253 InitFontList times MacBookPro, all optional fonts, few downloaded fonts, 351 total After reboot: 3053ms(!!!) After reboot, after starting FontBook: 787ms After browser run once: 201ms InitFontList called again: 87ms When font added: 121ms (there's a notification callback) When font deleted: 85ms For each font we're digesting the entire name table to pull out the font family and full name. ATS clearly has a system-wide cache, hence the improved performance with subsequent browser runs. And the per-app shared NSFontManager instance must also be caching information, because calling InitFontList a second time runs in less than half the time. Most of the time in InitFontList is spent reading through the name table, the initial block of code scanning all family names takes less than 10% of the total time in the worst case scenario. We should be able to avoid some of this time spent at startup by reading some sort of select list of fonts (e.g. all fonts listed in the prefs?) at startup and reading in the rest as we go along. If localized names are problem, it might make sense to write an explicit routine to pull in the name table.

After the Postscript name and family name are output, the output here reflects the output returned from ATSUGetIndFontName for each name table entry for a given font: http://mxr.mozilla.org/mozilla/source/gfx/thebes/src/gfxQuartzFontCache.mm#373 Note that there are a lot of fonts that include <font-family localized stylename> as a full name entry. The current code caches this to use in the font matching process.

This is the current version of my patch. It's fully functional but I need to do more testing and measure the performance and the effect of this on Ts, Tp numbers.

trimmed down version, still contains test code

This bug blocks other blockers, 361986 and 396732.

This bug should be marked a blocker as well, then.

Strictly speaking you don't need sr here. I'm happy to do the review if Stuart doesn't want it, though. Stuart?

i'm happy to review this. do we want to try to land it tonight for b2?

Comment on attachment 291416 [details] [diff] [review] patch, v.1.0 use cmap information to match fonts under Mac OS X i would suggest removing the UPDATE_RANGES code, NO_RANGE_FOUND, UnicodeRangeTableEntry, gUnicodeRanges stuff entirely. we can resurrect that code later if it is needed.

or, i suppose we need some of it for CharRangeBit() in the windows code, so maybe that stuff should just get moved back?

Comment on attachment 291416 [details] [diff] [review] patch, v.1.0 use cmap information to match fonts under Mac OS X John, I cleared the approval request here, but please re-request approval once the reviews are completed.

This patch trims out UPDATE_RANGES code and fixes a periodic crash that occurred when running through the page sets. Next step is to compare the performance to existing code for the Tp page set. Notes: (1) cmap loading is done completely lazily. Loading all fonts at startup is a bit of a waste, whenver a Java applet loads the JVM adds fonts to the system that cause InitFontList to get called, reinitializing everything. This means there should be no effect on startup time but there may be an effect on page load time when system fallback occurs the first time. (2) per-lang group pref font lists are generated *per* character, no attempt is made to cache them currently. This is probably the first thing to consider optimizing.

List of fonts containing two format 4 cmap tables, one with platform == Unicode, the other with platform == Microsoft: Aegean Aegyptus Akkadian Akshar Unicode Arial Italic Arial Bodoni Ornaments ITC TT Century Code2000 Euphemia UCAS Bold Euphemia UCAS Italic Helvetica CY Bold Helvetica CY BoldOblique Helvetica CY Oblique Helvetica CY Plain Hiragino Kaku Gothic Pro W3 Hiragino Kaku Gothic Pro W6 Hiragino Kaku Gothic Std W8 Hiragino Maru Gothic Pro W4 Hiragino Mincho Pro W3 Hiragino Mincho Pro W6 Musical Symbols Plantagenet Cherokee SBL Hebrew SPEdessa SchoolHouse Cursive B Times New Roman Bold Italic Times New Roman Bold Times New Roman Italic Times New Roman Trebuchet MS Bold Italic Trebuchet MS Bold Trebuchet MS Italic Trebuchet MS Unicode Symbols Verdana Bold Italic Verdana Bold Verdana Italic Verdana Except for the Helvetica CY font faces the two cmap tables match. For some reason, the Helvetica CY fonts have roughly a 100 fewer entries in their Microsoft platform tables. So in the case of fonts containing both flavors, prefer the Unicode one. This is actually important since Helvetica CY is used as a default font for Cyrillic.

Tested patch, ready for inspection. I haven't done a lot of optimization on this yet, specifically I haven't set up code to cache setting up pref font lists so these are still being created per-character when needed (very suckful). Running times for running through the full talos page set 5 times: Current trunk: 18min, 23secs With patch added: 15min, 52secs (86% of original) options: -tpcycles 5 -tpoffline I ran against with the trunk version first so this may not be the fairest test, the font matching code is definitely sensitive to the "warmness" of font server caches. I'll run this again tomorrow repeatedly to come up with some better numbers. But at least we're not sucking wind *before* optimizing things! From here, the next step is to profile the font matching code and start to tune things. The notes from Comment #17 describe two obvious places to start. The prefs code probably should be moved over into the Quartz font list code and cached there. Also, the CJK prefs code is not really ideal right now, we'll end up preferring Japanese fonts for obviously Chinese pages on Japanese systems, we should be using the style language to try and determine the order for matching CJK pref fonts before referencing accept languages. Eventually it would be nice to generalize this logic too for other languages that share Unicode code space (e.g. Arabic/Urdu/Persian for Arabic codepoints) but not for FF3.

If we can incrementally land this (as in land as is) and continue to tune that would be great.

Yeah, we need to run this ASAP. Some quick numbers with 5 pages that I picked out that were extremely slow (5 cycles)... Without patch: www.phoenixtv.com 14432 7709 7856 7785 7728 www.verycd.com 26019 20220 20204 20139 20230 www.qihoo.com 19085 5638 5132 5233 5423 www.it.com.cn 12033 4089 3791 3881 4473 www.mofile.com 14774 9619 9715 9488 9383 With patch: www.phoenixtv.com 8365 2335 2358 2454 2513 www.verycd.com 10209 3850 3863 4053 3906 www.qihoo.com 13528 5320 5313 4979 5170 www.it.com.cn 10859 3921 3607 3858 4096 www.mofile.com 8390 4895 4334 4870 4654

Note that in both pre and post patch, the reason these pages take so long overall is that we're doing a LOT of incremental reflow here; you can basically watch the pages get constructed. The browser is interactive during this time (especially post-patch), it's not like we freeze for 10s. But I don't know why we're doing so much reflow here.

Wow - I know a bunch of those pages show up on Windows as slower in 1.9/1.8 as well. Are the core optimizaitons here we can share across platforms?

not really -- this bug ports the Windows code to Mac, so we already have all of these wins on Windows.

John-san: I didn't touch to gfxFontUtils.h code, you should remove my name from it.

some more slow mac sites to be found in bug 407462

(In reply to comment #25) > I didn't touch to gfxFontUtils.h code, you should remove my name from it. Sure, no problem.

Profile results using Shark with top 50 slowest pages in the Talos pageset, focused on InitTextRun (21.2% of total time): 52.9% XUL AppendPrefFonts(nsTArray<nsRefPtr<gfxFont> >*, eFontPrefLang, unsigned&, gfxFontStyle const*) 20.3% XUL AppendGenericFontFromPref(nsString&, char const*, char const*) 14.9% XUL gfxAtsuiFontGroup::InitTextRun(gfxTextRun*, unsigned short const*, unsigned, int, unsigned, unsigned) 6.7% XUL AppendFontToList(nsAString_internal const&, nsACString_internal const&, void*) 3.9% XUL gfxAtsuiFontGroup::FindFontForChar(unsigned, unsigned, unsigned, nsRefPtr<gfxAtsuiFont>&, nsRefPtr<gfxAtsuiFont>&) 0.5% XUL PostLayoutOperationCallback(unsigned long, ATSGlyphVector*, unsigned long, void*, unsigned long*) 0.3% XUL gfxAtsuiFont::gfxAtsuiFont[in-charge](unsigned long, nsAString_internal const&, gfxFontStyle const*) 0.1% XUL SetGlyphsForCharacterGroup(ATSLayoutRecord*, unsigned, long*, unsigned, gfxTextRun*, unsigned, unsigned char const*, unsigned short const*, unsigned) 0.1% XUL gfxAtsuiFont::TestCharacterMap(unsigned) 0.1% XUL gfxTextRun::SortGlyphRuns() Profile settings: 100,000 WTF sample (running sample size, start point defined by stop point), 1ms interval Fold all calls to other routines into the ones above (i.e. select and hit cmd-E until other calls disappear) So for these pages, 80% of the time spent within InitTextRun is setting up pref font lists. Seems reasonable that this is the next place to optimize. ;)

Comment on attachment 292750 [details] [diff] [review] patch, v1.2, cmap font matching fix Overall, this looks pretty good. Some comments: -/* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*- +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- Please leave these at 20 -- they're done that way on purpose so that any tabs that might sneak in will be seen very quickly. + inline gfxAtsuiFont* WhichFontSupportsChar(nsTArray< nsRefPtr<gfxFont> >& fontList, PRUint32 ch) { + for (PRUint32 i = 0; i < fontList.Length(); i++) { fontList should be const. you should also pull the length argument out of the loop... while i'm nitpicking this function, should you also get rid of gfxAtsuiFont::TextCharacterMap and just call mFontEntry->TestCharacterMap. Maybe it doesn't matter -- you're already branching for cmap initialization flag inside the font entry's version. + void FindFontForChar(PRUint32 ch, PRUint32 prevCh, PRUint32 nextCh, nsRefPtr<gfxAtsuiFont>& aFont, nsRefPtr<gfxAtsuiFont>& matchedFont); I would change this to: gfxAtsuiFont *FindFontForChar(PRUint32 ch, PRUint32 prevCh, PRUint32 nextCh, gfxAtsuiFont* aFont); No real reason to pass nsRefPtr references through gfxSparseBitSet might should move to xpcom/glue(?) as nsSparseBitset or something. roc, thoughts? +gfxFontUtils::ReadCMAP(PRUint8 *aBuf, PRUint32 bufLength, gfxSparseBitSet& aCharacterMap, std::bitset<128>& aUnicodeRanges, + PRPackedBool& aUnicodeFont, PRPackedBool& aSymbolFont) The use of aUnicodeFont and aSymbolFont is a little weird here. They're only set in the symbol font case, and I don't see where they are set for the others The mac FontEntry code inits mUnicodeFont to false in its constructor but doesn't seem to do anything else with it. On windows we set it based on some things and fsUsb being true. we should probably set it in other cases as well. We also on windows set mUnicodeFont to false is ReadCMAP fails. + if (NS_SUCCEEDED(rv) && !nameListValue.Equals(nameValue)) { How often does this get hit? +nsresult +gfxQuartzFontCache::FindFontEntry(ATSUFontID fid, nsRefPtr<FontEntry>& fe) This should return a FontEntry * -- null if not available. lets avoid the nsresult plague where we can. +gfxQuartzFontCache::FindFontForCharProc(nsUint32HashKey::KeyType aKey, nsRefPtr<FontEntry>& aFontEntry, You probably want to skip bitmap fonts, non-unicode ones, etc. something like window's FontEntry::IsCrappyFont()

Will update based on review comments tomorrow.

The Mac version of the Symbol font contains a format-4 cmap table that maps symbols into normal Unicode codepoints. So if someone is setting up a webpage using ASCII codepoints and the Symbol font, this page will display with symbols on Windows but with plain ASCII on Mac/Linux. Webdings/Wingdings fonts contain the same cmap tables as on Windows, a special symbol table (encoding == 0) and a format-4 cmap table that defines codepoints in the Private Use space. Based on decisions made on 399636, we may need to add support for fonts with the special symbol table on Mac/Linux. Not sure this really matters though.

Changes: * tab-width reset to 20 * WhichFontSupportsChar changed but it's difficult to inline everything because FontEntry lives in an Objective-C file * switched FindFontForChar to return alreadyAddRefed<gfxAtsuiFont> blobs * didn't change anything to gfxSparseBitSet * trimmed out mUnicodeFont and mSymbolFont (see previous comment)

> + if (NS_SUCCEEDED(rv) && !nameListValue.Equals(nameValue)) { > How often does this get hit? The check in the pref font handling code is hit most of the time. It will only be different if the user has explicitly dug into the prefs panel and changed the value.

+ inline gfxAtsuiFont* GetFontAt(PRInt32 i) { Lose the inline and trust the compiler, unless you know that the compiler is not inlining it and that matters for performance. I know it's kind of a pain, but would you mind using the aParam convention? +// The lang IDs for font prefs +enum eFontPrefLang { + eFontPrefLang_Western = 0, Can you document where these come from? I know you're just moving the code but I'd like to know :-). - ATSUDisposeFontFallbacks(mFallbacks); + You don't need a blank line here. In fact you don't need the destructor at all. - CreateFontFallbacksFromFontList(&mFonts, &mFallbacks, kATSUSequentialFallbacksExclusive); You don't need to leave a blank line here either. + // xxx - god this sucks to be doing this per-character in the fallback case Yes, yes it does. After this patch, perhaps we could speed it up by keeping a table per eFontPrefLang value, lazily constructed, that maps characters to the first pref font containing that character? I assume we have less than 16 pref fonts per language so we could use 4 bits per character to store an index into the pref font list and easily pack each table into 32K. gfxQuartzFontCache::FindFontForChar could have its own kind of table if that gets hit much. I assume these optimizations could be applied to Windows too? Could you document CmapFontMatcher? Would it make sense to move it to gfxFontUtils and use it from Windows too? With this lazy CMAP reading setup, the first time we see a bizarro character we're going to load the CMAPs for all fonts in the system, synchronously? Any plan to address that, like load CMAPs in the background after browser startup? (Say one per XPCOM event so we don't block anything else...) ? gfx/thebes/public/gfxFontUtils.h ? gfx/thebes/src/gfxFontUtils.cpp You need to put these in the patch, although I assume they're just the copied Windows code so it's not too big a deal.

Yeah, the first thing I want to improve about this is to cache the pref fonts in some compact form just as you suggest. On Windows we already do this in gfxWindowsPlatform::GetPrefFontEntries and SetPrefFontEntries. The tricky part is to figure out how to translate these efficiently into gfxAtsuiFont objects which are defined per style (i.e. includes <bold/italic> and <lang group>). Yes, right now an unknown character will cause all cmaps to be read in synchronously (suckful). We could read in these at startup time (also suckful). As you suggest, I think "sipping" these in via an idle thread/work queue/nsRunnable subclass thingy is the best way to avoid a huge performance hit. For normal pages this will probably *not* occur, since pref fonts are checked before checking all other fonts and most of these pref fonts exist on most users' machines.

(In reply to comment #36) > For normal pages this will probably *not* occur, since pref fonts are checked > before checking all other fonts and most of these pref fonts exist on most > users' machines. yeah, but all it takes is one character that's not covered by any font and you're in trouble ... could easily happen I imagine if a page happens to have a scrap of garbage in it.

Will this patch help the #3 top crash for Beta 2, gfxAtsuiFont::SetupCairoFont(gfxContext*)? Seems this is only crashing on Leopard.

(In reply to comment #38) > Will this patch help the #3 top crash for Beta 2, > gfxAtsuiFont::SetupCairoFont(gfxContext*)? Seems this is only crashing on > Leopard. Looks like roc has already set up a patch for bug 407761 but just to answer your question directly, no, this patch for this fix will not resolve the 10.5 problem in bug 407761. It may however resolve one of the other bugs list in the crash reports, the gfxAtsuiFontGroup::InitTextRun crasher in bug 401534.

Comment on attachment 294004 [details] [diff] [review] patch, v.1.5, updated based on roc's comments trailing whitespace: + nsRefPtr<FontEntry> mFontEntry; you've got a couple tabs in here... + nsRefPtr<FontEntry> fe; In +FontEntry::ReadCMAP() you're doing: + nsAutoPtr<PRUint8> cmap; + cmap = (PRUint8*) malloc(size); nsAutoPtr will call delete on cmap which could end up with a allocator mismatch. Use new here instead or change it to use an nsAutoTArray<PRUint8,16384> like we do on windows. (or get some stats on cmap sizees and change it to match that if smaller -- wouldn't do much bigger than 16k on the stack) aside from that this looks good to go.

Changes based on comment 40. In addition, I put back in the call to SortGlyphRuns after doing font matching. Seems like this shouldn't be needed but without this, sequences of unmatching characters produce "glyph runs not coalesced" assertions and the text run word cache doesn't get cleaned properly. Will investigate possible changes to AddGlyphRun to handle unmatched character runs properly.

Comment on attachment 294611 [details] [diff] [review] patch, v.1.6, updated based on pav's comments on 1.5 patch looks good -- lets get this in.

bombs away, patch 1.6 checked in... Notes: * followup performance improvements in bug 409342 * reftest for bug 391045 marked as skip on Mac/Linux since the original bug was a Windows-specific bug and the reftest relies on the trailing period rendering the same across fonts. smontagu suggested this.