Closed Bug 1124102 Opened 5 years ago Closed 5 years ago

Preloading mozSettings fails

Categories

(Core :: DOM: Device Interfaces, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file, 1 obsolete file)

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.
Flags: needinfo?
Flags: needinfo? → needinfo?(21)
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.
(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
got the infos I needed face to face :)
Flags: needinfo?(21)
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.
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.
Attachment #8552505 - Attachment is obsolete: true
Attachment #8552505 - Flags: review?(bent.mozilla)
Attachment #8552508 - Flags: review?(fabrice)
Attachment #8552508 - Flags: review?(fabrice) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9151efa94f1f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Attachment #8552505 - Flags: review?(bent.mozilla)
Whiteboard: [systemsfe]
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.
blocking-b2g: --- → 2.2?
Alexandre, can you please request an approval?
Flags: needinfo?(lissyx+mozillians)
Flags: needinfo?(lissyx+mozillians)
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
Attachment #8552508 - Flags: approval-mozilla-b2g37?
Not blocking the release but lets get it uplifted.
blocking-b2g: 2.2? → ---
I thought that only blockers could get uplifted now ? :)
(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 :)
Attachment #8552508 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.