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)

defect
Not set
normal

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
Blocks: 859558
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.
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
(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.
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.
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.
Depends on: 947812
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.
Attachment #8349235 - Attachment is obsolete: true
Move the gfxFontInfoLoader class over to a separate file so that later diffs are clearer
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.
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.
Attachment #8360919 - Attachment is obsolete: true
Attachment #8360925 - Attachment is obsolete: true
Attachment #8362388 - Attachment is obsolete: true
Attachment #8362389 - Attachment is obsolete: true
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.
Depends on: 962440
Attachment #8360918 - Attachment is obsolete: true
Attachment #8360921 - Attachment is obsolete: true
Attachment #8362387 - Attachment is obsolete: true
Attachment #8362824 - Attachment is obsolete: true
Attachment #8362825 - Attachment is obsolete: true
Attachment #8362826 - Attachment is obsolete: true
Moved async font loader implementation over to bug 962440.
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...
Depends on: 964613
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
Revised to latest trunk code.
Attachment #8403833 - Attachment is obsolete: true
Attachment #8404447 - Flags: review?(jfkthame)
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)
Attached file testcase, localized name test (obsolete) —
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.
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.
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.
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)
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
Updated based on changes in facename handling patch.
Attachment #8404454 - Attachment is obsolete: true
Attachment #8404454 - Flags: review?(jfkthame)
Attachment #8408028 - Flags: review?(jfkthame)
review ping...
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 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+
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
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)
Attachment #8411799 - Flags: review?(jdaggett) → review+
Depends on: 1368531
You need to log in before you can comment on or make changes to this bug.