417444 - load in other font family names lazily

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[John Daggett (:jtd)]

On the Mac, over 95% of the time in InitFontList is spent reading in other font family names, primarily localized family names.  As these aren't needed for most Latin-based languages, we should be able to get away with loading them lazily when the first font family name lookup miss occurs.  This will shift the bulk of the time spent reading these in out of the startup time.

These can be read in gradually using the work done for bug 416946.

Comment 1

[John Daggett (:jtd)]

Attachment: patch, v.0.1, read in other font family names lazily (in progress)

Move the reading in of other font names out of gfxQuartzFontCache::InitFontList and into a separate procedure that is called only when these names are absolutely needed.  The initialization occurs after startup in gfxQuartzFontCache::FindFamily when a font lookup miss occurs.

Comment 2

[John Daggett (:jtd)]

Attachment: list of fonts read in at startup along with fonts containing other family names

Dumped using font info logging enabled with a debug build:

export NSPR_LOG_FILE=fontlist.log
export NSPR_LOG_MODULES=fontInfoLog:5

Data for each face read in at startup, with weight and traits (see NSFontManager docs).  On my system, 554 total font faces, only 62 other family names, 16 of these are Hiragino xxx names.

Comment 3

[John Daggett (:jtd)]

Attachment: patch, v.0.2, read in other font family names lazily

Instead of reading in other font family names (primarily localized names) at startup, defer this until absolutely needed when a lookup miss occurs.  By avoiding this, we shift a large amount of the time spent in InitFontList to when the data is actually needed.  This information is still needed even in cases where no localized names are needed since we can't distinguish between a localized name and a name of a font that doesn't exist (e.g. a Windows font like Arial Unicode MS).

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    PRBool mOtherFamilyNamesInitialized;

PRPackedBool in structs

+    PRBool mOtherFamilyNamesInitialized;

ditto

+    if (mOtherFamilyNamesInitialized) return;

We generally put the "return" on its own line

+    // if other family names exist, iterate through other faces
+    if ([otherFamilyNames count] != 0) {

So the assumption here is that if the "normal" face has only one name, then all faces have only that name? It sounds reasonable, but maybe it could be more clearly documented. I guess we're using it already anyway.

Are there ever enough names that the linear search through the array for duplicate names becomes a problem? Would it be simpler and faster to just pass aOtherFamilyFunctor into ReadOtherFamilyNamesForFace, and have it return true if it found any?

+    // to avoid full search of font name tables, seed the other names table with localized names from 
+    // some of the prefs fonts which are accessed via their localized names.  changes in the pref fonts will only cause
+    // a font lookup miss earlier. this is a simple optimization, it's not required for correctness

Would it make sense to actually iterate through all the pref fonts here? Is that something you're planning to do later?

+    // lookup in other family names list (mostly localized names)
+    if (mOtherFamilyNames.Get(key, &familyEntry)) {
         return familyEntry;
     }
+
+    // name not found and other family names not yet fully initialized so
+    // initialize the rest of the list and try again.  this is done lazily
+    // since reading name table entries is expensive
+    if (!mOtherFamilyNamesInitialized) {
+        InitOtherFamilyNames();
+        if (mOtherFamilyNames.Get(key, &familyEntry)) {
+            return familyEntry;
+        }
+    }

If mOtherFamilyNamesInitialized is false, how can mOtherFamilyNames return anything? Couldn't we simplify this so we check mOtherFamilyNamesInitialized first, call InitOtherFamilyNames if necessary, and then unconditionally lookup mOtherFamilyNames?

+    nsDataHashtable<nsStringHashKey, nsRefPtr<MacOSFamilyEntry> > mOtherFamilyNames;    

Why not nsRefPtrHashtable? Then you don't need these extra assignments to nsRefPtr locals when you put something into the hashtable.

Comment 5

[John Daggett (:jtd)]

(In reply to comment #4)
> Are there ever enough names that the linear search through the array for
> duplicate names becomes a problem? 

The most extreme example would be some instances of Adobe font families like Minion Pro that have a large number of faces with extra family names in each, but this is generally the rare case.  In the majority of cases we're dealing with a single localized family name.  See attached fontinfo listing for examples of other family names that show up.

> +    // to avoid full search of font name tables, seed the other names table
> with localized names from 
> +    // some of the prefs fonts which are accessed via their localized names. 
> changes in the pref fonts will only cause
> +    // a font lookup miss earlier. this is a simple optimization, it's not
> required for correctness
> 
> Would it make sense to actually iterate through all the pref fonts here? Is
> that something you're planning to do later?

Yes, I think that might make sense but the simplest way to do this for now is just to prime mOtherFamilyNames with pref fonts that use localized names.

> +    // name not found and other family names not yet fully initialized so
> +    // initialize the rest of the list and try again.  this is done lazily
> +    // since reading name table entries is expensive
> +    if (!mOtherFamilyNamesInitialized) {
> +        InitOtherFamilyNames();
> +        if (mOtherFamilyNames.Get(key, &familyEntry)) {
> +            return familyEntry;
> +        }
> +    }
> 
> If mOtherFamilyNamesInitialized is false, how can mOtherFamilyNames return
> anything? Couldn't we simplify this so we check mOtherFamilyNamesInitialized
> first, call InitOtherFamilyNames if necessary, and then unconditionally lookup
> mOtherFamilyNames?

mOtherFamilyNamesInitialized is only set to true when the name tables for *all* fonts have been read in.  By explicitly calling ReadOtherFamilyNamesForFamily for a few pref font families that use localized names we avoid the need to read in all name tables until we hit a page that causes a name lookup miss.  This effectively shifts the need to do this work well past startup.

I'll set up a new patch tomorrow with the other changes you suggested.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

One thing to keep in mind is that thanks to Session Restore, it's reasonably common for users to hit a lot of Web pages immediately after startup, some of which could have weird glyphs.

Comment 7

[John Daggett (:jtd)]

(In reply to comment #6)
> One thing to keep in mind is that thanks to Session Restore, it's reasonably
> common for users to hit a lot of Web pages immediately after startup, some of
> which could have weird glyphs.

No doubt.  The main thing here is simply to move the reading of the font name tables out of the startup process, but if session restore is involved then that increases the chance that a font lookup miss will occur, necessitating the reading of font name tables.  Note that weird glyphs are fine, it's weird font names that will necessitate pulling in the font name tables.

Comment 8

[John Daggett (:jtd)]

Attachment: patch, v.0.3, read in other font family names lazily

With changes based on comment #4.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> Are there ever enough names that the linear search through the array for
> duplicate names becomes a problem? Would it be simpler and faster to just pass
> aOtherFamilyFunctor into ReadOtherFamilyNamesForFace, and have it return true
> if it found any?

You don't seem to have addressed this? You're passing in the functor but you're still maintaining and searching otherFamilyNames which I think you don't actually need?

Comment 10

[John Daggett (:jtd)]

(In reply to comment #9)
> > Are there ever enough names that the linear search through the array for
> > duplicate names becomes a problem? Would it be simpler and faster to just pass
> > aOtherFamilyFunctor into ReadOtherFamilyNamesForFace, and have it return true
> > if it found any?
> 
> You don't seem to have addressed this? You're passing in the functor but you're
> still maintaining and searching otherFamilyNames which I think you don't
> actually need?

Passing the otherFamilyNames array is needed to avoid adding the same other family name in multiple times.  Consider Hiragino Kaku Gothic Pro as an example, there are two faces with different weights.  Each contains *both* "Hiragino Kaku Gothic Pro" and "ヒラギノ角ゴ Pro", the same name in Japanese.  Without keeping the otherFamilyNames array around, you'd be adding the Japanese name in twice when ReadOtherFamilyNamesForFace was called for each of the faces.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What's wrong with that? If you want the first face added to win in mOtherFamilyNames, just do a Get before you do a Put and don't Put if there was already an entry for that name.

Comment 12

[John Daggett (:jtd)]

Attachment: patch, v.0.4, read in other font family names lazily

Eliminated the otherFamilyNames array.  I didn't add the code in call ReadOtherFamilyNamesForFamily with family names supplied by a pref, that's lower priority work right now.  There's some pref handling work that needs to get done for 390901, so I think I'll work on including the code in a patch for that bug.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 304407 [details] [diff] [review]
patch, v.0.4, read in other font family names lazily

This could be sped up more in two ways:

-- gfxQuartzFontCache::AddOtherFamilyName could take an NSString* parameter for aOtherFamilyName, which we convert to an nsAutoString just once, lowercasing it at the same time.

-- Using nsTHashtable you could avoid the need to do a Get and a Put in the same function. You would just do a Put to get the current entry for the key and bail out if it was already occupied.

Only worth worrying about if those code points show up in profiles.

Comment 14

[John Daggett (:jtd)]

Makes sense.  Name tables are fairly sparse, most fonts don't have localized family names so adding names isn't the time consuming part of this process (although many fonts localize the *full* name to include the localization of style descriptions like Bold).

Checked in.

Comment 15

[Mike Schroepfer]

There was 1 cycle with this in - a 4% Ts win.  

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

The relanding shaved 1000ms (6.67%) off of Ts on boxset, Camino's perf box, so folks with older/slower/PPC machines should see a slightly nicer startup win :D

Comment 17

[John Daggett (:jtd)]

(In reply to comment #16)
> The relanding shaved 1000ms (6.67%) off of Ts on boxset, Camino's perf box, so
> folks with older/slower/PPC machines should see a slightly nicer startup win :D

Even more so for those folks with a large number of fonts on their machine!