Performance regression on Model.objects.count() between Django 4.2 and 5.0

[Anthony Shaw]

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

[Anthony Shaw]

Profile for Django 4.2

[Anthony Shaw]

Profile for Django 5.1-dev

[Anthony Shaw]

This is the commit that added the extra call tree ​https://github.com/django/django/commit/2ee01747c32a7275a7a1a5f7862acba7db764921

[Mariusz Felisiak]

Thanks for the report, I think we could make it faster by avoiding cloning every time:

django/db/models/expressions.py

@@ -404 +404 @@
     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?

[Mariusz Felisiak]

Replying to Mariusz Felisiak:

Does it work for you?

According to my experiments, it reduces regression to ~15%.

[Mariusz Felisiak]

We could avoid cloning in two more places:

django/db/models/sql/query.py

@@ -422 +422 @@
         return obj
 
     def relabeled_clone(self, change_map):
+        if not change_map:
+            return self
         clone = self.clone()
         clone.change_aliases(change_map)
         return clone

django/db/models/sql/where.py

@@ -225 +225 @@
         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)

[Mariusz Felisiak]

Please don't assign ticket to yourself. For now, we're discussing a potential approach, and some improvements have already been proposed.

[Simon Charette]

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

[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.

[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.

django/db/models/expressions.py

@@ -485 +485 @@
 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)

[Anthony Shaw]

Replying to 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)

[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)

For the query_aggregate benchmark a marginal improvement over 4.2:

djangobench --control 4.2 --experiment exp_1 query_aggregate -t 200
Running benchmarks: query_aggregate
Control: Django 4.2 (in git branch 4.2)
Experiment: Django 5.1 (in git branch exp_1)

Running 'query_aggregate' benchmark ...
Min: 0.000211 -> 0.000192: 1.0973x faster
Avg: 0.000238 -> 0.000217: 1.0976x faster
Significant (t=3.652858)
Stddev: 0.00004 -> 0.00007: 1.9551x larger (N = 200)

And a more significant one to main/5.1

djangobench --control main --experiment exp_1 query_aggregate -t 200
Running benchmarks: query_aggregate
Control: Django 5.1 (in git branch main)
Experiment: Django 5.1 (in git branch exp_1)

Running 'query_aggregate' benchmark ...
Min: 0.000263 -> 0.000189: 1.3960x faster
Avg: 0.000290 -> 0.000211: 1.3708x faster
Significant (t=15.063702)
Stddev: 0.00005 -> 0.00005: 1.0220x smaller (N = 200)

[Simon Charette]

Alternate patch that avoids the cost at class definition time as that might have an effect on Django bootstrapping time given we define tens of Expression subclasses

django/db/models/expressions.py

@@ -14 +14 @@
 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 +485 @@
 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)

[Simon Charette]

Submitted a PR that confirms ​the suite is happy with both changes.

[Mariusz Felisiak <felisiak.mariusz@…>]

In d074c753:

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.

[Mariusz Felisiak <felisiak.mariusz@…>]

In f3d10546:

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 <charette.s@…>