Closed Bug 1235637 Opened 4 years ago Closed 4 years ago

Provide a simple way to update history without needing a cursor

Categories

(Firefox for Android :: Data Providers, defect)

defect
Not set

Tracking

()

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

People

(Reporter: mfinkle, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
Attachment #8702691 - Flags: review?(rnewman)
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 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".
Attachment #8702691 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/2314a86a52ef
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Blocks: 760394
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?
Flags: needinfo?(mark.finkle)
(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.
Flags: needinfo?(mark.finkle)
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
Attachment #8702691 - Flags: approval-mozilla-beta?
Attachment #8702691 - Flags: approval-mozilla-aurora?
(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 on attachment 8702691 [details] [diff] [review]
nocursor-update-history v0.1

Let's hope this improves Fennec performance, beta44+, aurora45+
Attachment #8702691 - Flags: approval-mozilla-beta?
Attachment #8702691 - Flags: approval-mozilla-beta+
Attachment #8702691 - Flags: approval-mozilla-aurora?
Attachment #8702691 - 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.