[Django] #35102: Performance regression on Model.objects.count() between Django 4.2 and 5.0
Django
-----------------------------------------+------------------------
Reporter: Anthony Shaw | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 5.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
I was running the djangobench tool against the 4.2 and 5.0.1 tags (and
main to verify) and noticed that some of the benchmarks had significant
regressions.
The one I wanted to highlight is `query_count`. The other two with
regressions were model creation and query_annotate.
`djangobench --control 4.2 --experiment 5.0.1 query_count -p profiles`
The profile for both versions shows that in v5, get_aggregation has an
additional call to replace_expressions which spends a lot of time in the
slow inspect module. In the profile reports I collected this accounts for
the 30-40% performance regression on the count aggregation query.
Performance regressions with new features are expected, but I wanted to
raise this because the [ASV environment for
Django](https://github.com/django/django-asv) seemed to have missed some
of these regressions when all of the benchmarks suddenly ran faster.
I've attached profiles for 4.2 and 5.0.1 which can be opened using
something like snakeviz
--
Ticket URL: <https://code.djangoproject.com/ticket/35102>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Anthony Shaw | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 5.0
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 Anthony Shaw):
* Attachment "con-query_count" added.
Profile for Django 4.2
Django
-------------------------------+--------------------------------------
Reporter: Anthony Shaw | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 5.0
Severity: Normal | Resolution:
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 Anthony Shaw):
* Attachment "exp-query_count" added.
Profile for Django 5.1-dev
Django
-------------------------------+--------------------------------------
Reporter: Anthony Shaw | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 5.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by Anthony Shaw):
This is the commit that added the extra call tree
https://github.com/django/django/commit/2ee01747c32a7275a7a1a5f7862acba7db764921
--
Ticket URL: <https://code.djangoproject.com/ticket/35102#comment:1>
Django
-------------------------------+--------------------------------------
Reporter: Anthony Shaw | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 5.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by Mariusz Felisiak):
Thanks for the report, I think we could make it faster by avoiding cloning
every time:
{{{#!diff
diff --git a/django/db/models/expressions.py
b/django/db/models/expressions.py
index c20de5995a..80b8fc8e6a 100644
--- a/django/db/models/expressions.py
+++ b/django/db/models/expressions.py
@@ -404,8 +404,9 @@ class BaseExpression:
def replace_expressions(self, replacements):
if replacement := replacements.get(self):
return replacement
+ if not (source_expressions := self.get_source_expressions()):
+ return self
clone = self.copy()
- source_expressions = clone.get_source_expressions()
clone.set_source_expressions(
[
expr.replace_expressions(replacements) if expr else None
}}}
Does it work for you?
--
Ticket URL: <https://code.djangoproject.com/ticket/35102#comment:2>
Django
Reporter: Anthony Shaw | Owner: nobody
Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* type: Uncategorized => Cleanup/optimization
* component: Uncategorized => Database layer (models, ORM)
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/35102#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Anthony Shaw | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:2 Mariusz Felisiak]:
> Does it work for you?
According to my experiments, it reduces regression to ~15%.
--
Ticket URL: <https://code.djangoproject.com/ticket/35102#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Anthony Shaw | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
We could avoid cloning in two more places:
{{{#!diff
diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py
index a79d66eb21..911d0c7848 100644
--- a/django/db/models/sql/query.py
+++ b/django/db/models/sql/query.py
@@ -422,6 +422,8 @@ class Query(BaseExpression):
return obj
def relabeled_clone(self, change_map):
+ if not change_map:
+ return self
clone = self.clone()
clone.change_aliases(change_map)
return clone
diff --git a/django/db/models/sql/where.py b/django/db/models/sql/where.py
index 2f23a2932c..ae7a2a1224 100644
--- a/django/db/models/sql/where.py
+++ b/django/db/models/sql/where.py
@@ -225,6 +225,8 @@ class WhereNode(tree.Node):
return clone
def replace_expressions(self, replacements):
+ if not replacements:
+ return self
if replacement := replacements.get(self):
return replacement
clone = self.create(connector=self.connector,
negated=self.negated)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35102#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Anthony Shaw | Owner:
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => AditiSingh1788
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/35102#comment:6>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: AditiSingh1788 => (none)
* status: assigned => new
Comment:
Please don't assign ticket to yourself. For now, we're discussing a
potential approach, and some improvements have already been proposed.
--
Ticket URL: <https://code.djangoproject.com/ticket/35102#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Anthony Shaw | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Simon Charette (added)
Comment:
> get_aggregation has an additional call to replace_expressions which
spends a lot of time in the slow inspect module
One way we could cut the time spent in `Expression.identity` significantly
would be to cache (e.g. at class initialization time) the
`inspect.signature(cls.__init__)` signature as it represents the vast
majority of the time spent by `replace_expressions`
[[Image(https://cdn.zappy.app/dd47f5e7d4c03baff4726eb106e0d440.png)]]
--
Ticket URL: <https://code.djangoproject.com/ticket/35102#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Anthony Shaw | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Anthony Shaw):
If Expression.__hash__ (Expression.identity()) were to implement another
mechanism for determining a unique hash that doesn't depend upon using
`inspect.signature` the benchmark would be much faster since this is
evaluated twice. I suspect this would impact other benchmarks as well,
hash functions should be as fast as possible but I understand why that is
challenging for something like expressions. inspect.signature is a very
slow function that does a huge amount of processing to build a Signature
object for the given callable.
--
Ticket URL: <https://code.djangoproject.com/ticket/35102#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Anthony Shaw | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
> I suspect this would impact other benchmarks as well, hash functions
should be as fast as possible but I understand why that is challenging for
something like expressions. inspect.signature is a very slow function that
does a huge amount of processing to build a Signature object for the given
callable.
yeah that's the crux of the issue here. This would require having each
expression implement their own correct identity functions based on their
`__init__` signature which would be a significant maintenance burden and
also backward incompatible for third party expressions.
I think we can get a lot of benefit by ''caching'' `inspect.signature`
calls like the following.
{{{#!diff
diff --git a/django/db/models/expressions.py
b/django/db/models/expressions.py
index c20de5995a..0703a134a9 100644
--- a/django/db/models/expressions.py
+++ b/django/db/models/expressions.py
@@ -485,15 +485,22 @@ def select_format(self, compiler, sql, params):
class Expression(BaseExpression, Combinable):
"""An expression that can be combined with other expressions."""
+ def __init_subclass__(cls, /, **kwargs):
+ super().__init_subclass__(**kwargs)
+ cls._constructor_signature = inspect.signature(cls.__init__)
+
+ _constructor_signature = inspect.signature(BaseExpression.__init__)
+
@cached_property
def identity(self):
- constructor_signature = inspect.signature(self.__init__)
args, kwargs = self._constructor_args
- signature = constructor_signature.bind_partial(*args, **kwargs)
+ signature = self._constructor_signature.bind_partial(self, *args,
**kwargs)
signature.apply_defaults()
arguments = signature.arguments.items()
identity = [self.__class__]
for arg, value in arguments:
+ if arg == 'self':
+ continue
if isinstance(value, fields.Field):
if value.name and value.model:
value = (value.model._meta.label, value.name)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35102#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Anthony Shaw | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Anthony Shaw):
Replying to [comment:10 Simon Charette]:
> I think we can get a lot of benefit by ''caching'' `inspect.signature`
calls like the following.
That patch alone is 1.4x faster
{{{
djangobench --control main --experiment exp_1 query_count -t 200
Running benchmarks: query_count
Control: Django 5.1 (in git branch main)
Experiment: Django 5.1 (in git branch exp_1)
Running 'query_count' benchmark ...
Min: 0.000266 -> 0.000186: 1.4310x faster
Avg: 0.000292 -> 0.000202: 1.4424x faster
Significant (t=23.676856)
Stddev: 0.00004 -> 0.00003: 1.4418x smaller (N = 200)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35102#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Anthony Shaw | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Anthony Shaw):
And then with Mariusz' patches the overall speedup is 1.8x compared with
main:
{{{
Running benchmarks: query_count
Control: Django 5.1 (in git branch main)
Experiment: Django 5.1 (in git branch exp_1)
Running 'query_count' benchmark ...
Min: 0.000294 -> 0.000163: 1.8003x faster
Avg: 0.000317 -> 0.000173: 1.8301x faster
Significant (t=53.998054)
Stddev: 0.00003 -> 0.00002: 1.4805x smaller (N = 200)
}}}
And an overall improvement on 4.2
{{{
Running benchmarks: query_count
Control: Django 4.2 (in git branch 4.2)
Experiment: Django 5.1 (in git branch exp_1)
Running 'query_count' benchmark ...
Min: 0.000191 -> 0.000174: 1.0997x faster
Avg: 0.000263 -> 0.000200: 1.3143x faster
Significant (t=3.949122)
Stddev: 0.00022 -> 0.00004: 5.2520x smaller (N = 200)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35102#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Anthony Shaw | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Alternate patch that avoids the cost at class definition time as that
might have an effect on Django bootstrapping time
{{{#!diff
diff --git a/django/db/models/expressions.py
b/django/db/models/expressions.py
index c20de5995a..0ecda38353 100644
--- a/django/db/models/expressions.py
+++ b/django/db/models/expressions.py
@@ -14,7 +14,7 @@
from django.db.models.constants import LOOKUP_SEP
from django.db.models.query_utils import Q
from django.utils.deconstruct import deconstructible
-from django.utils.functional import cached_property
+from django.utils.functional import cached_property, classproperty
from django.utils.hashable import make_hashable
@@ -485,15 +485,21 @@ def select_format(self, compiler, sql, params):
class Expression(BaseExpression, Combinable):
"""An expression that can be combined with other expressions."""
+ @classproperty
+ @functools.lru_cache(maxsize=128)
+ def _constructor_signature(cls):
+ return inspect.signature(cls.__init__)
+
@cached_property
def identity(self):
- constructor_signature = inspect.signature(self.__init__)
args, kwargs = self._constructor_args
- signature = constructor_signature.bind_partial(*args, **kwargs)
+ signature = self._constructor_signature.bind_partial(self, *args,
**kwargs)
signature.apply_defaults()
arguments = signature.arguments.items()
identity = [self.__class__]
for arg, value in arguments:
+ if arg == 'self':
+ continue
if isinstance(value, fields.Field):
if value.name and value.model:
value = (value.model._meta.label, value.name)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35102#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Anthony Shaw | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
Submitted a PR that confirms [https://github.com/django/django/pull/17730
the suite is happy with both changes].
--
Ticket URL: <https://code.djangoproject.com/ticket/35102#comment:14>
Django
-------------------------------------+-------------------------------------
Type: | Charette
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: (none) => Simon Charette
* status: new => assigned
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/35102#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Anthony Shaw | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"d074c7530b812a7b78c9a201c8a70f281ed3a36e" d074c753]:
{{{
#!CommitTicketReference repository=""
revision="d074c7530b812a7b78c9a201c8a70f281ed3a36e"
Refs #35102 -- Optimized Expression.identity used for equality and
hashing.
inspect.signature() is quite slow and produces the same object for each
instance of the same class as they share their __init__ method which
makes it a prime candidate for caching.
Thanks Anthony Shaw for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35102#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Anthony Shaw | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"f3d10546a850df4fe3796f972d5b7e16adf52f54" f3d10546]:
{{{
#!CommitTicketReference repository=""
revision="f3d10546a850df4fe3796f972d5b7e16adf52f54"
Refs #35102 -- Optimized replace_expressions()/relabelling aliases by
adding early return.
This avoids costly hashing.
Thanks Anthony Shaw for the report.
Co-Authored-By: Simon Charette <chare...@gmail.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35102#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Anthony Shaw | Owner: Simon
Type: | Charette
Component: Database layer | Version: 5.0
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/35102#comment:18>