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?

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.

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.

I'll test for you, Nick.

I'll test for you, Nick.

(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.

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 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 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.

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?

+static void +*CountingAlloc(FT_Memory memory, long size) That's certainly a creative placement of the '*'! :)

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

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.

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

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.

(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.

(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.

> How would a regression rear its head? Probably startup crashes.

(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.

(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.

(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.)

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

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