Improve PDF performance - #4425
Conversation
svenseeberg
commented
Jul 10, 2026
svenseeberg
commented
- Execute compute heavy page fetches only if no PDF exists
* Execute compute heavy page fetches only if no PDF exists
timobrembeck
left a comment
timobrembeck
left a comment
There was a problem hiding this comment.
Looks good, thanks a lot! 🚀
A minor suggestion: the existing tests guard correctness but not the performance goal. A small test using pytest-django's django_assert_num_queries (or asserting the hash-path SQL does not select content) would protect it. Optional, but cheap insurance.
| # Build a lightweight queryset with the (large) translation content deferred, so that the | ||
| # cache key computation and the existence check below do not load the page contents. The | ||
| # full content is only fetched further down if the PDF actually has to be rendered. | ||
| pages = pages.prefetch_related(None) |
There was a problem hiding this comment.
This makes GeneratePdfView.prefetch_public_translations = True in cms/views/pages/page_bulk_actions.py:50 dead config — the prefetch lookup is stripped here before the queryset is ever evaluated, so it never executes. If you want to, you could remove that flag in this PR so it doesn't mislead future readers into thinking the bulk view still prefetches translations.
| for page in hash_pages: | ||
| # add translation id and last_updated to hash key list if they exist | ||
| page_translation = page.get_public_translation(language_slug) | ||
| if page_translation and not page.archived: |
There was a problem hiding this comment.
Non-blocking, pre-existing: page.archived resolves implicitly_archived → get_cached_ancestors(), which runs one get_ancestors() query per page since nothing warms _cached_ancestors here. So the cache-hit path is still O(N) queries even with the content deferred. Also, for the whole-region branch the check is redundant — region.get_pages() already returns non_archived_pages. Might be worth a follow-up issue rather than this PR.
jonbulz
left a comment
jonbulz
left a comment
There was a problem hiding this comment.
Looks nice, thanks! I think once @timobrembeck's comments are addressed this is ready to go. 👍
| :return: The queryset of content objects | ||
| """ | ||
| return self.prefetch_translations( | ||
| to_attr="prefetched_public_translations", |
There was a problem hiding this comment.
I'd prefer to use a different attribute name to avoid confusion. Something like prefetched_public_translations_without_content maybe? I won't die on that hill, though.
3 participants
