Closed
Bug 752394
Opened 12 years ago
Closed 10 years ago
eliminate need to enumerate fonts for localized family names and postscript names
Categories
(Core :: Graphics: Text, defect)
Core
Graphics: Text
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: jtd, Assigned: jtd)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 16 obsolete files)
734 bytes,
text/html
|
Details | |
15.92 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
829 bytes,
text/html
|
Details | |
9.61 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
3.12 KB,
patch
|
jtd
:
review+
|
Details | Diff | Splinter Review |
Telemetry stats show that the initialization of localized family names and postscript names that occurs in some cases usually complete quickly (<1ms) but there's a set of cases that can take significantly longer. We need to eliminate those cases. E.g. https://metrics.mozilla.com/data/content/pentaho-cdf-dd/Render?solution=metrics2&path=telemetry&file=telemetryHistogram.wcdf&bookmarkState={%22impl%22%3A%22client%22%2C%22params%22%3A{%22startDate%22%3A%222012-04-07%22%2C%22endDate%22%3A%222012-05-06%22%2C%22appVersion%22%3A%22%22%2C%22appName%22%3A%22firefox%22%2C%22arch%22%3A%22%22%2C%22OS%22%3A%22%22%2C%22version%22%3A%22%22%2C%22channel%22%3A%22%22%2C%22appBuildID%22%3A%22%22%2C%22fromPlatformBuildID%22%3A%22%22%2C%22toPlatformBuildID%22%3A%22%22%2C%22excludeParam%22%3A%22%22%2C%22measure%22%3A%22FONTLIST_INITOTHERFAMILYNAMES%22%2C%22histogramCompareParam%22%3A%22appVersion%22%2C%22histogramVariablesParam%22%3A%22%22%2C%22platformBuildIDMode%22%3A%22LATEST%22%2C%22platformBuildIDTopCount%22%3A%2230%22%2C%22conditionsStatistic%22%3A%22%23conditionsStatistic%22%2C%22submissionsParameter%22%3A[[96793144]]}} The two Telemetry measures here are: FONTLIST_INITOTHERFAMILYNAMES FONTLIST_INITFACENAMELISTS
Hello :jtd, i had trouble recently with fonts and i wonder if this is related to this bug. Background: I have around 12.000 Files in my Font Folder (Windows 7 64 bit, Firefox 20.0.1). Now, whenever i load a website that has a custom font or a google API font (examples: g2play.net, surrenderat20.net, about:downthemall) my Firefox freezes for 2-3 Minutes while loading the website and does not respond. I temporarely fixed it by setting gfx.downloadable_fonts.enabled to false. I gladly volunteer as test subject if i can help in any way. Thank you in advance.
Assignee | ||
Comment 2•10 years ago
|
||
Latest results from Telemetry stats: http://jsfiddle.net/69QNY/2/ For the past few months, using Firefox 24 release data: InitFaceNameLists() takes >5secs 10.5million times InitOtherFamilyNames() takes >5secs 0.9million times data: release/24/FONTLIST_INITFACENAMELISTS Time(ms) spent on reading family names from all fonts index start end count pct cum pct 39 4111 5015 2030923 1.91 90.13 40 5015 6118 2018149 1.90 92.03 41 6118 7463 1897272 1.79 93.82 42 7463 9104 1568345 1.48 95.30 43 9104 11106 1217968 1.15 96.44 44 11106 13548 954160 0.90 97.34 45 13548 16527 733772 0.69 98.03 46 16527 20161 575310 0.54 98.57 47 20161 24593 448556 0.42 98.99 48 24593 30000 348238 0.33 99.32 49 >30000 719780 0.68 100.00 total counts: 106242667 data: release/24/FONTLIST_INITOTHERFAMILYNAMES Time(ms) spent on reading other family names from all fonts index start end count pct cum pct 39 4111 5015 228836 1.76 92.93 40 5015 6118 192489 1.48 94.41 41 6118 7463 161659 1.24 95.65 42 7463 9104 134220 1.03 96.68 43 9104 11106 109671 0.84 97.53 44 11106 13548 88879 0.68 98.21 45 13548 16527 68463 0.53 98.73 46 16527 20161 51638 0.40 99.13 47 20161 24593 37183 0.29 99.42 48 24593 30000 26250 0.20 99.62 49 >30000 49772 0.38 100.00 total counts: 13018817
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to FloRian-R from comment #1) > Background: I have around 12.000 Files in my Font Folder (Windows 7 64 bit, > Firefox 20.0.1). Now, whenever i load a website that has a custom font or a > google API font (examples: g2play.net, surrenderat20.net, about:downthemall) > my Firefox freezes for 2-3 Minutes while loading the website and does not > respond. I temporarely fixed it by setting gfx.downloadable_fonts.enabled to > false. I think this is unrelated but if you're still seeing this problem with downloadable fonts enabled, we should probably log another bug and track down what the problem is.
(In reply to John Daggett (:jtd) from comment #3) > (In reply to FloRian-R from comment #1) > > Background: I have around 12.000 Files in my Font Folder (Windows 7 64 bit, > > Firefox 20.0.1). Now, whenever i load a website that has a custom font or a > > google API font (examples: g2play.net, surrenderat20.net, about:downthemall) > > my Firefox freezes for 2-3 Minutes while loading the website and does not > > respond. I temporarely fixed it by setting gfx.downloadable_fonts.enabled to > > false. > > I think this is unrelated but if you're still seeing this problem with > downloadable fonts enabled, we should probably log another bug and track > down what the problem is. I deleted around 80% of my fonts so it works without hangs at the moment, if it helps i can reinstall them and we can track it down.
Assignee | ||
Comment 5•10 years ago
|
||
Font hang data from bug 859558 https://bugzilla.mozilla.org/show_bug.cgi?id=859558#c5 InitFaceNameLists hangs: 51047 DirectWrite: 34436 GDI: 16611 Almost all of these are within a call to gfxUserFontSet::LoadNext, handling the loading of src: local(xxx) descriptors in @font-face rules.
Assignee | ||
Comment 6•10 years ago
|
||
Right now OSX provides OS functions for looking up <fullname, psname>. Other platforms do not, so the font loader that runs on a timer after startup reads the name table of all fonts to pick up the <fullname, psname>. The LookupLocalFont methods access a hashtable mExtras to map a <fullname, psname> to a font entry. I think as a first pass here, the smart thing to do would be to: 1. Create a runnable that iterates over all font families on the system and reads the name table and comes up with a <fullname> ==> <family> mapping. In the completion event this would be passed back to the platform font list. This list would be strictly limited to string pairs (i.e. no underlying gfxFontEntry/gfxFontFamily usage). I'm proposing something that would be platform-specific and *not* something that uses the methods associated with gfxFontEntry::GetTable. 2. When looking up local names, use the <fullname> ==> <family> mapping. Then look up the family and make sure the individual faces have been read. This will populate the local name lookup table (mExtras) and the existing method of looking up the font entry in the local name lookup table can be used. 3. When a local lookup occurs before the name lookup thread completes, record the name that failed and treat it as "not found" but note that it failed. When the thread completes, restyle any content containing names that were found. With these changes, the logic in LookupLocalFont methods would be more complicated but we could eliminate calls to InitFaceNameLists that cause chrome hangs. There's a clear inefficiency here, namely that by iterating over the tables once, then reading those same tables again for a given family, we've duplicated reading the name table. But that will only occur for the font families referenced via src local lookups, so the impact should be very limited. Keeping this simple allows the off-main-thread name lookup in (1) to be independent of other table handling code and avoid the *very* painful effort of trying to make gfxFontEntry and gfxFontFamily and subclasses thread-safe. In the DirectWrite case, we'll be able to eliminate the use of the font loader and only ever run through all fonts only when cmap font fallback is explicitly enabled. One optimization would be to store the string list in the startup cache and update it when the fontlist changes (stat all font files listed in the registry and store the checksum for the moddates with the fontlist). Last time I looked at this it wasn't really feasible, the startup cache is made for storing one-time startup data, not for data that changes over time.
Assignee | ||
Comment 7•10 years ago
|
||
Instead of using the NSFontManager API, use the CoreText CTFontManager API. In general, I think we need to avoid AppKit API's within off-main-thread code. I fiddled a bit with using the CTFontDescriptor API's to switch over FindStyleVariations but the "trait" info is a mess and mapping it to CSS values is harder than using the NSFontManager API. This is basically the same problem as bug 931426. So I'll probably leave this as is for now.
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8349235 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Comment 10•10 years ago
|
||
Move the gfxFontInfoLoader class over to a separate file so that later diffs are clearer
Assignee | ||
Comment 11•10 years ago
|
||
Set up async loader thread and added loading code for fonts under OSX. Next step is to fix up the finalize process to load in from the data provided by the async loader. After that write the subclasses for FontInfoData for GDI, DirectWrite, Android.
Assignee | ||
Comment 12•10 years ago
|
||
Note that the code here is intended to implement async loading of localized names, fullnames/psnames and cmap data so it should also resolve the other "font hang" related bugs.
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8360919 -
Attachment is obsolete: true
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8360925 -
Attachment is obsolete: true
Assignee | ||
Comment 15•10 years ago
|
||
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8362388 -
Attachment is obsolete: true
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8362389 -
Attachment is obsolete: true
Assignee | ||
Comment 18•10 years ago
|
||
Assignee | ||
Comment 19•10 years ago
|
||
With the currently attached patches, the font loader that runs on desktop platforms has now been completely replaced by an async loader thread. After the async loader has run, the finalize method calls a method to move the loaded data into the gfxFontFamily/gfxFontEntry data. This is done on the main thread in intervals if needed (the OSX cmap loading code has some complexity to it that's hard to completely push into an async thread but this will only affect users with a non-default pref setting). Each one of the OSX/DirectWrite/GDI implementations implements a platform-specific LoadFontFamilyData method that loads all the needed data for a given font family without touching any other data structures in gfx (i.e. it calls only static helper methods and uses only thread-safe platform API's). Upon loader completion, the gfxPlatformFontList::LoadFontInfo method updates gfx data structures with the loaded data. Need to do some more testing/tryserver runs, etc. The final piece for this bug is to implement a timeout in the code for looking up fullname/postscript names and and after the loader completes do a global reflow if and only if a font was found that was looked up previously.
Assignee | ||
Updated•10 years ago
|
Attachment #8360918 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8360921 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8362387 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8362824 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8362825 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8362826 -
Attachment is obsolete: true
Assignee | ||
Comment 20•10 years ago
|
||
Moved async font loader implementation over to bug 962440.
Assignee | ||
Comment 21•10 years ago
|
||
Adds a timeout to the InitFaceNameLists method. The missed name that sparked the initialization is saved. After the font loader thread completes, that name is checked again. If that name can now be found, global reflow is triggered. This doesn't quite work yet, global reflow isn't enough, we need to also get the userfont set to redo the specific face that triggered the local lookup. In a similar vein, I suspect we don't behave correctly when faces are disabled -- the user font set will hold on to outdated font entries. If a user types into an element that uses a local font specified via src: local(blah), then I think we will fail if 'blah' is explicitly disabled and the user then types in the same element. Need to experiment more...
Assignee | ||
Comment 22•10 years ago
|
||
Revised on top of the fix for bug 964613. When looking up font face names before the lists have been fully initialized, first try to load all families starting with the same letter. So "Georgia Bold" will cause all families beginning with 'G' to have their facenames loaded. If that fails, all fonts are enumerated but only for a fixed time slot. If that times out, the lookup fails and the name is added to a set of missing face names. Once the font loader completes, these names are then checked again. If any of those now match, userfont sets are checked (using the code from bug 964613) and pages reflowed. This code will only be used on DirectWrite and GDI backends. Recent versions of DirectWrite provide cached access to the Postscript/fullname for fonts so given the logic above, a timeout that would affect display should almost never occur.
Attachment #8364908 -
Attachment is obsolete: true
Assignee | ||
Comment 23•10 years ago
|
||
Revised to latest trunk code.
Attachment #8403833 -
Attachment is obsolete: true
Attachment #8404447 -
Flags: review?(jfkthame)
Assignee | ||
Comment 24•10 years ago
|
||
Similar approach here to the face name lists. When a localized name lookup needs to happen, try to enumerate font family names within the time allowed. If that doesn't work, fail. After the async font loader completes, reflow to pick up the font that will now match (if such a font exists). Unlike in the facename case, there's no quick and dirty way to target names that might be loaded. If we want to do that we should cache the localized names from last time and use that. But that seems like too much effort for a relatively rarely used feature. This code is limited to XP and OSX. Some users in Japan use the Japanese name for common Japanese fonts, so while we want to support these users to be compatible with IE, don't cause a chrome hang to do it.
Attachment #8404454 -
Flags: review?(jfkthame)
Assignee | ||
Comment 25•10 years ago
|
||
Testcase for localized names. Steps to test: 1. Settings > Preferences > General, select 'Show my windows from last time' 2. Load the test 3. Quit and restart the browser Expected behavior: one of the lines will show initially in a serif face, then snap to a sans-serif face once the async font loader completes. Note: existing trunk behavior will always show the same because we synchronously read all localized font names.
Assignee | ||
Comment 26•10 years ago
|
||
Same steps as the previous test. One of the lines may show in the "wrong" face (sans-serif or serif) depending upon the platform. Under OSX the name will probably always fail, on Windows it may often succeed depending upon how quickly the OS reads through fonts starting with 'c' in the name.
Assignee | ||
Comment 27•10 years ago
|
||
tryserver build: https://tbpl.mozilla.org/?tree=Try&rev=47b0f04eb17b
Comment 28•10 years ago
|
||
Comment on attachment 8404447 [details] [diff] [review] patch, prevent facename list chrome hang Review of attachment 8404447 [details] [diff] [review]: ----------------------------------------------------------------- I found some of the method names here a bit confusing (see below); I'd like us to find ways to make things clearer before we land this. ::: gfx/thebes/gfxPlatformFontList.cpp @@ +224,5 @@ > + bool mFilter; > + > + // if mFilter is true, only load facenames for families that > + // start with the string below > + nsString mFilterStr; IIUC, you only filter on the initial letter. So just store that as a single char16_t. No need for the mFilter bool above, either; use mFilterChar==0 to indicate no filtering. @@ +228,5 @@ > + nsString mFilterStr; > +}; > + > +gfxFontEntry* > +gfxPlatformFontList::InitFaceNameLists(const nsAString& aFontName) This method isn't (just) an Init... method, it also does a lookup. As such, I think it should be named accordingly, including Lookup. @@ +316,5 @@ > + } > + > + // lookup in name lookup tables, return null if not found > + if (!(lookup = LookupLocalFaceNameLists(aFontName))) { > + // names not completely initialized, so keep track of lookup misses Either move the comment inside the if() below, or make the comment text itself express the condition: "if names not...then keep track..." @@ +320,5 @@ > + // names not completely initialized, so keep track of lookup misses > + if (!mFaceNameListsInitialized) { > + mFaceNamesMissed.PutEntry(aFontName); > + } > + return nullptr; Redundant, |lookup| is null anyway if we're here. @@ +959,5 @@ > mFontFamiliesToLoad.Clear(); > mNumFamilies = 0; > > + // if had missed face names that are now available, force reflow all > + bool checkUserFontSets = false; This is redundant... @@ +971,5 @@ > + mFaceNamesMissed.Clear(); > + } > + > + if (checkUserFontSets) { > + CheckUserFontSetLocalFonts(); ...if you simply do this inside the if()-block above in the case where mFoundName is true. ::: gfx/thebes/gfxPlatformFontList.h @@ +246,5 @@ > nsRefPtr<gfxFontFamily>& aFamilyEntry, > void* userArg); > > + // helper method for fullname/postscript name lookup > + gfxFontEntry* LookupLocalFaceNameLists(const nsAString& aFontName) { I don't think this needs to be inlined in the header; move it to the .cpp file. Also, does it need to be public? @@ +261,5 @@ > + } > + > + // look up a font by name, for cases where platform font list > + // maintains explicit mappings of fullname/psname ==> font > + virtual gfxFontEntry* LookupInFaceNameLists(const nsAString& aFontName); It's not clear from either the method names or comments here why we have two methods that both take aFontName as a string and return a gfxFontEntry. Reading the interface here, it looks like they should be equivalent - but they're not. Could we clarify things here somehow, please? @@ +329,5 @@ > }; > nsAutoPtr<ExtraNames> mExtraNames; > > + // face names missed when face name loading takes a long time > + nsTHashtable<nsStringHashKey> mFaceNamesMissed; I suspect that in most cases, for most users, this will never be used (do you have any insight into how commonly it'll be hit?). As such, maybe we should make it a pointer-to-hashtable and only create it on demand.
Assignee | ||
Comment 29•10 years ago
|
||
Revised based on review comments. I cleaned up the names of the various methods and simplified the functionality at bit. Until face name list initialization has completed, only font families with the same first letter as the face name will be enumerated when searching for a face name. On Windows, all standard fonts fit this pattern. This will avoid a delay enumerating all font families looking for a font that's not available on the system. If it is available and it doesn't fit the first letter pattern, the font will get picked up after the font loader runs.
Attachment #8404447 -
Attachment is obsolete: true
Attachment #8404447 -
Flags: review?(jfkthame)
Attachment #8408006 -
Flags: review?(jfkthame)
Assignee | ||
Comment 30•10 years ago
|
||
Revised to clearly label Windows vs. OSX fonts. Use steps described in comment 25. Expected results: under given OS, should see serif face initially, then after the font loader completes (10-15 secs after startup), serif face should show. On Windows, this will only affect Meiryo, MS PGothic will always show correctly.
Attachment #8404455 -
Attachment is obsolete: true
Assignee | ||
Comment 31•10 years ago
|
||
Updated based on changes in facename handling patch.
Attachment #8404454 -
Attachment is obsolete: true
Attachment #8404454 -
Flags: review?(jfkthame)
Attachment #8408028 -
Flags: review?(jfkthame)
Assignee | ||
Comment 32•10 years ago
|
||
review ping...
Comment 33•10 years ago
|
||
Comment on attachment 8408006 [details] [diff] [review] patch v3, prevent facename list chrome hang Review of attachment 8408006 [details] [diff] [review]: ----------------------------------------------------------------- r=me, with a couple minor fixes as noted. ::: gfx/thebes/gfxPlatformFontList.cpp @@ +236,3 @@ > > + // iterate over familes starting with the same letter > + faceNameListsData.mFirstChar = NS_ToLower(aFaceName.CharAt(0)); Shouldn't we use ToLowerCase (from nsUnicharUtils.h) here? NS_ToLower only takes a char argument. @@ +266,5 @@ > { > + ReadFaceNamesData *data = static_cast<ReadFaceNamesData*>(userArg); > + gfxPlatformFontList *fc = data->mFontList; > + > + // when filtering, skip names that don't start with the filter string s/string/character/ @@ +267,5 @@ > + ReadFaceNamesData *data = static_cast<ReadFaceNamesData*>(userArg); > + gfxPlatformFontList *fc = data->mFontList; > + > + // when filtering, skip names that don't start with the filter string > + if (data->mFirstChar && (NS_ToLower(aKey.CharAt(0)) != data->mFirstChar)) { ToLowerCase() @@ +931,5 @@ > mNumFamilies = 0; > + bool rebuilt = false; > + > + // if had missed face names that are now available, force reflow all > + if (NeedFullnamePostscriptNames() && This is redundant - if it's not true, there's no way mFaceNamesMissed will have been set.
Attachment #8408006 -
Flags: review?(jfkthame) → review+
Comment 34•10 years ago
|
||
Comment on attachment 8408028 [details] [diff] [review] patch v2, prevent chrome hang when searching for localized font family names Review of attachment 8408028 [details] [diff] [review]: ----------------------------------------------------------------- r=me, with mOtherNamesMissed as a pointer rather than a direct member of gfxPlatformFontList. ::: gfx/thebes/gfxPlatformFontList.h @@ +317,5 @@ > // face names missed when face name loading takes a long time > nsAutoPtr<nsTHashtable<nsStringHashKey> > mFaceNamesMissed; > > + // localized family names missed when face name loading takes a long time > + nsTHashtable<nsStringHashKey> mOtherNamesMissed; Make this an nsAutoPtr<...etc...>, like mFaceNamesMissed. In most sessions, I'm assuming it won't actually be used.
Attachment #8408028 -
Flags: review?(jfkthame) → review+
Assignee | ||
Comment 35•10 years ago
|
||
Pushed to inbound, with changes based on review comments https://hg.mozilla.org/integration/mozilla-inbound/rev/0c4cbcc72ae4 https://hg.mozilla.org/integration/mozilla-inbound/rev/555db81b5607
Comment 36•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0c4cbcc72ae4 https://hg.mozilla.org/mozilla-central/rev/555db81b5607
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 37•10 years ago
|
||
Uhhh... looking at 0c4cbcc72ae4 as landed, it seems mFirstChar has become a string again. That wasn't what I meant at all - it seems quite unnecessary.
Attachment #8411799 -
Flags: review?(jdaggett)
Assignee | ||
Updated•10 years ago
|
Attachment #8411799 -
Flags: review?(jdaggett) → review+
Comment 38•10 years ago
|
||
Pushed followup: https://hg.mozilla.org/integration/mozilla-inbound/rev/7118f282f343
You need to log in
before you can comment on or make changes to this bug.
Description
•