Closed Bug 623136 Opened 9 years ago Closed 9 years ago

speed up font loading on android

Categories

(Core :: Graphics, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: blassey, Assigned: blassey)

References

Details

(Keywords: perf)

Attachments

(1 file, 10 obsolete files)

We spend 100ms loading fonts at startup on android. Seems like we could cache that data to speed it up.
tracking-fennec: --- → ?
blocking2.0: --- → ?
tracking-fennec: ? → 2.0+
blocking2.0: ? → ---
tracking-fennec: 2.0+ → ?
Blocks: 622908
OS: Linux → Android
Hardware: x86_64 → ARM
Keywords: perf
John, I heard you had some font caching magic in the works. Is any of that applicable and will it be landing in the 2.0 time frame?
The cmap caching I'm working on is related but slightly different.  What
you're looking for is caching the family name list and better lazy
initialization for individual font families.

I assume the 100ms is spent here in gfxAndroidPlatform::UpdateFontList:

DIR *d = opendir("/system/fonts");
struct dirent *ent = NULL;
while(d && (ent = readdir(d)) != NULL) {
    int namelen = strlen(ent->d_name);
    if (namelen > 4 &&
        strcasecmp(ent->d_name + namelen - 4, ".ttf") == 0)
    {
        nsCString s("/system/fonts");
        s.Append("/");
        s.Append(nsDependentCString(ent->d_name));

        LOG("Font: %s", nsPromiseFlatCString(s).get());

        AppendFacesFromFontFile(nsPromiseFlatCString(s).get());
    }
}

The AppendFacesFromFontFile function loads in all faces from a file
although I would imagine almost all the fonts are .ttf files with only a
single face (only .ttc files will have more than one):  

FT_Face dummy;
if (FT_Err_Ok == FT_New_Face(GetFTLibrary(), fileName, -1, &dummy)) {
    for (FT_Long i = 0; i < dummy->num_faces; i++) {
        FT_Face face;
        if (FT_Err_Ok != FT_New_Face(GetFTLibrary(), fileName, 
                                     i, &face))
            continue;

        FontEntry* fe = FontEntry::CreateFontEntryFromFace(face);
        if (fe) {
            LOG("font family: %s", face->family_name);

            NS_ConvertUTF8toUTF16 name(face->family_name);
            ToLowerCase(name);

            nsRefPtr<FontFamily> ff;
            if (!mFonts.Get(name, &ff)) {
                ff = new FontFamily(name);
                mFonts.Put(name, ff);
            }

            ff->AddFontEntry(fe);
            ff->SetHasStyles(PR_TRUE);
        }
    }
    FT_Done_Face(dummy);
}

Both Windows/OSX platform font list code takes a two-step approach, read
in the set of font families at app init time and then lazy initialize
each family as needed (see FindStyleVariations).  The cmap loader that
runs on a delayed timer will also load the details of all the faces in a
given family.  Note that in the Android case the cmap loader never runs,
not sure what happens for the system font fallback case.

Since we're not relying on system font API's (do they exist in the
latest Android?) we should cache the family list the first time, then
stat the files to see if the fonts changed (probably never will).  Then
startup would only consist of reading the cached family name/style list
and the stat check, that should be significantly faster.

I'd love to take this on but I'm a bit swamped now with dwrite crap.  I
could probably take a look in February if that's still helpful.
Attached patch WIP (obsolete) — Splinter Review
this gets us down to 22ms on the i9000
Assignee: nobody → blassey.bugs
class gfxFontFamily {

  // find faces belonging to this family (platform implementations override this;
  virtual void FindStyleVariations() { }
Attached patch patch (obsolete) — Splinter Review
this gets us down to 2-4ms on my i9000
Attachment #503077 - Attachment is obsolete: true
Attachment #503094 - Flags: review?(jdaggett)
Comment on attachment 503094 [details] [diff] [review]
patch

(In reply to comment #5)
> Created attachment 503094 [details] [diff] [review]
> patch
> 
> this gets us down to 2-4ms on my i9000

Does this actually function correctly?  Do you actually get families with regular/bold/italic?  A font family contains a set of faces, usually with one face per font file.  So having mFace in FontFamily makes no sense, a face should be associated with a FontEntry.  Are you testing with some .ttc files to verify that you're code functions correctly for font files with more than one face?  Simple .ttf files have only one face.

I also really, really, really dislike the idea of using prefs code as a global cache, which is basically what you're doing.  Isn't there other code lying around that does chrome/content IPC the right way that you could crib from?  Maybe stuart or vlad know the right answer here?

Additionally, to cache this info correctly you need to check that the cache is still valid, which means taking the entire list of font files with associated file mod dates and sizes and verifying that it matches the previous state.

  if (cached fontlist exists)
    verify no change
    reconstitute fontlist
  else
    build fontlist from files in font folder
    store fontlist with associated dates/sizes

Your Init() function in FontFamily is duplicating the purpose of the virtual method FindStyleVariations() in gfxFontFamily.  Mimic the logic of Windows/OSX font list code.
Attachment #503094 - Flags: review?(jdaggett) → review-
(In reply to comment #6)
> Comment on attachment 503094 [details] [diff] [review]
> patch
> 
> (In reply to comment #5)
> > Created attachment 503094 [details] [diff] [review]
> > patch
> > 
> > this gets us down to 2-4ms on my i9000
> 
> Does this actually function correctly?  Do you actually get families with
> regular/bold/italic?  A font family contains a set of faces, usually with one
> face per font file.  So having mFace in FontFamily makes no sense, a face
> should be associated with a FontEntry.  Are you testing with some .ttc files to
> verify that you're code functions correctly for font files with more than one
> face?  Simple .ttf files have only one face.

There are only .ttf files on this device, no .ttc files. But you're right, the logic isn't quire right there.
> 
> I also really, really, really dislike the idea of using prefs code as a global
> cache, which is basically what you're doing.  Isn't there other code lying
> around that does chrome/content IPC the right way that you could crib from? 
> Maybe stuart or vlad know the right answer here?

I can certainly do it without using the prefs system, but this isn't a new use of the prefs system. Also, creating fewer, bug bigger files is better for perf
> 
> Additionally, to cache this info correctly you need to check that the cache is
> still valid, which means taking the entire list of font files with associated
> file mod dates and sizes and verifying that it matches the previous state.
> 
>   if (cached fontlist exists)
>     verify no change
>     reconstitute fontlist
>   else
>     build fontlist from files in font folder
>     store fontlist with associated dates/sizes
> 
> Your Init() function in FontFamily is duplicating the purpose of the virtual
> method FindStyleVariations() in gfxFontFamily.  Mimic the logic of Windows/OSX
> font list code.

yup, need to stat the files
Attached patch patch (obsolete) — Splinter Review
I believe this is functionally correct in that it handles multiple faces per font family and stats the font files. Its still using the prefs system to store the cache.
Attachment #503094 - Attachment is obsolete: true
Attachment #503404 - Flags: review?(jdaggett)
The startup cache is another option for storing that data. It's pretty much designed for caching data like this.
yeah, cache sounds right.
Attached patch WIP patch (obsolete) — Splinter Review
this patch has an implementation of using both the prefs system and the startup cache, but the startup cache impl doesn't work. Neither the facelist nor the timestamp get a value from the startup cache.

John, I'd like to get your feedback/review on the font specific stuff. Obviously if we're going to use the startup cache that needs more work.
Attachment #503404 - Attachment is obsolete: true
Attachment #503427 - Flags: feedback?(jdaggett)
Attachment #503404 - Flags: review?(jdaggett)
Attached patch patch (obsolete) — Splinter Review
It looks like the startup cache isn't available to the child process. Given that, storing this info in the prefs seems to make the most sense.
Attachment #503427 - Attachment is obsolete: true
Attachment #504356 - Flags: review?(jdaggett)
Attachment #503427 - Flags: feedback?(jdaggett)
tracking-fennec: ? → 2.0b4+
is the location to the startup cache not available ( you don't know what the file path is in the child ) or do not have read permissions to that location?
(In reply to comment #13)
> is the location to the startup cache not available ( you don't know what the
> file path is in the child ) or do not have read permissions to that location?

yup, see the discussion in bug 626814
Comment on attachment 504356 [details] [diff] [review]
patch

We should absolutely not be using prefs in place of setting up an IPC calls to obtain a copy of a shared font list or a shared font name cache (we already have other things called "font caches").  I think that means setting up a need IPDL interface and implementing the code on both ends.

bsmedberg, does that sound right?
s /need/new/
(In reply to comment #15)
> Comment on attachment 504356 [details] [diff] [review]
> patch
> 
> We should absolutely not be using prefs in place of setting up an IPC calls to
> obtain a copy of a shared font list or a shared font name cache (we already
> have other things called "font caches").  I think that means setting up a need
> IPDL interface and implementing the code on both ends.
> 
> bsmedberg, does that sound right?

John, we have bug 626814 for making the startup cache work in the content process. Could you please just review the font changes? Since the patch has an abstraction for the FontCache, it'll be simple to drop in the StartupCache backed font cache when bug 626814 is fixed.
+class FontCache {

We already have other things called font caches (e.g. gfxFontCache) for keeping around metrics, you're setting up a font family name cache here.

FontNameCache?

+class PrefFontCache : public FontCache {

See comment 15.

+    GetFacesForFile(const char* aFileName, char** aFaceList) {
+        nsCAutoString prefName("font.cache.");
+        prefName.Append( aFileName);
+        prefs->GetCharPref(prefName.get(), aFaceList);
+
+    }

I think you should use normal strings here, since family names elsewhere are
stored as normal strings, not C-strings.  It looks like this is a side effect of
using FreeType which stores an English name.  OpenType fonts can have a set
of family names and these are used typically for CJK fonts.  I don't think we need to worry about getting the localized names out right now but at least we should not propogate limitations like this into our code.

+    PRInt32 timestamp = 0;

Should be PRUint32.

+    if (aFontCache) {
+        aFontCache->GetFacesForFile(fileName, getter_Copies(faceList));
+        aFontCache->GetTimeStampForFile(fileName, &timestamp);
+    }
+
+    struct stat s;
+    if (!faceList.IsEmpty() && !stat(fileName, &s) && s.st_mtime == timestamp) 

I think you should store/compare the size also, since stat returns that also.

+void
+FontFamily::FindStyleVariations()
+{

Check mHasStyles at the start, then set to true.  For an example,
see the DirectWrite code:

> gfxDWriteFontFamily::FindStyleVariations()
> {
>     HRESULT hr;
>     if (mHasStyles) {
>         return;
>     }
>     mHasStyles = PR_TRUE;

+    PRUint32 i = mFilenames.Length();
+    while (i > 0) {
+        i--;
+        FT_Face face;
+        gfxAndroidPlatform* platform = gfxToolkitPlatform::GetPlatform();
+        if (FT_Err_Ok == FT_New_Face(platform->GetFTLibrary(),
+                                     mFilenames[i].filename.get(), 
+                                     mFilenames[i].index, &face))
+            AddFontFace(face);
+        mFilenames.RemoveElementAt(i);
+    }

Seems like a simple for loop with mFilenames.Clear() at the end would be simpler.

+    i = mFaces.Length();
+    while (i > 0) {
+        i--;
+        if (mFaces[i]) {
+            FontEntry* fe = FontEntry::CreateFontEntryFromFace(mFaces[i]);
+            if (fe) {
+                AddFontEntry(fe);
+                SetHasStyles(PR_TRUE);
+            }
+        }
+        mFaces.RemoveElementAt(i);
+    }

Same here.

>  PRBool
>  FontFamily::FindWeightsForStyle(gfxFontEntry* aFontsForWeights[],
>                                  PRBool anItalic, PRInt16 aStretch)
>  {
> +    if (!mFaces.IsEmpty() || !mFilenames.IsEmpty())
> +        FindStyleVariations();
>      PRBool matchesSomething = PR_FALSE;

If mHasStyles is set correctly, there's no need to override this method.  This code should just be trimmed out.

Additionally, I think you should stick a comment somewhere to explain the logic you're using, specifically the role that mFilenames, mFaces play in relation to the list of font entries for a given family.  Seems like an important goal here is to assure that a font family that's never used wastes the least amount of space possible.

I think you should definitely test with some form of font file with a non-ASCII name to verify that the code here works correctly.  I know that Docomo and AU ship an Android phone in Japan that includes a number of Japanese fonts from Morisawa.  It currently ships with Android 1.6 (WTF?) but I'm sure that will probably change in the near future.
we should also get bsmedberg to sign off on the short-term approach.  not sure how much work it is to do-it-right.
Attached patch patch (obsolete) — Splinter Review
Attachment #504356 - Attachment is obsolete: true
Attachment #505834 - Flags: review?(jdaggett)
Attachment #504356 - Flags: review?(jdaggett)
this requires bug 626814 to get the cache in the content process. Also, as noted in bug 626814 the ipc overhead slows down the cache in the content process.
Depends on: 626814
tracking-fennec: 2.0b4+ → 2.0+
(In reply to comment #21)
> this requires bug 626814 to get the cache in the content process. Also, as
> noted in bug 626814 the ipc overhead slows down the cache in the content
> process.

I think given the ipc overhead and that fixing it would involve tweaking startup cache design late in ff4 cycle. I also vote for the short term approach for now + a followup bug for later. If storing in prefs isn't ideal, use a separate file for now.
(In reply to comment #22)
> I think given the ipc overhead and that fixing it would involve tweaking
> startup cache design late in ff4 cycle. I also vote for the short term approach
> for now + a followup bug for later. If storing in prefs isn't ideal, use a
> separate file for now.

Given that I'd prefer just to use the existing pref system.
(In reply to comment #23)
> (In reply to comment #22)
> > I think given the ipc overhead and that fixing it would involve tweaking
> > startup cache design late in ff4 cycle. I also vote for the short term approach
> > for now + a followup bug for later. If storing in prefs isn't ideal, use a
> > separate file for now.
> 
> Given that I'd prefer just to use the existing pref system.

Doesn't the pref version carry the same IPC overhead?
(In reply to comment #24)
> 
> Doesn't the pref version carry the same IPC overhead?

No, implementing something like the prefs system to avoid the ipc over head is what taras is talking about.
(In reply to comment #21)
> this requires bug 626814 to get the cache in the content process. Also, as
> noted in bug 626814 the ipc overhead slows down the cache in the content
> process.

No, this is completely the wrong approach.  Prefs are supplied to
the content process via an IPC call and what I'm proposing is
that you do the same thing for the system fontlist.  I chatted
with cjones on IRC today and I think the right approach is this:

  chrome process InitFontList:
    - read serialized fontlist with associated mod dates and file
      sizes from startup cache
    - iterate over fonts in /system/fonts and verify, adjust
      list as needed
  
  content process InitFontList:
    fetch the font list via an IPC call to the chrome process
  
To do this, mimic the use of ReadPrefsArray in the prefs service code:
http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/nsPrefService.cpp#144

Within the IPDL definition of PContent add:
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PContent.ipdl#145

struct FontListEntry
{
  nsCString familyName;
  nsCString filepath;
};

sync ReadFontList() returns (FontListEntry[] retValue);

In the implementation of ContentParent, add:
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#277

bool
ContentParent::RecvReadFontList(nsTArray<FontListEntry> *fontlist)

This method calls through to gfx code to retrieve a copy of the fontlist.

Within the implementation of InitFontList, add code something like:
http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/nsPrefService.cpp#144

#ifdef MOZ_IPC
    if (XRE_GetProcessType() == GeckoProcessType_Content) {
        nsTArray<FontListEntry> fontlist;
        ContentChild::GetSingleton()->SendReadFontList(&fontlist);

        // iterate over the list and populate mFonts
        // no need to scan font folder

        return NS_OK;
    }
#endif

For the non-content case, the InitFontList code does this:

struct CachedFontListEntry {
    FontListEntry entry;
    PRUint32      modTime;
    PRUint32      fileSize;
}

if (startup cache contains cached font entries) {
  read cached font entry blob from startup cache and deserialize it
}

for each file in /system/fonts

    ... construct font list, using cached data if possible ...

if (fontlist different compared to cached version) {
  serialize font list to font entry blob and write to startup cache
}

With this logic, you only iterate over the font folder once per
browser run and you'll only need to create FT faces the first
time, after that you can do everything on demand.  Only the
chrome process talks to the startup cache in this scenario.

I think something like this is what we'll need to do for other 
platforms in the future.
Attachment #505834 - Flags: review?(jdaggett) → review-
Note that the logic above doesn't handle localized names (fonts can have multiple localized family names) and we need to deal with fontlist updates on non-mobile platforms.  But I don't think it's necessary to address those issues for this bug.
I think for the future what we'll need is a way to share the font list among all processes via shared memory. Making a separate copy of it in each content process sounds like a bad idea.
roc, and preferences!
Storing the font list in shmem probably isn't a great idea unless the list is really really big and immutable.
Or unless it's really really really big.
(In reply to comment #28)
> I think for the future what we'll need is a way to share the font list among
> all processes via shared memory. Making a separate copy of it in each content
> process sounds like a bad idea.

The font list here is simply a list of family names, probably not more than a few K in size.  I don't think it will be more than 500bytes on Android.

Caching/sharing font metrics would be a much more worthy subject for investigation, since those are much larger and we cache one *per font size*.
OK, but there's also the CMAP data.
Attached patch patch (obsolete) — Splinter Review
this uses a file in the android specified cache directory so both the chrome and content process have access to it.
Attachment #505834 - Attachment is obsolete: true
Attachment #507892 - Flags: review?
Attachment #507892 - Flags: review? → review?(jdaggett)
Comment on attachment 507892 [details] [diff] [review]
patch

I don't see why this is a better approach than the one outlined in comment 26, seems like you're writing a lot of code to avoid a simple IPC call.

Unless there's something fundamentally wrong with the approach in comment 26, I would prefer to implement that because I think it's something we could use on all platforms in future versions of Gecko.  The code you've written doesn't seem simpler nor does it seem like it's going to perform any better.
Attached patch patch (obsolete) — Splinter Review
Attachment #507892 - Attachment is obsolete: true
Attachment #509346 - Flags: review?(jdaggett)
Attachment #507892 - Flags: review?(jdaggett)
Comment on attachment 509346 [details] [diff] [review]
patch

Overall this is much better, a little bit of restructuring and I
think this will be fine.

> +	PFontListEntry.h \
>  	$(NULL)
>  
>  ifdef MOZ_IPC
> diff --git a/gfx/thebes/PFontListEntry.h b/gfx/thebes/PFontListEntry.h
> new file mode 100644
> --- /dev/null
> +++ b/gfx/thebes/PFontListEntry.h

You don't need to define the FontListEntry explicitly, simply
define it as a struct in PContent.ipdl:

struct FontListEntry {
    nsCString familyName;
    nsCString filepath;
    PRUint32  index;
};

This will automatically generate all the Read/Write methods.  You'll need to add
PContent.h as an include.

+#include "mozilla/dom/ContentChild.h"

+#include "nsXULAppAPI.h"

These need to be wrapped in #ifdef MOZ_IPC.  I know Android only
builds with IPC enabled but I'd prefer to keep code within gfx
consistent so that we don't break non-libxul builds for folks
building on Windows.

+    virtual void
+    CacheFileInfo(nsCString& aFileName, nsAString& aFaceList,
+                  PRUint32 aTimestamp, PRUint32 aFileSize)
+    {
+        nsCAutoString prefName("font.cache.");
+        prefName.Append( aFileName);
+        nsCAutoString timePrefName("font.cache.timestamp.");
+        timePrefName.Append(aFileName);
+        nsCAutoString sizePrefName("font.cache.filesize.");
+        sizePrefName.Append(aFileName);
+        const PRUnichar* data;
+        PRUint32 size = NS_StringGetData(aFaceList, &data);
+        mCache->PutBuffer(prefName.get(), (char*)data, size * sizeof(PRUnichar) / sizeof(char));
+        mCache->PutBuffer(timePrefName.get(), (char*)&aTimestamp, 4);
+        mCache->PutBuffer(sizePrefName.get(), (char*)&aFileSize, 4);
+    }

I think you're using the startup cache at a granularity that's
way too fine.  Blobs that you give to the startup cache end up as
zipfile entries, I don't think it makes sense to store each
individual font list entry as a zipfile entry.  Instead,
serialize the array of familyname/filename/index/modtime/filesize
values into a blob and store it as a single entry in the startup
cache.  You'll get much better compression/efficiency that way. 
When using it, read it back in and make a hashtable for use while
running through the fonts in the fonts folder (or something
simpler if we know that the number of fonts is always small). 
Write the entire list of fonts back out to the startup cache in
one operation.

+inline PRUint32 
+ParseInt(nsString string, PRUint32 start, PRUint32 end) {
+    PRUint32 val = 0;
+    for (int i = end; i >= start; i--) {
+        PRUnichar c = string[i];
+        if (c >= '0' && c <= '9') {
+            val *= 10;
+            val += string[i] - '0';
+        }
+    }
+    return val;
+}

Shouldn't need to serialize/deserialize ints.  And if it was
needed, using strtol/strtoul makes more sense.


+    if (XRE_GetProcessType() != GeckoProcessType_Default) {
+        mozilla::dom::ContentChild::GetSingleton()->SendReadFontList(retValue);
+        return;
+    }

Wrap with #ifdef MOZ_IPC.

+FontFamily::AddFontFace(FT_Face aFace)
+{
+    SetHasStyles(PR_FALSE);
+    mFaces.AppendElement(aFace);
+}
+
+void FontFamily::AddFontFileAndIndex(nsCString aFilename, PRUint32 aIndex)
+{
+    SetHasStyles(PR_FALSE);
+    mFilenames.AppendElement(new FileAndIndex(aFilename, aIndex));
+}
+    
+
+
+void
+FontFamily::FindStyleVariations()
+{
+    if (mHasStyles) {
+        return;
+    }
+    mHasStyles = PR_TRUE;
+
+    for (int i = 0; i < mFilenames.Length(); i++) {
+        FT_Face face;
+        gfxAndroidPlatform* platform = gfxToolkitPlatform::GetPlatform();
+        if (FT_Err_Ok == FT_New_Face(platform->GetFTLibrary(),
+                                     mFilenames[i].filename.get(), 
+                                     mFilenames[i].index, &face))
+            AddFontFace(face);
+    }
+    mFilenames.Clear();
+
+    for (int i = 0; i < mFaces.Length(); i++) {
+        if (mFaces[i]) {
+            FontEntry* fe = FontEntry::CreateFontEntryFromFace(mFaces[i]);
+            if (fe) {
+                AddFontEntry(fe);
+                SetHasStyles(PR_TRUE);
+            }
+        }
+    }
+    mFaces.Clear();
+    SetHasStyles(PR_TRUE);
+}
+

I don't see the reason for mFaces and the need for two loops
here.  Iterate over mFilenames, create a face, then pass it to
AddFontEntry.  Am I just missing something that requires this
code structure?

>  FontEntry *
>  FontFamily::FindFontEntry(const gfxFontStyle& aFontStyle)
>  {
> +    FindStyleVariations();
>      PRBool needsBold = PR_FALSE;
>      return static_cast<FontEntry*>(FindFontForStyle(aFontStyle, needsBold));
>  }

FindFontForStyle already includes this logic, remove.


>  PRBool
>  FontFamily::FindWeightsForStyle(gfxFontEntry* aFontsForWeights[],
>                                  PRBool anItalic, PRInt16 aStretch)
>  {
> +    FindStyleVariations();
>      PRBool matchesSomething = PR_FALSE;

Delete this method entirely, the base class already defines it.

+typedef struct {
+        nsCString familyName;
+        nsCString filepath;
+        PRUint32  index;
+} FontListEntry;

As noted above, this isn't needed, definition in the .ipdl file
will generate this automatically.
Attachment #509346 - Flags: review?(jdaggett) → review-
> I don't see the reason for mFaces and the need for two loops
> here.  Iterate over mFilenames, create a face, then pass it to
> AddFontEntry.  Am I just missing something that requires this
> code structure?

mFaces allows us to reuse the faces we create in gfxAndroidPlatform when we miss the in the cache, which will happen after every nightly update since we're string the startup cache
(In reply to comment #40)
> > I don't see the reason for mFaces and the need for two loops
> > here.  Iterate over mFilenames, create a face, then pass it to
> > AddFontEntry.  Am I just missing something that requires this
> > code structure?
> 
> mFaces allows us to reuse the faces we create in gfxAndroidPlatform when we
> miss the in the cache, which will happen after every nightly update since we're
> string the startup cache

Hmmm, the only place faces are ever added to mFaces is within the first loop in FindStyleVariations.  As such, it's completely local.  I don't see the logic that implements what you're suggesting it does.
you're right, this version of the patch doesn't do that
Attached patch WIP patch (obsolete) — Splinter Review
this is just a checkpoint, addresses all of John's comments except for storing all fonts in the cache as a single blob
Attachment #509346 - Attachment is obsolete: true
Attachment #511306 - Attachment is obsolete: true
Comment on attachment 511519 [details] [diff] [review]
WIP patch

> diff --git a/gfx/thebes/PFontListEntry.h b/gfx/thebes/PFontListEntry.h
> new file mode 100644
> --- /dev/null
> +++ b/gfx/thebes/PFontListEntry.h

Don't need this file, trim.

+    static InfallibleTArray<FontListEntry> sFontList;

Hmmm, the platform object is a singleton, no need for a static here.
Attached patch patchSplinter Review
Attachment #511519 - Attachment is obsolete: true
Attachment #513351 - Flags: review?(jdaggett)
Comment on attachment 513351 [details] [diff] [review]
patch

>+        LOG("got: %s from the cache", nsDependentCString(buf, size).get());
missed this logging statement, it should be removed
Comment on attachment 513351 [details] [diff] [review]
patch

Looks good.  Couple nits, please fix before landing:

+    InfallibleTArray<FontListEntry> sFontList;

This should be mFontList since it's member variable not a static.

+            timestamp = strtol(entry, &endptr, 10);
+            fileSize = strtol(entry, &endptr, 10);

These should be strtoul I think, given that those are unsigned ints.
Attachment #513351 - Flags: review?(jdaggett) → review+
pushed http://hg.mozilla.org/mozilla-central/rev/ad6b7107bbe6
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.