Closed Bug 812957 Opened 7 years ago Closed 7 years ago

Add memory reporter for Freetype on B2G / Android

Categories

(Core :: Graphics: Text, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
B2G C2 (20nov-10dec)
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

(Keywords: qawanted, Whiteboard: [MemShrink][soft-blocker])

Attachments

(1 file, 1 obsolete file)

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?
Whiteboard: [MemShrink]
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.
Assignee: nobody → n.nethercote
I'll test for you, Nick.
Flags: needinfo?(justin.lebar+bug)
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.
Blocks: 256meg
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.
Attachment #683009 - Attachment is obsolete: true
Attachment #683360 - Flags: review?(n.nethercote)
Flags: needinfo?(justin.lebar+bug)
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).
Attachment #683360 - Flags: review?(n.nethercote)
Attachment #683360 - Flags: review?(karlt)
Attachment #683360 - Flags: review+
Summary: Do something about Freetype fonts on B2G → Add memory reporter for Freetype on B2G / Android
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.
Attachment #683360 - Flags: review?(karlt) → review+
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?
blocking-basecamp: --- → +
Whiteboard: [MemShrink] → [MemShrink][soft-blocker]
+static void
+*CountingAlloc(FT_Memory memory, long size)

That's certainly a creative placement of the '*'! :)
Target Milestone: --- → B2G C2 (20nov-10dec)
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 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.
Attachment #683360 - Flags: approval-mozilla-beta?
Attachment #683360 - Flags: approval-mozilla-aurora?
(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.
Keywords: qawanted
QA Contact: aaron.train
> 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.)
Attachment #683360 - Flags: approval-mozilla-beta?
Attachment #683360 - Flags: approval-mozilla-beta+
Attachment #683360 - Flags: approval-mozilla-aurora?
Attachment #683360 - Flags: approval-mozilla-aurora+
See Also: → 997007
You need to log in before you can comment on or make changes to this bug.