The default bug view has changed. See this FAQ.

Start library decompression earlier

RESOLVED DUPLICATE of bug 888482

Status

()

Firefox for Android
General
RESOLVED DUPLICATE of bug 888482
5 years ago
4 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

(Blocks: 1 bug)

unspecified
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox15+ wontfix, firefox16+ wontfix, firefox17 wontfix)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Attachment #628769 - Flags: review?(blassey.bugs)
(Assignee)

Updated

5 years ago
Depends on: 760165
(Assignee)

Comment 2

5 years ago
Created attachment 628779 [details] [diff] [review]
Start library decompression earlier

Refreshed after bug 760165
Attachment #628779 - Flags: review?(blassey.bugs)
(Assignee)

Updated

5 years ago
Attachment #628769 - Attachment is obsolete: true
Attachment #628769 - Flags: review?(blassey.bugs)
(Assignee)

Comment 3

5 years ago
Created attachment 628816 [details] [diff] [review]
Start library decompression earlier

Fixed typo.
Attachment #628816 - Flags: review?(blassey.bugs)
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 5

5 years ago
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.
Attachment #629731 - Flags: review?(blassey.bugs)
(Assignee)

Updated

5 years ago
Attachment #628816 - Attachment is obsolete: true
Attachment #628816 - Flags: review?(blassey.bugs)
Attachment #629731 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d945ae198516
>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
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 9

5 years ago
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
Will, see comment 9
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

5 years ago
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+

Updated

5 years ago
Attachment #629731 - Flags: approval-mozilla-beta+
http://hg.mozilla.org/releases/mozilla-beta/rev/b4e4ba4d5d6f
status-firefox15: --- → fixed

Updated

5 years ago
Depends on: 763166

Comment 16

5 years ago
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.
(Assignee)

Comment 19

5 years ago
Created attachment 647874 [details] [diff] [review]
Tentative patch for backout

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+
(Assignee)

Comment 21

5 years ago
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+
(Assignee)

Comment 23

5 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/e670dfc55dc8

Not sure what to do with the bug status/flags.

Updated

5 years ago
status-firefox16: --- → affected
Target Milestone: Firefox 16 → Firefox 17
This needs to be backed out of beta too surely?
(Bug 763166 is occurring on beta too)
status-firefox16: affected → wontfix
status-firefox17: --- → fixed
(Assignee)

Comment 25

5 years ago
(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.
tracking-firefox15: --- → +
tracking-firefox16: --- → +
(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.
(Assignee)

Comment 30

5 years ago
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+
(Assignee)

Comment 32

5 years ago
http://hg.mozilla.org/releases/mozilla-beta/rev/fc84fbdb000c
status-firefox15: fixed → wontfix
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.
(Assignee)

Comment 34

5 years ago
(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
status-firefox17: fixed → wontfix
Target Milestone: Firefox 17 → ---
(Assignee)

Updated

5 years ago
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
(Assignee)

Updated

4 years ago
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 888482
You need to log in before you can comment on or make changes to this bug.