612281 - Remove some unnecessary reads from visits and icons addition

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

Attachment: patch v1.0

This removes a couple calls to read methods, by using urls instead of ids and reducing the timeframe of immediate transactions. This combination is what gave me better numbers locally.
Applies on top of non-step-visits and non-step-icons.

There is some noise (also because Places tip does not yet have double talos) but globally looks like working and a win, I confirmed better numbers locally too.
http://tinyurl.com/327urgo

Comment 1

[Marco Bonardo [:mak]]

these are instead on top of mozilla-central (not tip, but the last merge we did)
http://tinyurl.com/35dhrgs
Tp4 is in noise but on XP and Linux. ts is mostly dominated by the issue I sent you email about.

Comment 2

[Shawn Wilsher :sdwilsh]

Comment on attachment 490569 [details] [diff] [review]
patch v1.0

>+++ b/toolkit/components/places/src/History.cpp
>@@ -406,17 +346,70 @@ 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);
>+    nsCOMPtr<mozIStorageStatement> stmt;
Let's move our transaction up here, just above stmt please.

>+    // 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);
>+
>+      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);
And then pull typed and hidden out side of this if

>+    }
>+    // 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 = 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);
And typed and hidden outside of this one too.

>   nsresult AddVisit(VisitData& _place,
>                     const VisitData& aReferrer)
>   {
>     nsCOMPtr<mozIStorageStatement> 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) "
>+        "VALUES (:from_visit, (SELECT id FROM moz_places WHERE url = :page_url), :visit_date, :visit_type, :session) "
I think we want to continue to bind by page_id here when we can.  Doing a subselect is slower, and we'll always have a page_id for existing visits.

>   nsresult UpdateFrecency(const VisitData& aPlace)
>   {
>     { // First, set our frecency to the proper value.
>       nsCOMPtr<mozIStorageStatement> stmt =
>         mHistory->syncStatements.GetCachedStatement(
>           "UPDATE moz_places "
>-          "SET frecency = CALCULATE_FRECENCY(:page_id) "
>-          "WHERE id = :page_id"
>+          "SET frecency = CALCULATE_FRECENCY(id) "
>+          "WHERE url = :page_url"
>         );
ditto here

>     { // Now, we need to mark the page as not hidden if the frecency is now
>       // nonzero.
>       nsCOMPtr<mozIStorageStatement> stmt =
>         mHistory->syncStatements.GetCachedStatement(
>           "UPDATE moz_places "
>           "SET hidden = 0 "
>-          "WHERE id = :page_id AND frecency <> 0"
>+          "WHERE url = :page_url AND frecency <> 0"
And here.

r=sdwilsh

Comment 3

[Shawn Wilsher :sdwilsh]

Less reads = blocking.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v1.1

http://hg.mozilla.org/projects/places/rev/b2a35e7e44bb

Comment 5

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/b2a35e7e44bb