Closed Bug 612281 Opened 9 years ago Closed 9 years ago

Remove some unnecessary reads from visits and icons addition

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b9
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mak, Assigned: mak)

References

Details

(Whiteboard: [fixed-in-places])

Attachments

(1 file, 1 obsolete file)

Attached patch patch v1.0 (obsolete) — Splinter Review
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
Attachment #490569 - Flags: review?(sdwilsh)
Summary: Remove some unneded reads from visits addition → Remove some unnecessary reads from visits addition
Summary: Remove some unnecessary reads from visits addition → Remove some unnecessary reads from visits and icons addition
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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
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
Attachment #490569 - Flags: review?(sdwilsh) → review+
Less reads = blocking.
blocking2.0: --- → betaN+
Whiteboard: [fixed-in-places]
Depends on: 612592
http://hg.mozilla.org/mozilla-central/rev/b2a35e7e44bb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b9
You need to log in before you can comment on or make changes to this bug.