Preloading mozSettings fails

RESOLVED FIXED in Firefox 38, Firefox OS v2.2

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: gerard, Assigned: gerard)

Tracking

unspecified
mozilla38
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [systemsfe])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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?
(Assignee)

Updated

4 years ago
Flags: needinfo? → needinfo?(21)
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Comment 2

4 years ago
(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
(Assignee)

Comment 3

4 years ago
got the infos I needed face to face :)
Flags: needinfo?(21)
(Assignee)

Comment 4

4 years ago
Created attachment 8552505 [details] [diff] [review]
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.
(Assignee)

Comment 5

4 years ago
Created attachment 8552508 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #8552505 - Attachment is obsolete: true
Attachment #8552505 - Flags: review?(bent.mozilla)
(Assignee)

Updated

4 years ago
Attachment #8552508 - Flags: review?(fabrice)
Attachment #8552508 - Flags: review?(fabrice) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9151efa94f1f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Assignee)

Updated

4 years ago
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)
(Assignee)

Updated

3 years ago
Flags: needinfo?(lissyx+mozillians)
(Assignee)

Comment 11

3 years ago
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 :)

Updated

3 years ago
Attachment #8552508 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/af435405f6c8
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
status-firefox36: --- → wontfix
status-firefox37: --- → wontfix
status-firefox38: --- → fixed
You need to log in before you can comment on or make changes to this bug.