New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce duplicates places queries #3695

Merged
merged 12 commits into from Oct 18, 2017

Conversation

Projects
None yet
4 participants
@piatra
Collaborator

piatra commented Oct 12, 2017

Closes #3638 and fix #3697

  • Removed fetchHiglights call when there are less than 9 highlights cards
  • added an flag in Topsites and Highlights to signal that data is currently being fetched
    • this will prevent INIT and NEW_TAB_LOAD actions from triggering two update calls
      • covers the case when the events are fast enough that they overlap
      • otherwise lastUpdated field will prevent requests from happening
  • on TOP_SITES_UPDATED check that Topsites and Highlights have items in common before triggering fetchHighlights
    • fetchHiglights ensures deduping but we can skip it if there is nothing to dedupe
@Mardak

This comment has been minimized.

Show comment
Hide comment
@Mardak

Mardak Oct 12, 2017

Member

This seems to revert the fix for #3377 from #3386

Member

Mardak commented Oct 12, 2017

This seems to revert the fix for #3377 from #3386

@piatra

This comment has been minimized.

Show comment
Hide comment
@piatra

piatra Oct 12, 2017

Collaborator

And probably something similar can happen for Highlights as well?
I'll do some more testing.

Collaborator

piatra commented Oct 12, 2017

And probably something similar can happen for Highlights as well?
I'll do some more testing.

@piatra piatra requested review from Mardak and removed request for k88hudson Oct 12, 2017

@piatra piatra assigned piatra and unassigned k88hudson Oct 12, 2017

@piatra piatra removed the request for review from Mardak Oct 12, 2017

@piatra

This comment has been minimized.

Show comment
Hide comment
@piatra

piatra Oct 12, 2017

Collaborator

@Mardak I’ve simulated a delayed TopSitesFeed startup by doing await new Promise(resolve => setTimeout(resolve, 3000)); in the refresh method. On startup I also open several other new tabs. They are all missing TopSites initially but as soon as the first one is initialized they all get the data.

Does this flow match the one in the issue? I'm trying to reproduce it so that maybe we can fix it some other way that doesn't involve multiple calls to refresh.

Collaborator

piatra commented Oct 12, 2017

@Mardak I’ve simulated a delayed TopSitesFeed startup by doing await new Promise(resolve => setTimeout(resolve, 3000)); in the refresh method. On startup I also open several other new tabs. They are all missing TopSites initially but as soon as the first one is initialized they all get the data.

Does this flow match the one in the issue? I'm trying to reproduce it so that maybe we can fix it some other way that doesn't involve multiple calls to refresh.

@Mardak

This comment has been minimized.

Show comment
Hide comment
@Mardak

Mardak Oct 12, 2017

Member

That's how things behaved before #3386. I just tracked the actual cause: NEW_TAB_LOAD happens before INIT, so the broadcast doesn't happen because that one gets skipped for a pending refresh from LOAD. Filed #3697

Member

Mardak commented Oct 12, 2017

That's how things behaved before #3386. I just tracked the actual cause: NEW_TAB_LOAD happens before INIT, so the broadcast doesn't happen because that one gets skipped for a pending refresh from LOAD. Filed #3697

@Mardak

This comment has been minimized.

Show comment
Hide comment
@Mardak

Mardak Oct 12, 2017

Member

@k88hudson I wonder if this is the right place/time to just get rid of refreshing sites/highlights on NEW_TAB_LOAD especially if both are refreshing based on last updated time. Probably do it on SYSTEM_TICK?

In terms of avoiding double Highlights places call on INIT and TOP_SITES_UPDATED, instead of awaiting for top sites from INIT, maybe just return early and remember that the next TOP_SITES_UPDATED should broadcast. Although if TopSitesFeed always broadcasts… should Highlights getting TOP_SITES_UPDATED also broadcast?

Member

Mardak commented Oct 12, 2017

@k88hudson I wonder if this is the right place/time to just get rid of refreshing sites/highlights on NEW_TAB_LOAD especially if both are refreshing based on last updated time. Probably do it on SYSTEM_TICK?

In terms of avoiding double Highlights places call on INIT and TOP_SITES_UPDATED, instead of awaiting for top sites from INIT, maybe just return early and remember that the next TOP_SITES_UPDATED should broadcast. Although if TopSitesFeed always broadcasts… should Highlights getting TOP_SITES_UPDATED also broadcast?

@Mardak

This comment has been minimized.

Show comment
Hide comment
@Mardak

Mardak Oct 12, 2017

Member

Actually nevermind the last question. I would think SYSTEM_TICK does /not/ broadcast and just updates the store for the next rehydration. So similarly highlights shouldn't broadcast on TOP_SITES_UPDATED unless a previous broadcast was skipped because there was no top sites data.

Member

Mardak commented Oct 12, 2017

Actually nevermind the last question. I would think SYSTEM_TICK does /not/ broadcast and just updates the store for the next rehydration. So similarly highlights shouldn't broadcast on TOP_SITES_UPDATED unless a previous broadcast was skipped because there was no top sites data.

@piatra

This comment has been minimized.

Show comment
Hide comment
@piatra

piatra Oct 12, 2017

Collaborator

I wonder if this is the right place/time to just get rid of refreshing sites/highlights on NEW_TAB_LOAD especially if both are refreshing based on last updated time. Probably do it on SYSTEM_TICK?

This would fix the extra calls on startup for both TopSites and Highlights.

In terms of avoiding double Highlights places call on INIT and TOP_SITES_UPDATED, instead of awaiting for top sites from INIT, maybe just return early and remember that the next TOP_SITES_UPDATED should broadcast.

Highlights should return early if Topsites are not ready and broadcast if its store state is not initialized (this should only happen for initialization).

Collaborator

piatra commented Oct 12, 2017

I wonder if this is the right place/time to just get rid of refreshing sites/highlights on NEW_TAB_LOAD especially if both are refreshing based on last updated time. Probably do it on SYSTEM_TICK?

This would fix the extra calls on startup for both TopSites and Highlights.

In terms of avoiding double Highlights places call on INIT and TOP_SITES_UPDATED, instead of awaiting for top sites from INIT, maybe just return early and remember that the next TOP_SITES_UPDATED should broadcast.

Highlights should return early if Topsites are not ready and broadcast if its store state is not initialized (this should only happen for initialization).

@Mardak

This comment has been minimized.

Show comment
Hide comment
@Mardak

Mardak Oct 12, 2017

Member

Oh, checking for Highlights's section initialized value should work. I just realized Highlights probably shouldn't refresh on any actions that TopSitesFeed is also watching; otherwise, we end up with the double updates as we do now when TOP_SITES_UPDATED comes in. Although I can't think of a good way to maintain that dependency right now…

Or maybe something like:

constructor() {
  this.broadcastNextUpdate = true;
}
onAction() {
  case MIGRATION:
  case PLACES_*:
    this.broadcastNextUpdate = true;
    break;
  case TOP_SITES_UPDATED:
    this.fetchHighlights();
}
Member

Mardak commented Oct 12, 2017

Oh, checking for Highlights's section initialized value should work. I just realized Highlights probably shouldn't refresh on any actions that TopSitesFeed is also watching; otherwise, we end up with the double updates as we do now when TOP_SITES_UPDATED comes in. Although I can't think of a good way to maintain that dependency right now…

Or maybe something like:

constructor() {
  this.broadcastNextUpdate = true;
}
onAction() {
  case MIGRATION:
  case PLACES_*:
    this.broadcastNextUpdate = true;
    break;
  case TOP_SITES_UPDATED:
    this.fetchHighlights();
}
@piatra

This comment has been minimized.

Show comment
Hide comment
@piatra

piatra Oct 13, 2017

Collaborator

I just realized Highlights probably shouldn't refresh on any actions that TopSitesFeed is also watching; otherwise, we end up with the double updates as we do now when TOP_SITES_UPDATED comes in.

That's what I was trying to do with this check, before calling fetchHiglights on TOP_SITES_UPDATED

on TOP_SITES_UPDATED, HighlightsFeed should only call fetchHighlights if we know some items will get deduped (removed).
The only time we need to bypass that is when Highlights.rows.length === 0 because we returned early when Topsites weren't initialized.

There is the case where you unpin something and it falls out of your Topsites (which shouldn't even happen #3676) case in which maybe a Highlight should show up. But having that show up at the next refresh seems totally resonable (to simplify the logic).

Collaborator

piatra commented Oct 13, 2017

I just realized Highlights probably shouldn't refresh on any actions that TopSitesFeed is also watching; otherwise, we end up with the double updates as we do now when TOP_SITES_UPDATED comes in.

That's what I was trying to do with this check, before calling fetchHiglights on TOP_SITES_UPDATED

on TOP_SITES_UPDATED, HighlightsFeed should only call fetchHighlights if we know some items will get deduped (removed).
The only time we need to bypass that is when Highlights.rows.length === 0 because we returned early when Topsites weren't initialized.

There is the case where you unpin something and it falls out of your Topsites (which shouldn't even happen #3676) case in which maybe a Highlight should show up. But having that show up at the next refresh seems totally resonable (to simplify the logic).

@Mardak

This comment has been minimized.

Show comment
Hide comment
@Mardak

Mardak Oct 13, 2017

Member

One drawback of highlightsWillDedupe approach is that it conditionally fetches twice. In a state where that method returns true, e.g., a top site was bookmarked (now a bug with the implementation in the PR because bookmarks don't dedupe), every TOP_SITES_UPDATED will do a 2nd fetch for the next 5 days. So in the best case for a given action, there's one fetch and worst case there's two.

So a different approach that could make it in the worst case 1 fetch would perhaps be better.

Member

Mardak commented Oct 13, 2017

One drawback of highlightsWillDedupe approach is that it conditionally fetches twice. In a state where that method returns true, e.g., a top site was bookmarked (now a bug with the implementation in the PR because bookmarks don't dedupe), every TOP_SITES_UPDATED will do a 2nd fetch for the next 5 days. So in the best case for a given action, there's one fetch and worst case there's two.

So a different approach that could make it in the worst case 1 fetch would perhaps be better.

@piatra

This comment has been minimized.

Show comment
Hide comment
@piatra

piatra Oct 13, 2017

Collaborator

Instead of checking TopSites.rows and Highlights.rows for matching URLs I could directly call dedupe and see if the length of the result is different.
It should definitely be better than invalidating the cache & doing a new query.

Collaborator

piatra commented Oct 13, 2017

Instead of checking TopSites.rows and Highlights.rows for matching URLs I could directly call dedupe and see if the length of the result is different.
It should definitely be better than invalidating the cache & doing a new query.

@Mardak

This comment has been minimized.

Show comment
Hide comment
@Mardak

Mardak Oct 13, 2017

Member

One thing I just noticed from my own browsing is that because we currently broadcast fetchHighlights on a NEW_TAB_LOAD, often times I end up seeing highlights without images because it only just realized there are new highlights and should fetch an image. But by doing it some time other than TAB_LOAD, there's a higher chance that the image will already be fetched.

Member

Mardak commented Oct 13, 2017

One thing I just noticed from my own browsing is that because we currently broadcast fetchHighlights on a NEW_TAB_LOAD, often times I end up seeing highlights without images because it only just realized there are new highlights and should fetch an image. But by doing it some time other than TAB_LOAD, there's a higher chance that the image will already be fetched.

@Mardak

Let's remove the feed update time stuff and shorten LinksCache to be less than SYSTEM_TICK. Also would be good to clean up the bool arg APIs without defaults.

With those, this should be good to land

Show outdated Hide outdated system-addon/lib/HighlightsFeed.jsm Outdated
Show outdated Hide outdated system-addon/lib/HighlightsFeed.jsm Outdated
Show outdated Hide outdated system-addon/lib/HighlightsFeed.jsm Outdated
Show outdated Hide outdated system-addon/lib/Store.jsm Outdated
Show outdated Hide outdated system-addon/lib/TopSitesFeed.jsm Outdated
Show outdated Hide outdated system-addon/lib/TopSitesFeed.jsm Outdated
@Mardak

This comment has been minimized.

Show comment
Hide comment
@Mardak

Mardak Oct 17, 2017

Member

One thing I noticed when testing the PR, but this probably should be its own issue/PR. #3675 added expiring the highlights links cache on BOOKMARK_ADDED. A new profile causes adds of several bookmarks causing the cache to be expired and fetched from places multiple times on startup.

Member

Mardak commented Oct 17, 2017

One thing I noticed when testing the PR, but this probably should be its own issue/PR. #3675 added expiring the highlights links cache on BOOKMARK_ADDED. A new profile causes adds of several bookmarks causing the cache to be expired and fetched from places multiple times on startup.

@Mardak

Mardak approved these changes Oct 17, 2017

whew! Thanks!

Show outdated Hide outdated system-addon/lib/HighlightsFeed.jsm Outdated
Show outdated Hide outdated system-addon/lib/HighlightsFeed.jsm Outdated
Show outdated Hide outdated system-addon/lib/HighlightsFeed.jsm Outdated
Show outdated Hide outdated system-addon/lib/LinksCache.jsm Outdated
Show outdated Hide outdated system-addon/test/unit/lib/HighlightsFeed.test.js Outdated
Show outdated Hide outdated system-addon/test/unit/lib/TopSitesFeed.test.js Outdated

@piatra piatra merged commit 8850b8b into mozilla:master Oct 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@piatra piatra deleted the piatra:bug-3638-reduce-calls branch Oct 18, 2017

@as-pine-proxy

This comment has been minimized.

Show comment
Hide comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment