Closed Bug 760152 Opened 7 years ago Closed 6 years ago

Start library decompression earlier

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 888482
Tracking Status
firefox15 + wontfix
firefox16 + wontfix
firefox17 --- wontfix

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

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.
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.
Attachment #628769 - Flags: review?(blassey.bugs)
Depends on: 760165
Refreshed after bug 760165
Attachment #628779 - Flags: review?(blassey.bugs)
Attachment #628769 - Attachment is obsolete: true
Attachment #628769 - Flags: review?(blassey.bugs)
Fixed typo.
Attachment #628816 - Flags: review?(blassey.bugs)
Attachment #628779 - Attachment is obsolete: true
Attachment #628779 - Flags: review?(blassey.bugs)
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.
This addresses your concern about about:home. Early loading is only triggered when there is an url given to the intent.
Attachment #629731 - Flags: review?(blassey.bugs)
Attachment #628816 - Attachment is obsolete: true
Attachment #628816 - Flags: review?(blassey.bugs)
Attachment #629731 - Flags: review?(blassey.bugs) → review+
>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.
Target Milestone: --- → Firefox 16
https://hg.mozilla.org/mozilla-central/rev/d945ae198516
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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.
Keywords: qawanted
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!
Based on comment 11 the qawanted keyword can be removed.
Keywords: qawanted
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
Attachment #629731 - Flags: approval-mozilla-aurora?
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.
Attachment #629731 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #629731 - Flags: approval-mozilla-beta+
Depends on: 763166
It should be backed out at least from Beta because of the regression: #3 top crasher in 15.0b2.
(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.
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.
Not sure if the if !mIsRestoringActivity needs to be larger.
Attachment #647874 - Flags: review?(bugmail.mozilla)
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.
Attachment #647874 - Flags: review?(bugmail.mozilla) → review+
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
Attachment #647874 - Flags: approval-mozilla-beta?
Attachment #647874 - Flags: approval-mozilla-aurora?
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.
Attachment #647874 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-aurora/rev/e670dfc55dc8

Not sure what to do with the bug status/flags.
Target Milestone: Firefox 16 → Firefox 17
This needs to be backed out of beta too surely?
(Bug 763166 is occurring on beta too)
(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).
Oops, read comment 21, but missed comment 22. Sorry! :-)
(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.
(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.
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.
According to bug 763166 comment 52, it looks like the backout fixed the crashes.
Comment on attachment 647874 [details] [diff] [review]
Tentative patch for backout

Excellent, let's get this out on beta then.
Attachment #647874 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
(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.
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.
The try run was green, so I pushed the backout to inbound as well.

https://hg.mozilla.org/integration/mozilla-inbound/rev/56ae921df543
Target Milestone: Firefox 17 → ---
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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
Blocks: 807322
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 888482
You need to log in before you can comment on or make changes to this bug.