Closed Bug 970604 Opened 6 years ago Closed 6 years ago

Tab loads hit the DB twice when once will do

Categories

(Firefox for Android :: Data Providers, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: rnewman, Assigned: rnewman)

References

(Blocks 3 open bugs)

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

mBookmark = BrowserDB.isBookmark(getContentResolver(), url);
  mReadingListItem = BrowserDB.isReadingListItem(getContentResolver(), url);


We should pull all of these flags out of the DB in one go. Bam, that's a 20msec round-trip to the DB saved on every tab event.
Attached patch WIP (obsolete) — Splinter Review
This is untested, but illustrates the principle.

We directly extract a single flag value from the DB based on a trivial LIMITed query for a single column, transformed via a CASE statement and using SUM as an aggregate to union bitfields.

This should be a relatively efficient DB operation, and eliminates the need to make multiple queries to find the various roles a URL plays.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Doing this drops the price of these two checks from 85msec to 2msec on my HTC One.
Blocks: 971232
Blocks: 971239
Attachment #8373886 - Attachment is obsolete: true
Comment on attachment 8374407 [details] [diff] [review]
Tab loads hit the DB twice when once will do.

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

::: mobile/android/base/db/BrowserProvider.java
@@ +2521,5 @@
> +            // SELECT COALESCE((SELECT ...), 0) | COALESCE(...) | ...
> +            final String query = "SELECT COALESCE(SUM(flag), 0) AS flags " +
> +                "FROM ( SELECT DISTINCT CASE" +
> +                " WHEN " + Bookmarks.PARENT + " = " + Bookmarks.FIXED_READING_LIST_ID +
> +                " THEN " + Bookmarks.FLAG_READING +

These modifications to BrowserProvider seem unnecessary. If I were tasked with this bug, I probably would have left BrowserProvider untouched, have getItemFlags in LocalBrowserDB query for the bookmark's parent, then have done the flag building in Java. Any advantage to your approach? If it's for efficiency, I'm curious what the performance difference would be between these two scenarios.
(In reply to Brian Nicholson (:bnicholson) from comment #4)

> These modifications to BrowserProvider seem unnecessary. If I were tasked
> with this bug, I probably would have left BrowserProvider untouched, have
> getItemFlags in LocalBrowserDB query for the bookmark's parent, then have
> done the flag building in Java. Any advantage to your approach? If it's for
> efficiency, I'm curious what the performance difference would be between
> these two scenarios.

In this specific case, to do this procedurally you'd need to query for all parents of the bookmark (not just one), then walk the cursor doing the translation into flags. That would start off in approximately the same ballpark as this solution; probably slightly worse in space and time due to the additional returned rows in some cases, but still better than a second query.

If I had to interpolate:

  Two queries:                                     85ms
  One query using something like the old approach: 40ms?
  Raw query returning flag:                         2ms

Or, phrased differently: it's hard to imagine anything beating the approach in this patch for speed while still actually doing a read from the DB!


But regardless, let's say we did stick to Java processing of existing BrowserProvider entry points. Now you'd have another little chunk of procedural query code, which would gradually spiral out of that starting complexity and perf envelope.

For example, in a month or so reading list will be in a separate table, not smushed into bookmarks (Bug 959290).

The LocalBrowserDB-Java-side solution would now be back in the scenario of making two queries -- one to the bookmarks URI and one to the reading list URI. Being closer to the DB would allow us to simply turn one of those `WHEN` clauses into a separate `| COALESCE`, or a trivial `UNION ALL`. We'd still pay the cost of the additional work (in exchange for simplifying the bookmarks part), but nowhere near the overhead of making two separate queries.

Generally I'd like more concepts like this to move into BrowserProvider and be given a name. It's more efficient to do combining work closer to the DB, for one thing -- especially writes! -- and it also improves the reuse:complexity ratio and reduces coupling. For example, Sync can use stuff in BrowserProvider, and we might opt to rewrite things like `isReadingListItem` in terms of this flag query.

Finally, the Android CR API is hamstrung compared to the DB API: e.g., you can't use LIMIT on queries.

tl;dr: write queries in SQL, not Java.
Comment on attachment 8374407 [details] [diff] [review]
Tab loads hit the DB twice when once will do.

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

Sounds good to me!
Attachment #8374407 - Flags: review?(bnicholson) → review+
https://hg.mozilla.org/integration/fx-team/rev/0ed0f9d090ca

Let's see how this looks on Eideticker. It's straightforward enough that we might consider uplifting to Aurora if there's a visible perf win.
Target Milestone: --- → Firefox 30
https://hg.mozilla.org/mozilla-central/rev/0ed0f9d090ca
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 974291
You need to log in before you can comment on or make changes to this bug.