688125 - Add memory reporters for thebes font objects

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - report memory used for the platform font list (gfxFontFamily and gfxFontEntry objects, including char-maps)

We can use a significant amount of memory managing the list of available fonts, character coverage, etc. This should be shown in about:memory.

Comment 1

[Jonathan Kew [:jfkthame]]

Comment on attachment 561421 [details] [diff] [review]
part 1 - report memory used for the platform font list (gfxFontFamily and gfxFontEntry objects, including char-maps)

This is a first approximation to reporting memory used for the font-list and character maps. I'm sure it's not precisely accurate (e.g. I haven't looked into the exact overhead of various hashtables, etc), but it provides at least some idea of what we're using.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - report "live" font instances from the gfxFontCache

Report size of gfxFont objects found in the global font cache. This is usually a fairly modest amount of memory, but can grow dramatically if many different styles and sizes are in use at once.

The reporting here is incomplete in that it doesn't account for the memory used by the cairo objects that end up getting created. (Those sizes are not readily available outside cairo itself, AFAICS.)

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - report memory used for the platform font list (gfxFontFamily and gfxFontEntry objects, including char-maps)

Updated to resolve crash when using GDI fonts.

Comment 4

[Justin Lebar (not reading bugmail)]

> +gfxPlatformFontList::ComputeSize()
> +{
> +    PRUint32 total = GetObjectSize();
> +
> +    mFontFamilies.EnumerateRead(CollectFamilyNameSizes, &total);
> +    mFontFamilies.EnumerateRead(CollectFamilySizes, &total);

To drive by: It doesn't look like this function accounts for the size of the hashtables in excess of the objects stored in the hashtable.  (That is, a hashtable storing 100 elements might have space for 256 elements.)

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Justin Lebar [:jlebar] from comment #4)
> To drive by: It doesn't look like this function accounts for the size of the
> hashtables in excess of the objects stored in the hashtable.  (That is, a
> hashtable storing 100 elements might have space for 256 elements.)

This is true (mentioned in comment #1, actually). Afaics, most of our hashtables don't expose this info (do they?) - we have nsTHashtable<>::Count() but not CurrentCapacity(). We could presumably add such an API if we want to have more complete accounting here.

Comment 6

[Justin Lebar (not reading bugmail)]

> We could presumably add such an API if we want to have more complete accounting here.

Indeed, I think we should.  I don't know if it's important for this bug; how big does the hashtable get?  AFAICT, PLDHash doubles its size when it's 75% loaded, so you're potentially wasting 1 - (.75 * 2) = 50% of the hashtable.

The nsTHashtable size code is in bug 682437 if you need it.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - add memory reporting for the platform font list

Updated to account for the size of the hashtables, including space for unused entries, as per Justin's comment. (This depends on bug 682437 to provide nsTHashtable::SizeOf etc.)

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - add memory reporting for fonts in the gfxFontCache

Minor refresh to harmonize with updated part 1.

Comment 9

[Jonathan Kew [:jfkthame]]

review ping...?

Comment 10

[Marco Castelluccio [:marco]]

Anyone could review this? It's a P2 bug.

Comment 11

[Jet Villegas (inactive)]

Changes for bug 703100 will affect these patches.

Comment 12

[Nicholas Nethercote [inactive]]

The changes from bug 698968 will affect things too.  jdaggett, any chance you could review these patches?  It's been over two months since the review request was made.

Comment 13

[John Daggett (:jtd)]

Jonathan, the patches need refreshing for the PRBool ==> bool switch.  If you post new patches today, I'll do the review tomorrow.

What do the numbers look like?  On OSX/Windows what are the breakdowns of cmap vs. font list data?

Comment 14

[John Daggett (:jtd)]

Comment on attachment 562713 [details] [diff] [review]
part 1 - add memory reporting for the platform font list

+NS_MEMORY_REPORTER_IMPLEMENT(
+    FontList,
+    "explicit/graphics/fonts/font-list",
+    KIND_HEAP,
+    UNITS_BYTES,
+    GetFontListSize,
+    "Memory used to manage the list of installed font families and faces "
+    "for CSS font matching.")

+NS_MEMORY_REPORTER_IMPLEMENT(
+    CharMaps,
+    "explicit/graphics/fonts/character-maps",
+    KIND_HEAP,
+    UNITS_BYTES,
+    GetCharMapsSize,
+     "Memory used to record the character coverage of installed fonts.")

This is setting up two memory usage numbers, one for cmap bitvector data, the other for everything else.  I think it's important to call out the table data explicitly:

fonts/character-maps
fonts/table-data (i.e. FontTableHashEntry::CollectTableSize)
fonts/other

The gfxDWriteFontEntry class has two COM objects that aren't being measured:

    nsRefPtr<IDWriteFont> mFont;
    nsRefPtr<IDWriteFontFile> mFontFile;

Not sure how you do this but I'm sure those have non-trivial memory impacts
associated with them.

Rather than have two separate reporters that each enumerate the font
list, I think it would make sense to instead use the
nsIMemoryMultiReporter interface.  Then you could collect all data in
one pass.  The code would be simpler to read that way also.

The MemoryReporter class in nsPresShell seems like a good model:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.h#901
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#625

+    mFontFamilies.EnumerateRead(CollectFamilyNameSizes, &total);
+    mFontFamilies.EnumerateRead(CollectFamilySizes, &total);

This doesn't make much sense to me, use a single pass.  If there are
multiple values to collect, use a struct instead of a single int.

I think the model you have for the 'SizeOf' method is problematic, it
gives no way for subclasses to include memory referenced in the base
class (e.g. mUVSData).  Seems like you need 'SizeOf' and
'SizeOfReferencedData':

size_t Base::SizeOf() { return sizeof(Base) + SizeOfReferencedData(); }
size_t Base::SizeOfReferencedData() { // calculate size of referenced data in Base }
size_t Derived::SizeOf() { return sizeof(Derived) + SizeOfReferencedData(); }
size_t Derived::SizeOfReferencedData() {
  size_t superSize = Base::SizeOfReferencedData();
  // calculate size of referenced data in Derived
}

Seems like another patch for the user font set is needed but I'm fine
if that's on a follow-on bug.

Comment 15

[John Daggett (:jtd)]

Bas, any idea how to measure the size of the DirectWrite font objects?

Comment 16

[Nicholas Nethercote [inactive]]

> Rather than have two separate reporters that each enumerate the font
> list, I think it would make sense to instead use the
> nsIMemoryMultiReporter interface.  Then you could collect all data in
> one pass.  The code would be simpler to read that way also.

nsIMemoryMultiReporter is generally best used when you have a variable number of reports to make.  If you have a fixed number, nsIMemoryReporter is probably better.

> This doesn't make much sense to me, use a single pass.  If there are
> multiple values to collect, use a struct instead of a single int.

Having said that, if using nsIMemoryReporter would require multiple traversals, then a multi-reporter is probably better.

 
> I think the model you have for the 'SizeOf' method is problematic, it
> gives no way for subclasses to include memory referenced in the base
> class (e.g. mUVSData).  Seems like you need 'SizeOf' and
> 'SizeOfReferencedData':
> 
> size_t Base::SizeOf() { return sizeof(Base) + SizeOfReferencedData(); }
> size_t Base::SizeOfReferencedData() { // calculate size of referenced data
> in Base }
> size_t Derived::SizeOf() { return sizeof(Derived) + SizeOfReferencedData(); }
> size_t Derived::SizeOfReferencedData() {
>   size_t superSize = Base::SizeOfReferencedData();
>   // calculate size of referenced data in Derived
> }

I've been doing exactly that elsewhere (to be fair to jkew, he wrote this patch before I started doing that) but I've been calling them |SizeOfIncludingThis| and |SizeOfExcludingThis|.  See https://wiki.mozilla.org/Memory_Reporting.

Comment 17

[John Daggett (:jtd)]

> Having said that, if using nsIMemoryReporter would require multiple
> traversals, then a multi-reporter is probably better.

Right, the comment I made was about traversals within a single family
but when I realized the patch code was doing one traversal of all font
families per reporter, that's when I sniffed around for
nsIMemoryMultiReporter.

> I've been doing exactly that elsewhere (to be fair to jkew, he wrote
> this patch before I started doing that) but I've been calling them
> |SizeOfIncludingThis| and |SizeOfExcludingThis|.  See
> https://wiki.mozilla.org/Memory_Reporting.

This probably applies to the font cache patch also, to gfxFont and subclasses.

Comment 18

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - add memory reporting for the platform font list, v2

Comment 19

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - add memory reporting for fonts in the gfxFontCache, v2

Comment 20

[John Daggett (:jtd)]

Comment on attachment 580866 [details] [diff] [review]
part 1 - add memory reporting for the platform font list, v2

> +void
> +gfxDWriteFontFamily::ComputeMemoryUsageIncludingThis(FontListMemoryUsage *aUsage)
> 
> +void
> +gfxDWriteFontEntry::ComputeMemoryUsageIncludingThis(FontListMemoryUsage *aUsage)

As Nicholas suggests in comment 16, I think I'd prefer the SizeOfIncludingThis/SizeOfExcludingThis pattern.

+//  FIXME: Would be nice to account for the memory used by the DirectWrite objects...

I think it's fine to defer this but let's log a bug in case anyone has a clever idea about this.

Comment 21

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #20)
> Comment on attachment 580866 [details] [diff] [review]
> part 1 - add memory reporting for the platform font list, v2
> 
> > +void
> > +gfxDWriteFontFamily::ComputeMemoryUsageIncludingThis(FontListMemoryUsage *aUsage)
> > 
> > +void
> > +gfxDWriteFontEntry::ComputeMemoryUsageIncludingThis(FontListMemoryUsage *aUsage)
> 
> As Nicholas suggests in comment 16, I think I'd prefer the
> SizeOfIncludingThis/SizeOfExcludingThis pattern.

Are you saying you'd prefer these functions to be renamed (but otherwise unchanged), or something more than that? I've used SizeOf{In|Ex}cludingThis() elsewhere, but for the font-list/family/entry functions the API is different - it's not a function returning a single size_t, but rather a void function that's adding its contribution to one or more fields of the struct that is passed in (so that we can report the memory usage split into several categories). So as it's such a different usage pattern, it didn't seem appropriate to use the same name here IMO.

(In reply to John Daggett (:jtd) from comment #13)
> What do the numbers look like?  On OSX/Windows what are the breakdowns of
> cmap vs. font list data?

An example from OS X - I'm seeing around 2MB for cmaps, and 200K for the rest of the font-list data. Obviously, these figures will vary greatly depending on the installed fonts.

Comment 22

[John Daggett (:jtd)]

I was only referring to the names, they seem verbose.  Even though the signature is different, the functionality is the same:

FontListMemoryUsage ==> FontListMemory

SizeOfIncludingThis(FontListMemory *aTotals)

I'll leave it up to you.

Comment 23

[Nicholas Nethercote [inactive]]

> > As Nicholas suggests in comment 16, I think I'd prefer the
> > SizeOfIncludingThis/SizeOfExcludingThis pattern.
> 
> Are you saying you'd prefer these functions to be renamed (but otherwise
> unchanged), or something more than that? I've used
> SizeOf{In|Ex}cludingThis() elsewhere, but for the font-list/family/entry
> functions the API is different - it's not a function returning a single
> size_t, but rather a void function that's adding its contribution to one or
> more fields of the struct that is passed in (so that we can report the
> memory usage split into several categories). So as it's such a different
> usage pattern, it didn't seem appropriate to use the same name here IMO.

Please use the SizeOf-style names -- I've been meaning to add an example to https://wiki.mozilla.org/Memory_Reporting to show a function returning multiple results just like this :)

Comment 24

[Nicholas Nethercote [inactive]]

Comment on attachment 580866 [details] [diff] [review]
part 1 - add memory reporting for the platform font list, v2

Review of attachment 580866 [details] [diff] [review]:
-----------------------------------------------------------------

In bug 707865 I've removed nsTHashTable::ShallowSizeOfExcludingThis() and added nsTHashTable::SizeOfExcludingThis() which provides a much nicer interface for getting the size of things hanging off nsTHashtables.  And in bug 697335 I did the same for nsBaseHashtable.  Both patches have r+, I'm just waiting for the trees to reopen to land them.

Would you mind reworking this patch to use the new functions?  I realize that's a pain -- you're suffering for the fact that you're writing memory reporters at the same time that I'm doing a lot of infrastructure work.  But the new functions are much nicer than having to use hash table enumeration, and if you don't change this patch before landing I'll just have to do it subsequently.  We might as well get it right the first time.  (And if I land my patches first you'll have to update this patch anyway! :)

I'm giving an r- because I'd like to see the patch again once this reworking has happened, but it's definitely heading in the right direction.

::: gfx/thebes/gfxDWriteFontList.cpp
@@ +1173,5 @@
> +    aUsage->mFontListSize += mNonExistingFonts.SizeOf();
> +    for (PRUint32 i = 0; i < mNonExistingFonts.Length(); ++i) {
> +        // FIXME: is there a canonical way to get the SizeOf() a string?
> +        aUsage->mFontListSize +=
> +            mNonExistingFonts[i].Length() * sizeof(PRUnichar);

I just filed bug 710054 to add nsString::SizeOf{In,Ex}cludingThis.  Can you change the comment to refer to it?  In the meantime can you call mSizeOfFn on the buffer and then use Length()*sizeof(PRUnichar) as the fallback?

::: gfx/thebes/gfxFT2FontList.cpp
@@ +394,5 @@
> +void
> +FT2FontEntry::ComputeMemoryUsageExcludingThis(FontListMemoryUsage *aUsage)
> +{
> +    gfxFontEntry::ComputeMemoryUsageExcludingThis(aUsage);
> +    aUsage->mFontListSize += mFilename.Length();

Again, should use mSizeOfFn first, then fall back to the length, and refer to bug 710054 in the comment.

@@ +401,5 @@
> +void
> +FT2FontEntry::ComputeMemoryUsageIncludingThis(FontListMemoryUsage *aUsage)
> +{
> +    ComputeMemoryUsageExcludingThis(aUsage);
> +    aUsage->mFontListSize += aUsage->mSizeOfFun(this, sizeof(FT2FontEntry));

Same here.

::: gfx/thebes/gfxFT2FontList.h
@@ +104,5 @@
>  
> +    virtual void
> +    ComputeMemoryUsageExcludingThis(FontListMemoryUsage *aUsage);
> +    virtual void
> +    ComputeMemoryUsageIncludingThis(FontListMemoryUsage *aUsage);

Can you add NS_MUST_OVERRIDE and MOZ_OVERRIDE to these, and all other similar cases?  See https://hg.mozilla.org/mozilla-central/rev/2d5421ea1758#l5.44 for an example.

(I'm also going to add some examples involving inheritance to https://wiki.mozilla.org/Memory_Reporting, you've beaten me to it.)

::: gfx/thebes/gfxFont.cpp
@@ +398,5 @@
> +void
> +gfxFontEntry::ComputeMemoryUsageExcludingThis(FontListMemoryUsage *aUsage)
> +{
> +    aUsage->mFontListSize += mName.Length() * sizeof(PRUnichar);
> +    aUsage->mCharMapsSize += mCharacterMap.GetSize();

GetSize() needs to be converted to the new memory reporting style, i.e. take a nsMallocSizeOfFun.  Are you interested in doing it?

::: gfx/thebes/gfxGDIFontList.cpp
@@ +1076,5 @@
> +                                   &aUsage->mFontListSize);
> +
> +    aUsage->mFontListSize += mNonExistingFonts.SizeOf();
> +    for (PRUint32 i = 0; i < mNonExistingFonts.Length(); ++i) {
> +        // FIXME: is there a canonical way to get the SizeOf() a string?

As above.

::: gfx/thebes/gfxPlatformFontList.cpp
@@ +131,5 @@
>      gfxFontCache::GetCache()->AgeAllGenerations();
>      return NS_OK;
>  }
>  
> +NS_IMPL_ISUPPORTS1(gfxPlatformFontList::MemoryReporter, nsIMemoryMultiReporter)

Can you call this |MemoryMultiReporter| instead, to make it obvious it's an nsIMemoryMultiReporter not an nsIMemoryReporter?  Thanks.

@@ +139,5 @@
> +    (nsIMemoryMultiReporterCallback* aCb,
> +     nsISupports* aClosure)
> +{
> +    FontListMemoryUsage usage;
> +    usage.mSizeOfFun = &MemoryReporterMallocSizeOf;

MemoryReporterMallocSizeOf no longer exists.  You'll need to add a declaration outside this class:

NS_MEMORY_REPORTER_MALLOC_SIZEOF_FUN_UN(GfxFontStuffMallocSizeOf, "gfx-font-stuff")

You can then use |GfxFontStuffMallocSizeOf| where you currently use |MemoryReporterMallocSizeOf|.  (See https://wiki.mozilla.org/Platform/Memory_Reporting again for why this is necessary;  basically it's for DMD.)

@@ +707,5 @@
> +                                            gfxFontFamily*,
> +                                            void* aUserArg)
> +{
> +    PRUint32 *totalSize = static_cast<PRUint32*>(aUserArg);
> +    *totalSize += aKey.Length() * sizeof(PRUnichar);

Again, use mSizeOfFn first and the compute the length as a fallback, and refer to bug 710054 in the comment.

@@ +717,5 @@
> +                     gfxFontEntry*,
> +                     void* aUserArg)
> +{
> +    PRUint32 *totalSize = static_cast<PRUint32*>(aUserArg);
> +    *totalSize += aKey.Length() * sizeof(PRUnichar);

Ditto.

::: gfx/thebes/gfxPlatformFontList.h
@@ +57,5 @@
>  
>  // Much of this is based on the old gfxQuartzFontCache, but adapted for use on all platforms.
>  
> +struct FontListMemoryUsage {
> +    nsMallocSizeOfFun mSizeOfFun;

Can you rename this |mMallocSizeOf|?  That's the naming convention used everywhere else.

Comment 25

[Nicholas Nethercote [inactive]]

Comment on attachment 580867 [details] [diff] [review]
part 2 - add memory reporting for fonts in the gfxFontCache, v2

Review of attachment 580867 [details] [diff] [review]:
-----------------------------------------------------------------

Just like the other patch -- heading very much in the right direction, but I'm giving r- because I'd like to see it again after reworking for the nsTHashtable changes.

::: gfx/thebes/gfxDWriteFonts.cpp
@@ +759,5 @@
> +size_t
> +gfxDWriteFont::SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf)
> +{
> +    return gfxFont::SizeOfExcludingThis(aMallocSizeOf) +
> +        sizeof(gfxFont::Metrics) + mGlyphWidths.ShallowSizeOfExcludingThis(aMallocSizeOf);

Like the other patch, this'll have to be reworked for my nsTHashtable improvements.

::: gfx/thebes/gfxDWriteFonts.h
@@ +90,5 @@
>  
>      virtual PRInt32 GetGlyphWidth(gfxContext *aCtx, PRUint16 aGID);
>  
> +    virtual size_t SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf);
> +    virtual size_t SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf);

Again, can you please add the NS_MUST_OVERRIDE and MOZ_OVERRIDE markings, here and on all other virtual SizeOf* functions?

::: gfx/thebes/gfxFont.cpp
@@ +1013,5 @@
>      gGlobalCache = new gfxFontCache();
> +    if (!gGlobalCache) {
> +        return NS_ERROR_OUT_OF_MEMORY;
> +    }
> +    NS_RegisterMemoryReporter(new NS_MEMORY_REPORTER_NAME(FontCache));

Since you have gfxFontCache::Shutdown(), I'd like it if you had a member to hold the memory reporter.  You can then unregister the reporter in gfxFontCache::Shutdown().  That's what's generally done in other case, even for classes that are only shutdown when the browser exits.

@@ +1119,5 @@
> +    return PL_DHASH_NEXT;
> +}
> +
> +size_t
> +gfxFontCache::SizeOf(nsMallocSizeOfFun aMallocSizeOf)

This should be called SizeOfExcludingThis().

@@ +2089,5 @@
>  PRUint32
>  gfxGlyphExtents::GlyphWidths::ComputeSize()
>  {
>      PRUint32 i;
>      PRUint32 size = mBlocks.Capacity()*sizeof(PtrBits);

This function should follow the new style -- be called SizeOfExcludingThis (or SizeOfIncludingThis), and take a nsMallocSizeOfFun parameter.

@@ +2150,5 @@
>      entry->height = aExtentsAppUnits.Height();
>  }
>  
> +size_t
> +gfxGlyphExtents::SizeOf(nsMallocSizeOfFun aMallocSizeOf)

This should be called SizeOfIncludingThis.

Comment 26

[Jonathan Kew [:jfkthame]]

I was going to update these on top of your patches from bug 697335 and bug 707865, but bug 707865 doesn't seem to apply to current m-c tip; it looks like it depends on some other patch that I don't have.

Comment 27

[Nicholas Nethercote [inactive]]

(In reply to Jonathan Kew (:jfkthame) from comment #26)
> I was going to update these on top of your patches from bug 697335 and bug
> 707865, but bug 707865 doesn't seem to apply to current m-c tip; it looks
> like it depends on some other patch that I don't have.

Yes, I have a stack of memory reporter infrastructure patches waiting to land, and the two patches you're relying on are somewhere in the middle... sorry that you're suffering :(  I'm going to land them as soon as the trees are back open.

BTW, I updated https://wiki.mozilla.org/Platform/Memory_Reporting to talk about multiple return values and inheritance, which might be helpful for you.

Comment 28

[Nicholas Nethercote [inactive]]

Bug 707865 has landed now, sorry for the inconvenience!

Comment 29

[John Daggett (:jtd)]

I'd be happy to review the new patches but maybe Nick should be doing the final review since he had concerns with the last patches.  It would be nice to land this ASAP so that it makes the Tuesday Aurora cutoff.

Comment 30

[Nicholas Nethercote [inactive]]

Yeah, I'm happy to review.  I'm not worried about making Aurora, this is only a couple of MB.

Comment 31

[Jonathan Kew [:jfkthame]]

(In reply to Nicholas Nethercote [:njn] from comment #28)
> Bug 707865 has landed now, sorry for the inconvenience!

Thanks, I'll revisit this shortly and post an updated version.

(In reply to Nicholas Nethercote [:njn] from comment #30)
> Yeah, I'm happy to review.  I'm not worried about making Aurora, this is
> only a couple of MB.

Agreed, I don't see it as urgent; it's nice to account properly for a bit more of our memory use, but it's not something that will fluctuate wildly or balloon to huge amounts in the way some measurements can.

Also, in bug 710727 I'm posting a patch that will substantially reduce the memory used here (by well over 50%, in my tests on both Windows and OS X) - and will involve significant changes to the reporter here.

Comment 32

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #21)
> (In reply to John Daggett (:jtd) from comment #13)
> > What do the numbers look like?  On OSX/Windows what are the breakdowns of
> > cmap vs. font list data?
> 
> An example from OS X - I'm seeing around 2MB for cmaps, and 200K for the
> rest of the font-list data. Obviously, these figures will vary greatly
> depending on the installed fonts.

This is misleading because the size of font tables used by harfbuzz is obscured by the use of platform-specific API's that aren't covered by the current reporters.  This specifically applies to the GetFontTable calls with the signature "virtual hb_blob_t *GetFontTable(PRUint32 aTag)".  Take the OSX case:

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxMacFont.cpp#400

This code effectively allocates memory via CFData objects.  I stuck in debugging code to print out the size of these and just loading www.wikipedia.org consumes 1.9MB of font table data, accounting for shared data.  There's a similar mechanism in the DirectWrite version of this function.  In the GDI case, the default blob cache will be used and reflected in the memory reporter, so the 1.9MB for Wikipedia should show up there under the font table category.

Comment 33

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) (Away 23 Dec - 4 Jan) from comment #32)
> (In reply to Jonathan Kew (:jfkthame) from comment #21)
> > (In reply to John Daggett (:jtd) from comment #13)
> > > What do the numbers look like?  On OSX/Windows what are the breakdowns of
> > > cmap vs. font list data?
> > 
> > An example from OS X - I'm seeing around 2MB for cmaps, and 200K for the
> > rest of the font-list data. Obviously, these figures will vary greatly
> > depending on the installed fonts.
> 
> This is misleading because the size of font tables used by harfbuzz is
> obscured by the use of platform-specific API's that aren't covered by the
> current reporters.  This specifically applies to the GetFontTable calls with
> the signature "virtual hb_blob_t *GetFontTable(PRUint32 aTag)".  Take the
> OSX case:
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxMacFont.cpp#400
> 
> This code effectively allocates memory via CFData objects.  I stuck in
> debugging code to print out the size of these and just loading
> www.wikipedia.org consumes 1.9MB of font table data, accounting for shared
> data.  There's a similar mechanism in the DirectWrite version of this
> function. 

The platform-specific implementations in gfxMacFont and gfxDWriteFont retrieve the font tables are not necessarily allocating memory - they return references to shared table data that [may be? is always?] owned by the OS. I'd guess that if the font in question has been used at all by any application on the system, it's likely that the system font service will have already loaded or memory-mapped the tables, and so we may not be allocating any significant amount of memory here. Certainly, "accounting" the length of all the CFData objects that wrap font table data would be wrong.

> In the GDI case, the default blob cache will be used and
> reflected in the memory reporter, so the 1.9MB for Wikipedia should show up
> there under the font table category.

That's correct; GDI doesn't offer an API for access to the font data except by copying the tables, which is why we do it on the font entry (not per gfxFont instance), and have to allocate memory for that table data.

Comment 34

[Jonathan Kew [:jfkthame]]

FWIW, I added code to report the CF retain count of the CFData objects that CGFontCopyTableForTag returns to us; in the testing I did, it's always greater than 1, indicating that we're not the sole owner of the data. It's quite often 2, which presumably means the only other reference is one held by a Core Graphics font data cache, but we have no way of knowing whether it was already present in the cache because it had been used previously (not necessarily by our process), or whether it was loaded/mem-mapped specifically as a result of our call.

Comment 35

[Nicholas Nethercote [inactive]]

I landed bug 715453 which means that mallocSizeOf functions no longer need a |computedSize| as their second argument, they just the need the pointer.  This will require your patch to be updated, but only by reducing the amount of code :)

Comment 36

[Jonathan Kew [:jfkthame]]

Attachment: memory reporting for the platform font list, v3

Substantially updated for font-list changes and newer memory reporting functions; please see if I'm going in the right direction here.

Comment 37

[Nicholas Nethercote [inactive]]

Comment on attachment 607470 [details] [diff] [review]
memory reporting for the platform font list, v3

Review of attachment 607470 [details] [diff] [review]:
-----------------------------------------------------------------

I have lots of nits, but it generally looks good.  r=me with them fixed.

BTW, I don't know these data structures at all, and while I looked at some of their definitions, I'm trusting you to a certain degree that the important things are measured and nothing is measured twice :)

::: gfx/thebes/gfxDWriteFontList.cpp
@@ +253,5 @@
> +gfxDWriteFontFamily::SizeOfIncludingThis(nsMallocSizeOfFun    aMallocSizeOf,
> +                                         FontListMemoryUsage *aUsage)
> +{
> +    SizeOfExcludingThis(aMallocSizeOf, aUsage);
> +    aUsage->mFontListSize += aMallocSizeOf(this);

Nit: In other reporters I measure |this| first and then call SizeOfExcludingThis().

@@ +556,5 @@
> +gfxDWriteFontEntry::SizeOfIncludingThis(nsMallocSizeOfFun    aMallocSizeOf,
> +                                        FontListMemoryUsage *aUsage)
> +{
> +    SizeOfExcludingThis(aMallocSizeOf, aUsage);
> +    aUsage->mFontListSize += aMallocSizeOf(this);

Ditto.

@@ +1232,5 @@
> +gfxDWriteFontList::SizeOfIncludingThis(nsMallocSizeOfFun    aMallocSizeOf,
> +                                       FontListMemoryUsage *aUsage)
> +{
> +    SizeOfExcludingThis(aMallocSizeOf, aUsage);
> +    aUsage->mFontListSize += aMallocSizeOf(this);

Ditto.

::: gfx/thebes/gfxDWriteFontList.h
@@ +80,5 @@
>      void SetForceGDIClassic(bool aForce) { mForceGDIClassic = aForce; }
>  
> +//  FIXME: Would be nice to account for the memory used by the DirectWrite objects...
> +//    virtual void SizeOfExcludingThis(nsMallocSizeOfFun    aMallocSizeOf,
> +//                                     FontListMemoryUsage *aUsage);

Does this refer to the |nsRefPtr<IDWriteFontFamily> mDWFamily| member in gfxDWriteFontFamily?

It would be nicer to add SizeOfExcludingThis(), have it call gfxFontFamily::SizeOfExcludingThis(), and add a comment that |mDWFamily| might be measured later.  See http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.cpp#1656 for an example.

@@ +186,5 @@
>      bool GetForceGDIClassic() { return mForceGDIClassic; }
>  
> +//  FIXME: Would be nice to account for the memory used by the DirectWrite objects...
> +//    virtual void SizeOfExcludingThis(nsMallocSizeOfFun    aMallocSizeOf,
> +//                                     FontListMemoryUsage *aUsage);

Ditto.

@@ +394,5 @@
>  
> +    virtual void SizeOfExcludingThis(nsMallocSizeOfFun    aMallocSizeOf,
> +                                     FontListMemoryUsage *aUsage);
> +    virtual void SizeOfIncludingThis(nsMallocSizeOfFun    aMallocSizeOf,
> +                                     FontListMemoryUsage *aUsage);

All the SizeOf{In,Ex}cludingThis functions should be |const| unless there's a good reason not to. (This comment applies throughout this patch.)

::: gfx/thebes/gfxFT2FontList.cpp
@@ +402,5 @@
> +FT2FontEntry::SizeOfIncludingThis(nsMallocSizeOfFun    aMallocSizeOf,
> +                                  FontListMemoryUsage *aUsage)
> +{
> +    SizeOfExcludingThis(aMallocSizeOf, aUsage);
> +    aUsage->mFontListSize += aMallocSizeOf(this);

Nit: As above -- measure |this| before calling SizeOfExcludingThis().

::: gfx/thebes/gfxFont.cpp
@@ +404,5 @@
>  
> +/* static */ size_t
> +gfxFontEntry::FontTableHashEntry::SizeOfTable(FontTableHashEntry *aEntry,
> +                                              nsMallocSizeOfFun   aMallocSizeOf,
> +                                              void*               aUserArg)

|SizeOfEntryExcludingThis| would be a better name for this function -- the "Entry" makes it clear it's a hash entry, and the |ExcludingThis| makes it clear we're not measuring the entry itself, but only the things hanging off it.

@@ +409,5 @@
> +{
> +    if (aEntry->mBlob) {
> +        FontListMemoryUsage *usage =
> +            static_cast<FontListMemoryUsage*>(aUserArg);
> +        usage->mFontTableCacheSize += hb_blob_get_length(aEntry->mBlob);

It'd be better if this was a SizeOfIncludingThis call, so that its measurement would include any slop bytes and DMD would know about it... can we modify harfbuzz?  In some DMD runs I saw non-trivial amounts of memory usage within harfbuzz.

@@ +420,5 @@
> +                                  FontListMemoryUsage *aUsage)
> +{
> +    aUsage->mFontListSize +=
> +        mName.SizeOfExcludingThisIfUnshared(aMallocSizeOf);
> +    aUsage->mCharMapsSize += mCharacterMap.GetSize();

Likewise, please add a SizeOfExcludingThis() function to gfxSparseBitSet.

@@ +421,5 @@
> +{
> +    aUsage->mFontListSize +=
> +        mName.SizeOfExcludingThisIfUnshared(aMallocSizeOf);
> +    aUsage->mCharMapsSize += mCharacterMap.GetSize();
> +    if (mFontTableCache.IsInitialized()) {

I think you don't need to check for initialization, because nsTHashtable::SizeOfExcludingThis() does.

@@ +434,5 @@
> +gfxFontEntry::SizeOfIncludingThis(nsMallocSizeOfFun    aMallocSizeOf,
> +                                  FontListMemoryUsage *aUsage)
> +{
> +    SizeOfExcludingThis(aMallocSizeOf, aUsage);
> +    aUsage->mFontListSize += aMallocSizeOf(this);

Again, measure |this| first.

@@ +1051,5 @@
> +                                   FontListMemoryUsage *aUsage)
> +{
> +    aUsage->mFontListSize +=
> +        mName.SizeOfExcludingThisIfUnshared(aMallocSizeOf);
> +    aUsage->mCharMapsSize += mCharacterMap.GetSize();

You'll be able to use gfxSparseBitSet::SizeOfExcludingThis() here too.

@@ +1068,5 @@
> +gfxFontFamily::SizeOfIncludingThis(nsMallocSizeOfFun    aMallocSizeOf,
> +                                   FontListMemoryUsage *aUsage)
> +{
> +    aUsage->mFontListSize += aMallocSizeOf(this);
> +    SizeOfExcludingThis(aMallocSizeOf, aUsage);

You measured |this| first here :)

::: gfx/thebes/gfxGDIFontList.cpp
@@ +447,5 @@
> +GDIFontEntry::SizeOfIncludingThis(nsMallocSizeOfFun    aMallocSizeOf,
> +                                  FontListMemoryUsage *aUsage)
> +{
> +    SizeOfExcludingThis(aMallocSizeOf, aUsage);
> +    aUsage->mFontListSize += aMallocSizeOf(this);

Again, measure |this| first, please.

@@ +1061,5 @@
> +gfxGDIFontList::SizeOfIncludingThis(nsMallocSizeOfFun    aMallocSizeOf,
> +                                    FontListMemoryUsage *aUsage)
> +{
> +    SizeOfExcludingThis(aMallocSizeOf, aUsage);
> +    aUsage->mFontListSize += aMallocSizeOf(this);

Measure |this| first.

::: gfx/thebes/gfxMacPlatformFontList.mm
@@ +402,5 @@
> +ATSFontEntry::SizeOfIncludingThis(nsMallocSizeOfFun    aMallocSizeOf,
> +                                  FontListMemoryUsage *aUsage)
> +{
> +    SizeOfExcludingThis(aMallocSizeOf, aUsage);
> +    aUsage->mFontListSize += aMallocSizeOf(this);

Measure |this| first.

@@ +498,5 @@
> +CGFontEntry::SizeOfIncludingThis(nsMallocSizeOfFun    aMallocSizeOf,
> +                                 FontListMemoryUsage *aUsage)
> +{
> +    SizeOfExcludingThis(aMallocSizeOf, aUsage);
> +    aUsage->mFontListSize += aMallocSizeOf(this);

Ditto.

::: gfx/thebes/gfxPlatformFontList.cpp
@@ +769,5 @@
> +
> +// Support for memory reporting
> +
> +static size_t
> +SizeOfFamilyEntry(const nsAString&               aKey,

Nit: As mentioned above, I prefer it if "SizeOfEntry"-style function have "ExcludingThis" in their name.

@@ +829,5 @@
> +{
> +    aUsage->mFontListSize +=
> +        mFontFamilies.SizeOfExcludingThis(SizeOfFamilyEntry,
> +                                          aMallocSizeOf,
> +                                          aUsage);

Hmm... SizeOfFamilyEntry returns 0, because it adds its measurements to the FontListMemoryUsage.  So the return value for this function call will just be the size of the table storage;  it will exclude the things pointed to by the table storage.  Is that what you want?  Either way, it's non-obvious and so a comment here and in SizeOfFamilyEntry would be very helpful.

@@ +833,5 @@
> +                                          aUsage);
> +
> +    aUsage->mFontListSize +=
> +        mOtherFamilyNames.SizeOfExcludingThis(SizeOfFamilyName,
> +                                              aMallocSizeOf);

I think you can move |aMallocSizeOf| up to the previous line without exceeding 80 chars.  (The same comment applies throughout the rest of this function.)

@@ +864,5 @@
> +gfxPlatformFontList::SizeOfIncludingThis(nsMallocSizeOfFun    aMallocSizeOf,
> +                                         FontListMemoryUsage *aUsage)
> +{
> +    SizeOfExcludingThis(aMallocSizeOf, aUsage);
> +    aUsage->mFontListSize += aMallocSizeOf(this);

Measure |this| first.

::: gfx/thebes/gfxPlatformFontList.h
@@ +57,5 @@
>  // so we do our own font family/style management here instead.
>  
>  // Much of this is based on the old gfxQuartzFontCache, but adapted for use on all platforms.
>  
> +struct FontListMemoryUsage {

You could call this |FontListSizes| -- it's shorter and matches what I've done elsewhere (e.g. |nsWindowSizes|).  The parameter name for objects of this type could then be |aSizes| instead of |aUsage|.  I'll let you decide which you prefer.

Comment 38

[Jonathan Kew [:jfkthame]]

Attachment: memory reporting for the platform font list, v4

Updated patch to deal with review comments - thanks! Carrying forward r= n.nethercote from comment #37, though feel free to check over the updates if you like.

Comment 39

[Jonathan Kew [:jfkthame]]

Attachment: memory reporting for gfxFont instances, v3

Comment 40

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #39)
> Created attachment 608320 [details] [diff] [review]
> memory reporting for gfxFont instances, v3

Ugh - tryserver found a typo in the Windows-specific code here.... I'll post an updated patch after double-checking that it all builds OK.

Comment 41

[Jonathan Kew [:jfkthame]]

Attachment: memory reporting for gfxFont instances, v4

OK, now tryserver is happier. :)

Comment 42

[Nicholas Nethercote [inactive]]

Comment on attachment 608493 [details] [diff] [review]
memory reporting for gfxFont instances, v4

Review of attachment 608493 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

How big are the measurements made by these new reporters?  KBs, MBs?

::: gfx/thebes/gfxFont.cpp
@@ +1064,5 @@
> +     nsISupports* aClosure)
> +{
> +    FontCacheSizes sizes;
> +    sizes.mFontInstances = 0;
> +    sizes.mShapedWords = 0;

It'd be nicer to add a constructor to FontCacheSizes that zeroes all its elements -- encapsulation and all that.  (The same comment applies to FontListSizes in your other patch.)

@@ +2748,2 @@
>  PRUint32
> +gfxGlyphExtents::GlyphWidths::SizeOfExcludingThis(nsMallocSizeOfFun aMallocSizeOf) const

Something to beware of:  mallocSizeOf can, in some obscure configurations (e.g. --disable-jemalloc on Solaris), always return zero.  So you shouldn't rely on it for things other than things like memory reporters.

In this case it looks like debug-only stats code, so that's ok.  But you wouldn't want to rely on mallocSizeOf for sizing some important cache or anything like that.

::: gfx/thebes/gfxMacFont.cpp
@@ +510,5 @@
>  }
>  
> +void
> +gfxMacFont::SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf,
> +                                FontCacheSizes*   aSizes) const

The other sub-classes of gfxFont had a SizeOfExcludingThis function as well, but gfxMacFont doesn't.  Is this because none of the members in gfxMacFont are worth measuring?  One possibility would be to add a SizeOfExcludingThis that just calls gfxFont::SizeOfExcludingThis and contains a comment explaining why the other members aren't measured.

Comment 43

[Jonathan Kew [:jfkthame]]

(In reply to Nicholas Nethercote [:njn] from comment #42)
> How big are the measurements made by these new reporters?  KBs, MBs?

On a freshly-launched browser just showing about:home, I see a total of around 100KB for them all.

After opening 8 assorted tabs, I have 4MB for font-shaped-words, 500KB for font-charmaps, and <100KB for the other parts.

With several more text-heavy tabs, including a tinderbox log, font-shaped-words rose to 29MB.

Comment 44

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/730c2e84247f
https://hg.mozilla.org/integration/mozilla-inbound/rev/7beb8fd46629

Comment 45

[Ryan VanderMeulen [:RyanVM]]

One of the patches in this push caused winxp debug mochitests to perma-hang in test_memoryReporters.xul. Backed the entire push out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0dfb71a2df9

Comment 46

[Jonathan Kew [:jfkthame]]

Re-landed part 2 only:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9094735bcb46

Please leave open when merging to m-c, as part 1 still needs to land after further investigation/testing.

Comment 47

[Ed Morley [:emorley]]

(In reply to Jonathan Kew (:jfkthame) from comment #46)
> Re-landed part 2 only:

https://hg.mozilla.org/mozilla-central/rev/9094735bcb46

Comment 48

[Jonathan Kew [:jfkthame]]

And re-landed part 1, with fix for the WinXP breakage:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97157235087f

Comment 49

[Ed Morley [:emorley]]

(In reply to Jonathan Kew (:jfkthame) from comment #48)
> And re-landed part 1, with fix for the WinXP breakage:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/97157235087f

https://hg.mozilla.org/mozilla-central/rev/97157235087f