Last Comment Bug 760152 - Start library decompression earlier
: Start library decompression earlier
Status: RESOLVED DUPLICATE of bug 888482
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: ---
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on: 760136 760165 763166
Blocks: 807322
  Show dependency treegraph
 
Reported: 2012-05-31 09:03 PDT by Mike Hommey [:glandium]
Modified: 2013-08-15 07:02 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
+
wontfix
wontfix


Attachments
Start library decompression earlier (4.62 KB, patch)
2012-05-31 09:09 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Start library decompression earlier (4.61 KB, patch)
2012-05-31 09:38 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Start library decompression earlier (4.61 KB, patch)
2012-05-31 11:06 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Start library decompression earlier (4.81 KB, patch)
2012-06-04 02:44 PDT, Mike Hommey [:glandium]
blassey.bugs: review+
mark.finkle: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Tentative patch for backout (5.17 KB, patch)
2012-08-01 00:52 PDT, Mike Hommey [:glandium]
bugmail: review+
akeybl: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Mike Hommey [:glandium] 2012-05-31 09:03:12 PDT
A lot of things happen during startup before we even start decompressing the libraries. These things take quite some time (several hundreds of milleseconds). It would be beneficial (especially on multi-core systems) to start decompressing the libraries at the earliest time. However, actually initializing Gecko requires that some things are up, otherwise crashes occur when Gecko calls back in Java through JNI.
Comment 1 Mike Hommey [:glandium] 2012-05-31 09:09:46 PDT
Created attachment 628769 [details] [diff] [review]
Start library decompression earlier

This starts decompression just after loading libmozglue, but actual Gecko initialization is not moved.
From my testing, it doesn't seem to be a loss on single-core devices, but is a clear win on dual-core. If it appears to be a loss on single-core, this could be made conditional to the number of cores (Runtime.availableProcessors may return 1 on multi-core devices, so this would require a native implementation using sysconf(_SC_NPROCESSORS_CONF))

For single-core devices, I've had better startup times by having library decompression and gecko initialization done synchronously, but that blocks UI during that time, so I just dropped that.
Comment 2 Mike Hommey [:glandium] 2012-05-31 09:38:45 PDT
Created attachment 628779 [details] [diff] [review]
Start library decompression earlier

Refreshed after bug 760165
Comment 3 Mike Hommey [:glandium] 2012-05-31 11:06:38 PDT
Created attachment 628816 [details] [diff] [review]
Start library decompression earlier

Fixed typo.
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2012-05-31 17:51:04 PDT
I'm concerned with how this affects how long it takes us to display about:home. I'd want some timing to to show it doesn't affect it before landing.
Comment 5 Mike Hommey [:glandium] 2012-06-04 02:44:14 PDT
Created attachment 629731 [details] [diff] [review]
Start library decompression earlier

This addresses your concern about about:home. Early loading is only triggered when there is an url given to the intent.
Comment 7 Gian-Carlo Pascutto [:gcp] 2012-06-04 08:06:17 PDT
>If it appears to be a loss on single-core, this could be made conditional to the 
>number of cores (Runtime.availableProcessors may return 1 on multi-core devices, 
>so this would require a native implementation using sysconf(_SC_NPROCESSORS_CONF))

If you you do this, you need to check if bionic libc has the correct implementation. At least in Android 2.3.x era it would get it wrong.

See https://bugzilla.mozilla.org/show_bug.cgi?id=663970

Also related see the changelog of NDK 7c.
Comment 8 Geoff Lankow (:darktrojan) 2012-06-05 06:05:10 PDT
https://hg.mozilla.org/mozilla-central/rev/d945ae198516
Comment 9 Mike Hommey [:glandium] 2012-06-11 08:43:33 PDT
It would be interesting to run the nighlies before and after this landing to see how startup time is affected on single and dual-core devices, when opening an url and firefox is not started already.
Comment 10 Brad Lassey [:blassey] (use needinfo?) 2012-06-13 17:41:13 PDT
Will, see comment 9
Comment 11 William Lachance (:wlach) 2012-06-15 07:08:34 PDT
Ran the eideticker startup tests that measure time in seconds to completed web page rendering after launch on a Galaxy Nexus (Android 4.0.4):

Results for 2012-06-05 nightly (before landing): 7.716666666666667, 6.666666666666667, 7.083333333333333
Results for 2012-06-06 nightly (after landing): 7.966666666666667, 7.666666666666667, 6.683333333333334

Preliminary interpretation: No perceptible difference. I dug into the data a bit more by watching the videos that eideticker generated, and it looks like we might be seeing the beginnings of a webpage a few hundred milliseconds earlier. The fundamental issue, however, is that it seems like we take longer than the competition to actually get a completely rendered page (bug 765228). That's really what dominates startup time in this case and I suspect it's somewhat variable so it's hard to measure an improvement.

Unfortunately I don't currently have a single core device to test with eideticker, so I can't really compare the performance here.

Hope this helps!
Comment 12 Andreea Pod 2012-06-27 09:03:04 PDT
Based on comment 11 the qawanted keyword can be removed.
Comment 13 Brian Nicholson (:bnicholson) (PTO through August 1) 2012-07-15 16:28:33 PDT
Comment on attachment 629731 [details] [diff] [review]
Start library decompression earlier

Needed to land bug 769269.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: none
Comment 14 Mark Finkle (:mfinkle) (use needinfo?) 2012-07-15 19:49:08 PDT
Comment on attachment 629731 [details] [diff] [review]
Start library decompression earlier

On it's own, I would let this patch ride the trains, but it makes landing bug 769269 easier and it's been baking a fairly long time.
Comment 15 Brian Nicholson (:bnicholson) (PTO through August 1) 2012-07-18 21:07:21 PDT
http://hg.mozilla.org/releases/mozilla-beta/rev/b4e4ba4d5d6f
Comment 16 Scoobidiver (away) 2012-07-30 14:21:17 PDT
It should be backed out at least from Beta because of the regression: #3 top crasher in 15.0b2.
Comment 17 Alex Keybl [:akeybl] 2012-07-30 16:22:32 PDT
(In reply to Scoobidiver from comment #16)
> It should be backed out at least from Beta because of the regression: #3 top
> crasher in 15.0b2.

Agreed - comment 11 suggests that this didn't have a major effect on performance, and bug 763166 appears to be a regression caused by this.

Let's check in bug 760152 to see how difficult it'll be to land without this patch.
Comment 18 Alex Keybl [:akeybl] 2012-07-31 09:57:51 PDT
Checked in bug 769269 (not this bug, as the previous comment suggests).  Looks like we're good to back this out. 

glandium - please prepare the backout patch on all branches and nominate for approval.
Comment 19 Mike Hommey [:glandium] 2012-08-01 00:52:06 PDT
Created attachment 647874 [details] [diff] [review]
Tentative patch for backout

Not sure if the if !mIsRestoringActivity needs to be larger.
Comment 20 Kartikaya Gupta (email:kats@mozilla.com) 2012-08-01 07:04:20 PDT
Comment on attachment 647874 [details] [diff] [review]
Tentative patch for backout

Review of attachment 647874 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me. The launch state stuff is all static so it shouldn't need to be wrapped in a mIsRestoringActivity block.
Comment 21 Mike Hommey [:glandium] 2012-08-01 07:59:36 PDT
Comment on attachment 647874 [details] [diff] [review]
Tentative patch for backout

[Approval Request Comment]
This is a backout for this bug because we believe it is the reason for a top crash. Kats prefers not to back it out on m-c for now, so that we see if other patches help fix the problem.
Testing completed (on m-c, etc.): The patch is untested, but is not completely different from the reverse of the patch that landed. Only a few things changed due to bug 769269.
Risk to taking this patch (and alternatives if risky): Low risk, although there could be side effects from the parts that conflicted with bug 769269, but i believe we're early enough in the release process that we would catch these if there are any.
String or UUID changes made by this patch: None
Comment 22 Alex Keybl [:akeybl] 2012-08-01 10:44:20 PDT
Comment on attachment 647874 [details] [diff] [review]
Tentative patch for backout

[Triage Comment]
Since this is untested, we'll start with Aurora and make sure nothing blows up before approving prior to our next beta.
Comment 23 Mike Hommey [:glandium] 2012-08-01 23:11:44 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/e670dfc55dc8

Not sure what to do with the bug status/flags.
Comment 24 Ed Morley [:emorley] 2012-08-02 04:57:46 PDT
This needs to be backed out of beta too surely?
(Bug 763166 is occurring on beta too)
Comment 25 Mike Hommey [:glandium] 2012-08-02 04:59:39 PDT
(In reply to Ed Morley [:edmorley] from comment #24)
> This needs to be backed out of beta too surely?

cf. comment 21 and comment 22, we want to see the backout doesn't break aurora first (since it's not a straight backout, bug 769269 introduced conflicts).
Comment 26 Ed Morley [:emorley] 2012-08-02 05:00:35 PDT
Oops, read comment 21, but missed comment 22. Sorry! :-)
Comment 27 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-02 14:55:02 PDT
(In reply to Ed Morley [:edmorley] from comment #24)
> This needs to be backed out of beta too surely?
> (Bug 763166 is occurring on beta too)

Looks like this was successful on Aurora, so please do go ahead with backout on Beta.
Comment 28 Kartikaya Gupta (email:kats@mozilla.com) 2012-08-02 18:22:33 PDT
(In reply to Lukas Blakk [:lsblakk] from comment #27)
> Looks like this was successful on Aurora, so please do go ahead with backout
> on Beta.

What did you use to determine this was successful? It hasn't been long enough for crash-stats to really give meaningful data, I think.
Comment 29 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-03 11:23:50 PDT
So I was basing my statement on not having a report back after landing that this had broken Aurora, but I see that we need to get crash stat data and will wait then for this information to be gathered a few more days before we make a call on beta approval.
Comment 30 Mike Hommey [:glandium] 2012-08-06 00:44:17 PDT
According to bug 763166 comment 52, it looks like the backout fixed the crashes.
Comment 31 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-06 12:02:25 PDT
Comment on attachment 647874 [details] [diff] [review]
Tentative patch for backout

Excellent, let's get this out on beta then.
Comment 32 Mike Hommey [:glandium] 2012-08-06 12:20:20 PDT
http://hg.mozilla.org/releases/mozilla-beta/rev/fc84fbdb000c
Comment 33 Kartikaya Gupta (email:kats@mozilla.com) 2012-08-14 08:18:51 PDT
I'd like the backout to land on 17 as well, seeing as backing out this patch fixed the crashes on 15 and 16, and we're still seeing them on 17.
Comment 34 Mike Hommey [:glandium] 2012-08-14 08:24:02 PDT
(In reply to Kartikaya Gupta (:kats) from comment #33)
> I'd like the backout to land on 17 as well, seeing as backing out this patch
> fixed the crashes on 15 and 16, and we're still seeing them on 17.

Go ahead, but it would be good if someone from the mobile team could figure out what's going on.
Comment 35 Kartikaya Gupta (email:kats@mozilla.com) 2012-08-14 15:27:53 PDT
I have a backout patch on try here: https://tbpl.mozilla.org/?tree=Try&rev=abe9add35a5a
If it's green I'll push it to inbound. It's basically the same patch that you pushed to aurora.
Comment 36 Kartikaya Gupta (email:kats@mozilla.com) 2012-08-15 20:37:00 PDT
The try run was green, so I pushed the backout to inbound as well.

https://hg.mozilla.org/integration/mozilla-inbound/rev/56ae921df543
Comment 37 Ed Morley [:emorley] 2012-08-16 06:16:23 PDT
(In reply to Kartikaya Gupta (:kats) from comment #36)
> The try run was green, so I pushed the backout to inbound as well.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/56ae921df543

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/56ae921df543
Comment 38 Mike Hommey [:glandium] 2013-08-15 07:02:01 PDT

*** This bug has been marked as a duplicate of bug 888482 ***

Note You need to log in before you can comment on or make changes to this bug.