Bug 1372496 - Speed up bookmarks.eraseEverything and bookmarks.remove APIs by handling updating of frencencies outside of the main transaction. r?mak draft
authorMark Banner <standard8@mozilla.com>
Thu, 15 Jun 2017 11:38:22 +0100
changeset 594864 b29217048c010ad74d45e1c4b52331046add4f70
parent 594702 035c25bef7b5e4175006e63eff10c61c2eef73f1
child 633557 095213ca6a1744c7254936cebf21d8a2cd19697e
push id64175
push userbmo:standard8@mozilla.com
push dateThu, 15 Jun 2017 16:02:08 +0000
reviewersmak
bugs1372496
milestone56.0a1
Bug 1372496 - Speed up bookmarks.eraseEverything and bookmarks.remove APIs by handling updating of frencencies outside of the main transaction. r?mak MozReview-Commit-ID: FuSLHB23yNt
toolkit/components/places/Bookmarks.jsm
toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -696,29 +696,38 @@ var Bookmarks = Object.freeze({
   eraseEverything(options = {}) {
     if (!options.source) {
       options.source = Bookmarks.SOURCES.DEFAULT;
     }
 
     const folderGuids = [this.toolbarGuid, this.menuGuid, this.unfiledGuid,
                           this.mobileGuid];
     return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: eraseEverything",
-      db => db.executeTransaction(async function() {
-        await removeFoldersContents(db, folderGuids, options);
-        const time = PlacesUtils.toPRTime(new Date());
-        const syncChangeDelta =
-          PlacesSyncUtils.bookmarks.determineSyncChangeDelta(options.source);
-        for (let folderGuid of folderGuids) {
-          await db.executeCached(
-            `UPDATE moz_bookmarks SET lastModified = :time,
-                                      syncChangeCounter = syncChangeCounter + :syncChangeDelta
-             WHERE id IN (SELECT id FROM moz_bookmarks WHERE guid = :folderGuid )
-            `, { folderGuid, time, syncChangeDelta });
+      async function(db) {
+        let urls;
+
+        await db.executeTransaction(async function() {
+          urls = await removeFoldersContents(db, folderGuids, options);
+          const time = PlacesUtils.toPRTime(new Date());
+          const syncChangeDelta =
+            PlacesSyncUtils.bookmarks.determineSyncChangeDelta(options.source);
+          for (let folderGuid of folderGuids) {
+            await db.executeCached(
+              `UPDATE moz_bookmarks SET lastModified = :time,
+                                        syncChangeCounter = syncChangeCounter + :syncChangeDelta
+               WHERE id IN (SELECT id FROM moz_bookmarks WHERE guid = :folderGuid )
+              `, { folderGuid, time, syncChangeDelta });
+          }
+        });
+
+        // We don't wait for the frecency calculation.
+        if (urls && urls.length) {
+          updateFrecency(db, urls, true).then(null, Cu.reportError);
         }
-      })
+      }
     );
   },
 
   /**
    * Returns a list of recently bookmarked items.
    * Only includes actual bookmarks. Excludes folders, separators and queries.
    *
    * @param {integer} numberOfItems
@@ -1536,25 +1545,26 @@ function fetchBookmarksByParent(info) {
   });
 }
 
 // Remove implementation.
 
 function removeBookmark(item, options) {
   return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: removeBookmark",
     async function(db) {
+    let urls;
     let isUntagging = item._grandParentId == PlacesUtils.tagsFolderId;
 
     await db.executeTransaction(async function transaction() {
       // If it's a folder, remove its contents first.
       if (item.type == Bookmarks.TYPE_FOLDER) {
         if (options.preventRemovalOfNonEmptyFolders && item._childCount > 0) {
           throw new Error("Cannot remove a non-empty folder.");
         }
-        await removeFoldersContents(db, [item.guid], options);
+        urls = await removeFoldersContents(db, [item.guid], options);
       }
 
       // Remove annotations first.  If it's a tag, we can avoid paying that cost.
       if (!isUntagging) {
         // We don't go through the annotations service for this cause otherwise
         // we'd get a pointless onItemChanged notification and it would also
         // set lastModified to an unexpected value.
         await removeAnnotationsForItem(db, item._id);
@@ -1591,16 +1601,20 @@ function removeBookmark(item, options) {
     });
 
     // If not a tag recalculate frecency...
     if (item.type == Bookmarks.TYPE_BOOKMARK && !isUntagging) {
       // ...though we don't wait for the calculation.
       updateFrecency(db, [item.url]).catch(Cu.reportError);
     }
 
+    if (urls && urls.length && item.type == Bookmarks.TYPE_FOLDER) {
+      updateFrecency(db, urls, true).catch(Cu.reportError);
+    }
+
     return item;
   });
 }
 
 // Reorder implementation.
 
 function reorderChildren(parent, orderedChildrenGuids, options) {
   return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: updateBookmark",
@@ -1945,20 +1959,25 @@ var setAncestorsLastModified = async fun
       WHERE guid = :guid`,
       { guid: folderGuid, syncChangeDelta });
   }
 };
 
 /**
  * Remove all descendants of one or more bookmark folders.
  *
- * @param db
+ * @param {Object} db
  *        the Sqlite.jsm connection handle.
- * @param folderGuids
+ * @param {Array} folderGuids
  *        array of folder guids.
+ * @return {Array}
+ *         An array of urls that will need to be updated for frecency. These
+ *         are returned rather than updated immediately so that the caller
+ *         can decide when they need to be updated - they do not need to
+ *         stop this function from completing.
  */
 var removeFoldersContents =
 async function(db, folderGuids, options) {
   let syncChangeDelta =
     PlacesSyncUtils.bookmarks.determineSyncChangeDelta(options.source);
 
   let itemsRemoved = [];
   for (let folderGuid of folderGuids) {
@@ -2003,19 +2022,16 @@ async function(db, folderGuids, options)
   // folders.
   await addSyncChangesForRemovedTagFolders(db, itemsRemoved, syncChangeDelta);
 
   // Cleanup orphans.
   await removeOrphanAnnotations(db);
 
   // TODO (Bug 1087576): this may leave orphan tags behind.
 
-  let urls = itemsRemoved.filter(item => "url" in item).map(item => item.url);
-  updateFrecency(db, urls, true).then(null, Cu.reportError);
-
   // Send onItemRemoved notifications to listeners.
   // TODO (Bug 1087580): for the case of eraseEverything, this should send a
   // single clear bookmarks notification rather than notifying for each
   // bookmark.
 
   // Notify listeners in reverse order to serve children before parents.
   let { source = Bookmarks.SOURCES.DEFAULT } = options;
   let observers = PlacesUtils.bookmarks.getObservers();
@@ -2035,16 +2051,17 @@ async function(db, folderGuids, options)
         notify(observers, "onItemChanged", [ entry._id, "tags", false, "",
                                              PlacesUtils.toPRTime(entry.lastModified),
                                              entry.type, entry._parentId,
                                              entry.guid, entry.parentGuid,
                                              "", source ]);
       }
     }
   }
+  return itemsRemoved.filter(item => "url" in item).map(item => item.url);
 };
 
 /**
  * Tries to insert a new place if it doesn't exist yet.
  * @param url
  *        A valid URL object.
  * @return {Promise} resolved when the operation is complete.
  */
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
@@ -169,47 +169,72 @@ add_task(async function remove_bookmark_
 });
 
 add_task(async function remove_folder() {
   let bm1 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
                                                  type: PlacesUtils.bookmarks.TYPE_FOLDER,
                                                  title: "a folder" });
   checkBookmarkObject(bm1);
 
-  let manyFrencenciesPromise = promiseManyFrecenciesChanged();
-
   let bm2 = await PlacesUtils.bookmarks.remove(bm1.guid);
   checkBookmarkObject(bm2);
 
   Assert.deepEqual(bm1, bm2);
   Assert.equal(bm2.parentGuid, PlacesUtils.bookmarks.unfiledGuid);
   Assert.equal(bm2.index, 0);
   Assert.deepEqual(bm2.dateAdded, bm2.lastModified);
   Assert.equal(bm2.type, PlacesUtils.bookmarks.TYPE_FOLDER);
   Assert.equal(bm2.title, "a folder");
   Assert.ok(!("url" in bm2));
 
-  // We should get an onManyFrecenciesChanged notification with the remove of
-  // a folder.
+  // No promiseManyFrecenciesChanged in this test as the folder doesn't have
+  // any children that would need updating.
+});
+
+add_task(async function test_contents_removed() {
+  let folder1 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+                                                     type: PlacesUtils.bookmarks.TYPE_FOLDER,
+                                                     title: "a folder" });
+  let bm1 = await PlacesUtils.bookmarks.insert({ parentGuid: folder1.guid,
+                                                 type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+                                                 url: "http://example.com/",
+                                                 title: "" });
+
+  let manyFrencenciesPromise = promiseManyFrecenciesChanged();
+  await PlacesUtils.bookmarks.remove(folder1);
+  Assert.strictEqual((await PlacesUtils.bookmarks.fetch(folder1.guid)), null);
+  Assert.strictEqual((await PlacesUtils.bookmarks.fetch(bm1.guid)), null);
+
+  // We should get an onManyFrecenciesChanged notification with the removal of
+  // a folder with children.
   await manyFrencenciesPromise;
 });
 
+
 add_task(async function test_nested_contents_removed() {
   let folder1 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
                                                      type: PlacesUtils.bookmarks.TYPE_FOLDER,
                                                      title: "a folder" });
   let folder2 = await PlacesUtils.bookmarks.insert({ parentGuid: folder1.guid,
                                                      type: PlacesUtils.bookmarks.TYPE_FOLDER,
                                                      title: "a folder" });
-  let sep = await PlacesUtils.bookmarks.insert({ parentGuid: folder2.guid,
-                                                 type: PlacesUtils.bookmarks.TYPE_SEPARATOR });
+  let bm1 = await PlacesUtils.bookmarks.insert({ parentGuid: folder2.guid,
+                                                 type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+                                                 url: "http://example.com/",
+                                                 title: "" });
+
+  let manyFrencenciesPromise = promiseManyFrecenciesChanged();
   await PlacesUtils.bookmarks.remove(folder1);
   Assert.strictEqual((await PlacesUtils.bookmarks.fetch(folder1.guid)), null);
   Assert.strictEqual((await PlacesUtils.bookmarks.fetch(folder2.guid)), null);
-  Assert.strictEqual((await PlacesUtils.bookmarks.fetch(sep.guid)), null);
+  Assert.strictEqual((await PlacesUtils.bookmarks.fetch(bm1.guid)), null);
+
+  // We should get an onManyFrecenciesChanged notification with the removal of
+  // a folder with children.
+  await manyFrencenciesPromise;
 });
 
 add_task(async function remove_folder_empty_title() {
   let bm1 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
                                                  type: PlacesUtils.bookmarks.TYPE_FOLDER,
                                                  title: "" });
   checkBookmarkObject(bm1);
 
@@ -243,12 +268,8 @@ add_task(async function test_nested_cont
                                                      type: PlacesUtils.bookmarks.TYPE_FOLDER,
                                                      title: "a folder" });
   await PlacesUtils.bookmarks.insert({ parentGuid: folder1.guid,
                                        type: PlacesUtils.bookmarks.TYPE_FOLDER,
                                        title: "a folder" });
   await Assert.rejects(PlacesUtils.bookmarks.remove(folder1, {preventRemovalOfNonEmptyFolders: true}),
                        /Cannot remove a non-empty folder./);
 });
-
-function run_test() {
-  run_next_test();
-}