Closed
Bug 878674
Opened 12 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)
Tracking
(fennec+)
RESOLVED
FIXED
Firefox 25
Tracking | Status | |
---|---|---|
fennec | + | --- |
People
(Reporter: stefan, Assigned: jfkthame)
References
Details
(Keywords: productwanted)
Attachments
(2 files, 5 obsolete files)
45.72 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
6.52 KB,
patch
|
blassey
:
review+
|
Details | Diff | Splinter Review |
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/
Reporter | ||
Updated•12 years ago
|
OS: Windows 8 → Android
Hardware: x86_64 → ARM
Comment 2•12 years ago
|
||
We'll talk about it at triage
tracking-fennec: --- → ?
Flags: needinfo?(blassey.bugs)
Assignee | ||
Comment 3•12 years ago
|
||
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.
Comment 4•12 years ago
|
||
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: ? → +
Assignee | ||
Comment 5•12 years ago
|
||
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
Assignee | ||
Comment 6•12 years ago
|
||
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.
Assignee | ||
Comment 7•12 years ago
|
||
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)
Comment 8•12 years ago
|
||
Yup, will do. Thanks for remembering AWSY! :) I'll leave the needinfo flag on me until I have the numbers.
Assignee | ||
Comment 9•12 years ago
|
||
Slightly updated patch, should produce fewer crashes. :) New tryserver run: https://tbpl.mozilla.org/?tree=Try&rev=9e756a4e2fa5.
Assignee | ||
Updated•12 years ago
|
Attachment #762051 -
Attachment is obsolete: true
Comment 10•12 years ago
|
||
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)
Assignee | ||
Comment 11•12 years ago
|
||
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!)
Comment 12•12 years ago
|
||
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.
Assignee | ||
Comment 13•12 years ago
|
||
(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.
Comment 14•12 years ago
|
||
(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)
Comment 15•11 years ago
|
||
Forgot mfinkle is on vacation, moving needinfo to blassey.
Flags: needinfo?(mark.finkle) → needinfo?(blassey.bugs)
Comment 16•11 years ago
|
||
(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
Assignee | ||
Comment 17•11 years ago
|
||
Updated patch - skips loading Fennec bundled fonts on low-memory devices, falls back to the device's installed fonts instead.
Assignee | ||
Updated•11 years ago
|
Attachment #762150 -
Attachment is obsolete: true
Assignee | ||
Comment 18•11 years ago
|
||
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)
Comment 19•11 years ago
|
||
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)
Assignee | ||
Comment 20•11 years ago
|
||
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.)
Assignee | ||
Comment 21•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #764309 -
Attachment is obsolete: true
Assignee | ||
Comment 22•11 years ago
|
||
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+
Assignee | ||
Comment 24•11 years ago
|
||
(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.
Assignee | ||
Comment 25•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Attachment #764620 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #764700 -
Flags: review+
Assignee | ||
Comment 26•11 years ago
|
||
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.
Comment 27•11 years ago
|
||
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)
Assignee | ||
Comment 28•11 years ago
|
||
Attachment #765314 -
Flags: review?(blassey.bugs)
Comment 29•11 years ago
|
||
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+
Assignee | ||
Comment 30•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #765314 -
Attachment is obsolete: true
Comment 31•11 years ago
|
||
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+
Assignee | ||
Comment 32•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8284007baae4
https://hg.mozilla.org/integration/mozilla-inbound/rev/30bf232b44f7
Target Milestone: --- → Firefox 25
Comment 33•11 years ago
|
||
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
Assignee | ||
Comment 34•11 years ago
|
||
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...
Assignee | ||
Comment 35•11 years ago
|
||
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
Comment 36•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7aaddad27d17
https://hg.mozilla.org/mozilla-central/rev/48e25edf3e28
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 37•11 years ago
|
||
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.
Assignee | ||
Comment 38•11 years ago
|
||
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.)
Depends on: 1373964
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•