Sync shouldn't hammer preferences so much.

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tcsc, Assigned: tcsc)

Tracking

unspecified
Firefox 59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(2 attachments)

We use preferences very heavily, and bug 1425613 indicates that maybe this is not such a good idea. I spent some time playing around with Gijs's patch and found the following (all numbers are from a full xpcshell-test run of services/sync/test)

Top 20 files reading/writing preferences

171906 resource://gre/modules/Preferences.jsm
 28219 resource://services-sync/engines.js
 25876 resource://gre/modules/TelemetryEnvironment.jsm
 15102 resource://services-sync/service.js
 11250 resource://services-sync/resource.js
 10846 resource://services-sync/telemetry.js
  9952 resource://services-sync/policies.js
  9548 resource://services-common/observers.js
  7425 resource://services-sync/engines/prefs.js
  6529 resource://services-sync/util.js
  4597 resource://services-sync/engines/clients.js
  3538 resource://services-sync/browserid_identity.js
  3112 resource://services-sync/stages/enginesync.js
  2746 resource://services-sync/record.js
  1385 resource://gre/modules/Task.jsm
  1070 resource://services-common/utils.js
   995 resource://services-sync/stages/declined.js
   900 resource://services-common/logmanager.js
   873 resource://services-sync/status.js

Of the preferences read or written by Preferences.jsm, our top 20 are:

11352 log.logger.network.resources
 5072 client.GUID
 4750 sendVersionInfo
 4615 globalScore
 3089 clients.lastSync
 2991 client.type
 2376 debug.ignoreCachedAuthCredentials
 2285 nextSync
 2216 engine.history
 1864 engine.addons
 1740 clients.devices.mobile
 1717 clients.devices.desktop
 1410 engine.bookmarks
 1380 bookmarks.lastSync
 1260 engine.prefs
 1245 engine.passwords
 1240 engine.tabs
 1131 username
 1025 client.syncID
 1004 syncInterval

Most of the worst ones shouldn't be that bad to handle, the first of them is especially silly though. We probably should just have a top level logger in Resource.jsm or something, instead of instantiating it each time.

I've attached a file that contains the non-truncated version of this data, as well as a breakdown of the first list containing line numbers, which would be useful for figuring out what e.g. engines.js is reading (not that it's that hard to guess)
Assignee: nobody → tchiovoloni
Priority: -- → P1
Comment on attachment 8938160 [details]
Bug 1425960 - Optimize sync preference usage

https://reviewboard.mozilla.org/r/208874/#review215300

Looks great, thanks!

::: services/sync/modules/policies.js:35
(Diff revision 1)
>                                     "nsICaptivePortalService");
>  
>  // Get the value for an interval that's stored in preferences. To save users
>  // from themselves (and us from them!) the minimum time they can specify
>  // is 60s.
> -function getThrottledIntervalPreference(prefName) {
> +function defineThrottledIntervalPrefGetters(object, prefs) {

I don't think these "throttled interval" changes are worthwhile - we get the pref values only once (assuming no "startOver" etc, which I think is fine)

::: services/sync/modules/util.js:669
(Diff revision 1)
> +   * };
> +   */
> +  defineLazyIDProperty(object, propName, prefName) {
> +    // An object that exists to be the target of the lazy pref getter.
> +    // We can't use `object` (at least, not using `propName`) since XPCOMUtils
> +    // will stop on any setter we define.

s/stop/stomp/ ?

(Neat impl though)
Attachment #8938160 - Flags: review?(markh) → review+
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/39686aa3f685
Optimize sync preference usage r=markh
https://hg.mozilla.org/mozilla-central/rev/39686aa3f685
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.