Add memory reporter for Freetype on B2G / Android

RESOLVED FIXED in Firefox 18

Status

()

Core
Graphics: Text
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks: 1 bug, {qawanted})

unspecified
B2G C2 (20nov-10dec)
x86_64
Linux
qawanted
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed, firefox20 fixed)

Details

(Whiteboard: [MemShrink][soft-blocker])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 683009 [details] [diff] [review]
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → n.nethercote
I'll test for you, Nick.
Flags: needinfo?(justin.lebar+bug)
I'll test for you, Nick.
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Updated

5 years ago
Blocks: 792131
Created attachment 683360 [details] [diff] [review]
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.
Attachment #683009 - Attachment is obsolete: true
Attachment #683360 - Flags: review?(n.nethercote)
Flags: needinfo?(justin.lebar+bug)
(Assignee)

Comment 7

5 years ago
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]
(Assignee)

Comment 10

5 years ago
+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

Updated

5 years ago
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.
tracking-firefox18: --- → ?
https://hg.mozilla.org/mozilla-central/rev/3c6f2b27ad61
https://hg.mozilla.org/mozilla-central/rev/aae7ca541654
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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.
tracking-firefox18: ? → ---
(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.)

Updated

5 years ago
Attachment #683360 - Flags: approval-mozilla-beta?
Attachment #683360 - Flags: approval-mozilla-beta+
Attachment #683360 - Flags: approval-mozilla-aurora?
Attachment #683360 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/2337b9d21743
https://hg.mozilla.org/releases/mozilla-beta/rev/06744fc86dcc
status-firefox18: --- → fixed
status-firefox19: --- → fixed
status-firefox20: --- → fixed
And the followup:
https://hg.mozilla.org/releases/mozilla-aurora/rev/e6265a22c963
https://hg.mozilla.org/releases/mozilla-beta/rev/43bb139d67f4

Updated

3 years ago
See Also: → bug 997007
You need to log in before you can comment on or make changes to this bug.