Closed Bug 1386052 Opened 3 years ago Closed 3 years ago

Join PageMetadata with AS top sites

Categories

(Firefox for Android :: General, enhancement, P1)

All
Android
enhancement

Tracking

()

RESOLVED FIXED
Firefox 57
Iteration:
1.27
Tracking Status
firefox57 --- fixed

People

(Reporter: mcomella, Assigned: mcomella)

References

(Blocks 1 open bug)

Details

(Whiteboard: [mobileAS] 1.27)

Attachments

(4 files)

In order to do bug 1382036 (use PageMetadata provider title from DB in AS top sites) and the top sites part of bug 1301718 / bug 1322501 (get PageMetadata image_url from gecko), we'll first need to get the PageMetadata info from the DB.

---

Adding to the current sprint because it blocks a few things in the current sprint.
Blocks: 1322501
No longer blocks: 1301718
In order to join with the PageMetadata table, we need to have the History.GUID column. However, I'm not sure where it's optimal to add this in.

The top sites query currently looks like:
- We create a combined bookmarks and history view
- For the top sites query, we create a temporary table, reading from the combined view
- We insert into the temporary table
- We query the temporary table to get the top sites results

I see three options to add the History.GUID column:

1) (my wip) Add History.GUID when creating the Combined view. Consequences:
- Requires a database migration
- Other queries which is the combined view have an extra column (filterAllSites - for searching?, getRecentHistory) - this seems negligible to me
- The simplest implementation: it's simple to add to the Combined view and doesn't require additional joins

2) Join the combined view with the history table when creating the temporary table
- Unclear if the DB is smart enough to just add the History.GUID (since the History table is being queried for the Combined view already) or if it'll actually join the history table with the Combined view, which is really inefficient
- Slightly more complicated code-wise

3) Join the history table with the final top sites query
- Most complicated code-wise: we already have to join the results of a union with the PageMetadata table `SELECT * FROM (SELECT ... UNION ALL ...) LEFT JOIN ...` but we'll have to make an additional join to add the History table back in
- Probably inefficient to do this join

I prefer 1 for perf and simplicity but I don't know if I'm missing anything.

---

Grisha, do you have an opinion on what's optimal here? See my wip for a sample implementation of 1.
Flags: needinfo?(gkruglov)
Now that I think about it, I'm not sure why I thought the other options seemed viable but I'll leave the NI open just in case I'm missing something (given that SQL is not my area of expertise).
Well, this is unfortunate. I think using history_guid in the pagemetadata table was a mistake. It's really a sync-centric field, we won't be syncing this data in this shape or form anyway, and so using a local _id field likely would have been just fine. As thing stand, without either an additional compound select ran after top sites are generated (_id -> guid -> metadata), or a what you're proposing as option 1, you can't really associate the two tables easily.

Anyway, if you feel like righting that wrong, feel free (the migration will be annoying...) - but at this point proceeding with option 1 seems fine as well.
Flags: needinfo?(gkruglov)
Comment on attachment 8892701 [details]
Bug 1386052: Correct PageMetadata.HISTORY_GUID foreign key.

https://reviewboard.mozilla.org/r/163680/#review169386

Oops!
Attachment #8892701 - Flags: review?(gkruglov) → review+
(In reply to :Grisha Kruglov from comment #6)
> Anyway, if you feel like righting that wrong, feel free (the migration will
> be annoying...) - but at this point proceeding with option 1 seems fine as
> well.

Since I'm focused on getting AS out the door, I deferred until later: I filed bug 1386868.
Comment on attachment 8893146 [details]
Bug 1386052: Update testBrowserDatabaseHelperUpgrades from new info.

https://reviewboard.mozilla.org/r/164144/#review169936
Attachment #8893146 - Flags: review?(gkruglov) → review+
Comment on attachment 8892703 [details]
Bug 1386052: Join PageMetadata table with Top Sites query.

https://reviewboard.mozilla.org/r/163684/#review169942

And so this query keeps growing. I wonder what impact that additional subquery (and a join) will have. Let's keep an eye out on metrics once this lands.

::: commit-message-d9326:33
(Diff revision 2)
> +
> +- Both adapters don't use any columns I haven't specified. They also call out
> +to TwoLinePageRow.updateFromCursor:
> +https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java#318
> +
> +Which also doesn't use new columns.

Thanks for being thorough!
Attachment #8892703 - Flags: review?(gkruglov) → review+
> I wonder what impact that additional subquery (and a join) will have. Let's keep an eye out on metrics once this lands.

NI self to follow-up.

Thanks for the quick review!
Flags: needinfo?(michael.l.comella)
Sorry, apparently I forgot to flag you for review on that one part.
Comment on attachment 8892702 [details]
Bug 1386052: add Combined.HISTORY_GUID; upgrade db to 38.

https://reviewboard.mozilla.org/r/163682/#review169980

Up-up-up the version goes!

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testBrowserDatabaseHelperUpgrades.java:56
(Diff revision 2)
>   * Test DBs must be created on the oldest version of Android that is currently supported. SQLite
>   * is not forwards compatible. E.g. uploading a DB created on a 6.0 device will cause failures
>   * when robocop tests running on 4.3 are unable to load it.
>   *
> + * [mcomella] I was unable to find an API 15 device so I created the v37 DB using API 18 (Jelly Bean). According to
> + * https://stackoverflow.com/a/4377116/2219998, API 16-20 share the same version of SQLite so only API 15 devices

We dropped API 15 support in Bug 1316462.
Attachment #8892702 - Flags: review?(gkruglov) → review+
Comment on attachment 8892702 [details]
Bug 1386052: add Combined.HISTORY_GUID; upgrade db to 38.

https://reviewboard.mozilla.org/r/163682/#review169980

> We dropped API 15 support in Bug 1316462.

I updated this comment in the following patch - sorry for my sloppy version controlling.
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/91ac0a2b022c
Correct PageMetadata.HISTORY_GUID foreign key. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/35b56190a1a9
add Combined.HISTORY_GUID; upgrade db to 38. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/13f138fbb0bb
Update testBrowserDatabaseHelperUpgrades from new info. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/b9198b3db389
Join PageMetadata table with Top Sites query. r=Grisha
(In reply to Michael Comella (:mcomella) from comment #14)
> > I wonder what impact that additional subquery (and a join) will have. Let's keep an eye out on metrics once this lands.
> 
> NI self to follow-up.

This patch landed on 8/4 so:

- (pre-landing) 8/2-8/5 [1]
- (post-landing) 8/5-8/6 [2]

The median has gone up 0.3ms (to 60.2ms) and 95th percentile has gone up ~18ms (to 482.28ms), which seems to be an acceptable change.

Caveat: I don't know that 3 days of data (pre-landing) and 2 days of data (post-landing) are statistically significant but I don't want to directly compare it to FF55 [3] (which is roughly similar) because I don't know what changes may have be made during that release cycle.

[1]: https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-08-04&keys=__none__!__none__!__none__&max_channel_version=nightly%252F57&measure=FENNEC_ACTIVITY_STREAM_TOPSITES_LOADER_TIME_MS&min_channel_version=null&processType=*&product=Fennec&sanitize=1&sort_keys=submissions&start_date=2017-08-02&table=0&trim=1&use_submission_date=0
[2]: https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-08-06&keys=__none__!__none__!__none__&max_channel_version=nightly%252F57&measure=FENNEC_ACTIVITY_STREAM_TOPSITES_LOADER_TIME_MS&min_channel_version=null&processType=*&product=Fennec&sanitize=1&sort_keys=submissions&start_date=2017-08-05&table=0&trim=1&use_submission_date=0

[3]: https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-06-12&keys=__none__!__none__!__none__&max_channel_version=nightly%252F55&measure=FENNEC_ACTIVITY_STREAM_TOPSITES_LOADER_TIME_MS&min_channel_version=null&processType=*&product=Fennec&sanitize=1&sort_keys=submissions&start_date=2017-03-28&table=0&trim=1&use_submission_date=0
Flags: needinfo?(michael.l.comella)
Unfortunately, bug 1382036 is getting replaced by bug 1386902, so this is unused in the top sites titles. Also, Sebastian doesn't think `image_url` is necessary for top sites (bug 1301718 comment 13) so this DB change will actually be unused.

I can't back this out due to the migration. However, we may need this change in the future so let's not undo it (it's unfortunate about the minor DB performance hit though).

I'll add a comment that this part of the query may be unused so someone trying to improve perf of the DB later knows to undo this change to fix it simply.
You need to log in before you can comment on or make changes to this bug.