Opened 7 years ago

Closed 7 years ago

#18087 closed Bug (fixed)

Performance issue in date-based generic views

Reported by: Aymeric Augustin Owned by: Aymeric Augustin
Component: Generic views Version: 1.4
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no


When BaseDateListView.allow_empty = False -- which is the default -- BaseDateListView.get_dated_queryset evaluates the unpaginated queryset, unnecessarily loading the entire table into memory.

Suggested fix:

Index: django/views/generic/
--- django/views/generic/	(revision 17859)
+++ django/views/generic/	(working copy)
@@ -194,7 +194,7 @@
         if not allow_future:
             qs = qs.filter(**{'%s__lte' % date_field:})
-        if not allow_empty and not qs:
+        if not allow_empty and not qs.exists():
             raise Http404(_(u"No %(verbose_name_plural)s available") % {
                     'verbose_name_plural': force_unicode(qs.model._meta.verbose_name_plural)

At this point, I have no idea to capture this behavior in tests :(

Attachments (1)

18087.patch (1.0 KB) - added by Aymeric Augustin 7 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 7 years ago by Aymeric Augustin

Note: to work around this problem, you can set allow_empty = True.

comment:2 Changed 7 years ago by Claude Paroz

Triage Stage: UnreviewedDesign decision needed

I remember having discussed this on another ticket, but cannot find it again.

If you have a big queryset, your proposal makes sense, but the problem is that you are causing an extra query for all smaller querysets. As we cannot know the size of the queryset beforehand, it's a question about which scenario we want to privilege here.

comment:3 Changed 7 years ago by Aymeric Augustin

Indeed, I vaguely remember a discussion about this too, but I can't find a ticket. Maybe it was on IRC. #16394 is related but doesn't discuss this specific problem.

To be precise, the problem only occurs when pagination is enabled. One would expect pagination to improve performance, but since allow_empty = False by default, it doesn't change anything. If you have a large prefetch_related (which is how I noticed the problem), performance becomes ugly quickly.

I'm attaching a new patch showing another technique: when pagination is enabled, use qs.exists(); when it isn't, use qs, since it will be loaded anyway.

Changed 7 years ago by Aymeric Augustin

Attachment: 18087.patch added

comment:4 Changed 7 years ago by Claude Paroz

Has patch: set
Triage Stage: Design decision neededReady for checkin

I think it's a good compromise.

comment:5 Changed 7 years ago by Aymeric Augustin

Needs tests: set
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

It's possible to write tests for this behavior with assertNumQueries.

comment:6 Changed 7 years ago by Aymeric Augustin

Owner: changed from nobody to Aymeric Augustin

comment:7 Changed 7 years ago by Aymeric Augustin

Resolution: fixed
Status: newclosed

In [17893]:

Fixed #18087 -- Prevented date-based generic views from loading entire tables in memory when pagination is enabled.

Note: See TracTickets for help on using tickets.
Back to Top