Closed Bug 806383 Opened 12 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: justin.lebar+bug, Assigned: fabrice)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(2 files, 1 obsolete file)

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.
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.
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.
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.
Whiteboard: [MemShrink] → [MemShrink:P2]
Attached patch Gaia patch (deleted) — Splinter Review
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.
Assignee: nobody → fabrice
Attachment #677595 - Flags: review?(justin.lebar+bug)
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?
> 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.
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).
Looks like nsZipReaderCache is germane.
Attachment #677595 - Attachment description: patch → Gaia patch
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
Attachment #677595 - Flags: review?(justin.lebar+bug)
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?
No, that's enough to rebuild the .zip files.
I just found bug 504217, which advocates for removing nsZipCacheReader altogether. Hear hear.
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.
I guess the first question is this: does the real device have the same behaviour as b2g-desktop?
Attached file cache hits & misses (obsolete) (deleted) —
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.
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.
> 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.
Attached file cache hits & misses, v2 (deleted) —
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.
Attachment #678962 - Attachment is obsolete: true
Unfocused pointed out that the nsZipReaderCache is flushed on memory pressure events. So that diminishes the importance of this.
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.
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.
> 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. :)
Closing out old Firefox OS specific memshrink bugs.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: