Bug 612281 - Remove some unnecessary reads from visits and icons addition.
authorMarco Bonardo <mbonardo@mozilla.com>
Tue, 16 Nov 2010 13:44:05 +0100
changeset 59338 b2a35e7e44bb9a3c0d98665a6c53a4a609acda50
parent 59337 6617dcb0d48a98ceb139674d2542c6901b09192e
child 59339 68f6d3af58622fc13bb0bf88ed7a5554ccad05b4
push id1
push usershaver@mozilla.com
push dateTue, 04 Jan 2011 17:58:04 +0000
bugs612281
milestone2.0b8pre
Bug 612281 - Remove some unnecessary reads from visits and icons addition. r=sdwilsh a=blocking
toolkit/components/places/src/AsyncFaviconHelpers.cpp
toolkit/components/places/src/History.cpp
toolkit/components/places/src/nsNavHistory.h
--- a/toolkit/components/places/src/AsyncFaviconHelpers.cpp
+++ b/toolkit/components/places/src/AsyncFaviconHelpers.cpp
@@ -769,34 +769,41 @@ AsyncAssociateIconToPage::Run()
       rv = stmt->BindStringByName(NS_LITERAL_CSTRING("rev_host"), mPage.revHost);
     }
     NS_ENSURE_SUCCESS(rv, rv);
     rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("favicon_id"), mIcon.id);
     NS_ENSURE_SUCCESS(rv, rv);
     rv = stmt->Execute();
     NS_ENSURE_SUCCESS(rv, rv);
 
-    // Get the new page id.
-    rv = FetchPageInfo(mFaviconSvc->mSyncStatements, mPage);
-    NS_ENSURE_SUCCESS(rv, rv);
-
     mIcon.status |= ICON_STATUS_ASSOCIATED;
   }
   // Otherwise just associate the icon to the page, if needed.
   else if (mPage.iconId != mIcon.id) {
-    nsCOMPtr<mozIStorageStatement> stmt =
-      mFaviconSvc->mSyncStatements.GetCachedStatement(NS_LITERAL_CSTRING(
+    nsCOMPtr<mozIStorageStatement> stmt;
+    if (mPage.id) {
+      stmt = mFaviconSvc->mSyncStatements.GetCachedStatement(NS_LITERAL_CSTRING(
         "UPDATE moz_places SET favicon_id = :icon_id WHERE id = :page_id"
       ));
-    NS_ENSURE_STATE(stmt);
-    mozStorageStatementScoper scoper(stmt);
+      NS_ENSURE_STATE(stmt);
+      rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("page_id"), mPage.id);
+      NS_ENSURE_SUCCESS(rv, rv);
+    }
+    else {
+      stmt = mFaviconSvc->mSyncStatements.GetCachedStatement(NS_LITERAL_CSTRING(
+        "UPDATE moz_places SET favicon_id = :icon_id WHERE url = :page_url"
+      ));
+      NS_ENSURE_STATE(stmt);
+      rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), mPage.spec);
+      NS_ENSURE_SUCCESS(rv, rv);
+    }
     rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("icon_id"), mIcon.id);
     NS_ENSURE_SUCCESS(rv, rv);
-    rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("page_id"), mPage.id);
-    NS_ENSURE_SUCCESS(rv, rv);
+
+    mozStorageStatementScoper scoper(stmt);
     rv = stmt->Execute();
     NS_ENSURE_SUCCESS(rv, rv);
 
     mIcon.status |= ICON_STATUS_ASSOCIATED;
   }
 
   rv = transaction.Commit();
   NS_ENSURE_SUCCESS(rv, rv);
--- a/toolkit/components/places/src/History.cpp
+++ b/toolkit/components/places/src/History.cpp
@@ -326,78 +326,18 @@ public:
     return NS_OK;
   }
 
   NS_IMETHOD Run()
   {
     NS_PRECONDITION(!NS_IsMainThread(),
                     "This should not be called on the main thread");
 
-    mozStorageTransaction transaction(mDBConn, PR_FALSE,
-                                      mozIStorageConnection::TRANSACTION_IMMEDIATE);
     bool known = FetchPageInfo(mPlace);
 
-    // If the page was in moz_places, we need to update the entry.
-    if (known) {
-      NS_ASSERTION(mPlace.placeId > 0, "must have a valid place id!");
-
-      nsCOMPtr<mozIStorageStatement> stmt =
-        mHistory->syncStatements.GetCachedStatement(
-          "UPDATE moz_places "
-          "SET hidden = :hidden, typed = :typed "
-          "WHERE id = :page_id "
-        );
-      NS_ENSURE_STATE(stmt);
-      mozStorageStatementScoper scoper(stmt);
-
-      nsresult rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("typed"),
-                                          mPlace.typed);
-      NS_ENSURE_SUCCESS(rv, rv);
-      rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("hidden"), mPlace.hidden);
-      NS_ENSURE_SUCCESS(rv, rv);
-      rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("page_id"), mPlace.placeId);
-      NS_ENSURE_SUCCESS(rv, rv);
-
-      rv = stmt->Execute();
-      NS_ENSURE_SUCCESS(rv, rv);
-    }
-    // Otherwise, the page was not in moz_places, so now we have to add it.
-    else {
-      NS_ASSERTION(mPlace.placeId == 0, "should not have a valid place id!");
-
-      nsCOMPtr<mozIStorageStatement> stmt =
-        mHistory->syncStatements.GetCachedStatement(
-          "INSERT INTO moz_places "
-            "(url, rev_host, hidden, typed) "
-          "VALUES (:page_url, :rev_host, :hidden, :typed) "
-        );
-      NS_ENSURE_STATE(stmt);
-      mozStorageStatementScoper scoper(stmt);
-
-      nsAutoString revHost;
-      nsresult rv = GetReversedHostname(mPlace.uri, revHost);
-      NS_ENSURE_SUCCESS(rv, rv);
-
-      rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), mPlace.uri);
-      NS_ENSURE_SUCCESS(rv, rv);
-      rv = stmt->BindStringByName(NS_LITERAL_CSTRING("rev_host"), revHost);
-      NS_ENSURE_SUCCESS(rv, rv);
-      rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("typed"), mPlace.typed);
-      NS_ENSURE_SUCCESS(rv, rv);
-      rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("hidden"), mPlace.hidden);
-      NS_ENSURE_SUCCESS(rv, rv);
-
-      rv = stmt->Execute();
-      NS_ENSURE_SUCCESS(rv, rv);
-
-      // Now, we need to get the id of what we just added.
-      bool added = FetchPageInfo(mPlace);
-      NS_ASSERTION(added, "not known after adding the place!");
-    }
-
     // If we had a referrer, we want to know about its last visit to put this
     // new visit into the same session.
     if (mReferrer.uri) {
       bool recentVisit = FetchVisitInfo(mReferrer, mPlace.visitTime);
       // At this point, we know the referrer's session id, which this new visit
       // should also share.
       if (recentVisit) {
         mPlace.sessionId = mReferrer.sessionId;
@@ -406,17 +346,61 @@ public:
       // the referrer and we'll start a new session.
       else {
         // This is sufficient to ignore our referrer.  This behavior has test
         // coverage, so if this invariant changes, we'll know.
         mReferrer.visitId = 0;
       }
     }
 
-    nsresult rv = AddVisit(mPlace, mReferrer);
+    mozStorageTransaction transaction(mDBConn, PR_FALSE,
+                                      mozIStorageConnection::TRANSACTION_IMMEDIATE);
+    nsresult rv;
+    nsCOMPtr<mozIStorageStatement> stmt;
+    // If the page was in moz_places, we need to update the entry.
+    if (known) {
+      NS_ASSERTION(mPlace.placeId > 0, "must have a valid place id!");
+
+      stmt = mHistory->syncStatements.GetCachedStatement(
+          "UPDATE moz_places "
+          "SET hidden = :hidden, typed = :typed "
+          "WHERE id = :page_id "
+        );
+      NS_ENSURE_STATE(stmt);
+      rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("page_id"), mPlace.placeId);
+      NS_ENSURE_SUCCESS(rv, rv);
+    }
+    // Otherwise, the page was not in moz_places, so now we have to add it.
+    else {
+      NS_ASSERTION(mPlace.placeId == 0, "should not have a valid place id!");
+
+      stmt = mHistory->syncStatements.GetCachedStatement(
+          "INSERT INTO moz_places "
+            "(url, rev_host, hidden, typed) "
+          "VALUES (:page_url, :rev_host, :hidden, :typed) "
+        );
+      NS_ENSURE_STATE(stmt);
+      nsAutoString revHost;
+      nsresult rv = GetReversedHostname(mPlace.uri, revHost);
+      NS_ENSURE_SUCCESS(rv, rv);
+      rv = stmt->BindStringByName(NS_LITERAL_CSTRING("rev_host"), revHost);
+      NS_ENSURE_SUCCESS(rv, rv);
+      rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), mPlace.uri);
+      NS_ENSURE_SUCCESS(rv, rv);
+    }
+    rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("typed"), mPlace.typed);
+    NS_ENSURE_SUCCESS(rv, rv);
+    rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("hidden"), mPlace.hidden);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    mozStorageStatementScoper scoper(stmt);
+    rv = stmt->Execute();
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    rv = AddVisit(mPlace, mReferrer);
     NS_ENSURE_SUCCESS(rv, rv);
 
     rv = UpdateFrecency(mPlace);
     NS_ENSURE_SUCCESS(rv, rv);
 
     rv = transaction.Commit();
     NS_ENSURE_SUCCESS(rv, rv);
 
@@ -555,45 +539,56 @@ private:
    * @param _place
    *        The VisitData for the place we need to know visit information about.
    * @param aReferrer
    *        A reference to the referrer's visit data.
    */
   nsresult AddVisit(VisitData& _place,
                     const VisitData& aReferrer)
   {
-    nsCOMPtr<mozIStorageStatement> stmt =
-      mHistory->syncStatements.GetCachedStatement(
+    nsresult rv;
+    nsCOMPtr<mozIStorageStatement> stmt;
+    if (_place.placeId) {
+      stmt = mHistory->syncStatements.GetCachedStatement(
         "INSERT INTO moz_historyvisits "
           "(from_visit, place_id, visit_date, visit_type, session) "
         "VALUES (:from_visit, :page_id, :visit_date, :visit_type, :session) "
       );
-    NS_ENSURE_STATE(stmt);
-    mozStorageStatementScoper scoper(stmt);
-
-    nsresult rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("from_visit"),
-                                        aReferrer.visitId);
-    NS_ENSURE_SUCCESS(rv, rv);
-    rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("page_id"),
-                               _place.placeId);
+      NS_ENSURE_STATE(stmt);
+      rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("page_id"), mPlace.placeId);
+      NS_ENSURE_SUCCESS(rv, rv);
+    }
+    else {
+      stmt = mHistory->syncStatements.GetCachedStatement(
+        "INSERT INTO moz_historyvisits "
+          "(from_visit, place_id, visit_date, visit_type, session) "
+        "VALUES (:from_visit, (SELECT id FROM moz_places WHERE url = :page_url), :visit_date, :visit_type, :session) "      
+      );
+      NS_ENSURE_STATE(stmt);
+      rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), _place.uri);
+      NS_ENSURE_SUCCESS(rv, rv);
+    }
+    rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("from_visit"),
+                               aReferrer.visitId);
     NS_ENSURE_SUCCESS(rv, rv);
     rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("visit_date"),
                                _place.visitTime);
     NS_ENSURE_SUCCESS(rv, rv);
     PRInt32 transitionType = _place.transitionType;
     NS_ASSERTION(transitionType >= nsINavHistoryService::TRANSITION_LINK &&
                  transitionType <= nsINavHistoryService::TRANSITION_FRAMED_LINK,
                  "Invalid transition type!");
     rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("visit_type"),
                                transitionType);
     NS_ENSURE_SUCCESS(rv, rv);
     rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("session"),
                                _place.sessionId);
     NS_ENSURE_SUCCESS(rv, rv);
 
+    mozStorageStatementScoper scoper(stmt);
     rv = stmt->Execute();
     NS_ENSURE_SUCCESS(rv, rv);
 
     // Now that it should be in the database, we need to obtain the id of the
     // visit we just added.
     bool visited = FetchVisitInfo(_place);
     NS_ASSERTION(!visited, "Not visited after adding a visit!");
 
@@ -603,47 +598,70 @@ private:
   /**
    * Updates the frecency, and possibly the hidden-ness of aPlace.
    *
    * @param aPlace
    *        The VisitData for the place we want to update.
    */
   nsresult UpdateFrecency(const VisitData& aPlace)
   {
+    nsresult rv;
     { // First, set our frecency to the proper value.
-      nsCOMPtr<mozIStorageStatement> stmt =
-        mHistory->syncStatements.GetCachedStatement(
+      nsCOMPtr<mozIStorageStatement> stmt;
+      if (aPlace.placeId) {
+        stmt = mHistory->syncStatements.GetCachedStatement(
           "UPDATE moz_places "
           "SET frecency = CALCULATE_FRECENCY(:page_id) "
           "WHERE id = :page_id"
         );
-      NS_ENSURE_STATE(stmt);
+        NS_ENSURE_STATE(stmt);
+        rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("page_id"), mPlace.placeId);
+        NS_ENSURE_SUCCESS(rv, rv);
+      }
+      else {
+        stmt = mHistory->syncStatements.GetCachedStatement(
+          "UPDATE moz_places "
+          "SET frecency = CALCULATE_FRECENCY(id) "
+          "WHERE url = :page_url"
+        );
+        NS_ENSURE_STATE(stmt);
+        rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), aPlace.uri);
+        NS_ENSURE_SUCCESS(rv, rv);      
+      }
       mozStorageStatementScoper scoper(stmt);
 
-      nsresult rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("page_id"),
-                                          aPlace.placeId);
-      NS_ENSURE_SUCCESS(rv, rv);
       rv = stmt->Execute();
       NS_ENSURE_SUCCESS(rv, rv);
     }
 
     { // Now, we need to mark the page as not hidden if the frecency is now
       // nonzero.
-      nsCOMPtr<mozIStorageStatement> stmt =
-        mHistory->syncStatements.GetCachedStatement(
+      nsCOMPtr<mozIStorageStatement> stmt;
+      if (aPlace.placeId) {
+        stmt = mHistory->syncStatements.GetCachedStatement(
           "UPDATE moz_places "
           "SET hidden = 0 "
           "WHERE id = :page_id AND frecency <> 0"
         );
-      NS_ENSURE_STATE(stmt);
-      mozStorageStatementScoper scoper(stmt);
+        NS_ENSURE_STATE(stmt);
+        rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("page_id"), mPlace.placeId);
+        NS_ENSURE_SUCCESS(rv, rv);
+      }
+      else {
+        stmt = mHistory->syncStatements.GetCachedStatement(
+          "UPDATE moz_places "
+          "SET hidden = 0 "
+          "WHERE url = :page_url AND frecency <> 0"
+        );
+        NS_ENSURE_STATE(stmt);
+        rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), aPlace.uri);
+        NS_ENSURE_SUCCESS(rv, rv);      
+      }
 
-      nsresult rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("page_id"),
-                                          aPlace.placeId);
-      NS_ENSURE_SUCCESS(rv, rv);
+      mozStorageStatementScoper scoper(stmt);
       rv = stmt->Execute();
       NS_ENSURE_SUCCESS(rv, rv);
     }
 
     return NS_OK;
   }
 
   mozIStorageConnection* mDBConn;
--- a/toolkit/components/places/src/nsNavHistory.h
+++ b/toolkit/components/places/src/nsNavHistory.h
@@ -514,16 +514,18 @@ public:
 
     switch(aStatementId) {
       case DB_PAGE_INFO_FOR_FRECENCY:
         return NS_IsMainThread() ? mDBPageInfoForFrecency
                                  : mDBAsyncThreadPageInfoForFrecency;
       case DB_VISITS_FOR_FRECENCY:
         return NS_IsMainThread() ? mDBVisitsForFrecency
                                  : mDBAsyncThreadVisitsForFrecency;
+      default:
+        NS_NOTREACHED("Trying to handle an unknown statement");
     }
     return nsnull;
   }
 
   PRInt32 GetFrecencyAgedWeight(PRInt32 aAgeInDays) const
   {
     if (aAgeInDays <= mFirstBucketCutoffInDays) {
       return mFirstBucketWeight;