Closed
Bug 760152
Opened 12 years ago
Closed 11 years ago
Start library decompression earlier
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox15+ wontfix, firefox16+ wontfix, firefox17 wontfix)
RESOLVED
DUPLICATE
of bug 888482
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
blassey
:
review+
mfinkle
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
kats
:
review+
akeybl
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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 | ||
Comment 2•12 years ago
|
||
Refreshed after bug 760165
Attachment #628779 -
Flags: review?(blassey.bugs)
Assignee | ||
Updated•12 years ago
|
Attachment #628769 -
Attachment is obsolete: true
Attachment #628769 -
Flags: review?(blassey.bugs)
Assignee | ||
Updated•12 years ago
|
Attachment #628779 -
Attachment is obsolete: true
Attachment #628779 -
Flags: review?(blassey.bugs)
Comment 4•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
Attachment #628816 -
Attachment is obsolete: true
Attachment #628816 -
Flags: review?(blassey.bugs)
Updated•12 years ago
|
Attachment #629731 -
Flags: review?(blassey.bugs) → review+
Assignee | ||
Comment 6•12 years ago
|
||
Comment 7•12 years ago
|
||
>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.
Updated•12 years ago
|
Target Milestone: --- → Firefox 16
Comment 8•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 9•12 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
Comment 10•12 years ago
|
||
Will, see comment 9
Comment 11•12 years ago
|
||
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•12 years ago
|
||
Based on comment 11 the qawanted keyword can be removed.
Keywords: qawanted
Comment 13•12 years ago
|
||
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 14•12 years ago
|
||
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•12 years ago
|
Attachment #629731 -
Flags: approval-mozilla-beta+
Comment 15•12 years ago
|
||
status-firefox15:
--- → fixed
Comment 16•12 years ago
|
||
It should be backed out at least from Beta because of the regression: #3 top crasher in 15.0b2.
Comment 17•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
Not sure if the if !mIsRestoringActivity needs to be larger.
Attachment #647874 -
Flags: review?(bugmail.mozilla)
Comment 20•12 years ago
|
||
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•12 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 22•12 years ago
|
||
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•12 years ago
|
||
http://hg.mozilla.org/releases/mozilla-aurora/rev/e670dfc55dc8
Not sure what to do with the bug status/flags.
Updated•12 years ago
|
status-firefox16:
--- → affected
Target Milestone: Firefox 16 → Firefox 17
Comment 24•12 years ago
|
||
This needs to be backed out of beta too surely?
(Bug 763166 is occurring on beta too)
status-firefox17:
--- → fixed
Assignee | ||
Comment 25•12 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).
Comment 26•12 years ago
|
||
Oops, read comment 21, but missed comment 22. Sorry! :-)
Comment 27•12 years ago
|
||
(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.
Updated•12 years ago
|
tracking-firefox15:
--- → +
tracking-firefox16:
--- → +
Comment 28•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
According to bug 763166 comment 52, it looks like the backout fixed the crashes.
Comment 31•12 years ago
|
||
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•12 years ago
|
||
Comment 33•12 years ago
|
||
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•12 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.
Comment 35•12 years ago
|
||
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•12 years ago
|
||
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 → ---
Assignee | ||
Updated•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 37•12 years ago
|
||
(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
Assignee | ||
Updated•11 years ago
|
Status: REOPENED → RESOLVED
Closed: 12 years ago → 11 years ago
Resolution: --- → DUPLICATE
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•