Closed Bug 878674 Opened 11 years ago Closed 11 years ago

Avoid extracting bundled fonts to the filesystem, to reduce the footprint of the app on devices with limited storage

Categories

(Firefox for Android Graveyard :: General, defect)

21 Branch
ARM
Android
defect
Not set
normal

Tracking

(fennec+)

RESOLVED FIXED
Firefox 25
Tracking Status
fennec + ---

People

(Reporter: stefan, Assigned: jfkthame)

References

Details

(Keywords: productwanted)

Attachments

(2 files, 5 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0 (Beta/Release)
Build ID: 20130511120803

Steps to reproduce:

Mozilla wants to provide Firefox for ARMv6 phones, as discussed at link [0].

I am running Firefox on an ARMv6 phone running Android 2.3.4. Like most ARMv6 phones, this device doesn't have much internal storage. Firefox is very large for an Android app [1] and uses about 60MB  (including the disk cache) on my phone's internal storage by default. I can reduce this usage to about 20-30MB if I move Firefox to the SD Card and tune various `browser.cache.*` configuration settings. Even so, Firefox still consumes twice as much internal storage as the next largest app on my phone (Google Maps and Google Drive). [2]

Starting with Firefox 21, Firefox ships with several open source fonts [3]. While the fonts are nice, they consume an additional 8MB of precious internal storage. The font files themselves are larger then most apps on my phone. The fonts cannot be moved to the SD Card.

# pwd
/data/data/org.mozilla.firefox
# du -ks * | sort -n | tail -5
2       cache
9       shared_prefs
240     databases
7873    res
23649   files
# du -ks res/fonts
7871    res/fonts
#

Please allow these fonts to be moved to the SD Card if I move Firefox to my SD Card using Android's "Move to SD Card" feature. This will reduce Firefox's usage of internal storage by 8MB, make Firefox more usable on ARMv6 phones and encourage adoption of Firefox in the ARMv6 phone world.


[0] https://blog.mozilla.org/futurereleases/2012/09/07/firefox-for-android-beta-is-expanding-support-to-new-devices-help-us-test/
[1] https://support.mozilla.org/en-US/questions/947996?esab=a&s=&r=0&as=s
[2] https://www.dropbox.com/sh/7di3oh1p98bh68v/AoxNuqMKd4/2013.01.20-16.17.40.jpeg
[3] https://blog.mozilla.org/blog/2013/05/14/firefox-for-android-includes-open-source-fonts-and-html5-improvements/
OS: Windows 8 → Android
Hardware: x86_64 → ARM
Is this something we want to address?
Flags: needinfo?(blassey.bugs)
We'll talk about it at triage
tracking-fennec: --- → ?
Flags: needinfo?(blassey.bugs)
This isn't a topic I am familiar with at all, so I tried to learn a bit about it. According to http://developer.android.com/guide/topics/data/install-location.html:

"When your application is installed on the external storage:
...
    The .apk file is saved on the external storage, but all private user data, databases, optimized .dex files, and extracted native code are saved on the internal device memory."

To avoid using internal storage for the fonts, it looks to me like we'd have to do one of two things, modifying the packaged-fonts support from bug 785291:

(a) On first run, explicitly extract them to (and subsequently load them from) a path that's located on the external sdcard. This may be complicated by the fact that the mount point of the external sdcard varies between devices. Also, if the user moves the app between internal storage and external sdcard, we'd need to manually "clean up" the extracted fonts from the old location.

(b) Instead of extracting the fonts to the filesystem, as we currently do, and then loading them just like the device's installed fonts, we could modify our code to load them directly into RAM from the .apk. That would reduce the app's storage footprint in all cases, whether internal or external. However, it's possible that decompressing the font data on-the-fly would have a significant runtime performance impact, which we currently avoid by extracting/decompressing the fonts on first launch.

IMO, we should experiment with (b), as - if it works well - this provides a very "clean" solution and could benefit all devices, regardless of where the app is installed; but if it has an unacceptable performance impact whenever we use fonts (i.e. all the time!), we'd have to abandon the idea.
Jonathan - Do you have cycles to take a look at option (b)? It sounds reasonable. Brad spent some time looking into a similar solution but found Freetype did not know how to handle fonts in nested ZIPs (APK > omni.jar)

Maybe we could change the build system to put the fonts in the APK as Android assests.
tracking-fennec: ? → +
It'll probably be a few days before I get to it, but I'll try to take a look.
Assignee: nobody → jfkthame
Status: UNCONFIRMED → NEW
Ever confirmed: true
Here's my current WIP. The idea is to avoid copying the bundled fonts out to the filesystem as separate files; instead, we just decompress them into memory on demand and instantiate the FreeType face directly from the memory buffer. Not sure it's entirely review-ready yet; in particular, I need to look at how this would deal with low-memory situations where we might be unable to allocate memory for the decompressed font. Seems to work fine in my local testing, though. Tryserver job in progress at https://tbpl.mozilla.org/?tree=Try&rev=295f9431bc73.
Kats, could you maybe run some AWSY figures for the above patch? It's clear this will have some impact on memory use, because of the buffers it needs to hold decompressed font data that previously lived permanently in the filesystem, but it'd be good to have actual measurements to show how bad it is.
Flags: needinfo?(bugmail.mozilla)
Yup, will do. Thanks for remembering AWSY! :) I'll leave the needinfo flag on me until I have the numbers.
Slightly updated patch, should produce fewer crashes. :) New tryserver run: https://tbpl.mozilla.org/?tree=Try&rev=9e756a4e2fa5.
Attachment #762051 - Attachment is obsolete: true
Here is the average resident memory usage 30 seconds after startup.

Try build 10199dd93d1f (baseline):
* Samples: 4
* Average: 147651584.00

Try build 5f6c2b774ec6 (baseline + patch from comment 6):
* Samples: 4
* Average: 156015616.00

Try build 6caab19b29e0 (baseline + patch from comment 9):
* Samples: 4
* Average: 155983872.00

So there does appear to be a ~8MiB increase in resident memory usage.
Flags: needinfo?(bugmail.mozilla)
OK, thanks; that's pretty much as I'd expect, given that the patch decompresses the fonts from the APK into RAM in order to use them.

Can we afford to spend this amount of RAM here, in order to reduce our (permanent) use of internal filesystem storage by the same amount, or is that a bad tradeoff? (If it means we'll frequently run out of RAM at runtime on these ARMv6 devices, then the fact that we've reduced the storage footprint doesn't really help people!)
I think it depends on the device - some ARMv6 devices can spare the memory but not the storage, and for others it's the opposite. Are we able to run this code dynamically based on runtime detection? If we're on a device with a small amount of storage and more RAM then we could run it.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12)
> I think it depends on the device - some ARMv6 devices can spare the memory
> but not the storage, and for others it's the opposite. Are we able to run
> this code dynamically based on runtime detection? If we're on a device with
> a small amount of storage and more RAM then we could run it.

Would you like to suggest some metrics we should be using to make the decision? (E.g. don't unpack fonts to RAM if the device has less than 384MB RAM, or something. But what's an appropriate threshold?)

What about a device with relatively small amounts of both RAM -and- storage? The best chance of running successfully there would be to skip unpacking our bundled fonts altogether - either to RAM or to the filesystem - and fall back to the device's installed fonts (Droid Sans, Roboto, whatever) instead. Should we consider that a supported scenario? We'd miss out on the nice bundled fonts, but presumably a browser that runs using Droid Sans is better than a browser that simply OOMs or overfills the user's storage.
(In reply to Jonathan Kew (:jfkthame) from comment #13)
> Would you like to suggest some metrics we should be using to make the
> decision? (E.g. don't unpack fonts to RAM if the device has less than 384MB
> RAM, or something. But what's an appropriate threshold?)

I'm probably not the best person to make that call. I can tell you that for some of the other memory-saving features we've used nsMemoryImpl::IsLowMemoryPlatform, which uses a threshold of around 384MB.

> What about a device with relatively small amounts of both RAM -and- storage?
> The best chance of running successfully there would be to skip unpacking our
> bundled fonts altogether - either to RAM or to the filesystem - and fall
> back to the device's installed fonts (Droid Sans, Roboto, whatever) instead.
> Should we consider that a supported scenario? We'd miss out on the nice
> bundled fonts, but presumably a browser that runs using Droid Sans is better
> than a browser that simply OOMs or overfills the user's storage.

I agree, but that's probably a product or UX call.
Flags: needinfo?(mark.finkle)
Forgot mfinkle is on vacation, moving needinfo to blassey.
Flags: needinfo?(mark.finkle) → needinfo?(blassey.bugs)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> (In reply to Jonathan Kew (:jfkthame) from comment #13)
> > Would you like to suggest some metrics we should be using to make the
> > decision? (E.g. don't unpack fonts to RAM if the device has less than 384MB
> > RAM, or something. But what's an appropriate threshold?)
> 
> I'm probably not the best person to make that call. I can tell you that for
> some of the other memory-saving features we've used
> nsMemoryImpl::IsLowMemoryPlatform, which uses a threshold of around 384MB.

I think using nsMemoryImpl::IsLowMemoryPlatform is the right thing to do. If user feedback says we need more granularity than that, we can add it to nsMemoryImpl at some later point.

 
> > What about a device with relatively small amounts of both RAM -and- storage?
> > The best chance of running successfully there would be to skip unpacking our
> > bundled fonts altogether - either to RAM or to the filesystem - and fall
> > back to the device's installed fonts (Droid Sans, Roboto, whatever) instead.
> > Should we consider that a supported scenario? We'd miss out on the nice
> > bundled fonts, but presumably a browser that runs using Droid Sans is better
> > than a browser that simply OOMs or overfills the user's storage.
> 
> I agree, but that's probably a product or UX call.
I also agree. But as Kats says, that would be a product call. need-info to Karen now.
Flags: needinfo?(blassey.bugs) → needinfo?(krudnitski)
Keywords: productwanted
Updated patch - skips loading Fennec bundled fonts on low-memory devices, falls back to the device's installed fonts instead.
Attachment #762150 - Attachment is obsolete: true
New tryserver run at https://tbpl.mozilla.org/?tree=Try&rev=13cb5c34ae6c.

Kats, could you run AWSY numbers again, please? I believe the previous patch inadvertently leaked buffers it was using during startup, so this version may show a smaller impact (I hope).
Flags: needinfo?(bugmail.mozilla)
Using the same baseline cset as before, I tested the patch from comment 17 and got:

Samples: 4
Average: 148190208.00

This is much better than before; the regression is only ~0.5 MB now!
Flags: needinfo?(bugmail.mozilla)
Excellent, thanks. Note however that the memory usage could definitely go higher than that, though, depending on how many of the bundled fonts are actually being used at any one time. So it -could- still rise as high as about 8MB, but only when all styles of the fonts are in use. I guess your AWSY test is probably only loading a couple of faces of Open Sans, hence the smaller impact.

(The tp4m_main_rss_nochrome figures on my tryserver jobs suggest an increase of around 4MB there, which seems plausible given that the page set being used will no doubt load several of the bundled fonts, both sans-serif and serif. The memory would be released once the fonts are no longer in use and drop out of the global font cache, but as that relies on an expiration timer, it isn't immediate. So rapidly cycling through a set of pages will tend to mean an artificially large number of font instances may be around at any one time.)
Slightly tidied up patch, now passes tryserver cleanly: https://tbpl.mozilla.org/?tree=Try&rev=49867e6b4555. This reduces our footprint on the device storage by 8MB, as the bundled fonts are never extracted to the filesystem; on low-memory devices, it will skip the bundled fonts altogether and fall back to Android's installed fonts. (Product/UX decision needed on whether that's the appropriate thing to do.) If we take this patch, we should also add code to delete the existing copies of the fonts from the filesystem when updating to the new Firefox version; this could presumably be done in the front-end version migration code.
Attachment #764620 - Flags: review?(roc)
Attachment #764309 - Attachment is obsolete: true
Morphing the bug summary to better reflect the approach being taken here.
Summary: Move new fonts from internal storage to sdcard so that ARMv6 phones are better supported → Avoid extracting bundled fonts to the filesystem, to reduce the footprint of the app on devices with limited storage
Comment on attachment 764620 [details] [diff] [review]
load Fennec bundled fonts directly from omnijar without copying them out to the filesystem

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

::: gfx/thebes/gfxFT2FontList.cpp
@@ +978,5 @@
> +        "res/fonts/*.ttf$",
> +    };
> +    nsRefPtr<nsZipArchive> reader = Omnijar::GetReader(Omnijar::Type::GRE);
> +    for (unsigned i = 0; i < ArrayLength(sJarSearchPaths); i++) {
> +        nsZipFind* find;

Shouldn't this be nsAutoPtr? We seem to be leaking the object.

@@ +983,5 @@
> +        reader->FindInit(sJarSearchPaths[i], &find);
> +        const char* path;
> +        uint16_t len;
> +        while (NS_SUCCEEDED(find->FindNext(&path, &len))) {
> +            nsCString entryName(path, len);

nsDependentCString?
Attachment #764620 - Flags: review?(roc) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23)
> Comment on attachment 764620 [details] [diff] [review]
> load Fennec bundled fonts directly from omnijar without copying them out to
> the filesystem
> 
> Review of attachment 764620 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxFT2FontList.cpp
> @@ +978,5 @@
> > +        "res/fonts/*.ttf$",
> > +    };
> > +    nsRefPtr<nsZipArchive> reader = Omnijar::GetReader(Omnijar::Type::GRE);
> > +    for (unsigned i = 0; i < ArrayLength(sJarSearchPaths); i++) {
> > +        nsZipFind* find;
> 
> Shouldn't this be nsAutoPtr? We seem to be leaking the object.

So we are. Seems easier to explicitly "delete find;" afterwards than to pass an nsAutoPtr to FindInit's outparam, though, so I'll do it that way.

We should also check the FindInit call for success, to avoid the risk of trying to use a null pointer if it fails.

> @@ +983,5 @@
> > +        reader->FindInit(sJarSearchPaths[i], &find);
> > +        const char* path;
> > +        uint16_t len;
> > +        while (NS_SUCCEEDED(find->FindNext(&path, &len))) {
> > +            nsCString entryName(path, len);
> 
> nsDependentCString?

No, because the pathname in the zip directory indicated by the pointer/length pair is not null-terminated.
Updated patch to address review comments; also added missing 'const' to a bunch of parameters where we're passing nsCString& references around. Carrying forward r=roc.
Attachment #764620 - Attachment is obsolete: true
Attachment #764700 - Flags: review+
Karen, to summarize the issues here:

(a) The patch reduces our permanent footprint on device storage by around 8MB. The tradeoff is slightly higher RAM usage at runtime - it'll likely be less than a megabyte much of the time, but could temporarily peak nearer 8MB in the worst-case scenario where the user manages to use -all- our bundled font faces at once.

(b) To avoid increasing pressure on RAM for low-memory devices, the patch disables use of our bundled fonts when IsLowMemoryPlatform() returns true (currently this means devices with less than 384MB of RAM). In this case, we'll fall back to using the device's installed Droid Sans (or Roboto) and Droid Serif fonts instead of our bundled ones.

Is this a reasonable position? The aim is to benefit everyone (via a substantial decrease in the application footprint) while still degrading gracefully on resource-limited devices.
This is fantastic work, and I agree with the approach and position. 

I can also assume that the trade-off in (a) would be an edge case, as I can't imagine many scenarios where all fonts would be used at once.
Flags: needinfo?(krudnitski)
Comment on attachment 765314 [details] [diff] [review]
pt 2 - clean up obsolete copies of packaged fonts from the Android filesystem

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

::: mobile/android/base/GeckoApp.java
@@ +1559,5 @@
> +        // to deal with "residue" from older versions.
> +        // The migration-version setting is recorded to avoid repeating the same
> +        // tasks on subsequent startups; CURRENT_MIGRATION_VERSION may be updated
> +        // if we need to do additional cleanup for future Gecko versions.
> +        ThreadUtils.getBackgroundHandler().postDelayed(new Runnable() {

I'd like to move this into ProfileMigrator.java, just so all of our migration code is in one place (and to avoid GeckoApp being a dumping ground). There is already similar code there for cleaning up the extracted library files

https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ProfileMigrator.java#673

@@ +1596,5 @@
> +                    editor.putInt(getPackageName() + MIGRATION_VERSION, CURRENT_MIGRATION_VERSION);
> +                    editor.apply();
> +                }
> +            }
> +        }, 2000);

2s will probably land right in the middle of page load. It is entirely arbitrary, but let's go with something a bit longer, like 15s so we can hopefully avoid adversely affecting the user's experience.
Attachment #765314 - Flags: review?(blassey.bugs) → review+
Updated patch to move the cleanup code to ProfileMigrator.java, and defer until 15s after startup, as suggested. With this, the installed size of my Fennec build (as reported in Settings/Apps) drops by around 8MB, as expected. Functionally equivalent to the previously-reviewed patch, but flagging for re-review to check that I haven't done anything too silly in moving the code around (given that I've never hacked on Java code before).
Attachment #766600 - Flags: review?(blassey.bugs)
Attachment #765314 - Attachment is obsolete: true
Comment on attachment 766600 [details] [diff] [review]
pt 2 - clean up obsolete copies of packaged fonts from the Android filesystem.

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

Good stuff, thanks for the updated patch.
Attachment #766600 - Flags: review?(blassey.bugs) → review+
Backed out for java exceptions on startup:
https://tbpl.mozilla.org/php/getParsedLog.php?id=24560087&tree=Mozilla-Inbound
{
PROCESS-CRASH | java-exception | java.lang.NoSuchMethodError: android.content.SharedPreferences$Editor.applyat org.mozilla.gecko.ProfileMigrator$DeferredCleanupTask.run(ProfileMigrator.java:746)
}

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fc4ef42eb153
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1d18a9213af9
Sigh... it seems Android 2.2 doesn't have SharedPreferences.Editor.apply(), and I'd only tested on Android 4.x. Back to tryserver to verify a fix...
Replaced the use of SharedPreferences.Editor.apply() with commit(), which is available on older Android versions. (The same issue came up in bug 786826, for example.)

Try run: https://tbpl.mozilla.org/?tree=Try&rev=5ded680378fb

Re-landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7aaddad27d17
https://hg.mozilla.org/integration/mozilla-inbound/rev/48e25edf3e28
This generated Android Talos alerts for RSS in dev-tree-management Digest, Vol 54, Issue 117:

Subject: <Regression> Mozilla-Inbound - Tp4 Mobile NoChrome (Main RSS)
        -        Android 4.0.4 - 2.78%

Regression: Mozilla-Inbound - Tp4 Mobile NoChrome (Main RSS) - Android 4.0.4 - 2.78% increase
---------------------------------------------------------------------------------------------
    Previous: avg 157345166.667 stddev 314821.113 of 12 runs up to revision 05ba2e9812df
    New     : avg 161713666.667 stddev 502909.776 of 12 runs since revision 48e25edf3e28
    Change  : +4368500.000 (2.78% / z=13.876)
    Graph   : http://mzl.la/10k0PVs

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=05ba2e9812df&tochange=48e25edf3e28


Subject: <Regression> Mozilla-Inbound - Tp4 Mobile NoChrome (Main RSS)
        -        Android 2.2 (Native) - 3.46%

Regression: Mozilla-Inbound - Tp4 Mobile NoChrome (Main RSS) - Android 2.2 (Native) - 3.46% increase
----------------------------------------------------------------------------------------------------
    Previous: avg 124049666.667 stddev 740557.079 of 12 runs up to revision 05ba2e9812df
    New     : avg 128339000.000 stddev 336715.148 of 12 runs since revision 48e25edf3e28
    Change  : +4289333.333 (3.46% / z=5.792)
    Graph   : http://mzl.la/10k33El

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=05ba2e9812df&tochange=48e25edf3e28


This seems consistent with the expected impact stated in Comment 26.
Yes, this looks very much in line with expectations. (BTW, note that because Tp4 cycles quickly through a variety of pages, it probably ends up with more distinct font faces "alive" at one time than users would experience during typical browsing, and so the impact is somewhat exaggerated in the Tp4 context.)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: