Join PageMetadata with AS top sites

RESOLVED FIXED in Firefox 57

Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 57
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [mobileAS] 1.27)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

3 months ago
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.
(Assignee)

Updated

3 months ago
Blocks: 1322501
No longer blocks: 1301718
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

3 months ago
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)
(Assignee)

Comment 5

3 months ago
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).

Comment 6

3 months ago
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 7

3 months ago
mozreview-review
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+
(Assignee)

Comment 8

3 months ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

3 months ago
mozreview-review
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 13

3 months ago
mozreview-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+
(Assignee)

Comment 14

3 months ago
> 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)
(Assignee)

Updated

3 months ago
Attachment #8892702 - Flags: review?(gkruglov)
(Assignee)

Comment 15

3 months ago
Sorry, apparently I forgot to flag you for review on that one part.

Comment 16

3 months ago
mozreview-review
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+
(Assignee)

Comment 17

3 months ago
mozreview-review-reply
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.

Comment 18

3 months ago
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
(Assignee)

Updated

3 months ago
Blocks: 1301718
https://hg.mozilla.org/mozilla-central/rev/91ac0a2b022c
https://hg.mozilla.org/mozilla-central/rev/35b56190a1a9
https://hg.mozilla.org/mozilla-central/rev/13f138fbb0bb
https://hg.mozilla.org/mozilla-central/rev/b9198b3db389
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Assignee)

Comment 20

2 months ago
(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)
(Assignee)

Comment 21

2 months ago
(In reply to Michael Comella (:mcomella) from comment #20)
> but I don't want to directly
> compare it to FF55 [3]

This should be FF56:

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-08-01&keys=__none__!__none__!__none__&max_channel_version=nightly%252F56&measure=FENNEC_ACTIVITY_STREAM_TOPSITES_LOADER_TIME_MS&min_channel_version=null&processType=*&product=Fennec&sanitize=1&sort_keys=submissions&start_date=2017-06-12&table=0&trim=1&use_submission_date=0
(Assignee)

Comment 22

2 months ago
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.