Opened 16 months ago

Last modified 6 months ago

#22209 new Cleanup/optimization

Django internals call len(queryset) instead of queryset.count()

Reported by: gcc Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The documentation says:

Note: Don’t use len() on QuerySets if all you want to do is determine the number of records in the set. It’s much more efficient to handle a count at the database level, using SQL’s SELECT COUNT(*), and Django provides a count() method for precisely this reason. See count() below.

But Django does this itself in a few places:

$ grep -r 'len.*queryset' django/
django/forms/models.py:            return len(self.get_queryset())
django/forms/models.py:        return (len(self.queryset) +
django/contrib/admin/actions.py:    if len(queryset) == 1:

Change History (8)

comment:1 Changed 16 months ago by gcc

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Unfortunately it seems that calling count() is sometimes more expensive than len(queryset). When I made this change, admin_views.tests.AdminCustomQuerysetTest.test_changelist_view_count_queries started failing because the number of queries increased from 4 to 15!

So is this a wontfix, or can we improve the efficiency of count()?

https://github.com/aptivate/django/tree/ticket_22209

comment:2 Changed 16 months ago by akaariai

  • Easy pickings set
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization

Calling len is cheap if the queryset is already executed (or it is going to be executed again later on). Adding comments in the places where this is the case and fixing other places is the right fix for this.

Last edited 16 months ago by akaariai (previous) (diff)

comment:3 Changed 16 months ago by gcc

@akaariai how about we make count() check whether the queryset is already executed, and return its length if it is? Then all Django users will get the benefit without worrying about tricky cases like different code paths and passed-in querysets.

comment:4 Changed 16 months ago by akaariai

Looking at the code this seems to be already the case. So, here using len will execute the query, and later on the results can be reused. Not sure if the results are actually reused in all cases.

Adding comments where results are reused and changing other places to use count seems still like thw best option.

Similar issue is using "if qs:" - this is very inefficient if the qs isn't reused. Not sure if django has any problems from this antipattern.

comment:5 Changed 15 months ago by timo

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Easy pickings unset

comment:6 Changed 9 months ago by jelenak

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

comment:7 Changed 9 months ago by timgraham

  • Resolution fixed deleted
  • Status changed from closed to new

Please don't close a ticket without comment.

comment:8 Changed 6 months ago by collinanderson

Changing the behavior of count() to magically check the length of the cached data if it exists only helps if the queryset is evaluated first. It's still better to use len() rather than count() in this case:

if queryset.count():
    for x in queryset:
        # do something.
}}

I've actually gone through a lot of my code and changed count() to len() in a lot of cases it where it was actually making things slower.
It seems to me it's not worth changing anything here except for maybe changing the warning to be less severe.
Note: See TracTickets for help on using tickets.
Back to Top