-
Notifications
You must be signed in to change notification settings - Fork 110
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
Optimize and simplify filterable list preview rendering #8450
Conversation
if self._count is None: | ||
self._count = self.search_obj.count() | ||
return self._count |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the changes in this file are to cache the ES results count where possible. Currently we make multiple calls to retrieve the count but we don't need to do that unless the filtering criteria changes.
@@ -13,6 +13,19 @@ | |||
from v1.util.ref import get_category_children | |||
|
|||
|
|||
class SearchResultsPaginator(Paginator): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic lets us move the pagination upstream into the Elasticsearch query instead of pulling back the full list of page IDs from ES and paginating that. I dug into the django-opensearch-dsl code and there doesn't seem to be anything built-in to support this.
Adding prefetch_related
here also lets us prefetch the page related objects to avoid the N+1 queries problem.
object_list = ( | ||
object_list.to_queryset() | ||
.specific() | ||
.live() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@willbarton calling this out specifically as a last-minute way to filter out any not-live pages that may still be in the ES index for some reason.
Although the current code does try to filter out not-live pages as part of the ES query, it doesn't actually check the status at the place those results get converted to Page objects.
Everything looks OK to me here but I'm definitely not the best one to do a more in-depth review of the code! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes make sense to me, not really steeped in the intricacies enough to have a specific opinion on some of the details. I did check that the prefetch_related
bit is being used correctly everywhere (no surprises that it is)
I'm curious about this choice — after prefetching related, what does this step do for us? My impression from reading the PR is that this moves the logic around page type out of the preview template, is that the intention? |
Yes, that's correct. This work was motivated by my experience using DRF on TCCP. There are benefits and drawbacks, but the main idea is to better separate the 3 pieces of code: fetching data from the DB, preparing that data for rendering, actually rendering. Django's template language is powerful and lets you do things like By pre-serializing the objects, we can pre-prepare the list of category names (if that's all we need) and the template can be Conceptually this also makes it easier to migrate to or even just think about a more loosely coupled backend API + frontend rendering model, where the backend produces some data and the frontend renders it, and neither need to worry about each other. |
That's what I was wondering about, and I love this. |
@chosak I definitely think so! |
88dbd7f
to
2ecccd3
Compare
This PR is now ready for review. I've added full test coverage and done more regression testing against various pages to ensure that all functionality continues to work. The coverage failure is because the PR changed settings/local.py, but this isn't a file that we had coverage for before. I want to restate known user-facing changes here:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the pattern of using DRF serializers to pull logic out of our templates. We should do more of this ❤️.
This works as advertised in all my testing!
cfgov/mega_menu/jinja2tags.py
Outdated
try: | ||
return Menu.objects.get(language=default_language) | ||
except Menu.DoesNotExist: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes you have here are fine, and no action is necessary on this comment, but I love nothing about this function. I wonder if there's a way to avoid repeating the try/except.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is, thanks to the code snippet you shared with me and I've pushed as a commit to this PR!
c01dd46
to
b68b70c
Compare
This PR rewrites the way that our filterable list results are rendered, in an attempt to simplify and optimize. The short summary of what this does is:
Details
Filterable list queries in the existing implementation before this PR include the following steps:
Because our Django caches are implemented in the database, this means that at best, even after retrieving the pages from the DB, each post preview render requires an additional DB query to pull out the cached HTML. On a page like the blog with 25 results, that means 25 DB queries just to pull those back.
If you start a fresh local Django development server with no caching (our default developer setup),using Django Debug Toolbar (
ENABLE_DEBUG_TOOLBAR=1 ./runserver.sh
), and visit http://localhost:8000/about-us/blog/, on the main branch (in an incognito window) we currently make 422 DB queries. If you enable local caching (ENABLE_DEBUG_TOOLBAR=1 ENABLE_DEFAULT_CACHE=1 ENABLE_POST_PREVIEW_CACHE=1 ./runserver.sh
) and load that page twice (to fill the caches), we then make 105 DB queries for that page.With these changes, we do the following instead:
prefetch_related
to pull back all page authors, categories, and tags at once.On this branch, using
ENABLE_DEBUG_TOOLBAR=1 ./runserver.sh
, visiting http://localhost:8000/about-us/blog/ makes 36 queries. If you enable the default cache usingENABLE_DEBUG_TOOLBAR=1 ENABLE_DEFAULT_CACHE=1 ./runserver.sh
and load the page twice, we make only 33 queries.(I will note that our basic page rendering of something simple like http://localhost:8000/ takes ~17 queries or so. A bunch of these (~7) are unavoidable Wagtail queries for page lookup. We add lookups for our sharing site, page banners, feature flags, breadcrumbs, and translations, depending on page type. There might be more to do to squeeze these in future should we want to look more at performance, but we definitely have a few places like FLs where we routinely make 100+ queries per page.)
How to test this PR
The best way to test this is to compare various filterable list pages like the blog with production, as there should be no user-facing changes. A good test URL is the events archive page where we list events with both a map or an image.
Notes and todos
Checklist