812957 - Add memory reporter for Freetype on B2G / Android

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Nicholas Nethercote [inactive]]

The profiles in bug 810105 indicate that on B2G we have multiple MiBs of memory being taken up by Freetype, from allocations with stack traces like this:

  ft_alloc /home/jlebar/code/moz/ff-git2/src/modules/freetype2/src/base/ftsystem.c:74
  ft_mem_qalloc /home/jlebar/code/moz/ff-git2/src/modules/freetype2/src/base/ftutil.c:76
  FT_Stream_EnterFrame /home/jlebar/code/moz/ff-git2/src/modules/freetype2/src/base/ftstream.c:267
  FT_Stream_ExtractFrame /home/jlebar/code/moz/ff-git2/src/modules/freetype2/src/base/ftstream.c:200
  tt_face_load_kern /home/jlebar/code/moz/ff-git2/src/modules/freetype2/src/sfnt/ttkern.c:68
  sfnt_load_face /home/jlebar/code/moz/ff-git2/src/modules/freetype2/src/sfnt/sfobjs.c:748
  tt_face_init /home/jlebar/code/moz/ff-git2/src/modules/freetype2/src/truetype/ttobjs.c:537
  open_face /home/jlebar/code/moz/ff-git2/src/modules/freetype2/src/base/ftobjs.c:1153
  FT_Open_Face /home/jlebar/code/moz/ff-git2/src/modules/freetype2/src/base/ftobjs.c:2080
  FT_New_Face /home/jlebar/code/moz/ff-git2/src/modules/freetype2/src/base/ftobjs.c:1215
  gfxFT2FontList::AppendFacesFromFontFile(nsCString&, bool, FontNameCache*) /home/jlebar/code/moz/ff-git2/src/gfx/thebes/gfxFT2FontList.cpp:797
  ~nsACString_internal /home/jlebar/code/moz/B2G/objdir-gecko/dist/include/nsTSubstring.h:85
  ~nsCString /home/jlebar/code/moz/B2G/objdir-gecko/dist/include/nsTString.h:22
  ?? /home/jlebar/code/moz/ff-git2/src/gfx/thebes/gfxFT2FontList.cpp:1164
  gfxFT2FontList::FindFonts() /home/jlebar/code/moz/B2G/objdir-gecko/dist/include/nsTHashtable.h:131
  gfxFT2FontList::InitFontList() /home/jlebar/code/moz/ff-git2/src/gfx/thebes/gfxFT2FontList.cpp:1224
  gfxAndroidPlatform::CreatePlatformFontList() /home/jlebar/code/moz/B2G/objdir-gecko/dist/include/nsError.h:155

I see two ways forward:

- We could add a memory reporter for Freetype.  It's 3rd-party code, but provides a way to supply a custom allocator, which should let us avoid modifying the in-tree copy of the code.

- Perhaps this memory shouldn't be allocated.  Are we doing something excessive with fonts?

Comment 1

[Karl Tomlinson (:karlt)]

I think the gfxFT2Font* code was designed assuming the system didn't have many fonts, and so didn't need optimization of which fonts are open.

Usually cairo tries not to keep too many fonts open at a time, but that is only supported through its fontconfig interface.  On Android/Gonk, we don't want to use fontconfig, which leaves us needing an FT_Face open for a cairo font.

However, the font list code doesn't need a cairo font, it could use some other object that doesn't keep the FT_Face open.

CC'ing gw280 as this may affect how best to use FT fonts with skia.  Rather than adding new API to cairo, it might pay to consider our (or skia's) own face wrapper object that opens fonts only when they are needed.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Add a memory reporter for Freetype on Android.

This patch provides a custom allocator to Freetype which counts how much
memory it uses.  A new memory reporter called "explicit/freetype" is
implemented using this counter.

I don't have access to Android or b2g-device builds.  And I can't test similar
code on desktop because that requires --disable-pango and those builds are
currently broken on desktop (bug 703042).

Can someone who does have Android/b2g-device build access (kats?) try this
patch and see if it's giving sensible results?  I have confirmed that it
builds on all the try server b2g/arm platforms.

Comment 3

[Justin Lebar (not reading bugmail)]

I'll test for you, Nick.

Comment 4

[Justin Lebar (not reading bugmail)]

I'll test for you, Nick.

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #0)
> The profiles in bug 810105 indicate that on B2G we have multiple MiBs of
> memory being taken up by Freetype, from allocations with stack traces like
> this:

I looked more closely, and added up 2,085,825 bytes in stacks similar to the mentioned one.

Comment 6

[Justin Lebar (not reading bugmail)]

Attachment: Freetype memory reporter, v2

r=me on your patch; r=you on these changes?

I put the reporter in explicit/freetype, btw; it had been in plain ol'
"freetype", but that didn't seem like what you wanted.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 683360 [details] [diff] [review]
Freetype memory reporter, v2

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

r=me, but I'd like karlt to review the Freetype bits (esp. the call to FT_Done_Library).

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 683360 [details] [diff] [review]
Freetype memory reporter, v2

>+void *CountingAlloc(FT_Memory memory, long size)

>+void CountingFree(FT_Memory memory, void* p)

>+void *CountingRealloc(FT_Memory memory, long cur_size, long new_size, void* p)

Use internal linkage for file-local functions.
Style is to put function names at the beginning of a new line.

r=me on the FreeType interaction.

Comment 9

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c6f2b27ad61

This is a simple change which gives us important insight into B2G's memory usage.  Only affects B2G and Fennec.  soft-blocking+.

I don't know how to land B2G changes anymore; they don't go on beta, do they?

Comment 10

[Nicholas Nethercote [inactive]]

+static void
+*CountingAlloc(FT_Memory memory, long size)

That's certainly a creative placement of the '*'! :)

Comment 11

[Justin Lebar (not reading bugmail)]

Heh, whoops.

https://hg.mozilla.org/integration/mozilla-inbound/rev/aae7ca541654

Comment 12

[Alex Keybl [:akeybl]]

The fix for this bug has been called out as possibly having greater than normal risk to non-B2G platforms. Moving into the C2 milestone. We should try to resolve as soon as possible on mozilla-beta, to prevent the need for branching B2G from mozilla-beta earlier than planned.

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3c6f2b27ad61
https://hg.mozilla.org/mozilla-central/rev/aae7ca541654

Comment 14

[Justin Lebar (not reading bugmail)]

Comment on attachment 683360 [details] [diff] [review]
Freetype memory reporter, v2

[Approval Request Comment]
B2G and Android-only change.  Needed to measure memory usage on B2G, particularly since we aim to reduce the memory usage of Freetype.  (We need to measure before we can fix it.)

Relatively low-risk for Android, although I don't think the risk is acceptable for beta, under normal circumstances.

Comment 15

[George Wright (:gw280) (needinfo me!)]

(In reply to Karl Tomlinson (:karlt) from comment #1)
> CC'ing gw280 as this may affect how best to use FT fonts with skia.  Rather
> than adding new API to cairo, it might pay to consider our (or skia's) own
> face wrapper object that opens fonts only when they are needed.

Are we using Skia on B2G right now? Skia's font code isn't particularly great, and in fact, we will probably end up caching twice when using it (once within Cairo and once within Skia) as Skia's font code will be a thin wrapper around Cairo's. However, Skia's "cache" should just be a reference to the cairo object, so should be fairly lightweight; Cairo will cache the actual font data.

Comment 16

[Alex Keybl [:akeybl]]

(In reply to Justin Lebar [:jlebar] from comment #14)
> Comment on attachment 683360 [details] [diff] [review]
> Freetype memory reporter, v2
> 
> [Approval Request Comment]
> Relatively low-risk for Android, although I don't think the risk is
> acceptable for beta, under normal circumstances.

How would a regression rear its head? Including Aaron to help with exploratory testing, once answered. We'll wait a day on m-c before approving for Aurora/Beta.

Comment 17

[Justin Lebar (not reading bugmail)]

> How would a regression rear its head?

Probably startup crashes.

Comment 18

[Karl Tomlinson (:karlt)]

(In reply to George Wright (:gw280) from comment #15)
> (In reply to Karl Tomlinson (:karlt) from comment #1)
> > CC'ing gw280 as this may affect how best to use FT fonts with skia.  Rather
> > than adding new API to cairo, it might pay to consider our (or skia's) own
> > face wrapper object that opens fonts only when they are needed.
> 
> Are we using Skia on B2G right now?

I assume not.  I was just CC'ing you to keep you up to date with issues that are relevant to using FreeType fonts, as they might affect the direction of skia integration work.

> However, Skia's "cache" should just be a reference
> to the cairo object, so should be fairly lightweight; Cairo will cache the
> actual font data.

Yes, though the issue in this bug is that Cairo faces are keeping FT_Faces alive (when used the way gfxFT2Fonts uses cairo) and these FT_Faces use a bit of memory if there are many of them.

Comment 19

[Alex Keybl [:akeybl]]

(In reply to Justin Lebar [:jlebar] from comment #17)
> > How would a regression rear its head?
> 
> Probably startup crashes.

In that case, it'd be easy to find regressions caused by this patch.

Comment 20

[Jonathan Kew [:jfkthame]]

(In reply to Justin Lebar [:jlebar] from comment #17)
> > How would a regression rear its head?
> 
> Probably startup crashes.

Or possibly crashes during shutdown? (Which might be less obvious.)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2337b9d21743
https://hg.mozilla.org/releases/mozilla-beta/rev/06744fc86dcc

Comment 22

[Ryan VanderMeulen [:RyanVM]]

And the followup:
https://hg.mozilla.org/releases/mozilla-aurora/rev/e6265a22c963
https://hg.mozilla.org/releases/mozilla-beta/rev/43bb139d67f4