608042 - Add ability to make "fat" android builds that extract APK to disk

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

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

My phone has gigabytes of internal storage, so fennec using 50MB or so on disk isn't an issue.  Trading startup time and/or memory usage for a smaller disk footprint is not worth it to me.  This ought to become increasingly common (one hopes!).

It would be nice if we could have a configure option (say --enable-fatfox for historical lulz) that turned off our custom loader.  It would also be nice if we could offer these "fat" builds like we do the nothumb ones; I would use the fat builds, e.g.  As mobiles evolve, we could look into making fat builds the default, and offer "skinny" builds for more limited devices.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Is speed and memory usage more important than "disk" size? I think it is.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

This is a 730ms Ts improvement according to my stopwatch timing. It extracts 17.96Mb to the cache and cleans that up if we wind up running low on disk space. The low disk space threshold is set to 150Mb free. I just pulled that number out of my arse, but it seems reasonable to me.

Comment 3

[(dormant account)]

I agree with extracting libs given enough capacity. I disagree about ditching custom loader. I would rather move all of this logic into the custom loader where more optimizations can be done.

Brad convinced me that we can move logic into the loader + optimize in followup bugs.

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 501104 [details] [diff] [review]
patch

>+        boolean shouldExtact = freeSpace > kFreeSpaceThreshold;
>+        if (!shouldExtact) {
>+            // remove any previously extracted libs since we're apparently low

If the free space is close to the threshold, then we'll get in a loop where we alternately extract the libraries and then delete them.  If we want to avoid this, we could set the threshold for deletion somewhere below the threshold for extraction.

Also, I can't see where we re-use the cached files after extracting them.

Comment 5

[Matt Brubeck (:mbrubeck)]

(In reply to comment #4)
> Also, I can't see where we re-use the cached files after extracting them.

Nevermind, I see it now (at the start of extractFile).

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 501104 [details] [diff] [review]
patch

extractLibs is a terrible name for a global.  it would be nice if you used a prefix

what does cacheFile.getPath() typically return? /data/data/???

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #6)
> Comment on attachment 501104 [details] [diff] [review]
> patch
> 
> extractLibs is a terrible name for a global.  it would be nice if you used a
> prefix
its local to the file, not exported

> what does cacheFile.getPath() typically return? /data/data/???
/data/data/org.mozilla.fennec/cache

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

dougt pushed https://hg.mozilla.org/mozilla-central/rev/d999c9baa0b7

filed bug 623598 for mbrubeck's concern over repeatedly extracting and deleting libraries

Comment 9

[(dormant account)]

I think this should be backed out. This actually makes cold startup slower on my galaxy S. And there seems to be some perf bug with the plugin-container.
Here is cold startup without extraction disabled:
E/GeckoLibLoad( 3613): spent 780ms total, 4 faults
E/GeckoLibLoad( 3630): spent 134ms total, 0 faults

cold startup AFTER extraction(ie this patch working):
E/GeckoLibLoad( 3548): spent 973ms total, 113 faults
E/GeckoLibLoad( 3566): spent 6091ms total, 18 faults

The second line is plugin-container.

Comment 10

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

Interesting!

Comment 11

[(dormant account)]

Looks like this is the cause of 623999. I'm using ext4 for my cold startup. On mwu's rfs it was 14seconds for child process to start(1.3s for main).

I worked around this here by setting extract_files=1 in the child. Not sure about the proper way of doing that.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

 Bug 623912 added some code to make sure the child process opened the previously extracted libs rather than hitting the ashmem path. Is your tree up to date enough to have that? If that's not working correctly we should fix it.

Also, setting a environment variable may be the best way to tell the child to extract the libs a needed

Comment 13

[(dormant account)]

(In reply to comment #12)
>  Bug 623912 added some code to make sure the child process opened the
> previously extracted libs rather than hitting the ashmem path. Is your tree up
> to date enough to have that? If that's not working correctly we should fix it.

Yes,from noon today.

Comment 14

[(dormant account)]

Attachment: preload libxul to avoid expensive demand-paging overhead for it

This caps cold startup at < 4s for me.

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 503629 [details] [diff] [review]
preload libxul to avoid expensive demand-paging overhead for it

bug 623999 changes this code a bit and uses madvise to get a similar benefit

Comment 16

[Dietrich Ayala (:dietrich)]

(In reply to comment #15)
>
> bug 623999 changes this code a bit and uses madvise to get a similar benefit

Should this bug be duped against it? If not, should this bug block?

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

the patch on this bug already landed, not sure why it was reopened. Let's use follow up bugs for any other work