1124102 - Preloading mozSettings fails

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[:gerard-majax]

We do try preload mozSettings API in dom/ipc/preload.js. However, from my testing on device, this fails:

> 01-21 10:19:33.489  8148  8148 I Gecko   : [PRELOAD] Trying to access mozSettings
> 01-21 10:19:33.499  8148  8148 I Gecko   : [PRELOAD] Trying to access mozSettings: KO: ReferenceError: navigator is not defined

Vivien, this does not looks like the kind of error we could be expecting, the code says this might throw if we don't have permission. This error makes me believe that preloading mozSettings just does not work as expected.

Comment 1

[:gerard-majax]

Tried:
> Cc["@mozilla.org/settingsManager;1"].getService(Ci["nsISupports"]);

In dom/ipc/preload.js and dom/ipc/post-fork-preload.js, but app still creates a new SettingsManager object when accessing navigator.mozSettings: this costs ~8-10ms in the app, on the first call.

Comment 2

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #1)
> Tried:
> > Cc["@mozilla.org/settingsManager;1"].getService(Ci["nsISupports"]);
> 
> In dom/ipc/preload.js and dom/ipc/post-fork-preload.js, but app still
> creates a new SettingsManager object when accessing navigator.mozSettings:
> this costs ~8-10ms in the app, on the first call.

Looks like it gives an improvement, already. I tested on the SMS app, with patch from bug 1122649 attachment 8550403 [details] [diff] [review]:

Without the hack post-fork-preload:
> mozSettings : minuteur démarré gaia_build_defer_index.js:227
> mozSettings : 29.2ms gaia_build_defer_index.js:227
> mozSettings lock : minuteur démarré gaia_build_defer_index.js:227
> mozSettings lock : 11.77ms gaia_build_defer_index.js:227
> mozSettings get : minuteur démarré gaia_build_defer_index.js:227
> mozSettings get : 5.32ms

With the hack post-fork-preload:
> mozSettings : minuteur démarré gaia_build_defer_index.js:227
> mozSettings : 11.2ms gaia_build_defer_index.js:227
> mozSettings lock : minuteur démarré gaia_build_defer_index.js:227
> mozSettings lock : 12.8ms gaia_build_defer_index.js:227
> mozSettings get : minuteur démarré gaia_build_defer_index.js:227
> mozSettings get : 4.12ms

Comment 3

[:gerard-majax]

got the infos I needed face to face :)

Comment 4

[:gerard-majax]

Attachment: Fix preloading of mozSettings API r=bent

In the past, we used to use |navigator.mozSettings| call in preload.js
to make sure the SettingsManager.js was loaded for improving access
time. Checking the behavior, it turns out that |navigator| in non
existent at this point and thus this was not helping at all. We fix this
by instead forcing creating an instance of settingsManager. Measurements
shows that this reduces the time spent for the call
|var settings = navigator.mozSettings| by 20ms on Flame, going from
~30ms to ~10ms.

Comment 5

[:gerard-majax]

Attachment: Fix preloading of mozSettings API r=fabrice

In the past, we used to use |navigator.mozSettings| call in preload.js
to make sure the SettingsManager.js was loaded for improving access
time. Checking the behavior, it turns out that |navigator| in non
existent at this point and thus this was not helping at all. We fix this
by instead forcing creating an instance of settingsManager. Measurements
shows that this reduces the time spent for the call
|var settings = navigator.mozSettings| by 20ms on Flame, going from
~30ms to ~10ms.

Comment 6

[:gerard-majax]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79d0f59b1fd8

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9151efa94f1f

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9151efa94f1f

Comment 9

[Julien Wajsberg [:julienw]]

This slipped under my radar, but it should be uplifted to 2.2 because it gives a nice performance improvement to nearly all apps (~10 to 30ms) and especially the SMS app.

Comment 10

[Julien Wajsberg [:julienw]]

Alexandre, can you please request an approval?

Comment 11

[:gerard-majax]

Comment on attachment 8552508 [details] [diff] [review]
Fix preloading of mozSettings API r=fabrice

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: lower perf on app startup
Testing completed: running on master for more than one month
Risk to taking this patch (and alternatives if risky): low, and easy to backout in case of issue
String or UUID changes made by this patch: none

Comment 12

[Gregor Wagner [:gwagner]]

Not blocking the release but lets get it uplifted.

Comment 13

[Julien Wajsberg [:julienw]]

I thought that only blockers could get uplifted now ? :)

Comment 14

[bhavana bajaj [:bajaj]]

(In reply to Julien Wajsberg [:julienw] from comment #13)
> I thought that only blockers could get uplifted now ? :)

Given this is very low risk, improves perf and easy to backout, we can take this. I am always in  for low risk perf/stability fixes until FC :)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/af435405f6c8