970604 - Tab loads hit the DB twice when once will do

Status: RESOLVED FIXED

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

Description

[Richard Newman [:rnewman]]

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.

Comment 1

[Richard Newman [:rnewman]]

Attachment: WIP

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.

Comment 2

[Richard Newman [:rnewman]]

Doing this drops the price of these two checks from 85msec to 2msec on my HTC One.

Comment 3

[Richard Newman [:rnewman]]

Attachment: Tab loads hit the DB twice when once will do.

Comment 4

[Brian Nicholson (:bnicholson)]

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.

Comment 5

[Richard Newman [:rnewman]]

(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 6

[Brian Nicholson (:bnicholson)]

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!

Comment 7

[Richard Newman [:rnewman]]

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.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ed0f9d090ca