Bug 1341097 - Part 1: Group frecency notifications from history notifications. r=mak, a=lizzard
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Tue, 21 Feb 2017 20:00:24 +0000
changeset 376628 4c28ea6d6776a5a8b42d175fbdb568682f506b7b
parent 376627 f904536fd210f2bf145a1a53a0d62447eac10c5a
child 376629 15583c8ebe407a0de32a4c15c8aeea6554d8539c
push id6996
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 20:48:21 +0000
treeherdermozilla-beta@d89512dab048 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak, lizzard
bugs1341097
milestone53.0a2
Bug 1341097 - Part 1: Group frecency notifications from history notifications. r=mak, a=lizzard When updating a large number of places, sending runnables to the main thread for every single one of them whose frecency we update is not conducive to a responsive UI. This only gets worse once more observers care about these notifications (e.g. when the library is open). To avoid this on startup when importing from other browsers, this patch adds and uses an option to group the frecency notifications. Later patches will also use the option to avoid other notifications where possible. MozReview-Commit-ID: D5KqPDu86bo
browser/components/migration/MigrationUtils.jsm
browser/components/migration/tests/unit/test_automigration.js
toolkit/components/places/History.cpp
toolkit/components/places/History.h
toolkit/components/places/mozIAsyncHistory.idl
toolkit/components/places/tests/unit/test_async_history_api.js
--- a/browser/components/migration/MigrationUtils.jsm
+++ b/browser/components/migration/MigrationUtils.jsm
@@ -991,17 +991,17 @@ this.MigrationUtils = Object.freeze({
     });
   },
 
   insertVisitsWrapper(places, options) {
     this._importQuantities.history += places.length;
     if (gKeepUndoData) {
       this._updateHistoryUndo(places);
     }
-    return PlacesUtils.asyncHistory.updatePlaces(places, options);
+    return PlacesUtils.asyncHistory.updatePlaces(places, options, true);
   },
 
   insertLoginWrapper(login) {
     this._importQuantities.logins++;
     let insertedLogin = LoginHelper.maybeImportLogin(login);
     // Note that this means that if we import a login that has a newer password
     // than we know about, we will update the login, and an undo of the import
     // will not revert this. This seems preferable over removing the login
--- a/browser/components/migration/tests/unit/test_automigration.js
+++ b/browser/components/migration/tests/unit/test_automigration.js
@@ -245,23 +245,20 @@ add_task(function* checkUndoRemoval() {
   let bookmark = yield PlacesUtils.bookmarks.fetch({url: "http://www.example.org/"});
   Assert.ok(bookmark, "Should have a bookmark before undo");
   Assert.equal(bookmark.title, "Some example bookmark", "Should have correct bookmark before undo.");
 
   // Insert 2 history visits
   let now_uSec = Date.now() * 1000;
   let visitedURI = Services.io.newURI("http://www.example.com/");
   let frecencyUpdatePromise = new Promise(resolve => {
-    let expectedChanges = 2;
     let observer = {
-      onFrecencyChanged() {
-        if (!--expectedChanges) {
-          PlacesUtils.history.removeObserver(observer);
-          resolve();
-        }
+      onManyFrecenciesChanged() {
+        PlacesUtils.history.removeObserver(observer);
+        resolve();
       },
     };
     PlacesUtils.history.addObserver(observer, false);
   });
   yield MigrationUtils.insertVisitsWrapper([{
     uri: visitedURI,
     visits: [
       {
--- a/toolkit/components/places/History.cpp
+++ b/toolkit/components/places/History.cpp
@@ -860,50 +860,70 @@ CanAddURI(nsIURI* aURI,
     nsCOMPtr<nsIRunnable> event =
       new NotifyPlaceInfoCallback(callback, place, true, NS_ERROR_INVALID_ARG);
     (void)NS_DispatchToMainThread(event);
   }
 
   return false;
 }
 
+class NotifyManyFrecenciesChanged final : public Runnable
+{
+public:
+  NotifyManyFrecenciesChanged() {}
+
+  NS_IMETHOD Run() override
+  {
+    MOZ_ASSERT(NS_IsMainThread(), "This should be called on the main thread");
+    nsNavHistory* navHistory = nsNavHistory::GetHistoryService();
+    NS_ENSURE_STATE(navHistory);
+    navHistory->NotifyManyFrecenciesChanged();
+    return NS_OK;
+  }
+};
+
+
 /**
  * Adds a visit to the database.
  */
 class InsertVisitedURIs final: public Runnable
 {
 public:
   /**
    * Adds a visit to the database asynchronously.
    *
    * @param aConnection
    *        The database connection to use for these operations.
    * @param aPlaces
    *        The locations to record visits.
    * @param [optional] aCallback
    *        The callback to notify about the visit.
+   * @param [optional] aGroupNotifications
+   *        Whether to group any observer notifications rather than
+   *        sending them out individually.
    */
   static nsresult Start(mozIStorageConnection* aConnection,
                         nsTArray<VisitData>& aPlaces,
-                        mozIVisitInfoCallback* aCallback = nullptr)
+                        mozIVisitInfoCallback* aCallback = nullptr,
+                        bool aGroupNotifications = false)
   {
     MOZ_ASSERT(NS_IsMainThread(), "This should be called on the main thread");
     MOZ_ASSERT(aPlaces.Length() > 0, "Must pass a non-empty array!");
 
     // Make sure nsNavHistory service is up before proceeding:
     nsNavHistory* navHistory = nsNavHistory::GetHistoryService();
     MOZ_ASSERT(navHistory, "Could not get nsNavHistory?!");
     if (!navHistory) {
       return NS_ERROR_FAILURE;
     }
 
     nsMainThreadPtrHandle<mozIVisitInfoCallback>
       callback(new nsMainThreadPtrHolder<mozIVisitInfoCallback>(aCallback));
     RefPtr<InsertVisitedURIs> event =
-      new InsertVisitedURIs(aConnection, aPlaces, callback);
+      new InsertVisitedURIs(aConnection, aPlaces, callback, aGroupNotifications);
 
     // Get the target thread, and then start the work!
     nsCOMPtr<nsIEventTarget> target = do_GetInterface(aConnection);
     NS_ENSURE_TRUE(target, NS_ERROR_UNEXPECTED);
     nsresult rv = target->Dispatch(event, NS_DISPATCH_NORMAL);
     NS_ENSURE_SUCCESS(rv, rv);
 
     return NS_OK;
@@ -995,24 +1015,33 @@ public:
         rv = NS_DispatchToMainThread(event);
         NS_ENSURE_SUCCESS(rv, rv);
       }
     }
 
     nsresult rv = transaction.Commit();
     NS_ENSURE_SUCCESS(rv, rv);
 
+    if (mGroupNotifications) {
+      nsCOMPtr<nsIRunnable> event =
+        new NotifyManyFrecenciesChanged();
+      rv = NS_DispatchToMainThread(event);
+      NS_ENSURE_SUCCESS(rv, rv);
+    }
+
     return NS_OK;
   }
 private:
   InsertVisitedURIs(mozIStorageConnection* aConnection,
                     nsTArray<VisitData>& aPlaces,
-                    const nsMainThreadPtrHandle<mozIVisitInfoCallback>& aCallback)
+                    const nsMainThreadPtrHandle<mozIVisitInfoCallback>& aCallback,
+                    bool aGroupNotifications)
   : mDBConn(aConnection)
   , mCallback(aCallback)
+  , mGroupNotifications(aGroupNotifications)
   , mHistory(History::GetService())
   {
     MOZ_ASSERT(NS_IsMainThread(), "This should be called on the main thread");
 
     mPlaces.SwapElements(aPlaces);
 
 #ifdef DEBUG
     for (nsTArray<VisitData>::size_type i = 0; i < mPlaces.Length(); i++) {
@@ -1042,17 +1071,17 @@ private:
     // If the page was in moz_places, we need to update the entry.
     nsresult rv;
     if (aKnown) {
       rv = mHistory->UpdatePlace(aPlace);
       NS_ENSURE_SUCCESS(rv, rv);
     }
     // Otherwise, the page was not in moz_places, so now we have to add it.
     else {
-      rv = mHistory->InsertPlace(aPlace);
+      rv = mHistory->InsertPlace(aPlace, !mGroupNotifications);
       NS_ENSURE_SUCCESS(rv, rv);
       aPlace.placeId = nsNavHistory::sLastInsertedPlaceId;
     }
     MOZ_ASSERT(aPlace.placeId > 0);
 
     rv = AddVisit(aPlace);
     NS_ENSURE_SUCCESS(rv, rv);
 
@@ -1161,24 +1190,35 @@ private:
   nsresult UpdateFrecency(const VisitData& aPlace)
   {
     MOZ_ASSERT(aPlace.shouldUpdateFrecency);
     MOZ_ASSERT(aPlace.placeId > 0);
 
     nsresult rv;
     { // First, set our frecency to the proper value.
       nsCOMPtr<mozIStorageStatement> stmt;
-      stmt = mHistory->GetStatement(
-        "UPDATE moz_places "
-        "SET frecency = NOTIFY_FRECENCY("
-          "CALCULATE_FRECENCY(:page_id, :redirect), "
-          "url, guid, hidden, last_visit_date"
-        ") "
-        "WHERE id = :page_id"
-      );
+      if (!mGroupNotifications) {
+        // If we're notifying for individual frecency updates, use
+        // the notify_frecency sql function which will call us back.
+        stmt = mHistory->GetStatement(
+          "UPDATE moz_places "
+          "SET frecency = NOTIFY_FRECENCY("
+            "CALCULATE_FRECENCY(:page_id, :redirect), "
+            "url, guid, hidden, last_visit_date"
+          ") "
+          "WHERE id = :page_id"
+        );
+      } else {
+        // otherwise, just update the frecency without notifying.
+        stmt = mHistory->GetStatement(
+          "UPDATE moz_places "
+          "SET frecency = CALCULATE_FRECENCY(:page_id, :redirect) "
+          "WHERE id = :page_id"
+        );
+      }
       NS_ENSURE_STATE(stmt);
       mozStorageStatementScoper scoper(stmt);
 
       rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("page_id"), aPlace.placeId);
       NS_ENSURE_SUCCESS(rv, rv);
 
       rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("redirect"), aPlace.redirect);
       NS_ENSURE_SUCCESS(rv, rv);
@@ -1209,16 +1249,18 @@ private:
   }
 
   mozIStorageConnection* mDBConn;
 
   nsTArray<VisitData> mPlaces;
 
   nsMainThreadPtrHandle<mozIVisitInfoCallback> mCallback;
 
+  bool mGroupNotifications;
+
   /**
    * Strong reference to the History object because we do not want it to
    * disappear out from under us.
    */
   RefPtr<History> mHistory;
 };
 
 class GetPlaceInfo final : public Runnable {
@@ -2032,17 +2074,17 @@ History::GetIsVisitedStatement(mozIStora
     NS_ENSURE_STATE(dbConn);
     mConcurrentStatementsHolder = new ConcurrentStatementsHolder(dbConn);
   }
   mConcurrentStatementsHolder->GetIsVisitedStatement(aCallback);
   return NS_OK;
 }
 
 nsresult
-History::InsertPlace(VisitData& aPlace)
+History::InsertPlace(VisitData& aPlace, bool aShouldNotifyFrecencyChanged)
 {
   MOZ_ASSERT(aPlace.placeId == 0, "should not have a valid place id!");
   MOZ_ASSERT(!aPlace.shouldUpdateHidden, "We should not need to update hidden");
   MOZ_ASSERT(!NS_IsMainThread(), "must be called off of the main thread!");
 
   nsCOMPtr<mozIStorageStatement> stmt = GetStatement(
       "INSERT INTO moz_places "
         "(url, url_hash, title, rev_host, hidden, typed, frecency, guid) "
@@ -2080,22 +2122,24 @@ History::InsertPlace(VisitData& aPlace)
     NS_ENSURE_SUCCESS(rv, rv);
   }
   rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("guid"), aPlace.guid);
   NS_ENSURE_SUCCESS(rv, rv);
   rv = stmt->Execute();
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Post an onFrecencyChanged observer notification.
-  const nsNavHistory* navHistory = nsNavHistory::GetConstHistoryService();
-  NS_ENSURE_STATE(navHistory);
-  navHistory->DispatchFrecencyChangedNotification(aPlace.spec, frecency,
-                                                  aPlace.guid,
-                                                  aPlace.hidden,
-                                                  aPlace.visitTime);
+  if (aShouldNotifyFrecencyChanged) {
+    const nsNavHistory* navHistory = nsNavHistory::GetConstHistoryService();
+    NS_ENSURE_STATE(navHistory);
+    navHistory->DispatchFrecencyChangedNotification(aPlace.spec, frecency,
+                                                    aPlace.guid,
+                                                    aPlace.hidden,
+                                                    aPlace.visitTime);
+  }
 
   return NS_OK;
 }
 
 nsresult
 History::UpdatePlace(const VisitData& aPlace)
 {
   MOZ_ASSERT(!NS_IsMainThread(), "must be called off of the main thread!");
@@ -2782,16 +2826,17 @@ History::GetPlacesInfo(JS::Handle<JS::Va
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 History::UpdatePlaces(JS::Handle<JS::Value> aPlaceInfos,
                       mozIVisitInfoCallback* aCallback,
+                      bool aGroupNotifications,
                       JSContext* aCtx)
 {
   NS_ENSURE_TRUE(NS_IsMainThread(), NS_ERROR_UNEXPECTED);
   NS_ENSURE_TRUE(!aPlaceInfos.isPrimitive(), NS_ERROR_INVALID_ARG);
 
   uint32_t infosLength;
   JS::Rooted<JSObject*> infos(aCtx);
   nsresult rv = GetJSArrayFromJSValue(aPlaceInfos, aCtx, &infos, &infosLength);
@@ -2901,17 +2946,18 @@ History::UpdatePlaces(JS::Handle<JS::Val
 
   nsMainThreadPtrHandle<mozIVisitInfoCallback>
     callback(new nsMainThreadPtrHolder<mozIVisitInfoCallback>(aCallback));
 
   // It is possible that all of the visits we were passed were dissallowed by
   // CanAddURI, which isn't an error.  If we have no visits to add, however,
   // we should not call InsertVisitedURIs::Start.
   if (visitData.Length()) {
-    nsresult rv = InsertVisitedURIs::Start(dbConn, visitData, callback);
+    nsresult rv = InsertVisitedURIs::Start(dbConn, visitData,
+                                           callback, aGroupNotifications);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   // Be sure to notify that all of our operations are complete.  This
   // is dispatched to the background thread first and redirected to the
   // main thread from there to make sure that all database notifications
   // and all embed or canAddURI notifications have finished.
   if (aCallback) {
--- a/toolkit/components/places/History.h
+++ b/toolkit/components/places/History.h
@@ -63,18 +63,24 @@ public:
    */
   nsresult GetIsVisitedStatement(mozIStorageCompletionCallback* aCallback);
 
   /**
    * Adds an entry in moz_places with the data in aVisitData.
    *
    * @param aVisitData
    *        The visit data to use to populate a new row in moz_places.
+   * @param aShouldNotifyFrecencyChanged
+   *        Whether to dispatch OnFrecencyChanged notifications.
+   *        Defaults to true. Set to false if you (the caller) are
+   *        doing many inserts and will dispatch your own
+   *        OnManyFrecenciesChanged notification.
    */
-  nsresult InsertPlace(VisitData& aVisitData);
+  nsresult InsertPlace(VisitData& aVisitData,
+                       bool aShouldNotifyFrecencyChanged = true);
 
   /**
    * Updates an entry in moz_places with the data in aVisitData.
    *
    * @param aVisitData
    *        The visit data to use to update the existing row in moz_places.
    */
   nsresult UpdatePlace(const VisitData& aVisitData);
--- a/toolkit/components/places/mozIAsyncHistory.idl
+++ b/toolkit/components/places/mozIAsyncHistory.idl
@@ -155,30 +155,34 @@ interface mozIAsyncHistory : nsISupports
    * aCallback.handleResult is called for each visit added.
    *
    * @param aPlaceInfo
    *        The mozIPlaceInfo object[s] containing the information to store or
    *        update.  This can be a single object, or an array of objects.
    * @param [optional] aCallback
    *        A mozIVisitInfoCallback object which consists of callbacks to be
    *        notified for successful and/or failed changes.
+   * @param [optional] aGroupNotifications
+   *        If set to true, the implementation will attempt to avoid using
+   *        per-place/visit notifications as much as possible.
    *
    * @throws NS_ERROR_INVALID_ARG
    *         - Passing in NULL for aPlaceInfo.
    *         - Not providing at least one valid guid, or uri for all
    *           mozIPlaceInfo object[s].
    *         - Not providing an array or nothing for the visits property of
    *           mozIPlaceInfo.
    *         - Not providing a visitDate and transitionType for each
    *           mozIVisitInfo.
    *         - Providing an invalid transitionType for a mozIVisitInfo.
    */
   [implicit_jscontext]
   void updatePlaces(in jsval aPlaceInfo,
-                    [optional] in mozIVisitInfoCallback aCallback);
+                    [optional] in mozIVisitInfoCallback aCallback,
+                    [optional] in bool aGroupNotifications);
 
   /**
    * Checks if a given URI has been visited.
    *
    * @param aURI
    *        The URI to check for.
    * @param aCallback
    *        A mozIVisitStatusCallback object which receives the visited status.
--- a/toolkit/components/places/tests/unit/test_async_history_api.js
+++ b/toolkit/components/places/tests/unit/test_async_history_api.js
@@ -20,31 +20,31 @@ const RECENT_EVENT_THRESHOLD = 15 * 60 *
  */
 function VisitInfo(aTransitionType,
                    aVisitTime) {
   this.transitionType =
     aTransitionType === undefined ? TRANSITION_LINK : aTransitionType;
   this.visitDate = aVisitTime || Date.now() * 1000;
 }
 
-function promiseUpdatePlaces(aPlaces) {
+function promiseUpdatePlaces(aPlaces, aBatchFrecencyNotifications) {
   return new Promise((resolve, reject) => {
     PlacesUtils.asyncHistory.updatePlaces(aPlaces, {
       _errors: [],
       _results: [],
       handleError(aResultCode, aPlace) {
         this._errors.push({ resultCode: aResultCode, info: aPlace});
       },
       handleResult(aPlace) {
         this._results.push(aPlace);
       },
       handleCompletion() {
         resolve({ errors: this._errors, results: this._results });
       }
-    });
+    }, aBatchFrecencyNotifications);
   });
 }
 
 /**
  * Listens for a title change notification, and calls aCallback when it gets it.
  *
  * @param aURI
  *        The URI of the page we expect a notification for.
@@ -1088,13 +1088,43 @@ add_task(function* test_typed_hidden_not
   let db = yield PlacesUtils.promiseDBConnection();
   let rows = yield db.execute(
     "SELECT hidden, typed FROM moz_places WHERE url_hash = hash(:url) AND url = :url",
     { url: "http://mozilla.org/" });
   Assert.equal(rows[0].getResultByName("typed"), 1,
                "The page should be marked as typed");
   Assert.equal(rows[0].getResultByName("hidden"), 0,
                "The page should be marked as not hidden");
+  yield PlacesTestUtils.promiseAsyncUpdates();
 });
 
-function run_test() {
-  run_next_test();
-}
+add_task(function* test_omit_frecency_notifications() {
+  yield PlacesTestUtils.clearHistory();
+  let places = [
+    { uri: NetUtil.newURI("http://mozilla.org/"),
+      title: "test",
+      visits: [
+        new VisitInfo(TRANSITION_TYPED),
+      ]
+    },
+    { uri: NetUtil.newURI("http://example.org/"),
+      title: "test",
+      visits: [
+        new VisitInfo(TRANSITION_TYPED),
+      ]
+    },
+  ];
+  let promiseFrecenciesChanged = new Promise(resolve => {
+    let frecencyObserverCheck = {
+      onFrecencyChanged() {
+        ok(false, "Should not fire frecencyChanged because we explicitly asked not to do so.");
+      },
+      onManyFrecenciesChanged() {
+        ok(true, "Should fire many frecencies changed notification instead.");
+        PlacesUtils.history.removeObserver(frecencyObserverCheck);
+        resolve();
+      },
+    };
+    PlacesUtils.history.addObserver(frecencyObserverCheck, false);
+  });
+  yield promiseUpdatePlaces(places, true);
+  yield promiseFrecenciesChanged;
+});