Skip to content
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

Performance improvements #1536

Merged
merged 8 commits into from
Aug 20, 2020
Merged

Performance improvements #1536

merged 8 commits into from
Aug 20, 2020

Conversation

emilytoppm
Copy link
Contributor

@emilytoppm emilytoppm commented Aug 12, 2020

For #1533

  • Cache meta API, which is run on every request and is identical
  • Switch to using the Wagtail dynamic image serve view, as rendition links are often serialised and then not used
  • Optimise the Weekly view:
    • Add some prefetching/select_related
    • Split up serialisers, and don't serialise big unnecessary fields for listings where they're not used (particularly rendering the post and body)

…ries for them rather than adding the order_by each time
…, so as not to serialise bulky fields that aren't used on listing. Add select_related and prefetch_related to cut down queries."
…, rather that fetching renditions that might not actually be used on the frontend
@emilytoppm emilytoppm requested a review from kaedroho August 12, 2020 11:42
newamericadotorg/api/meta/views.py Outdated Show resolved Hide resolved
home/templatetags/utilities.py Show resolved Hide resolved
newamericadotorg/api/author/serializers.py Show resolved Hide resolved
@tomdyson
Copy link

@jacobtoppm do you have any before-and-after metrics, e.g. number of queries run, page response time?

@emilytoppm
Copy link
Contributor Author

emilytoppm commented Aug 13, 2020

@tomdyson Approximately 650-700 queries down to 60-90ish, and (on my local environment, with silk installed, which can affect things) 7s-9s to 1s-2s. This is specifically for The Weekly, and a few of those queries are then made by the dynamic image serve view, but it should be a substantial improvement. It also eliminates an extra API request that wasn't being used.

For the meta API, it takes the previously 2000ms+ view down to 100ms range. The cache is relatively short term, but given NewRelic suggests an rpm of about 13rpm minimum, it's still only running on average 1/25 of requests, so I'm hoping it should be a good improvement.

@tomdyson
Copy link

Thanks, @jacobtoppm, those sound very promising! Could we afford a longer cache for the meta API? 2000ms -> 100ms is a very dramatic improvement.

@emilytoppm
Copy link
Contributor Author

@tomdyson Because some of what it's caching is updated via the CMS (programs for instance) and it's a relatively high traffic view, Karl suggested a shorter cache (30s) than what I'd added originally and we compromised at 2mins, but I'd definitely be up for extending it if we don't think the update frequency justifies the short cache duration. It's a combination of about pages, programs, content type pages, and subscriptions that get returned, and the latter I don't have a good feeling for how often are updated/how fast you'd want them to respond to an update. @nmorduch, do you have any thoughts on this?

@kaedroho
Copy link
Contributor

kaedroho commented Aug 13, 2020

Looking at Jacob's numbers above, the current value of 2 minutes eliminates 96% of the traffic to that view. We could increase it more, but the returns of increasing it diminish rapidly the higher we go. For example, if we now double it to 4 minutes, we would only gain an extra 2% hit rate.

@emilytoppm
Copy link
Contributor Author

I suppose it depends whether we're going by hit rate or average request time - you're right about the hit rate improvement being slight though. Taking the cached view as somewhere between 100ms and 200ms, doubling the cache time gives us somewhere in the range of 13-20% reduction in average request time. Not huge, but noticeable.

@nmorduch
Copy link
Contributor

Is this good to merge?

@emilytoppm
Copy link
Contributor Author

Yes, for sure. The cache length is easily adjustable if we decide it's needed later.

@nmorduch nmorduch linked an issue Aug 19, 2020 that may be closed by this pull request
@nmorduch
Copy link
Contributor

I'm merging this to test on staging!

@nmorduch nmorduch merged commit a5f7777 into master Aug 20, 2020
@nmorduch nmorduch deleted the performance-improvements branch August 21, 2020 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[∞] Performance issues
4 participants