Code

Opened 2 years ago

Closed 2 years ago

#18087 closed Bug (fixed)

Performance issue in date-based generic views

Reported by: aaugustin Owned by: aaugustin
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

Description

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/dates.py
===================================================================
--- django/views/generic/dates.py	(revision 17859)
+++ django/views/generic/dates.py	(working copy)
@@ -194,7 +194,7 @@
         if not allow_future:
             qs = qs.filter(**{'%s__lte' % date_field: timezone.now()})
 
-        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 aaugustin 2 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 2 years ago by aaugustin

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

comment:2 Changed 2 years ago by claudep

  • Triage Stage changed from Unreviewed to Design 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 2 years ago by aaugustin

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 2 years ago by aaugustin

comment:4 Changed 2 years ago by claudep

  • Has patch set
  • Triage Stage changed from Design decision needed to Ready for checkin

I think it's a good compromise.

comment:5 Changed 2 years ago by aaugustin

  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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

comment:6 Changed 2 years ago by aaugustin

  • Owner changed from nobody to aaugustin

comment:7 Changed 2 years ago by aaugustin

  • Resolution set to fixed
  • Status changed from new to closed

In [17893]:

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.