Closed Bug 1268021 Opened 8 years ago Closed 8 years ago

Add memory reporting for the user font cache

Categories

(Core :: Graphics: Text, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

Attachments

(2 files, 1 obsolete file)

We have gfxUserFontSet::UserFontCache (a single cache per process), which tracks font entries for downloadable fonts and allows them to be shared across multiple pages that are using the same fonts; these font entries in turn own (possibly through platform-specific objects) the actual font data.

With a few font-heavy sites loaded, this can easily run into multiple megabytes; but the downloaded fonts don't get included anywhere in memory reporting, so all that data becomes "heap-unclassified".

Let's fix that, so we can tell how much is being used by downloadable font resources.
This makes user-fonts show up in the about:memory report. Nick, you're a memory-reporter guy, aren't you -- can I get you to review this? Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4395f3f545d.
Attachment #8746005 - Flags: review?(n.nethercote)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment on attachment 8746005 [details] [diff] [review]
Implement memory reporting for the user-font cache

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

Thank you for doing this. Improving memory reporting coverage is always good.

I've given f+ because I've made enough comments below that I'd like to see an updated patch before landing. This memory reporter has some unusual features that make it less straightforward than most.

::: gfx/thebes/gfxFontEntry.cpp
@@ +1110,5 @@
>      AddSizeOfExcludingThis(aMallocSizeOf, aSizes);
>  }
>  
> +size_t
> +gfxFontEntry::SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const

It's not clear to me when you use this function and when you use gfxFontEntry::AddSizeOfIncludingThis(). Is this one only called on entries in the global cache, while AddSizeOfIncludingThis() is called on entries elsewhere? Comments would help here.

@@ +1114,5 @@
> +gfxFontEntry::SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const
> +{
> +    FontListSizes s = { 0 };
> +    AddSizeOfExcludingThis(aMallocSizeOf, &s);
> +    size_t result = s.mFontListSize + s.mFontTableCacheSize + s.mCharMapsSize;

If we are reporting these three things separately elsewhere, does it make sense to do likewise here, rather than combining them?

@@ +1118,5 @@
> +    size_t result = s.mFontListSize + s.mFontTableCacheSize + s.mCharMapsSize;
> +
> +    if (mIsDataUserFont) {
> +        MOZ_ASSERT(mUserFontSize > 0, "user font with no data?");
> +        result += mUserFontSize;

Likewise, is it worth reporting this separately from the other three things above?

::: gfx/thebes/gfxFontEntry.h
@@ +538,5 @@
> +    // For memory reporting: size of user-font data belonging to this entry.
> +    // We record this in the font entry because the actual data block may be
> +    // handed over to platform APIs, so that it would become difficult (and
> +    // platform-specific) to measure it directly at report-gathering time.
> +    uint32_t mUserFontSize;

Hmm. I strongly prefer to avoid computing sizes like this (especially in advance), in favour of measuring sizes (with a MallocSizeOf) function. Because the latter approach is less error-prone, and integrates nicely with DMD. (https://wiki.mozilla.org/Memory_Reporting has more details about this.)

Is it *really* difficult to measure it on demand with MallocSizeOf? If it is, ok, but if not...

::: gfx/thebes/gfxUserFontSet.cpp
@@ +1144,5 @@
>              obs->AddObserver(flusher, "last-pb-context-exited", false);
>              obs->AddObserver(flusher, "xpcom-shutdown", false);
>          }
> +
> +        RegisterStrongMemoryReporter(new MemoryReporter());

It took me a while to understand why this is ok -- because |sUserData| is static, and it is only cleared at shutdown, and CollectReports() checks if |sUserData| is null before doing anything.

One way to make this clearer would be to add a static |sUserDataMemoryReporter| variable, and save the reporter in that varible, and then unregister the reporter and null the variable when you delete |sUserData|.

If that seems too much, at the very least a comment explaining why it's ok for this reporter to live until the memory reporter manager is shut down.

@@ +1276,5 @@
> +gfxUserFontSet::UserFontCache::Entry::ReportMemory(nsIMemoryReporterCallback* aCb,
> +                                                   nsISupports* aClosure,
> +                                                   bool aAnonymize)
> +{
> +    nsAutoCString path("explicit/gfx/user-fonts/font(");

The paths here are sufficiently complex that seeing some examples (copy+pasted from about:memory) would be helpful. Please include them when you upload the next version.

@@ +1315,5 @@
> +        // Include a clue as to who loaded this resource. (Note that because
> +        // of font entry sharing, other pages may now be using this resource,
> +        // and the original page may no longer be loaded at all.)
> +        description.AppendPrintf("\nPrincipal when loaded: %s.",
> +                                 principalSpec.get());

Putting info that's specific to this particular report into the description is sub-optimal because you won't see it if someone copies+pastes about:memory's output. Would it be possible to put it into the path instead? We often add auxiliary information in paths the following style: "top(http://foo.com/, id=123)".

@@ +1318,5 @@
> +        description.AppendPrintf("\nPrincipal when loaded: %s.",
> +                                 principalSpec.get());
> +    }
> +
> +    int64_t amount = mFontEntry->SizeOfExcludingThis(moz_malloc_size_of);

You should use a MallocSizeOf function created with MOZ_DEFINE_MALLOC_SIZE_OF. That way DMD will be informed when heap blocks get measured by that function. See gfx/thebes/gfxFont.cpp for an example.

@@ +1338,5 @@
> +        return NS_OK;
> +    }
> +
> +    // Currently, we just report the memory used by actual downloaded font
> +    // resources; we don't report the size of the cache hashtable itself.

Why not? It should be easy. Just use nsTHashTable::ShallowSizeOfIncludingThis(). And you'll have to come up with a different path and description for it.
Attachment #8746005 - Flags: review?(n.nethercote) → feedback+
(In reply to Nicholas Nethercote [:njn] from comment #2)
> @@ +1114,5 @@
> > +gfxFontEntry::SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const
> > +{
> > +    FontListSizes s = { 0 };
> > +    AddSizeOfExcludingThis(aMallocSizeOf, &s);
> > +    size_t result = s.mFontListSize + s.mFontTableCacheSize + s.mCharMapsSize;
> 
> If we are reporting these three things separately elsewhere, does it make
> sense to do likewise here, rather than combining them?

We do that for the main platform font list, where we're typically summing the totals for a few hundred font faces. But for downloaded user fonts, we're reporting each font individually, at which point it doesn't seem worth splitting out these components of the usage (which will be pretty small).

> @@ +1118,5 @@
> > +    size_t result = s.mFontListSize + s.mFontTableCacheSize + s.mCharMapsSize;
> > +
> > +    if (mIsDataUserFont) {
> > +        MOZ_ASSERT(mUserFontSize > 0, "user font with no data?");
> > +        result += mUserFontSize;
> 
> Likewise, is it worth reporting this separately from the other three things
> above?

I don't think so. *This* is really the piece we're interested in, for a web-font; it's the bulk of the font data. The items above will be a minor bit of overhead, in comparison, and it didn't seem worth cluttering the report with that level of detail.

> ::: gfx/thebes/gfxFontEntry.h
> @@ +538,5 @@
> > +    // For memory reporting: size of user-font data belonging to this entry.
> > +    // We record this in the font entry because the actual data block may be
> > +    // handed over to platform APIs, so that it would become difficult (and
> > +    // platform-specific) to measure it directly at report-gathering time.
> > +    uint32_t mUserFontSize;
> 
> Hmm. I strongly prefer to avoid computing sizes like this (especially in
> advance), in favour of measuring sizes (with a MallocSizeOf) function.
> Because the latter approach is less error-prone, and integrates nicely with
> DMD. (https://wiki.mozilla.org/Memory_Reporting has more details about this.)
> 
> Is it *really* difficult to measure it on demand with MallocSizeOf? If it
> is, ok, but if not...

Well...... when we load a webfont, we download, decode and sanitize the data; then we hand that block of data off to a platform-specific gfxPlatformFontList::MakePlatformFont implementation. What I did here was to record the size of that block of data for memory-reporting purposes. It's a kludge, but seemed like a reasonable approximation to the truth, given that the "actual truth" is hard to get at...

What becomes of the data block passed to MakePlatformFont depends very much on the platform back-end. On OS X, for example, we wrap it up in a Core Graphics CGDataProvider object (using CGDataProviderCreateWithData), then we pass that to CGFontCreateWithDataProvider, which creates a CGFont that will be holding a reference to the data-provider, which we no longer track ourselves. So at this point, we've relinquished ownership of the data; but it still exists for as long as the CGFont stays around and keeps its reference. But we can't actually measure it any longer.

On Windows, we have the GDI and DWrite backends, each of which handles this in its own (very different) way.... on GDI, it's relatively simple: we eventually call AddFontMemResourceEx, and then we release the data. So strictly speaking, that block of data has been freed.... but AddFontMemResourceEx has created its own copy internally (according to the MSDN docs -- and it'd have to, in order to use the font), so what we end up reporting is still a reasonable approximation of the memory being used by this font.

Then there's DWrite, where we create a custom IDWriteFontFile that takes ownership of the data block; so, a bit like the OS X case, we're handing off ownership of the memory to the system font manager, and no longer keeping track of it directly or able to access and measure it from Gecko.

Finally, we have the Freetype-based backends (gfxFT2FontList, gfxFcPlatformFontList); on these, I think it would be easier to implement a true measuring reporter, as we have more direct control of how the font data block is managed. IIRC, when we create the FT_Face, we end up saving the font data pointer in a userData field on the font entry or cairo face or something, so in principle we could retrieve it from there and measure it -- at least I think so. But given that we can't really do this for the main desktop platforms, I'm inclined to stick with the simple, universal version here, at least as a first effort.
Updated in the light of comments. Note that this still uses the approach of recording the size of the font data, and then reporting that, rather than measuring on-demand, because (as described above) on most platforms, once we've handed over the data to the OS font subsystem, we don't have any way to measure the data that's backing the resulting font object. So here, I basically assume that the system will hold on to the data block it was given, and report that size. In some cases this may not be strictly true (e.g. under GDI, it'll be copied internally by the OS, and the original block freed), but it seems better than nothing.
Attachment #8746581 - Flags: review?(n.nethercote)
Attachment #8746005 - Attachment is obsolete: true
Comment on attachment 8746581 [details] [diff] [review]
Implement memory reporting for the user-font cache

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

Fair enough about computing the size rather than measuring it. We have a similar situation with other kinds of gfx things such as textures, alas.

Thank you for the example output. For next time, you can just copy and paste about:memory's output as text. That's easier than taking a screenshot :)

Otherwise, looks good! r=me with the changes below.

::: gfx/thebes/gfxFontEntry.cpp
@@ +1116,5 @@
> +// above.)
> +size_t
> +gfxFontEntry::SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const
> +{
> +    FontListSizes s = { 0 };

A comment explaining why you combine the four measurements -- much like the one you wrote in comment 3 -- would be useful here.

::: gfx/thebes/gfxFontEntry.h
@@ +539,5 @@
> +    // For memory reporting: size of user-font data belonging to this entry.
> +    // We record this in the font entry because the actual data block may be
> +    // handed over to platform APIs, so that it would become difficult (and
> +    // platform-specific) to measure it directly at report-gathering time.
> +    uint32_t mUserFontSize;

Can you rename this as |mComputedSizeOfUserFont|? There's precedent of using a "ComputedSizeOf" in names to indicate when things aren't measured directly -- we have a few functions called ComputedSizeOf{In,Ex}cludingThis().

::: gfx/thebes/gfxUserFontSet.cpp
@@ +1304,5 @@
> +                spec.Length() > 255) {
> +                spec.Truncate(252);
> +                spec.Append("...");
> +            }
> +            path.Append(spec);

Nit: generally in paths we put all auxiliary info in a "key=value" form. So precede the url with "url=" here?

@@ +1316,5 @@
> +                // Include a clue as to who loaded this resource. (Note that
> +                // because of font entry sharing, other pages may now be using
> +                // this resource, and the original page may not even be loaded
> +                // any longer.)
> +                path.AppendPrintf(", principal: %s", spec.get());

Nit: and please use "principal=%s" for consistency with existing reporters.

::: gfx/thebes/gfxUserFontSet.h
@@ +322,5 @@
>  
> +        // Memory-reporting support.
> +        class MemoryReporter final : public nsIMemoryReporter
> +        {
> +            ~MemoryReporter() {}

Remove this empty destructor?
Attachment #8746581 - Flags: review?(n.nethercote) → review+
Comment on attachment 8746581 [details] [diff] [review]
Implement memory reporting for the user-font cache

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

::: gfx/thebes/gfxUserFontSet.cpp
@@ +655,5 @@
>      }
>  
>      if (fe) {
> +        // Record size for memory reporting purposes.
> +        fe->mUserFontSize = saneLen;

On second thoughts, Nick: would it be preferable to record moz_malloc_size_of(saneData) here, rather than just saneLen? That's the real size of the allocated block, while saneLen is just the length of the actual data within it. Depending on the allocation strategy used by the font decoder/sanitizer, the block we end up holding on to (and passing over to the platform APIs) is liable to have quite a bit of dead space at the end. (I'm seeing around 100K extra on the Fira fonts used by b.m.o, for example.)

::: gfx/thebes/gfxUserFontSet.h
@@ +322,5 @@
>  
> +        // Memory-reporting support.
> +        class MemoryReporter final : public nsIMemoryReporter
> +        {
> +            ~MemoryReporter() {}

NS_IMPL_ISUPPORTS will complain; it doesn't like refcounted classes to have a public destructor. So I've left it in.
ni? to :njn to confirm that moz_malloc_size_of(saneData) makes sense in comment 7.
Flags: needinfo?(n.nethercote)
> >      if (fe) {
> > +        // Record size for memory reporting purposes.
> > +        fe->mUserFontSize = saneLen;
> 
> On second thoughts, Nick: would it be preferable to record
> moz_malloc_size_of(saneData) here, rather than just saneLen? That's the real
> size of the allocated block, while saneLen is just the length of the actual
> data within it. Depending on the allocation strategy used by the font
> decoder/sanitizer, the block we end up holding on to (and passing over to
> the platform APIs) is liable to have quite a bit of dead space at the end.
> (I'm seeing around 100K extra on the Fira fonts used by b.m.o, for example.)

If that memory is guaranteed to be on the heap, then yes, it's a good idea.

In fact, there's a macro MOZ_DEFINE_MALLOC_SIZE_OF_ON_ALLOC that is similar to MOZ_DEFINE_MALLOC_SIZE_OF, but it tells DMD about the existence of a block as soon as it is allocated, rather than waiting for reporting time. It's normally used in conjunction with wrapping allocators but it should also work here. So you should use a function defined with that macro to measure saneData instead of moz_malloc_size_of(). That way, if the OS holds onto that heap block, DMD will know about it. If the OS frees that block, things will also work out ok.

A comment explaining this would be helpful. E.g. augment your existing comment about how we measure this stuff in advance, using the ON_ALLOC variant to likewise let DMD know in advance.

Hopefully that's clear! Let me know if it's not.
Flags: needinfo?(n.nethercote)
Yes, it'll be heap data (specifically, it will have been allocated by moz_xmalloc or moz_xrealloc within ExpandingMemoryStream as the downloaded data was decoded and checked).

I figured a MOZ_DEFINE_MALLOC_SIZE_OF function wasn't going to be appropriate here because it wasn't being done at reporting time. So MOZ_DEFINE_MALLOC_SIZE_OF_ON_ALLOC sounds like just the thing -- thanks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5b56d7097ba3ba8b77f88b7f57c6afba7e10c78
Backout 86bea1a981f9 (bug 1268021) for causing Windows crashes on a CLOSED TREE.
https://hg.mozilla.org/mozilla-central/rev/a8d46c58ce58
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1271164
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: