Closed
Bug 840538
Opened 12 years ago
Closed 12 years ago
RIL should not be instantianted on the main startup path of the dialer app
Categories
(Firefox OS Graveyard :: Gaia::Dialer, defect)
Tracking
(blocking-b2g:tef+, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)
RESOLVED
FIXED
blocking-b2g | tef+ |
People
(Reporter: vingtetun, Assigned: etienne)
References
Details
(Whiteboard: [FFOS_perf] QARegressExclude, [qa-])
Attachments
(3 files)
(deleted),
text/html
|
gtorodelvalle
:
review+
vingtetun
:
approval-gaia-v1+
|
Details |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
This slow down the dialer by more than 100ms.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → etienne
Reporter | ||
Comment 1•12 years ago
|
||
fwiw it is also on the main startup path of Settings and Contacts...
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #712923 -
Flags: review?(gtorodelvalle)
Assignee | ||
Comment 3•12 years ago
|
||
make test-perf is confirming a >100ms startup time gain \o/
Comment 4•12 years ago
|
||
Will take a low risk fix for v1.0.1 if ready this week.
blocking-b2g: tef? → -
tracking-b2g18:
--- → +
I'm seeing this on settings profiles too. The majority of time is spent compiling the RILContentHelper.js script.
Comment 6•12 years ago
|
||
Comments included in the pull request... ;-)
Reporter | ||
Comment 7•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> I'm seeing this on settings profiles too. The majority of time is spent
> compiling the RILContentHelper.js script.
Yeah that's what I was saying at #c1. This is also on Contacts but I have been told that it will be move out of the startup path by bug 835791.
Chris do you think it worth precompiling those similarly to what you did for form.js ? (I tried to add them to the preloading script at some point but that was crashing the process because some code or RIL have security assertions about the permission of the app/process).
Updated•12 years ago
|
Attachment #712923 -
Flags: review?(gtorodelvalle) → review+
Comment 8•12 years ago
|
||
We are good to go on my side ;-) Thanks Etienne :-)
Assignee | ||
Comment 9•12 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/4cfe0507e51e962ee6911c477c589e47ffdd6506
Vivien, I'll let you see for the potential uplift (it's a simple surgical patch).
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•12 years ago
|
Attachment #712923 -
Flags: approval-gaia-v1+
(D'oh, missed this question, sorry. Better to needinfo? me.)
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #7)
> Chris do you think it worth precompiling those similarly to what you did for
> form.js ? (I tried to add them to the preloading script at some point but
> that was crashing the process because some code or RIL have security
> assertions about the permission of the app/process).
I lean towards not using our precompile hack on scripts that don't have general use. The "right" way to do this is at gecko buildtime with one of the fastload caches. Right now the impact of this is relatively small and easily fixed in affected apps. Let's reevaluate if this keeps coming up, and do a better general job in v.next.
Comment 13•12 years ago
|
||
We were willing to take an uplift here if there was time and so marking this tef+ to get it uplifted now, watched for regressions, so we have time to back out if necessary before tef CS date (2/28).
blocking-b2g: tef? → tef+
status-b2g18:
--- → affected
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → affected
Comment 14•12 years ago
|
||
Commit 4cfe0507e51e962ee6911c477c589e47ffdd6506 does not apply to v1-train or v1.0.1. This means that there are merge conflicts which need to be resolved. If there are dependencies that are not approved for branch landing, or have yet to land on master, please let me know
If a manual merge is required, a good place to start might be:
cd gaia
git checkout v1-train
git cherry-pick -x -m1 4cfe0507e51e962ee6911c477c589e47ffdd6506
<RESOLVE MERGE CONFLICTS>
git checkout v1.0.1
git cherry-pick -x $(git log -n1 v1-train)
Flags: needinfo?(etienne)
Assignee | ||
Comment 15•12 years ago
|
||
Flags: needinfo?(etienne)
Comment 16•12 years ago
|
||
v1-train: 1734b5487418166451726e5df3a387ad6e1d055d
v1.0.1: 11752b8575250a8842691216c03dd800cb7db110
Comment 17•12 years ago
|
||
(In reply to Etienne Segonzac (:etienne) from comment #15)
> Created attachment 718375 [details] [diff] [review]
> Rebased on v1-train
Thanks for the rebase!
Updated•12 years ago
|
Comment 19•12 years ago
|
||
Can you please provide steps to verify this fix - as we will blackbox test from the UI?
Comment 20•12 years ago
|
||
Requesting info from Etienne although not easy to test it (mainly verifying it) using the UI :-( It just loads faster :-)
Flags: needinfo?(etienne)
Assignee | ||
Comment 21•12 years ago
|
||
Yes, this is pretty much tested by the automated performance tests.
Flags: needinfo?(etienne)
Comment 22•12 years ago
|
||
Understood.
Tagging as QARegressExclude.
Whiteboard: [FFOS_perf] → [FFOS_perf] QARegressExclude
Updated•12 years ago
|
Whiteboard: [FFOS_perf] QARegressExclude → [FFOS_perf] QARegressExclude, [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•