Bug 629742: more efficient handling of bookmark callbacks in Sync. r=philiKON
authorRichard Newman <rnewman@mozilla.com>
Thu, 07 Apr 2011 15:55:29 -0700
changeset 67777 4ad9a3906f713c5a38de8209e616150eea621034
parent 67776 7a31e654594358df05f20849c63fcd6f6b26ef02
child 67778 e35d87f188ebe3af32a41a001ff45c92f8d299ac
push id19427
push userpweitershausen@mozilla.com
push dateSun, 10 Apr 2011 18:54:44 +0000
treeherdermozilla-central@21ce62e6aebe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersphiliKON
bugs629742
milestone2.2a1pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 629742: more efficient handling of bookmark callbacks in Sync. r=philiKON
services/sync/modules/engines/bookmarks.js
services/sync/modules/engines/history.js
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -53,20 +53,19 @@ const SIDEBAR_ANNO         = "bookmarkPr
 const STATICTITLE_ANNO     = "bookmarks/staticTitle";
 const FEEDURI_ANNO         = "livemark/feedURI";
 const SITEURI_ANNO         = "livemark/siteURI";
 const GENERATORURI_ANNO    = "microsummary/generatorURI";
 const MOBILEROOT_ANNO      = "mobile/bookmarksRoot";
 const MOBILE_ANNO          = "MobileBookmarks";
 const EXCLUDEBACKUP_ANNO   = "places/excludeFromBackup";
 const SMART_BOOKMARKS_ANNO = "Places/SmartBookmark";
-const GUID_ANNO            = "sync/guid";
 const PARENT_ANNO          = "sync/parent";
 const ANNOS_TO_TRACK = [DESCRIPTION_ANNO, SIDEBAR_ANNO, STATICTITLE_ANNO,
-                       FEEDURI_ANNO, SITEURI_ANNO, GENERATORURI_ANNO];
+                        FEEDURI_ANNO, SITEURI_ANNO, GENERATORURI_ANNO];
 
 const SERVICE_NOT_SUPPORTED = "Service not supported on this platform";
 const FOLDER_SORTINDEX = 1000000;
 
 try {
   Cu.import("resource://gre/modules/PlacesUtils.jsm");
 }
 catch(ex) {
@@ -1409,44 +1408,54 @@ BookmarksTracker.prototype = {
    * folder is for livemarks
    *
    * @param itemId
    *        Item under consideration to ignore
    * @param folder (optional)
    *        Folder of the item being changed
    */
   _ignore: function BMT__ignore(itemId, folder) {
-    // Ignore unconditionally if the engine tells us to
+    // Ignore unconditionally if the engine tells us to.
     if (this.ignoreAll)
       return true;
 
-    // Ensure that the mobile bookmarks query is correct in the UI
-    this._ensureMobileQuery();
+    // Get the folder id if we weren't given one.
+    if (folder == null) {
+      try {
+        folder = this._bms.getFolderIdForItem(itemId);
+      } catch (ex) {
+        this._log.debug("getFolderIdForItem(" + itemId +
+                        ") threw; calling _ensureMobileQuery.");
+        // I'm guessing that gFIFI can throw, and perhaps that's why
+        // _ensureMobileQuery is here at all. Try not to call it.
+        this._ensureMobileQuery();
+        folder = this._bms.getFolderIdForItem(itemId);
+      }
+    }
 
-    // Make sure to remove items that have the exclude annotation
+    // Ignore livemark children.
+    if (this._ls.isLivemark(folder))
+      return true;
+
+    // Ignore changes to tags (folders under the tags folder).
+    let tags = kSpecialIds.tags;
+    if (folder == tags)
+      return true;
+
+    // Ignore tag items (the actual instance of a tag for a bookmark).
+    if (this._bms.getFolderIdForItem(folder) == tags)
+      return true;
+
+    // Make sure to remove items that have the exclude annotation.
     if (Svc.Annos.itemHasAnnotation(itemId, EXCLUDEBACKUP_ANNO)) {
       this.removeChangedID(this._GUIDForId(itemId));
       return true;
     }
 
-    // Get the folder id if we weren't given one
-    if (folder == null)
-      folder = this._bms.getFolderIdForItem(itemId);
-
-    let tags = kSpecialIds.tags;
-    // Ignore changes to tags (folders under the tags folder)
-    if (folder == tags)
-      return true;
-
-    // Ignore tag items (the actual instance of a tag for a bookmark)
-    if (this._bms.getFolderIdForItem(folder) == tags)
-      return true;
-
-    // Ignore livemark children
-    return this._ls.isLivemark(folder);
+    return false;
   },
 
   onItemAdded: function BMT_onEndUpdateBatch(itemId, folder, index) {
     if (this._ignore(itemId, folder))
       return;
 
     this._log.trace("onItemAdded: " + itemId);
     this._addId(itemId);
@@ -1475,17 +1484,17 @@ BookmarksTracker.prototype = {
 
     // Disable handling of notifications while changing the mobile query
     this.ignoreAll = true;
 
     let mobile = find(MOBILE_ANNO);
     let queryURI = Utils.makeURI("place:folder=" + kSpecialIds.mobile);
     let title = Str.sync.get("mobile.label");
 
-    // Don't add OR do remove the mobile bookmarks if there's nothing
+    // Don't add OR remove the mobile bookmarks if there's nothing.
     if (Svc.Bookmark.getIdForItemAt(kSpecialIds.mobile, 0) == -1) {
       if (mobile.length != 0)
         Svc.Bookmark.removeItem(mobile[0]);
     }
     // Add the mobile bookmarks query if it doesn't exist
     else if (mobile.length == 0) {
       let query = Svc.Bookmark.insertBookmark(all[0], queryURI, -1, title);
       Svc.Annos.setItemAnnotation(query, anno, MOBILE_ANNO, 0,
@@ -1495,36 +1504,31 @@ BookmarksTracker.prototype = {
     }
     // Make sure the existing title is correct
     else if (Svc.Bookmark.getItemTitle(mobile[0]) != title)
       Svc.Bookmark.setItemTitle(mobile[0], title);
 
     this.ignoreAll = false;
   },
 
+  // This method is oddly structured, but the idea is to return as quickly as
+  // possible -- this handler gets called *every time* a bookmark changes, for
+  // *each change*. That's particularly bad when a bunch of livemarks are
+  // updated.
   onItemChanged: function BMT_onItemChanged(itemId, property, isAnno, value) {
-    if (this._ignore(itemId))
+    // Quicker checks first.
+    // Ignore favicon changes to avoid unnecessary churn.
+    if (this.ignoreAll || property == "favicon")
       return;
 
-    // Allocate a new GUID if necessary.
-    // We only want to do it if there's a dupe, so use idForGUID to achieve that.
-    if (isAnno && (property == GUID_ANNO)) {
-      this._log.trace("onItemChanged for " + GUID_ANNO +
-                      ": probably needs a new one.");
-      this._idForGUID(this._GUIDForId(itemId));
-      this._addId(itemId);
-      return;
-    }
-
-    // ignore annotations except for the ones that we sync
-    if (isAnno && ANNOS_TO_TRACK.indexOf(property) == -1)
+    if (isAnno && (ANNOS_TO_TRACK.indexOf(anno) == -1))
+      // Ignore annotations except for the ones that we sync.
       return;
 
-    // Ignore favicon changes to avoid unnecessary churn
-    if (property == "favicon")
+    if (this._ignore(itemId))
       return;
 
     this._log.trace("onItemChanged: " + itemId +
                     (", " + property + (isAnno? " (anno)" : "")) +
                     (value ? (" = \"" + value + "\"") : ""));
     this._addId(itemId);
   },
 
--- a/services/sync/modules/engines/history.js
+++ b/services/sync/modules/engines/history.js
@@ -39,17 +39,16 @@
 
 const EXPORTED_SYMBOLS = ['HistoryEngine', 'HistoryRec'];
 
 const Cc = Components.classes;
 const Ci = Components.interfaces;
 const Cu = Components.utils;
 const Cr = Components.results;
 
-const GUID_ANNO = "sync/guid";
 const HISTORY_TTL = 5184000; // 60 days
 const TOPIC_UPDATEPLACES_COMPLETE = "places-updatePlaces-complete";
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://services-sync/constants.js");
 Cu.import("resource://services-sync/engines.js");
 Cu.import("resource://services-sync/record.js");
 Cu.import("resource://services-sync/util.js");