806383 - Don't load images from (compressed) JARs, to save memory

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

Extracting data from a JAR uses ~40kb of memory (bug 802446 comment 19).  If we extract lots of images from JARs (bug 802446 comment 14), that could be a substantial amount of memory used.  (It's not clear when the memory gets freed.)

Compressing already-compressed images in JARs doesn't make much sense anyway.  Zip lets you store files in the archive with no compression; I'd expect that reading such a file wouldn't require any work by zlib.  So maybe we can look into tweaking how we create these JARs so that our images aren't compressed.

Comment 1

[[:fabrice] Fabrice Desré]

We store all files in zips with the nsIZipWriter.COMPRESSION_DEFAULT flag at https://github.com/mozilla-b2g/gaia/blob/master/build/webapp-zip.js#L59

We could use a whitelist of file extensions that we don't want to compress and set the flag to nsIZipWriter.COMPRESSION_NONE for these instead.

Comment 2

[Thinker Li [:sinker]]

If we can mmap uncompressed files from a JAR directly, the kernel will drop the pages occupied by these files while memory is in pressure.  In the case, we can use memory more effective. 

http://www.pkware.com/documents/casestudies/APPNOTE.TXT provide some information of file format of zip.  It seems easy to extract the offset & size of a file.

Comment 3

[Thinker Li [:sinker]]

According nsZipHandle::Init(), nsZipArchive::GetData(), nsJARInputStream::InitFile(), and nsJARInputStream::Read(), Gecko has already mmap JAR files and copy directly from the mapped memory for no compression files.  So, what had mentioned in comment #0 is confirmed.

Comment 4

[[:fabrice] Fabrice Desré]

Attachment: Gaia patch

This patch stores the files with 'png', 'jpg', 'gif' and 'ogg' without compression. Some of the zips get very slightly larger as we can expect, but nothing to worry about.

I haven't done any memory report with/without this patch.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Hah, awesome :).

Comment 6

[Kris Maglione [:kmag]]

If we see any real win here, we should create an in-browser tool to repack add-on and especially theme jars the same way and an AMO validator warning for compressed images in jars. Will this be a big enough gain to be worth the effort?

Comment 7

[Justin Lebar (not reading bugmail)]

> Will this be a big enough gain to be worth the effort?

We don't know without the memory reporter.  :)

I'd really like to have the memory reporter before we land this.  In my experience with MemShrink, it's extremely easy to regress patches like this that don't have coverage.

I can write this reporter, if you like.

Comment 8

[Nicholas Nethercote [inactive]]

I'm also happy to write a memory reporter if someone can tell me where this data is stored (i.e. what fields in what objects).

Comment 9

[Nicholas Nethercote [inactive]]

Looks like nsZipReaderCache is germane.

Comment 10

[Justin Lebar (not reading bugmail)]

I'd guess there are other things you don't want to compress as well, unless none of the binary formats listed below get added to jars?

I'd r+ a patch which expanded the list of not-compressed things, but I'd really prefer that we didn't land this until we had the memory reporter.

$ git ls-files | grep --only-matching '\.[^\.]*$' | sort | uniq
.com
.css
.dict
.gif
.gitignore
.gitkeep
.glsl
.html
.ico
.ics
.ini
.jpg
.js
.jsm
.json
.manifest
.md
.me/tests/origin
.mk
.mp3
.mp4
.npmignore
.ogg
.ogv
.org
.org/telefonica/offline/home
.otf
.php
.png
.properties
.py
.rdf
.sh
.svg
.ttf
.wav
.webapp
.webm
.woff
.xml

Comment 11

[Nicholas Nethercote [inactive]]

I just tried this patch with DMD and didn't see any change in the memory allocated for nsZipReaderCache and related things.  I applied it to my gaia/ dir and re-ran |make| -- do I need to do anything else?

Comment 12

[[:fabrice] Fabrice Desré]

No, that's enough to rebuild the .zip files.

Comment 13

[Nicholas Nethercote [inactive]]

I just found bug 504217, which advocates for removing nsZipCacheReader altogether.  Hear hear.

Comment 14

[Nicholas Nethercote [inactive]]

Ok, I just discovered that this bug is totally off the mark -- it's not images that are the problem, it's apps.

I instrumented nsZipReaderCache::GetZip to print the URI each time a new JAR is created.  Here's the result when starting b2g-desktop (substitute |/home/njn/moz/gaia/profile| for $PROF in the list):

file:$PROF/webapps/system.gaiamobile.org/application.zip
file:$PROF/webapps/keyboard.gaiamobile.org/application.zip
file:$PROF/webapps/costcontrol.gaiamobile.org/application.zip
file:$PROF/webapps/sms.gaiamobile.org/application.zip
file:$PROF/webapps/homescreen.gaiamobile.org/application.zip
file:$PROF/webapps/camera.gaiamobile.org/application.zip
file:$PROF/webapps/gallery.gaiamobile.org/application.zip
file:$PROF/webapps/fm.gaiamobile.org/application.zip
file:$PROF/webapps/settings.gaiamobile.org/application.zip
file:$PROF/webapps/calendar.gaiamobile.org/application.zip
file:$PROF/webapps/clock.gaiamobile.org/application.zip
file:$PROF/webapps/costcontrol.gaiamobile.org/application.zip
file:$PROF/webapps/email.gaiamobile.org/application.zip
file:$PROF/webapps/music.gaiamobile.org/application.zip
file:$PROF/webapps/video.gaiamobile.org/application.zip
file:$PROF/webapps/calculator.gaiamobile.org/application.zip
file:$PROF/webapps/pdfjs.gaiamobile.org/application.zip
file:$PROF/webapps/wallpaper.gaiamobile.org/application.zip
file:$PROF/webapps/test-receiver-1.gaiamobile.org/application.zip
file:$PROF/webapps/test-agent.gaiamobile.org/application.zip
file:$PROF/webapps/template.gaiamobile.org/application.zip
file:$PROF/webapps/test-receiver-2.gaiamobile.org/application.zip
file:$PROF/webapps/test-receiver-inline.gaiamobile.org/application.zip
file:$PROF/webapps/image-uploader.gaiamobile.org/application.zip
file:$PROF/webapps/test-sensors.gaiamobile.org/application.zip
file:$PROF/webapps/membuster.gaiamobile.org/application.zip
file:$PROF/webapps/uitest.gaiamobile.org/application.zip
file:$PROF/webapps/tasks.gaiamobile.org/application.zip
file:$PROF/webapps/crystalskull.gaiamobile.org/application.zip
file:$PROF/webapps/penguinpop.gaiamobile.org/application.zip
file:$PROF/webapps/cubevid.gaiamobile.org/application.zip
file:$PROF/webapps/towerjelly.gaiamobile.org/application.zip
file:$PROF/webapps/communications.gaiamobile.org/application.zip
file:$PROF/webapps/sms.gaiamobile.org/application.zip
file:$PROF/webapps/browser.gaiamobile.org/application.zip
file:$PROF/webapps/feedback.gaiamobile.org/application.zip

This explains comment 11.

Comment 15

[Nicholas Nethercote [inactive]]

I guess the first question is this:  does the real device have the same behaviour as b2g-desktop?

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: cache hits & misses

The attached patch is the result of instrumenting both branches of the if/else in nsZipReaderCache::GetZip at startup -- "HIT" is the first branch, "ZIP" is the 2nd branch.

In short, we have lots of hits for the system and homescreen apps;  multiple hits for a few such as keyboard, sms and costcontrol;  and no hits (just the first read) for all the others, e.g. settings, calendar, etc.

Comment 17

[Nicholas Nethercote [inactive]]

Ugh, I confused myself:  the above instrumentation is just the Main process.  We saw the image-related stuff in the Homescreen process.  Sorry for the confusion.

Comment 18

[Nicholas Nethercote [inactive]]

> Ugh, I confused myself:  the above instrumentation is just the Main process.
> We saw the image-related stuff in the Homescreen process.  Sorry for the
> confusion.

Hmm, but my instrumentation should be active for both processes.  I'm going to stop commenting for a while to investigate properly.

Comment 19

[Nicholas Nethercote [inactive]]

Attachment: cache hits & misses, v2

I instrumented the AsyncOpen() call in imgLoader::LoadImage(), and I added PIDs to each line.  The attached file has the full sequence.  The key thing is near the end, where we have lots of pairs of lines like this:

  ZIP: AsyncOpen(16095): app://towerjelly.gaiamobile.org/style/icons/Towerjelly.png
  ZIP: MISS(16095): file:/home/njn/moz/gaia/profile/webapps/towerjelly.gaiamobile.org/application.zip

AFAICT, for each homescreen app we're loading its icon from its zip.  The nsZipReaderCache is doing exactly the wrong thing here, i.e. holding onto memory for multiple zips that are only accessed once.

Fabrice told me that bug 807529 will move the storage of these icons into IndexedDB.  Hopefully that will fix this problem.

Comment 20

[Nicholas Nethercote [inactive]]

Unfocused pointed out that the nsZipReaderCache is flushed on memory pressure events.  So that diminishes the importance of this.

Comment 21

[(dormant account)]

Are all of these zip files opened on startup at any point? If so having many mmap areas could cause OOM errors due to address space fragmentation.
Tt would be good to look into access-ordered zip files for apps. This can significantly reduce IO(due to better locality) resulting in faster startup and in some cases memory usage. Firefox already does this for omni.ja on desktop.

Comment 22

[[:fabrice] Fabrice Desré]

At startup we load 2 apps (the system app and the homescreen), and request a bunch of resources from their respective .zip files.

We can investigate access-ordered layout indeed.

Comment 23

[Justin Lebar (not reading bugmail)]

> If so having many mmap areas could cause OOM errors due to address space fragmentation.

This is more of a theoretical concern at this point; I've never seen come anywhere close to exhausting our memory space.  We run out of physical memory much sooner.

But if access-oriented layout gets us faster startup times, that's great.  :)

Comment 24

[Eric Rahm [:erahm]]

Closing out old Firefox OS specific memshrink bugs.