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)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: justin.lebar+bug, Assigned: fabrice)
References
Details
(Whiteboard: [MemShrink:P2])
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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.
Updated•12 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee | ||
Comment 4•12 years ago
|
||
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)
Hah, awesome :).
Comment 6•12 years ago
|
||
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?
Reporter | ||
Comment 7•12 years ago
|
||
> 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•12 years ago
|
||
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•12 years ago
|
||
Looks like nsZipReaderCache is germane.
Reporter | ||
Updated•12 years ago
|
Attachment #677595 -
Attachment description: patch → Gaia patch
Reporter | ||
Comment 10•12 years ago
|
||
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
Reporter | ||
Updated•12 years ago
|
Attachment #677595 -
Flags: review?(justin.lebar+bug)
Comment 11•12 years ago
|
||
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?
Assignee | ||
Comment 12•12 years ago
|
||
No, that's enough to rebuild the .zip files.
Comment 13•12 years ago
|
||
I just found bug 504217, which advocates for removing nsZipCacheReader altogether. Hear hear.
Comment 14•12 years ago
|
||
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•12 years ago
|
||
I guess the first question is this: does the real device have the same behaviour as b2g-desktop?
Comment 16•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
> 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•12 years ago
|
||
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
Comment 20•12 years ago
|
||
Unfocused pointed out that the nsZipReaderCache is flushed on memory pressure events. So that diminishes the importance of this.
Comment 21•12 years ago
|
||
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.
Assignee | ||
Comment 22•12 years ago
|
||
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.
Reporter | ||
Comment 23•12 years ago
|
||
> 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•7 years ago
|
||
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.
Description
•