If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Refactor ActivityStreamProvider in preparation for Highlights

RESOLVED FIXED in Firefox 57

Status

()

Firefox
Activity Streams: Newtab
RESOLVED FIXED
a month ago
12 days ago

People

(Reporter: standard8, Assigned: Mardak)

Tracking

unspecified
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

a month ago
Whilst examining various code, I noticed that there's a few functions in ActivityStreamProvider in NewTabUtils.jsm that are test-only functions:

- getHistorySize
- getBookmarksSize
- executePlacesQuery (only used in the above two functions)

It looks like they are only used in test_NewTabUtils.js, so they should probably be moved into that file unless we think they'll be useful elsewhere.
(Reporter)

Comment 1

19 days ago
It seems that executePlacesQuery is now being used elsewhere in NewTabUtils.js, so I'm not convinced that moving the other two small functions out of there is really worth it at the moment.
Status: NEW → RESOLVED
Last Resolved: 19 days ago
Resolution: --- → WONTFIX
(Assignee)

Comment 2

17 days ago
emtwo, do we still want getHistorySize and getBookmarksSize? Those seems to be only used by Test Pilot version, and we didn't end up adding them to the the system-addon… yet?
Flags: needinfo?(msamuel)
Comment hidden (mozreview-request)
(Assignee)

Updated

16 days ago
Assignee: nobody → edilee
Blocks: 1396282
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Summary: In NewTabUtils.jsm ActivityStreamProvider, getHistorySize and getBookmarksSize are only used in test files → Refactor ActivityStreamProvider in preparation for Highlights
(Assignee)

Comment 4

16 days ago
mozreview-review
Comment on attachment 8904052 [details]
Bug 1389125 - Refactor ActivityStreamProvider in preparation for Highlights.

https://reviewboard.mozilla.org/r/175788/#review180772

::: toolkit/modules/NewTabUtils.jsm:945
(Diff revision 1)
>     */
> -  async getTopFrecentSites(aOptions = {}) {
> -    let {ignoreBlocked} = aOptions;
> -
> -    // Get extra links in case they're blocked and double the usual limit in
> -    // case the host is deduped between with www or not-www (i.e., 2 hosts) as
> +  async getTopFrecentSites(aOptions) {
> +    const options = Object.assign({
> +      ignoreBlocked: false,
> +      numItems: ACTIVITY_STREAM_DEFAULT_LIMIT,
> +      topsiteFrecency: ACTIVITY_STREAM_DEFAULT_FRECENCY

This also allows us to better control the behavior from activity stream, e.g., having SQL filter out by frecency instead of via `TopSitesFeed` as well as selecting more items for better block/delete UI `Reducer` behavior or for supporting newtabpage.rows pref.
(Assignee)

Updated

16 days ago
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

13 days ago
After checking performance of SUBSTR(url) vs url_hash BETWEEN, it looks like SUBSTR is faster. In general, looks like url_hash will be faster for finding sparse (i.e., places) when it's being used as the index.

Code to test from Browser Console:

async function test(sql) {
  const RUNS = 50000;
  const DB = await PlacesUtils.promiseDBConnection();
  const times = [];
  for (let i = 0; i < RUNS; i++) {
    const start = performance.now();
    await DB.execute(sql);
    times.push(performance.now() - start);
  }
  console.log(times.sort().slice(RUNS * .1, RUNS * .9).reduce((t, v) => t + v, 0) / (RUNS * .8), sql);
}
(async () => {
  await test(`
    SELECT 1
    FROM moz_places
    ORDER BY frecency DESC
    LIMIT 10`);

  await test(`
    SELECT 1
    FROM moz_places
    WHERE (SUBSTR(moz_places.url, 1, 6) == "https:" OR SUBSTR(moz_places.url, 1, 5) == "http:")
    ORDER BY frecency DESC
    LIMIT 10`);

  await test(`
    SELECT 1
    FROM moz_places
    WHERE (+url_hash BETWEEN hash("https", "prefix_lo") AND hash("https", "prefix_hi")
        OR +url_hash BETWEEN hash("http", "prefix_lo") AND hash("http", "prefix_hi"))
    ORDER BY frecency DESC
    LIMIT 10`);

  await test("SELECT 1");
})()


Output:

0.17786700000036945 
    SELECT 1
    FROM moz_places
    ORDER BY frecency DESC
    LIMIT 10 
0.22160275000077237 
    SELECT 1
    FROM moz_places
    WHERE (SUBSTR(moz_places.url, 1, 6) == "https:" OR SUBSTR(moz_places.url, 1, 5) == "http:")
    ORDER BY frecency DESC
    LIMIT 10 
0.24352800000107236 
    SELECT 1
    FROM moz_places
    WHERE (+url_hash BETWEEN hash("https", "prefix_lo") AND hash("https", "prefix_hi")
        OR +url_hash BETWEEN hash("http", "prefix_lo") AND hash("http", "prefix_hi"))
    ORDER BY frecency DESC
    LIMIT 10 
0.12543137500191515 SELECT 1
Comment hidden (mozreview-request)

Comment 10

13 days ago
mozreview-review
Comment on attachment 8904052 [details]
Bug 1389125 - Refactor ActivityStreamProvider in preparation for Highlights.

https://reviewboard.mozilla.org/r/175788/#review181944

This looks great, thanks Ed!
Attachment #8904052 - Flags: review?(usarracini) → review+
Comment hidden (mozreview-request)

Comment 12

13 days ago
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/73c7371abb7f
Refactor ActivityStreamProvider in preparation for Highlights. r=ursula
https://hg.mozilla.org/mozilla-central/rev/73c7371abb7f
Status: REOPENED → RESOLVED
Last Resolved: 19 days ago12 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Assignee)

Updated

12 days ago
Flags: needinfo?(msamuel)
You need to log in before you can comment on or make changes to this bug.