1058515 - Homepage/New/Popular pages load slowly (request caching missing/broken?)

Status: VERIFIED FIXED

(Marketplace Graveyard :: Consumer Pages, defect, P2)

Description

[Christopher Van Wiemeersch [:cvan]]

In bug 980081, I created a caching layer that persisted our API responses to `localStorage`, increasing page load times tremendously.

Loading the homepage <https://marketplace-dev.allizom.org/>, the New <https://marketplace-dev.allizom.org/new> and <https://marketplace-dev.allizom.org/popular> pages, especially on mobile, are noticeably slow. And even after subsequent requests, they aren't any faster.

(1) Are we sending the correct Cache-Control and/or Expires headers on these requests?
(2) Did we remove the `localStorage` request+model caching for the requests? If so, why? Because the feed seems like a perfect candidate for caching these responses keyed off of URLs.

Comment 1

[Chuck Harmston [:chuck]]

Attachment: newpop.mov

So, I'm pretty sure that everything here is a-ok.

See the attached video, where everything loads snappily. Subsequent visits to new/popular don't hit the network layer. I've also verified with debug statements that the request cache is working correctly.

One thing was slightly fishy: we weren't sending cache=1&vary=0 to the feed/get endpoint. Fixed that here:

https://github.com/mozilla/fireplace/compare/8b7df341b6d4...a15e49d02ccb

I did not personally see notable any performance improvement for this, though there is certainly a hypothetical one. This shouldn't affect new/popular pages at all.

I'm going to close this unless somebody can provide STR of something not working as it should.

Comment 2

[Chuck Harmston [:chuck]]

STR for the commit made above:

1) With clean caches, visit https://marketplace-dev.allizom.org/
2) Use the network tab of the developer tools to inspect the XHR request to /api/v2/feed/get/
3) Ensure that the URL of that request contains cache=1&vary=0
3) Ensure that the response for that request contains the header "Cache-Control: must-revalidate, max-age=180"

Comment 3

[Kevin Ngo [:ngoke]]

We did not remove request caching. The new/pop pages are request+model cached as before.

The homepage is not model cached though because the API doesn't fully deserialize the apps to decrease the size.

Comment 4

[Christopher Van Wiemeersch [:cvan]]

What happens on the subsequent page load? Because we shouldn't be waiting for the second API request to finish before hiding the splash screen and rendering the page. Even if the response comes back as a 304. Does that happen?

Comment 5

[Kevin Ngo [:ngoke]]

It should find in the request cache, run from that, and then update asynchronously. I didn't change the caching logic.

Comment 6

[Mathieu Pillard [:mat]]

pop/new are using h/api/v2/fireplace/search/, which wasn't in api_cdn_whitelist/offline_cache_whitelist - only the v1 version was.

https://github.com/mozilla/fireplace/pull/627 which I made initially for bug 1058523, should fix this.

Comment 7

[Christopher Van Wiemeersch [:cvan]]

Awesome, good catch, mat!

Comment 8

[Iulian Timis]

Verified as fixed in https://marketplace-dev.allizom.org/ on FF34 (Win 7).
Postfix screencast http://screencast.com/t/bOOzYAlcDqh
Closing bug.