Closed Bug 1389125 Opened 6 years ago Closed 6 years ago

Refactor ActivityStreamProvider in preparation for Highlights

Categories

(Firefox :: New Tab Page, enhancement)

Product:
Firefox
Component:
New Tab Page
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: standard8, Assigned: Mardak)

References

Details

Attachments

(1 file)

Bug 1389125 - Refactor ActivityStreamProvider in preparation for Highlights.
59 bytes, text/x-review-board-request
ursula
: review+
Details 
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.
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
Closed: 6 years ago
Resolution: --- → WONTFIX
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)
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
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.
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 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+
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
Closed: 6 years ago → 6 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Flags: needinfo?(msamuel)
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.