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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Richard Newman [:rnewman]]

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.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: nocursor-inc-visits v0.1

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

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

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 3

[Richard Newman [:rnewman]]

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!

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

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);
     }

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: nocursor-inc-visits v0.2

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.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Full Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8145d0bbb26f

Comment 7

[Richard Newman [: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.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9126f599f632

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9126f599f632

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8703065 [details] [diff] [review]
nocursor-inc-visits v0.2

Let's hope this improves Fennec performance, beta44+, aurora45+

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This doesn't apply cleanly to beta due to the lack of bug 1107811. Can we get a rebased patch?

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8f4bd2002ea6

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-beta/rev/afffbf932c7d

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/afffbf932c7d