Closed Bug 1235642 Opened 4 years ago Closed 4 years ago

Don't query-then-update to bump history visit counts

Categories

(Firefox for Android :: Data Providers, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
firefox46 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: rnewman, Assigned: mfinkle, Mentored)

References

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

BrowserProvider.updateHistory uses a cursor to extract the VISIT count. It doesn't need to if we use raw SQL instead of the hamstrung Android sqlite API: just

  UPDATE SET visitCount = visitCount + 1 …

This'll save a cursor and a query, which is 10-100msec.
Attached patch nocursor-inc-visits v0.1 (obsolete) — Splinter Review
This patch uses the DBUtils.updateArrays method to pass the VISITS in as an EXPRESSION. Seemed to work locally.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=052140617c6e
Assignee: nobody → mark.finkle
Attachment #8702998 - Flags: review?(rnewman)
By "work locally" I mean that I see:

Update SQL: UPDATE history SET date= ?,deleted= ?,modified= ?,url= ?,visits = visits+1 WHERE url = ?

in logcat. I'll remove that debugging before any landing.
Comment on attachment 8702998 [details] [diff] [review]
nocursor-inc-visits v0.1

Review of attachment 8702998 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with logging removed. Lovely!

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
@@ +17,5 @@
>  import org.mozilla.gecko.db.BrowserContract.History;
>  import org.mozilla.gecko.db.BrowserContract.Schema;
>  import org.mozilla.gecko.db.BrowserContract.Tabs;
>  import org.mozilla.gecko.db.BrowserContract.Thumbnails;
> +import org.mozilla.gecko.db.DBUtils.UpdateOperation;

I'm increasingly glad that I added this!
Attachment #8702998 - Flags: review?(rnewman) → review+
Status: NEW → ASSIGNED
Whiteboard: [good first bug][lang=java]
Tests failed because I am | values.removeKey(History.VISITS); | and that breaks insertOrUpdateHistory which will also attempt to use the | values | if the updateHistory call does not update anything.

I updated the patch to make a copy of the ContentValues when we know we are in the "increment visits" code path. This passes testBrowserProvider locally.

diff --git a/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java b/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
@@ -1021,34 +1021,37 @@ public class BrowserProvider extends Sha
         // Use the simple code path for easy updates.
         if (!shouldIncrementVisits(uri)) {
             trace("Updating history meta data only");
             return db.update(TABLE_HISTORY, values, selection, selectionArgs);
         }
 
         trace("Updating history meta data and incrementing visits");
 
+        // We might be altering the ContentValues, so let's use a copy.
+        final ContentValues valuesForUpdate = new ContentValues(values);
+
         // Update data and increment visits by 1.
         long incVisits = 1;
-        if (values.containsKey(History.VISITS)) {
+        if (valuesForUpdate.containsKey(History.VISITS)) {
             // Use a given visit count, if found.
-            Long additional = values.getAsLong(History.VISITS);
+            Long additional = valuesForUpdate.getAsLong(History.VISITS);
             if (additional != null) {
                 incVisits = additional;
             }
             // Remove the visits from this set of values so we can pass the visits
             // as an expression.
-            values.remove(History.VISITS);
+            valuesForUpdate.remove(History.VISITS);
         }
 
         // Create a separate set of values that will be updated as an expression.
         final ContentValues visits = new ContentValues();
-        visits.put(History.VISITS, History.VISITS + "+" + incVisits);
+        visits.put(History.VISITS, History.VISITS + " + " + incVisits);
 
-        final ContentValues[] valuesAndVisits = { values,  visits };
+        final ContentValues[] valuesAndVisits = { valuesForUpdate,  visits };
         final UpdateOperation[] ops = { UpdateOperation.ASSIGN, UpdateOperation.EXPRESSION };
 
         return DBUtils.updateArrays(db, TABLE_HISTORY, valuesAndVisits, ops, selection, selectionArgs);
     }
Copying the ContentValues seems like a safe and cheap thing to do.

Alternate: I could cache the VISITS value and put it back if the update fails.
Attachment #8702998 - Attachment is obsolete: true
Attachment #8703065 - Flags: review?(rnewman)
Comment on attachment 8703065 [details] [diff] [review]
nocursor-inc-visits v0.2

(In reply to Mark Finkle (:mfinkle) from comment #4)
> Tests failed because I am | values.removeKey(History.VISITS); | and that
> breaks insertOrUpdateHistory which will also attempt to use the | values |
> if the updateHistory call does not update anything.

Heh, I was going to call that out, but I figured we were already mutating the provided CV, so it was likely fine.

> I updated the patch to make a copy of the ContentValues when we know we are
> in the "increment visits" code path. This passes testBrowserProvider locally.

SGTM. Interdiff looks fine.

Don't forget to remove the Log.i.
Attachment #8703065 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/9126f599f632
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Comment on attachment 8703065 [details] [diff] [review]
nocursor-inc-visits v0.2

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Might reduce cursor crashes. Should improve performance
[Describe test coverage new/current, TreeHerder]: Working on Nightly for a few days
[Risks and why]: Low risk. Very localized change.
[String/UUID change made/needed]: None

Should be taken with bug 1235637
Attachment #8703065 - Flags: approval-mozilla-beta?
Attachment #8703065 - Flags: approval-mozilla-aurora?
Comment on attachment 8703065 [details] [diff] [review]
nocursor-inc-visits v0.2

Let's hope this improves Fennec performance, beta44+, aurora45+
Attachment #8703065 - Flags: approval-mozilla-beta?
Attachment #8703065 - Flags: approval-mozilla-beta+
Attachment #8703065 - Flags: approval-mozilla-aurora?
Attachment #8703065 - Flags: approval-mozilla-aurora+
This doesn't apply cleanly to beta due to the lack of bug 1107811. Can we get a rebased patch?
Flags: needinfo?(mark.finkle)
You need to log in before you can comment on or make changes to this bug.