623999 - Takes 35s to start up

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Stuart Parmenter]

On my Vibrant, after today's nightly update, my build now takes 35 seconds to start.  If I kill the app and start again, another 35s.  The application settings show 18MB used by cache, so I am assuming that we are extracting libraries when we shouldn't be?

Comment 1

[Stuart Parmenter]

Am no longer seeing this, but am afraid there might still be a way to trigger it.  Will reopen if I can reproduce again

Comment 2

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

I still see this on the Galaxy Tab. Takes about 14secs to load after doing a force close. Occasionaly, I get a 2-3sec startup, but normally it takes 14sec

Comment 3

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

Matt - What about your Tab?

Comment 4

[Oleg Romashin (:romaxa)]

Also see same problem on my Tab.

Comment 5

[Oleg Romashin (:romaxa)]

After device reboot and just installed fennec it takes ~30-40 seconds to load fennec

Comment 6

[Matt Brubeck (:mbrubeck)]

After exiting Fennec "normally" using the Quit Fennec add-on, start-up took around 5s.  After killing Fennec with a task manager, start-up took from 8 to 30 seconds on my Verizon Galaxy Tab.

Comment 7

[Matt Brubeck (:mbrubeck)]

I'm no longer seeing this in my latest build from this evening's trunk (I was testing an old build by accident before).  Now all startups except firstrun are ~3 seconds on Galaxy Tab.

Comment 8

[Matt Brubeck (:mbrubeck)]

Nevermind, I just got a 35+ second startup (non-firstrun) with my new build.

Comment 9

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

Attachment: WIP patch

this is just a proof of concept. Looking for confirmation that this approach helps

Comment 10

[Matt Brubeck (:mbrubeck)]

Comment on attachment 503536 [details] [diff] [review]
WIP patch

This appears to work as intended.  On Galaxy Tab, first-run startup is now ~9 seconds.  Warm startup is consistently ~3 seconds.  (Previously, both were sometimes >40s.)

Cold startup after reboot is ~6 seconds, which I think is unchanged from trunk.

Curiously, the settings app on my Tab shows 0.00B of cache used for my test build, although Fennec says "using existing lib" on startup.

Comment 11

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

Attachment: patch

Comment 12

[(dormant account)]

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

This looks good to me.

use NULL instead of &offset.  mwu should review this given that it changes DEBUG behavior

Comment 13

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

(In reply to comment #12)
> Comment on attachment 503976 [details] [diff] [review]
> patch
> 
> This looks good to me.
> 
> use NULL instead of &offset.  mwu should review this given that it changes
> DEBUG behavior

here's the docs I've been going by for sendFile:

If offset is not NULL, then it points to a variable holding the file offset
       from which sendfile() will start reading data from in_fd.  When sendfile()
       returns, this variable will be set to the offset of the byte following the
       last byte that was read.  If offset is not NULL, then sendfile() does not
       modify the current file offset of in_fd; otherwise the current file offset is
       adjusted to reflect the number of bytes read from in_fd.

       If offset is NULL, then data will be read from in_fd starting at the current
       file offset, and the file offset will be updated by the call.

my reading of that is if we want to send all of the file and not modify the offset (not sure if matters if we do or not) then we need to pass in a pointer to an offset to 0.

Comment 14

[Matt Brubeck (:mbrubeck)]

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

This patch is not working on my Samsung Galaxy Tab or on my T-Mobile G2.  On both devices, sendfile fails with ESPIPE and write fails with EFAULT.

Comment 15

[Michael Wu [:mwu]]

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

Clearing review since I assume we'll get a more functional one.

Comment 16

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

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

this is still an overall improvement for mbrubeck even if we fail to write out to disk since he was seeing sporadic 35s start ups without it.

We'll need to dig into why writing to his cache directory is failing, but in the worst case that can be done as a follow up.

Comment 17

[Michael Wu [:mwu]]

(In reply to comment #16)
> We'll need to dig into why writing to his cache directory is failing, but in
> the worst case that can be done as a follow up.

If the writes to cache fail, it's no better than the in-memory compression we had before. We might as well remove the risk associated with these extraction changes and back them out if extraction doesn't work. Why add risk if there's no performance improvement?

Comment 18

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

(In reply to comment #17)
> (In reply to comment #16)
> > We'll need to dig into why writing to his cache directory is failing, but in
> > the worst case that can be done as a follow up.
> 
> If the writes to cache fail, it's no better than the in-memory compression we
> had before. We might as well remove the risk associated with these extraction
> changes and back them out if extraction doesn't work. Why add risk if there's
> no performance improvement?

For devices that the writes succeed there is an improvement. For devices that the writes fail we're back to the performance of the in memory decompression.

Comment 19

[Matt Brubeck (:mbrubeck)]

The patch is a definite improvement in the worst case on the Galaxy Tab, but it's a regression on the G2.  (On both devices, it's correct that we're more or less back to the performance of in-memory decompression.)

Comment 20

[Doug Turner (:dougt)]

Attachment: works on the g2

applied on top of the other patch.  this uses the existing buffer to just blast the file down.  probably should clean up a bit.... so no review request.

Comment 21

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

Attachment: patch

This works on my i9000 and nexus one and on doug's g2. Matt, can you confirm on your Tab please?

Comment 22

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

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

I'm having issues with this on the nexus one

Comment 23

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

Attachment: patch

Comment 24

[Doug Turner (:dougt)]

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

I'd rather you use another #define instead of DEBUG for the extraFile path.

I need some comments why you are sleeping and niceing in this code -- why 10?

if we fork() fails, we will not unmap those memory regions.  maybe we don't care.

With those fixes, r+.


follow up bugs to move this to another process?

Comment 25

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

(In reply to comment #24)
> Comment on attachment 505122 [details] [diff] [review]
> patch
> 
> I'd rather you use another #define instead of DEBUG for the extraFile path.
this code is only intended for debugging, so I'm not sure what the value of a #define other than DEBUG is, bug I can do something like this:

#ifdef DEBUG
#define DEBUG_EXTRACT_LIBS
#endif

> I need some comments why you are sleeping and niceing in this code -- why 10?
The 10s sleep is intended to make sure we get out of the startup/firstrun churn. Using nice(10) is just a round number, happy to change it.

> if we fork() fails, we will not unmap those memory regions.  maybe we don't
> care.
if fork fails, we'd be kinda screwed, not sure wha tthe value of unmapping those buffers is at that point
> 
> With those fixes, r+.
> 
> 
> follow up bugs to move this to another process?
fork() is creating a new process. Or am I misunderstanding you?

Comment 26

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

pushed http://hg.mozilla.org/mozilla-central/rev/6643235b9868

Comment 27

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Verified:
Mozilla/5.0 (Android; Linux armv71; rv2.0b10pre) Gecko/20110120 Firefox/4.0b10pre Fennec/4.0b4pre