Skip to content
This repository has been archived by the owner on Feb 29, 2020. It is now read-only.

Reduce duplicates places queries near startup #3638

Closed
Mardak opened this issue Oct 3, 2017 · 3 comments · Fixed by #3695
Closed

Reduce duplicates places queries near startup #3638

Mardak opened this issue Oct 3, 2017 · 3 comments · Fixed by #3695

Comments

@Mardak
Copy link
Member

Mardak commented Oct 3, 2017

Also see https://bugzilla.mozilla.org/show_bug.cgi?id=1401817
Right now TopSitesFeed and HighlightsFeed both call and expire places results at least 2 times on INIT, NEW_TAB_LOAD.

HighlightsFeed additionally does work on TOP_SITES_UPDATED.

@piatra
Copy link
Contributor

piatra commented Oct 11, 2017

Both TopSitesFeed and HighlightsFeed refresh twice from INIT and NEW_TAB_LOAD and it looks like NEW_TAB_LOAD is always second. The event is triggered by ActivityStreamMessageChannel that is created after the feeds are initialized.

  • TopSites are not affected by NEW_TAB_LOAD because they update only once every 30 minutes.
  • Highlights will update on every NEW_TAB_LOAD if they have less than 9 entries.
    • I think we could easily add an extra && Date.now() - this.lastUpdated > 30 seconds to counter this.
    • Highlights already clear the cache on changes to bookmarks and places so actual "stale data" could not occur as a result of a 30s delay.

The extra work done by the HighlightsFeed on TOP_SITES_UPDATED isn't that important. It will hit the cache. Additionally we could first check if there is any overlap between topsites and highlights before actually calling though to fetchHighlights.

Thoughts? @Mardak @k88hudson

@piatra
Copy link
Contributor

piatra commented Oct 11, 2017

I'm also looking into writing a mochitest that loads AS and tests how many times we call into NewTabUtils like getHighlights.

@Mardak
Copy link
Member Author

Mardak commented Oct 11, 2017

I believe if you look at dispatch calls during startup, you should be able to see two from TopSites because the refresh call from INIT and NEW_TAB_LOAD happen concurrently (blocked on this._tippyTopProvider.init).

Also, even if Highlights hits the cache, it's most likely unnecessarily dispatching. Although that could be treated as its own issue as one was filed in terms of places queries.

I believe @k88hudson has mentioned just removing the "fewer than 9" Highlights check similar to how it was removed from TopSites.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants