From 25d3c801bcda1b85d50bab310b7859b9ba1fda79 Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Wed, 20 Dec 2017 12:36:18 -0500 Subject: [PATCH] Bug 1425960 - Optimize sync preference usage r=markh MozReview-Commit-ID: AMwkvF7Dy3G --HG-- extra : rebase_source : f44ea9674ad0fd45191220ec6845aa85f68efa10 --- services/sync/modules/browserid_identity.js | 14 ++-- services/sync/modules/engines.js | 43 ++++++------ services/sync/modules/engines/clients.js | 30 ++++----- services/sync/modules/policies.js | 72 ++++++++++++++------- services/sync/modules/resource.js | 7 +- services/sync/modules/service.js | 10 +-- services/sync/modules/util.js | 59 +++++++++++++++-- 7 files changed, 149 insertions(+), 86 deletions(-) diff --git a/services/sync/modules/browserid_identity.js b/services/sync/modules/browserid_identity.js index eff034406c76..a6edeb206e5e 100644 --- a/services/sync/modules/browserid_identity.js +++ b/services/sync/modules/browserid_identity.js @@ -36,6 +36,9 @@ XPCOMUtils.defineLazyGetter(this, "log", function() { return log; }); +XPCOMUtils.defineLazyPreferenceGetter(this, "IGNORE_CACHED_AUTH_CREDENTIALS", + "services.sync.debug.ignoreCachedAuthCredentials"); + // FxAccountsCommon.js doesn't use a "namespace", so create one here. var fxAccountsCommon = {}; Cu.import("resource://gre/modules/FxAccountsCommon.js", fxAccountsCommon); @@ -170,6 +173,7 @@ this.BrowserIDManager = function BrowserIDManager() { // will be a promise that resolves when we are ready to authenticate this.whenReadyToAuthenticate = null; this._log = log; + XPCOMUtils.defineLazyPreferenceGetter(this, "_username", "services.sync.username"); }; this.BrowserIDManager.prototype = { @@ -432,7 +436,7 @@ this.BrowserIDManager.prototype = { }, get username() { - return Svc.Prefs.get("username", null); + return this._username; }, /** @@ -586,13 +590,7 @@ this.BrowserIDManager.prototype = { hasValidToken() { // If pref is set to ignore cached authentication credentials for debugging, // then return false to force the fetching of a new token. - let ignoreCachedAuthCredentials = false; - try { - ignoreCachedAuthCredentials = Svc.Prefs.get("debug.ignoreCachedAuthCredentials"); - } catch (e) { - // Pref doesn't exist - } - if (ignoreCachedAuthCredentials) { + if (IGNORE_CACHED_AUTH_CREDENTIALS) { return false; } if (!this._token) { diff --git a/services/sync/modules/engines.js b/services/sync/modules/engines.js index cd9203ec7a99..447f07b1ce3c 100644 --- a/services/sync/modules/engines.js +++ b/services/sync/modules/engines.js @@ -69,7 +69,6 @@ this.Tracker = function Tracker(name, engine) { Svc.Obs.add("weave:engine:start-tracking", this); Svc.Obs.add("weave:engine:stop-tracking", this); - Svc.Prefs.observe("engine." + this.engine.prefName, this); }; Tracker.prototype = { @@ -248,11 +247,6 @@ Tracker.prototype = { this.stopTracking(); this._isTracking = false; } - return; - case "nsPref:changed": - if (data == PREFS_BRANCH + "engine." + this.engine.prefName) { - this.onEngineEnabledChanged(this.engine.enabled); - } } }, @@ -261,7 +255,6 @@ Tracker.prototype = { // Important for tests where we unregister the engine during cleanup. Svc.Obs.remove("weave:engine:start-tracking", this); Svc.Obs.remove("weave:engine:stop-tracking", this); - Svc.Prefs.ignore("engine." + this.engine.prefName, this); // Persist all pending tracked changes to disk, and wait for the final write // to finish. @@ -663,6 +656,11 @@ this.Engine = function Engine(name, service) { this._modified = this.emptyChangeset(); this._tracker; // initialize tracker to load previously changed IDs this._log.debug("Engine constructed"); + + XPCOMUtils.defineLazyPreferenceGetter(this, "_enabled", + `services.sync.engine.${this.prefName}`, false, + (data, previous, latest) => + this._tracker.onEngineEnabledChanged(latest)); }; Engine.prototype = { // _storeObj, and _trackerObj should to be overridden in subclasses @@ -689,11 +687,13 @@ Engine.prototype = { }, get enabled() { - return Svc.Prefs.get("engine." + this.prefName, false); + return this._enabled; }, set enabled(val) { - Svc.Prefs.set("engine." + this.prefName, !!val); + if (!!val != this._enabled) { + Svc.Prefs.set("engine." + this.prefName, !!val); + } }, get score() { @@ -776,7 +776,16 @@ this.SyncEngine = function SyncEngine(name, service) { dataPostProcessor: json => this._metadataPostProcessor(json), beforeSave: () => this._beforeSaveMetadata(), }); + Utils.defineLazyIDProperty(this, "syncID", `services.sync.${this.name}.syncID`); + XPCOMUtils.defineLazyPreferenceGetter(this, "_lastSync", + `services.sync.${this.name}.lastSync`, + "0", null, + v => parseFloat(v)); + XPCOMUtils.defineLazyPreferenceGetter(this, "_lastSyncLocal", + `services.sync.${this.name}.lastSyncLocal`, + "0", null, + v => parseInt(v, 10)); // Async initializations can be made in the initialize() method. // The map of ids => metadata for records needing a weak upload. @@ -880,30 +889,18 @@ SyncEngine.prototype = { return this.storageURL + "meta/global"; }, - get syncID() { - // Generate a random syncID if we don't have one - let syncID = Svc.Prefs.get(this.name + ".syncID", ""); - return syncID == "" ? this.syncID = Utils.makeGUID() : syncID; - }, - set syncID(value) { - Svc.Prefs.set(this.name + ".syncID", value); - }, - /* * lastSync is a timestamp in server time. */ get lastSync() { - return parseFloat(Svc.Prefs.get(this.name + ".lastSync", "0")); + return this._lastSync; }, set lastSync(value) { - // Reset the pref in-case it's a number instead of a string - Svc.Prefs.reset(this.name + ".lastSync"); // Store the value as a string to keep floating point precision Svc.Prefs.set(this.name + ".lastSync", value.toString()); }, resetLastSync() { this._log.debug("Resetting " + this.name + " last sync time"); - Svc.Prefs.reset(this.name + ".lastSync"); Svc.Prefs.set(this.name + ".lastSync", "0"); this.lastSyncLocal = 0; }, @@ -931,7 +928,7 @@ SyncEngine.prototype = { * lastSyncLocal is a timestamp in local time. */ get lastSyncLocal() { - return parseInt(Svc.Prefs.get(this.name + ".lastSyncLocal", "0"), 10); + return this._lastSyncLocal; }, set lastSyncLocal(value) { // Store as a string because pref can only store C longs as numbers. diff --git a/services/sync/modules/engines/clients.js b/services/sync/modules/engines/clients.js index 53bf4e22324f..dd416358e96d 100644 --- a/services/sync/modules/engines/clients.js +++ b/services/sync/modules/engines/clients.js @@ -91,6 +91,7 @@ this.ClientEngine = function ClientEngine(service) { this.resetLastSync(); this.fxAccounts = fxAccounts; this.addClientCommandQueue = Promise.resolve(); + Utils.defineLazyIDProperty(this, "localID", "services.sync.client.GUID"); }; ClientEngine.prototype = { __proto__: SyncEngine.prototype, @@ -99,6 +100,7 @@ ClientEngine.prototype = { _trackerObj: ClientsTracker, allowSkippedRecord: false, _knownStaleFxADeviceIds: null, + _lastDeviceCounts: null, // These two properties allow us to avoid replaying the same commands // continuously if we cannot manage to upload our own record. @@ -186,15 +188,6 @@ ClientEngine.prototype = { return counts; }, - get localID() { - // Generate a random GUID id we don't have one - let localID = Svc.Prefs.get("client.GUID", ""); - return localID == "" ? this.localID = Utils.makeGUID() : localID; - }, - set localID(value) { - Svc.Prefs.set("client.GUID", value); - }, - get brandName() { let brand = Services.strings.createBundle( "chrome://branding/locale/brand.properties"); @@ -202,12 +195,7 @@ ClientEngine.prototype = { }, get localName() { - let name = Utils.getDeviceName(); - // If `getDeviceName` returns the default name, set the pref. FxA registers - // the device before syncing, so we don't need to update the registration - // in this case. - Svc.Prefs.set("client.name", name); - return name; + return Utils.getDeviceName(); }, set localName(value) { Svc.Prefs.set("client.name", value); @@ -562,7 +550,8 @@ ClientEngine.prototype = { // so non-histogram telemetry (eg, UITelemetry) and the sync scheduler // has easy access to them, and so they are accurate even before we've // successfully synced the first time after startup. - for (let [deviceType, count] of this.deviceTypes) { + let deviceTypeCounts = this.deviceTypes; + for (let [deviceType, count] of deviceTypeCounts) { let hid; let prefName = this.name + ".devices."; switch (deviceType) { @@ -579,8 +568,13 @@ ClientEngine.prototype = { continue; } Services.telemetry.getHistogramById(hid).add(count); - Svc.Prefs.set(prefName, count); + // Optimization: only write the pref if it changed since our last sync. + if (this._lastDeviceCounts == null || + this._lastDeviceCounts.get(prefName) != count) { + Svc.Prefs.set(prefName, count); + } } + this._lastDeviceCounts = deviceTypeCounts; return SyncEngine.prototype._syncFinish.call(this); }, @@ -1077,7 +1071,7 @@ ClientsTracker.prototype = { break; case "nsPref:changed": this._log.debug("client.name preference changed"); - this.addChangedID(Svc.Prefs.get("client.GUID")); + this.addChangedID(this.engine.localID); this.score += SCORE_INCREMENT_XLARGE; break; } diff --git a/services/sync/modules/policies.js b/services/sync/modules/policies.js index fd5e3da4506f..bfbf54ea49a3 100644 --- a/services/sync/modules/policies.js +++ b/services/sync/modules/policies.js @@ -85,32 +85,35 @@ SyncScheduler.prototype = { }, get syncInterval() { - return Svc.Prefs.get("syncInterval", this.singleDeviceInterval); + return this._syncInterval; }, set syncInterval(value) { - Svc.Prefs.set("syncInterval", value); + if (value != this._syncInterval) { + Services.prefs.setIntPref("services.sync.syncInterval", value); + } }, get syncThreshold() { - return Svc.Prefs.get("syncThreshold", SINGLE_USER_THRESHOLD); + return this._syncThreshold; }, set syncThreshold(value) { - Svc.Prefs.set("syncThreshold", value); + if (value != this._syncThreshold) { + Services.prefs.setIntPref("services.sync.syncThreshold", value); + } }, get globalScore() { - return Svc.Prefs.get("globalScore", 0); + return this._globalScore; }, set globalScore(value) { - Svc.Prefs.set("globalScore", value); + if (this._globalScore != value) { + Services.prefs.setIntPref("services.sync.globalScore", value); + } }, - // The number of clients we have is maintained in preferences via the - // clients engine, and only updated after a successsful sync. + // Managed by the clients engine (by way of prefs) get numClients() { - return Svc.Prefs.get("clients.devices.desktop", 0) + - Svc.Prefs.get("clients.devices.mobile", 0); - + return this.numDesktopClients + this.numMobileClients; }, set numClients(value) { throw new Error("Don't set numClients - the clients engine manages it."); @@ -137,9 +140,34 @@ SyncScheduler.prototype = { return false; }, + _initPrefGetters() { + + XPCOMUtils.defineLazyPreferenceGetter(this, + "idleTime", "services.sync.scheduler.idleTime"); + XPCOMUtils.defineLazyPreferenceGetter(this, + "maxResyncs", "services.sync.maxResyncs", 0); + + // The number of clients we have is maintained in preferences via the + // clients engine, and only updated after a successsful sync. + XPCOMUtils.defineLazyPreferenceGetter(this, + "numDesktopClients", "services.sync.clients.devices.desktop", 0); + XPCOMUtils.defineLazyPreferenceGetter(this, + "numMobileClients", "services.sync.clients.devices.mobile", 0); + + // Scheduler state that seems to be read more often than it's written. + // We also check if the value has changed before writing in the setters. + XPCOMUtils.defineLazyPreferenceGetter(this, + "_syncThreshold", "services.sync.syncThreshold", SINGLE_USER_THRESHOLD); + XPCOMUtils.defineLazyPreferenceGetter(this, + "_syncInterval", "services.sync.syncInterval", this.singleDeviceInterval); + XPCOMUtils.defineLazyPreferenceGetter(this, + "_globalScore", "services.sync.globalScore", 0); + }, + init: function init() { this._log.manageLevelFromPref("services.sync.log.logger.service.main"); this.setDefaults(); + this._initPrefGetters(); Svc.Obs.add("weave:engine:score:updated", this); Svc.Obs.add("network:offline-status-changed", this); Svc.Obs.add("network:link-status-changed", this); @@ -162,7 +190,7 @@ SyncScheduler.prototype = { Svc.Obs.add("wake_notification", this); Svc.Obs.add("captive-portal-login-success", this); Svc.Obs.add("sleep_notification", this); - IdleService.addIdleObserver(this, Svc.Prefs.get("scheduler.idleTime")); + IdleService.addIdleObserver(this, this.idleTime); } }, @@ -308,7 +336,7 @@ SyncScheduler.prototype = { break; case "weave:service:setup-complete": Services.prefs.savePrefFile(null); - IdleService.addIdleObserver(this, Svc.Prefs.get("scheduler.idleTime")); + IdleService.addIdleObserver(this, this.idleTime); Svc.Obs.add("wake_notification", this); Svc.Obs.add("captive-portal-login-success", this); Svc.Obs.add("sleep_notification", this); @@ -316,7 +344,7 @@ SyncScheduler.prototype = { case "weave:service:start-over": this.setDefaults(); try { - IdleService.removeIdleObserver(this, Svc.Prefs.get("scheduler.idleTime")); + IdleService.removeIdleObserver(this, this.idleTime); } catch (ex) { if (ex.result != Cr.NS_ERROR_FAILURE) { throw ex; @@ -412,13 +440,14 @@ SyncScheduler.prototype = { updateGlobalScore() { let engines = [this.service.clientsEngine].concat(this.service.engineManager.getEnabled()); + let globalScore = this.globalScore; for (let i = 0;i < engines.length;i++) { this._log.trace(engines[i].name + ": score: " + engines[i].score); - this.globalScore += engines[i].score; + globalScore += engines[i].score; engines[i]._tracker.resetScore(); } - - this._log.trace("Global score updated: " + this.globalScore); + this.globalScore = globalScore; + this._log.trace("Global score updated: " + globalScore); }, calculateScore() { @@ -516,11 +545,11 @@ SyncScheduler.prototype = { Status.backoffInterval + " ms instead."); interval = Status.backoffInterval; } - - if (this.nextSync != 0) { + let nextSync = this.nextSync; + if (nextSync != 0) { // There's already a sync scheduled. Don't reschedule if there's already // a timer scheduled for sooner than requested. - let currentInterval = this.nextSync - Date.now(); + let currentInterval = nextSync - Date.now(); this._log.trace("There's already a sync scheduled in " + currentInterval + " ms."); if (currentInterval < interval && this.syncTimer) { @@ -627,9 +656,6 @@ SyncScheduler.prototype = { this.syncTimer.clear(); }, - get maxResyncs() { - return Svc.Prefs.get("maxResyncs", 0); - }, }; this.ErrorHandler = function ErrorHandler(service) { diff --git a/services/sync/modules/resource.js b/services/sync/modules/resource.js index 3e9e7a34b0e3..d78881473ad0 100644 --- a/services/sync/modules/resource.js +++ b/services/sync/modules/resource.js @@ -37,6 +37,11 @@ this.Resource = function Resource(uri) { }; // (static) Caches the latest server timestamp (X-Weave-Timestamp header). Resource.serverTime = null; + +XPCOMUtils.defineLazyPreferenceGetter(Resource, + "SEND_VERSION_INFO", + "services.sync.sendVersionInfo", + true); Resource.prototype = { _logName: "Sync.Resource", @@ -91,7 +96,7 @@ Resource.prototype = { _buildHeaders(method) { const headers = new Headers(this._headers); - if (Svc.Prefs.get("sendVersionInfo", true)) { + if (Resource.SEND_VERSION_INFO) { headers.append("user-agent", Utils.userAgent); } diff --git a/services/sync/modules/service.js b/services/sync/modules/service.js index 14fc5f8df134..456aa7b5321f 100644 --- a/services/sync/modules/service.js +++ b/services/sync/modules/service.js @@ -69,6 +69,7 @@ XPCOMUtils.defineLazyGetter(this, "browserSessionID", Utils.makeGUID); function Sync11Service() { this._notify = Utils.notify("weave:service:"); + Utils.defineLazyIDProperty(this, "syncID", "services.sync.client.syncID"); } Sync11Service.prototype = { @@ -95,15 +96,6 @@ Sync11Service.prototype = { this._updateCachedURLs(); }, - get syncID() { - // Generate a random syncID id we don't have one - let syncID = Svc.Prefs.get("client.syncID", ""); - return syncID == "" ? this.syncID = Utils.makeGUID() : syncID; - }, - set syncID(value) { - Svc.Prefs.set("client.syncID", value); - }, - get isLoggedIn() { return this._loggedIn; }, get locked() { return this._locked; }, diff --git a/services/sync/modules/util.js b/services/sync/modules/util.js index b1ad514bf0d0..920991aee69e 100644 --- a/services/sync/modules/util.js +++ b/services/sync/modules/util.js @@ -27,6 +27,14 @@ XPCOMUtils.defineLazyServiceGetter(this, "cryptoSDR", "@mozilla.org/login-manager/crypto/SDR;1", "nsILoginManagerCrypto"); +XPCOMUtils.defineLazyPreferenceGetter(this, "localDeviceName", + "services.sync.client.name", + ""); + +XPCOMUtils.defineLazyPreferenceGetter(this, "localDeviceType", + "services.sync.client.type", + DEVICE_TYPE_DESKTOP); + /* * Custom exception types. */ @@ -80,7 +88,7 @@ this.Utils = { Services.appinfo.appBuildID + "."; // Build. /* eslint-enable no-multi-spaces */ } - return this._userAgent + Svc.Prefs.get("client.type", "desktop"); + return this._userAgent + localDeviceType; }, /** @@ -626,17 +634,60 @@ this.Utils = { }, getDeviceName() { - const deviceName = Svc.Prefs.get("client.name", ""); + let deviceName = localDeviceName; if (deviceName === "") { - return this.getDefaultDeviceName(); + deviceName = this.getDefaultDeviceName(); + Svc.Prefs.set("client.name", deviceName); } return deviceName; }, + /** + * Helper to implement a more efficient version of fairly common pattern: + * + * Utils.defineLazyIDProperty(this, "syncID", "services.sync.client.syncID") + * + * is equivalent to (but more efficient than) the following: + * + * Foo.prototype = { + * ... + * get syncID() { + * let syncID = Svc.Prefs.get("client.syncID", ""); + * return syncID == "" ? this.syncID = Utils.makeGUID() : syncID; + * }, + * set syncID(value) { + * Svc.Prefs.set("client.syncID", value); + * }, + * ... + * }; + */ + 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 stomp on any setter we define. + const storage = {}; + XPCOMUtils.defineLazyPreferenceGetter(storage, "value", prefName, ""); + Object.defineProperty(object, propName, { + configurable: true, + enumerable: true, + get() { + let value = storage.value; + if (!value) { + value = Utils.makeGUID(); + Services.prefs.setStringPref(prefName, value); + } + return value; + }, + set(value) { + Services.prefs.setStringPref(prefName, value); + } + }); + }, + getDeviceType() { - return Svc.Prefs.get("client.type", DEVICE_TYPE_DESKTOP); + return localDeviceType; }, formatTimestamp(date) {