add a generic cache class for storing records during reconciliation. cache has 100 item limit, and is cleared before and after reconciliation so the OS can reclaim memory after GC
authorDan Mills <thunder@mozilla.com>
Tue, 30 Dec 2008 23:52:20 -0800
changeset 45146 a9b97ff3a0f1bde91a0a61e94dbf25887a07f50d
parent 45145 7cd4d0d6f13b17482a3f2659f0b892e3172d3fa8
child 45147 c48a88b73576dee2712d0198c5e0c17ab769d488
push idunknown
push userunknown
push dateunknown
add a generic cache class for storing records during reconciliation. cache has 100 item limit, and is cleared before and after reconciliation so the OS can reclaim memory after GC
services/sync/modules/engines.js
services/sync/modules/engines/bookmarks.js
services/sync/modules/notifications.js
services/sync/modules/stores.js
--- a/services/sync/modules/engines.js
+++ b/services/sync/modules/engines.js
@@ -228,16 +228,22 @@ Engine.prototype = {
     this._notify("reset-client", "", this._resetClient).async(this, onComplete);
   }
 };
 
 function SyncEngine() { /* subclasses should call this._init() */ }
 SyncEngine.prototype = {
   __proto__: Engine.prototype,
 
+  get _memory() {
+    let mem = Cc["@mozilla.org/xpcom/memory-service;1"].getService(Ci.nsIMemory);
+    this.__defineGetter__("_memory" function() mem);
+    return mem;
+  },
+
   get baseURL() {
     let url = Utils.prefs.getCharPref("serverURL");
     if (url && url[url.length-1] != '/')
       url += '/';
     url += "0.3/user/";
     return url;
   },
 
@@ -290,16 +296,27 @@ SyncEngine.prototype = {
   _changeRecordRefs: function SyncEngine__changeRecordRefs(oldID, newID) {
     let self = yield;
     for each (let rec in this.outgoing) {
       if (rec.parentid == oldID)
         rec.parentid = newID;
     }
   },
 
+  _lowMemCheck: function SyncEngine__lowMemCheck() {
+    if (mem.isLowMemory()) {
+      this._log.warn("Low memory, forcing GC");
+      Cu.forceGC();
+      if (mem.isLowMemory()) {
+        this._log.warn("Low memory, aborting sync!");
+        throw "Low memory";
+      }
+    }
+  },
+
   // Any setup that needs to happen at the beginning of each sync.
   // Makes sure crypto records and keys are all set-up
   _syncStartup: function SyncEngine__syncStartup() {
     let self = yield;
 
     this._log.debug("Ensuring server crypto records are there");
 
     let meta = yield CryptoMetas.get(self.cb, this.cryptoMetaURL);
@@ -334,45 +351,46 @@ SyncEngine.prototype = {
   },
 
   // Generate outgoing records
   _processIncoming: function SyncEngine__processIncoming() {
     let self = yield;
 
     this._log.debug("Downloading & applying server changes");
 
+    // enable cache, and keep only the first few items.  Otherwise (when
+    // we have more outgoing items than can fit in the cache), we will
+    // keep rotating items in and out, perpetually getting cache misses
+    this._store.cache.enabled = true;
+    this._store.cache.fifo = false; // filo
+    this._store.cache.clear();
+
     let newitems = new Collection(this.engineURL);
     newitems.modified = this.lastSync;
     newitems.full = true;
     newitems.sort = "depthindex";
     yield newitems.get(self.cb);
 
-    let mem = Cc["@mozilla.org/xpcom/memory-service;1"].getService(Ci.nsIMemory);
+    let item;
     this._lastSyncTmp = 0;
-    let item;
     while ((item = yield newitems.iter.next(self.cb))) {
-      if (mem.isLowMemory()) {
-        this._log.warn("Low memory, forcing GC");
-        Cu.forceGC();
-        if (mem.isLowMemory()) {
-          this._log.warn("Low memory, aborting sync!");
-          throw "Low memory";
-        }
-      }
+      this._lowMemCheck();
       yield item.decrypt(self.cb, ID.get('WeaveCryptoID').password);
       if (yield this._reconcile.async(this, self.cb, item))
         yield this._applyIncoming.async(this, self.cb, item);
       else {
         this._log.debug("Skipping reconciled incoming item " + item.id);
         if (this._lastSyncTmp < item.modified)
           this._lastSyncTmp = item.modified;
       }
     }
     if (this.lastSync < this._lastSyncTmp)
         this.lastSync = this._lastSyncTmp;
+
+    this._store.cache.clear(); // free some memory
   },
 
   // Reconciliation has two steps:
   // 1) Check for the same item (same ID) on both the incoming and outgoing
   // queues.  This means the same item was modified on this profile and another
   // at the same time.  In this case, this client wins (which really means, the
   // last profile you sync wins).
   // 2) Check if any incoming & outgoing items are actually the same, even
@@ -403,23 +421,17 @@ SyncEngine.prototype = {
     // Check for the incoming item's ID otherwise existing locally
     if (this._store.itemExists(item.id)) {
       self.done(true);
       return;
     }
 
     // Check for items with different IDs which we think are the same one
     for (let id in this._tracker.changedIDs) {
-      // Generate outgoing record or used a cached one
-      let out = (id in this.outgoing)?
-        this.outgoing[id] : this._createRecord(id);
-
-      // cache the first 100, after that we will throw them away - slower but less memory hungry
-      if ([i for (i in this.outgoing)].length <= 100)
-        this.outgoing[id] = out;
+      let out = this._createRecord(id);
 
       if (this._recordLike(item, out)) {
         // change refs in outgoing queue, then actual id of local item
         // XXX might it be better to just clear the outgoing queue?
         yield this._changeRecordRefs.async(this, self.cb, id, item.id);
         this._store.changeItemID(id, item.id);
 
         this._tracker.removeChangedID(item.id);
@@ -451,35 +463,36 @@ SyncEngine.prototype = {
   _uploadOutgoing: function SyncEngine__uploadOutgoing() {
     let self = yield;
 
     if (this.outgoing.length) {
       this._log.debug("Uploading client changes (" + this.outgoing.length + ")");
 
       // collection we'll upload
       let up = new Collection(this.engineURL);
-
-      // regen the store cache so we can get item depths
-      //this._store.cacheItemsHint();
       let depth = {};
 
-      let out;
-      while ((out = this.outgoing.pop())) {
+      // don't cache the outgoing items, we won't need them later
+      this._store.cache.enabled = false;
+
+      for (let id in this._tracker.changedIDs) {
+        let out = this._createRecord(id);
         this._log.trace("Outgoing:\n" + out);
         yield out.encrypt(self.cb, ID.get('WeaveCryptoID').password);
         yield up.pushRecord(self.cb, out);
         this._store.wrapDepth(out.id, depth);
       }
 
+      this._store.cache.enabled = true;
+
       // now add short depth-only records
       this._log.trace(depth.length + "outgoing depth records");
       for (let id in depth) {
         up.pushDepthRecord({id: id, depth: depth[id]});
       }
-      //this._store.clearItemCacheHint();
 
       // do the upload
       yield up.post(self.cb);
 
       // save last modified date
       let mod = up.data.modified;
       if (typeof(mod) == "string")
         mod = parseInt(mod);
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -485,17 +485,19 @@ BookmarksStore.prototype = {
       return "";
     }
   },
 
   // Create a record starting from the weave id (places guid)
   // NOTE: the record id will not be set, because WBOs generate it from
   //       the URL, which we don't have here.  The engine sets it.
   createRecord: function BStore_createRecord(guid) {
-    let record;
+    let record = this.cache.get(guid);
+    if (record)
+      return record;
 
     let placeId = this._bms.getItemIdForGUID(guid);
     if (placeId < 0) {
       record = new PlacesItem();
       record.cleartext = null; // deleted item
       return record;
     }
 
@@ -544,16 +546,17 @@ BookmarksStore.prototype = {
       this._log.warn("Unknown item type, cannot serialize: " +
                      this._bms.getItemType(placeId));
     }
 
     record.parentid = this._getWeaveIdForItem(this._bms.getFolderIdForItem(placeId));
     record.depth = this._itemDepth(placeId);
     record.sortindex = this._bms.getItemIndex(placeId);
 
+    this.cache.put(guid, record);
     return record;
   },
 
   getAllIDs: function BStore_getAllIDs(node, items) {
     if (!node) {
       let items = {};
       this.getAllIDs(this._getNode(this._bms.bookmarksMenuFolder), items);
       this.getAllIDs(this._getNode(this._bms.toolbarFolder), items);
--- a/services/sync/modules/notifications.js
+++ b/services/sync/modules/notifications.js
@@ -89,25 +89,21 @@ let Notifications = {
       // XXX Should we throw because we didn't find the existing notification?
       // XXX Should we notify observers about weave:notification:added?
     }
 
     // XXX Should we notify observers about weave:notification:replaced?
   },
 
   // replaces all existing notifications with the same title as the new one
-  // FIXME not working?
   replaceTitle: function Notifications_replaceTitle(notification) {
-    for each (let old in this.notifications) {
-      if (old.title == notification.title)
-        this.remove(old);
-    }
+    this.notifications.filter(function(old) old.title == notification.title)
+      .forEach(function(old) this.remove(old), this);
     this.add(notification);
   }
-
 };
 
 
 /**
  * A basic notification.  Subclass this to create more complex notifications.
  */
 function Notification(title, description, iconURL, priority, buttons) {
   this.title = title;
--- a/services/sync/modules/stores.js
+++ b/services/sync/modules/stores.js
@@ -75,16 +75,22 @@ Store.prototype = {
   __json: null,
   get _json() {
     if (!this.__json)
       this.__json = Cc["@mozilla.org/dom/json;1"].
         createInstance(Ci.nsIJSON);
     return this.__json;
   },
 
+  get cache() {
+    let cache = new RecordCache();
+    this.__defineGetter__("cache", function() cache);
+    return cache;
+  },
+
   _init: function Store__init() {
     this._log = Log4Moz.repository.getLogger("Store." + this._logName);
   },
 
   applyIncoming: function BStore_applyIncoming(onComplete, record) {
     let fn = function(rec) {
       let self = yield;
       if (!rec.cleartext)
@@ -92,24 +98,16 @@ Store.prototype = {
       else if (!this.itemExists(rec.id))
         this.create(rec);
       else
         this.update(rec);
     };
     fn.async(this, onComplete, record);
   },
 
-  cacheItemsHint: function Store_cacheItemsHint() {
-    this._itemCache = this.wrap();
-  },
-
-  clearItemCacheHint: function Store_clearItemCacheHint() {
-    this._itemCache = null;
-  },
-
   // override these in derived objects
 
   itemExists: function Store_itemExists(id) {
     throw "override itemExists in a subclass";
   },
 
   createRecord: function Store_createRecord() {
     throw "override createRecord in a subclass";
@@ -135,16 +133,69 @@ Store.prototype = {
     throw "override getAllIDs in a subclass";
   },
 
   wipe: function Store_wipe() {
     throw "override wipe in a subclass";
   }
 };
 
+function Cache() {
+  this.count = 0;
+  this.maxItems = 100;
+  this.fifo = true;
+  this.enabled = true;
+  this._head = this._tail = null;
+  this._items = {};
+}
+Cache.prototype = {
+  _pop: function Cache__pop() {
+    if (this.count <= 0)
+      return;
+    if (this.count == 1)
+      this.clear();
+    else {
+      delete this._items[this._tail.id];
+      this._tail = this._tail.prev;
+      this.count--;
+    }
+  },
+  put: function Cache_put(id, item) {
+    if (!this.enabled)
+      return;
+    let wrapper = {id: id, prev: null, next: this._head, item: item};
+    this._items[wrapper.id] = wrapper;
+
+    if (this.fifo) {
+      if (this._head)
+        this._head.prev = wrapper;
+      this._head = wrapper;
+    } else {
+      if (this._tail)
+        this._tail.next = wrapper;
+      this._tail = wrapper;
+    }
+    
+    this.count++;
+    if (this.count >= this.maxItems)
+      this._pop();
+  },
+  get: function Cache_get(id) {
+    if (id in this._items)
+      return this._items[id].item;
+    return undefined;
+  },
+  clear: function Cache_clear() {
+    this.count = 0;
+    this._head = null;
+    this._tail = null;
+    this._items = {};
+  }
+};
+
 function SnapshotStore(name) {
   this._init(name);
 }
 SnapshotStore.prototype = {
   _logName: "SnapStore",
 
   _filename: null,
   get filename() {