[Django] #22209: Django internals call len(queryset) instead of queryset.count()
Django
-------------------------------+--------------------
Reporter: gcc | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
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:
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22209>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: gcc | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by gcc):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/22209#comment:1>
Django
Reporter: gcc | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Changes (by akaariai):
* type: Uncategorized => Cleanup/optimization
* easy: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Calling len is cheap if the queryset is already executed. Adding comments
in the places where this is the case and fixing other places is the right
fix for this.
--
Ticket URL: <https://code.djangoproject.com/ticket/22209#comment:2>
Django
Reporter: gcc | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22209#comment:3>
Django
Reporter: gcc | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22209#comment:4>
Django
Reporter: gcc | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timo):
* component: Uncategorized => Database layer (models, ORM)
* easy: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/22209#comment:5>
Django
Reporter: gcc | Owner: nobody
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Database layer | Resolution: fixed
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/22209#comment:6>
Django
Reporter: gcc | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Database layer | Resolution:
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: fixed =>
Comment:
Please don't close a ticket without comment.
--
Ticket URL: <https://code.djangoproject.com/ticket/22209#comment:7>
Django
Reporter: gcc | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22209#comment:8>
Django
Reporter: gcc | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: master
Severity: Normal | worksforme
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: new => closed
* resolution: => worksforme
Comment:
The places in the ticket description look okay to me to use `len()`
instead of `count()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/22209#comment:9>