Last Comment Bug 844669 - Reduce start-up memory consumption hit caused by bug 831354
: Reduce start-up memory consumption hit caused by bug 831354
Status: VERIFIED FIXED
[MemShrink]
:
Product: Firefox for Android
Classification: Client Software
Component: Theme and Visual Design (show other bugs)
: unspecified
: All Android
: -- normal (vote)
: Firefox 22
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
Depends on:
Blocks: 256meg 831354
  Show dependency treegraph
 
Reported: 2013-02-24 15:46 PST by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2013-03-28 13:37 PDT (History)
22 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
21+


Attachments
pt 1 - don't keep freetype faces for all fonts around after building the font list on first-run (6.54 KB, patch)
2013-02-26 12:44 PST, Jonathan Kew (:jfkthame)
blassey.bugs: review+
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Review
pt 2 - use a transient FT_Face to read font tables such as cmap, if the font entry does not already have an existing face (5.95 KB, patch)
2013-02-26 12:44 PST, Jonathan Kew (:jfkthame)
blassey.bugs: review+
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Review
pt 3 - implement a simple GetCommonFallbackFonts for Android, to reduce the chances of hitting the global fallback codepath and loading all cmaps (4.01 KB, patch)
2013-02-26 13:20 PST, Jonathan Kew (:jfkthame)
no flags Details | Diff | Review
pt 3 - implement a simple GetCommonFallbackFonts for Android, to reduce the chances of hitting the global fallback codepath and loading all cmaps (4.01 KB, patch)
2013-02-26 13:46 PST, Jonathan Kew (:jfkthame)
blassey.bugs: review+
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2013-02-24 15:46:40 PST
Bug 831354 added two fonts.  This apparently made text easier to read, but increased the amount of memory in the about:memory's "freetype" bucket at start-up (according to AWSY) from 0.60 to 2.18 MiB, i.e. by 1.58 MiB.

This was known and deemed acceptable... but it's quite a chunk when we're trying to get Firefox to work well on 256 MiB phones.

Some questions that jkew might know the answers to:
- How many fonts did we have before that?
- Are these new fonts especially large?
- Can they be shrunk at all?
- Do we know how the freetype memory is divided up?
Comment 1 Jonathan Kew (:jfkthame) 2013-02-25 15:04:26 PST
Previously, we weren't shipping any fonts as part of the fennec package (ignoring the reader-mode fonts, which were only loaded "on demand" through @font-face).

So at startup, we'd be using the fonts that are installed as part of the device's OS; we'll build a list of all the fonts (typically about 30 or 40 faces, I think, depending on Android version), but initially we'll probably only need to instantiate Roboto or Droid Sans, and maybe Droid Serif, perhaps in both regular and bold. These are fairly small fonts (less than 200K per file).

With the added fonts (two families, containing a total of 10 faces), we're now going to instantiate Open Sans and Charis SIL instead - or rather, because of bug 754215, we probably instantiate these *as well as* the Droid fonts, as those are still present in our fallback list.

Open Sans is not all that much larger than Droid Sans (220K per face, roughly), though the fact that it comes with separate italic faces for each weight means that we may well be instantiating a larger number of distinct faces. Charis SIL, however, is -much- larger than Droid Serif - it's about 1.6MB per face. In large part, this is because it is a far more comprehensive font, with over 4600 glyphs compared to Droid's 900.

I have not investigated freetype's memory handling; depending how much of the font data it decides to load into memory up-front, it may well be that making any use whatsoever of a large font like Charis causes a significant memory hit. If this is a serious concern, I could try to look into it in more detail and see if, for example, we could make it lazier about loading the font data.
Comment 2 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-02-25 17:24:15 PST
> If this is a serious concern, I could try to look into it in
> more detail and see if, for example, we could make it lazier about loading
> the font data.

If you could spend at least a short time to see what you can learn, that would be very helpful.  Thanks.
Comment 3 Jonathan Kew (:jfkthame) 2013-02-26 06:48:30 PST
It looks like there are a whole bunch of things going on here! And yes, we'll be able to do some things to mitigate the issue. Some notes on findings so far:

1. Freetype memory consumption is significantly higher on the first startup after a new build is installed, because on first startup we read details of all fonts, and once we've done that we hold on to the FT_Face objects we created. We probably should release those as soon as we've loaded the font details we need, and then re-create them only if the fonts are actually used.

1a. The above only applies to first-run, since we cache the font list in the startup cache and thus avoid actually reading all the font files on subsequent startups.

1b. However, at least in my local build, access to the startup cache was failing, so that we were hitting the "first-run" case every time. I'm not sure why that was. After completely uninstalling the browser and reinstalling, I have not been able to reproduce that problem. (I suspected it might have been related to bug 842262, but attempting to reproduce the behavior by applying and removing the patch there did *not* confirm that, so perhaps my installation was broken in some other way.)

2. Opening "about:memory" is itself problematic, because the "box-drawing" characters it uses are not present in the default text fonts we're using; as a result, they hit the global font fallback code path, which requires us to load character maps for all the fonts. We can mitigate this by implementing GetCommonFallbackFonts in gfxAndroidPlatform, so that we'll try likely fonts such as Droid Sans Fallback before resorting to the read-all-fonts option.

3. When we do decide to read a font's character map, we create an FT_Face in order to access the data, and that FT_Face is then permanently held open. Given that in many cases, we read the cmap but then never actually instantiate and use the font, we should not be keeping those FT_Face objects around. This will make much of the cmap-reading memory usage become transient instead of permanent.

With these issues fixed, I think you'll see the freetype "bucket" drop back to a much smaller size. (How do I see these details on AWSY, btw? Just loading http://www.arewesmallyet.com/ doesn't show anything about Android...)
Comment 4 Andrew McCreight [:mccr8] 2013-02-26 07:48:19 PST
By "AWSY", Nick actually means https://areweslimyet.com/  He assumes a greater awareness of what that stands for than is the case. ;)  From there, click on "mobile", then you'll need to zoom in a little, then you can click on an individual circle to get a detailed about:memory breakdown.
Comment 5 Jonathan Kew (:jfkthame) 2013-02-26 08:00:44 PST
Ah, thanks ... "S" = slim, not small. Excuse my confusion! Yes, that's much more useful.
Comment 6 Kartikaya Gupta (email:kats@mozilla.com) 2013-02-26 08:09:05 PST
(In reply to Jonathan Kew (:jfkthame) from comment #3)
> 2. Opening "about:memory" is itself problematic, because the "box-drawing"
> characters it uses are not present in the default text fonts we're using; as
> a result, they hit the global font fallback code path, which requires us to
> load character maps for all the fonts. We can mitigate this by implementing
> GetCommonFallbackFonts in gfxAndroidPlatform, so that we'll try likely fonts
> such as Droid Sans Fallback before resorting to the read-all-fonts option.

You can also obtain the details of the about:memory dump in a JSON file without rendering it in the browser (that's what the data obtained on AWSY comes from). Details at https://wiki.mozilla.org/Mobile/Fennec/Android#about:memory
Comment 7 Jonathan Kew (:jfkthame) 2013-02-26 12:44:15 PST
Created attachment 718594 [details] [diff] [review]
pt 1 - don't keep freetype faces for all fonts around after building the font list on first-run
Comment 8 Jonathan Kew (:jfkthame) 2013-02-26 12:44:49 PST
Created attachment 718596 [details] [diff] [review]
pt 2 - use a transient FT_Face to read font tables such as cmap, if the font entry does not already have an existing face
Comment 9 Jonathan Kew (:jfkthame) 2013-02-26 13:20:01 PST
Created attachment 718628 [details] [diff] [review]
pt 3 - implement a simple GetCommonFallbackFonts for Android, to reduce the chances of hitting the global fallback codepath and loading all cmaps
Comment 10 Jonathan Kew (:jfkthame) 2013-02-26 13:31:59 PST
Tryserver build with these three patches in progress at https://tbpl.mozilla.org/?tree=Try&rev=542eb0405bd3. In my local testing, this brings the freetype memory usage after startup down to pre-bug 831354 levels, or even somewhat less.
Comment 11 Jonathan Kew (:jfkthame) 2013-02-26 13:46:08 PST
Created attachment 718637 [details] [diff] [review]
pt 3 - implement a simple GetCommonFallbackFonts for Android, to reduce the chances of hitting the global fallback codepath and loading all cmaps

Oops, stupid typo in previous version :( ... this version should work.
Comment 12 Jonathan Kew (:jfkthame) 2013-02-26 13:49:03 PST
https://tbpl.mozilla.org/?tree=Try&rev=13212b17ad3e
Comment 13 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-02-26 15:40:01 PST
> 1. Freetype memory consumption is significantly higher on the first startup
> after a new build is installed, because on first startup we read details of
> all fonts

Ok.  That's probably affecting the AWSY results, since I assume we do one build, one run, for each mozilla-inbound push.  I wonder if that's skewing desktop AWSY runs as well.

> 2. Opening "about:memory" is itself problematic, because the "box-drawing"
> characters it uses are not present in the default text fonts we're using; as
> a result, they hit the global font fallback code path, which requires us to
> load character maps for all the fonts.

As kats said, we don't load about:memory but access the underlying memory reporters directly.  So this won't be a factor in the AWSY numbers.


Thanks for investigating!  Sorry about the "AWSY" confusion, it appears that arewesmallyet.com is tracking Firefox's binary size, or something like that.  Huh.
Comment 14 Jonathan Kew (:jfkthame) 2013-02-27 00:57:35 PST
(In reply to Nicholas Nethercote [:njn] from comment #13)
> > 1. Freetype memory consumption is significantly higher on the first startup
> > after a new build is installed, because on first startup we read details of
> > all fonts
> 
> Ok.  That's probably affecting the AWSY results, since I assume we do one
> build, one run, for each mozilla-inbound push.  I wonder if that's skewing
> desktop AWSY runs as well.

As far as fonts are concerned, at least, it shouldn't be a factor on desktop, as the use of the startupcache for font info is (currently) mobile-only. But of course there might be other areas where the use (or not) of startupcache is significant.
Comment 15 Brad Lassey [:blassey] (use needinfo?) 2013-03-04 05:56:15 PST
Comment on attachment 718596 [details] [diff] [review]
pt 2 - use a transient FT_Face to read font tables such as cmap, if the font entry does not already have an existing face

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

::: gfx/thebes/gfxFT2FontList.cpp
@@ +120,5 @@
> +    }
> +
> +    operator FT_Face() { return mFace; }
> +
> +    FT_Face Forget() {

nit, lowercase "f" to be consistent with nsAutoPtr
Comment 17 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-04 14:19:10 PST
jkew: any estimate on the difference these patches make?  I don't see much change on AWSY/mobile.
Comment 18 Jonathan Kew (:jfkthame) 2013-03-04 15:08:58 PST
In my local testing, they reduced the freetype bucket immediately after startup from about 2.5MB to 0.6MB, IIRC. They're not likely to make any significant difference to longer-term memory use, as all (or most) of the available fonts will eventually get loaded, but they should reduce the number that are loaded and held in memory immediately on startup.

(Do they show on AWSY yet? The newest mobile results I see there are nearly a week old...)
Comment 19 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-04 15:23:36 PST
(In reply to Jonathan Kew (:jfkthame) from comment #18)
> In my local testing, they reduced the freetype bucket immediately after
> startup from about 2.5MB to 0.6MB, IIRC.

Good.

> (Do they show on AWSY yet? The newest mobile results I see there are nearly
> a week old...)

True.  But if you zoom in there's a big drop, which I assumed was caused by the graphite pref disabling in bug 700023, but that only just landed...
Comment 20 Kartikaya Gupta (email:kats@mozilla.com) 2013-03-04 15:26:35 PST
Yeah the AWSY/mobile data are on hold at the moment while I fix the heap-unclassified data and regenerate backlog. The last results *are* from about a week ago, and it'll take a couple of days to catch up probably once I get it going again.
Comment 22 Geoff Brown [:gbrown] 2013-03-06 06:51:55 PST
Talos approves:

(from dev-tree-management Digest, Vol 51, Issue 8)

Improvement: mobile - Tp4 Mobile NoChrome (Main RSS) - Android 2.2 (Native) - 2.36% decrease
--------------------------------------------------------------------------------------------
    Previous: avg 93508093.333 stddev 739686.056 of 30 runs up to revision 55477ae32280
    New     : avg 91300680.000 stddev 379142.740 of 5 runs since revision 9f18d05d1ba1
    Change  : -2207413.333 (2.36% / z=2.984)
    Graph   : http://mzl.la/103qH5v
Comment 23 Brad Lassey [:blassey] (use needinfo?) 2013-03-12 09:20:02 PDT
Comment on attachment 718594 [details] [diff] [review]
pt 1 - don't keep freetype faces for all fonts around after building the font list on first-run

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 831354 
User impact if declined: we use more memory
Testing completed (on m-c, etc.): on nightly
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:
Comment 24 Brad Lassey [:blassey] (use needinfo?) 2013-03-12 09:20:33 PDT
Comment on attachment 718596 [details] [diff] [review]
pt 2 - use a transient FT_Face to read font tables such as cmap, if the font entry does not already have an existing face

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 831354 
User impact if declined: we use more memory
Testing completed (on m-c, etc.): on nightly
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:
Comment 25 Brad Lassey [:blassey] (use needinfo?) 2013-03-12 09:21:13 PDT
Comment on attachment 718637 [details] [diff] [review]
pt 3 - implement a simple GetCommonFallbackFonts for Android, to reduce the chances of hitting the global fallback codepath and loading all cmaps

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 831354 
User impact if declined: we use more memory
Testing completed (on m-c, etc.): on nightly
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:
Comment 26 bhavana bajaj [:bajaj] 2013-03-12 10:27:57 PDT
Comment on attachment 718594 [details] [diff] [review]
pt 1 - don't keep freetype faces for all fonts around after building the font list on first-run

Helps avoid extra memory usage due to landing of new fonts in Bug 831354
Comment 28 Kartikaya Gupta (email:kats@mozilla.com) 2013-03-28 13:37:34 PDT
AWSY confirms that these patches reduced "fresh start" memory usage by ~2.86 MiB and "fresh start +30s" memory usage by ~2.97 MiB. Thanks!

72854a54aac64519d1f8a098499766b3da2066e6 100.80MiB Δ 248.00KiB
14493f3ee798f16f22fe7630c56f4c7fc4e79a1d 97.95MiB Δ -2.86MiB

72854a54aac64519d1f8a098499766b3da2066e6 110.34MiB Δ 24.00KiB
14493f3ee798f16f22fe7630c56f4c7fc4e79a1d 107.37MiB Δ -2.97MiB

Note You need to log in before you can comment on or make changes to this bug.