840538 - RIL should not be instantianted on the main startup path of the dialer app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Dialer, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

This slow down the dialer by more than 100ms.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

fwiw it is also on the main startup path of Settings and Contacts...

Comment 2

[Etienne Segonzac (:etienne)]

Attachment: Pointer to gaia PR

Comment 3

[Etienne Segonzac (:etienne)]

make test-perf is confirming a >100ms startup time gain \o/

Comment 4

[Alex Keybl [:akeybl]]

Will take a low risk fix for v1.0.1 if ready this week.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm seeing this on settings profiles too.  The majority of time is spent compiling the RILContentHelper.js script.

Comment 6

[Germán Toro del Valle (:gtorodelvalle)]

Comments included in the pull request... ;-)

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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).

Comment 8

[Germán Toro del Valle (:gtorodelvalle)]

We are good to go on my side ;-) Thanks Etienne :-)

Comment 9

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/4cfe0507e51e962ee6911c477c589e47ffdd6506

Vivien, I'll let you see for the potential uplift (it's a simple surgical patch).

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Rebased on 1.0.1

Comment 12

[David Scravaglieri [:scravag]]

Perf must be nominated for tef+

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

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).

Comment 14

[John Ford [:jhford] CET/CEST Berlin Time]

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)

Comment 15

[Etienne Segonzac (:etienne)]

Attachment: Rebased on v1-train

Comment 16

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train: 1734b5487418166451726e5df3a387ad6e1d055d
v1.0.1: 11752b8575250a8842691216c03dd800cb7db110

Comment 17

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Etienne Segonzac (:etienne) from comment #15)
> Created attachment 718375 [details] [diff] [review]
> Rebased on v1-train

Thanks for the rebase!

Comment 18

[amiller]

No Test Case is needed in Moztrap for this issue.

Comment 19

[Cody Roesch [:croesch]]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 20

[Germán Toro del Valle (:gtorodelvalle)]

Requesting info from Etienne although not easy to test it (mainly verifying it) using the UI :-( It just loads faster :-)

Comment 21

[Etienne Segonzac (:etienne)]

Yes, this is pretty much tested by the automated performance tests.

Comment 22

[Cody Roesch [:croesch]]

Understood.
Tagging as QARegressExclude.