1235637 - Provide a simple way to update history without needing a cursor

Status: RESOLVED FIXED

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

Description

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

Attachment: nocursor-update-history v0.1

BrowserProvider.updateHistory uses a cursor to extract the VISIT count, which seems like a lot of overhead. When updating the page title via LocalBrowserDB.updateHistoryTitle, we don't even need it. This patch adds a faster, simpler update path.

Part of this Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a5b28a789b0

Comment 1

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

Tested locally using http://people.mozilla.org/~mfinkle/tests/change-title.html and the code works. The title is updated. You can check by looking at the tabs tray, which shows the title.

Logcat (via adb shell setprop log.tag.GeckoBrowserProvider VERBOSE) also shows the right code paths being taken.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8702691 [details] [diff] [review]
nocursor-update-history v0.1

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

This code path is also hit through updateOrInsert, so I think it would be safer to put the short-circuit right at the top of `updateHistory` instead of in this particular dispatching switch statement.

r+ with that change.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
@@ +1061,5 @@
> +
> +        final SQLiteDatabase db = getWritableDatabase(uri);
> +
> +        if (!values.containsKey(History.DATE_MODIFIED)) {
> +            values.put(History.DATE_MODIFIED,  System.currentTimeMillis());

Nit: remove double space before "System".

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2314a86a52ef

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2314a86a52ef

Comment 5

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

Mark, this is awesome investigation and follow up to bug 760394, so thanks! :) If this helps fix some of the crashes in bug 760394, should we consider uplifting to Beta44?

Comment 6

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

(In reply to Ritu Kothari (:ritu) from comment #5)
> Mark, this is awesome investigation and follow up to bug 760394, so thanks!
> :) If this helps fix some of the crashes in bug 760394, should we consider
> uplifting to Beta44?

If this (and bug 1235642) help lower crash rates, I think we could uplift. The patches are fairly localized, have decent tests, and have performance benefits.

Comment 7

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

Comment on attachment 8702691 [details] [diff] [review]
nocursor-update-history v0.1

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 1235642

Comment 8

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

(In reply to Mark Finkle (:mfinkle) from comment #6)
> (In reply to Ritu Kothari (:ritu) from comment #5)
> > Mark, this is awesome investigation and follow up to bug 760394, so thanks!
> > :) If this helps fix some of the crashes in bug 760394, should we consider
> > uplifting to Beta44?
> 
> If this (and bug 1235642) help lower crash rates, I think we could uplift.
> The patches are fairly localized, have decent tests, and have performance
> benefits.

Mark, I looked at the Nightl46 data for crash signatures in bug 760394 and we still see crashes on builds with this fix. Given that crash-stats also lost data in the past week, I am still leaning towards taking this fix as the data from Nightly is insufficient to confirm whether this helps or not.

Comment 9

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

Comment on attachment 8702691 [details] [diff] [review]
nocursor-update-history v0.1

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

Comment 10

[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 11

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/aa335f59b09d

Comment 12

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

https://hg.mozilla.org/releases/mozilla-beta/rev/0a239969b875

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0a239969b875