[Django] #34211: Performance Regression After Upgrade to Django 3.2
Django
-------------------------------------+-------------------------------------
Reporter: polarmt | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database | Version: 3.2
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
== Overview
We recently upgraded from Django 2.2 to Django 3.2. After the upgrade,
several of our APIs slowed down significantly. For instance, our of our
APIs increased by 50% from ~0.28 seconds to ~0.41 seconds.
== Diagnosis
To diagnose the issue, I ran Flamegraphs on the API with Django 2.2 and
Django 3.2. The results were nearly identical except for one part:
{{{
#!div style="font-size: 80%"
Code highlighting:
{{{#!python
class ForeignKeyDeferredAttribute(DeferredAttribute):
def __set__(self, instance, value):
if instance.__dict__.get(self.field.attname) != value and
self.field.is_cached(instance):
self.field.delete_cached_value(instance)
instance.__dict__[self.field.attname] = value
}}}
}}}
[https://github.com/django/django/blob/stable/3.2.x/django/db/models/fields/related_descriptors.py#L75
django/db/models/fields/related_descriptors.py in Django 3.2]
This code might seem harmless, but it did not scale well when considering
how it is called. From my understanding, this scales in relation to the #
of rows returned by the query times the # of foreign key fields in the
table.
The function below creates an object for every row:
{{{
#!div style="font-size: 80%"
Code highlighting:
{{{#!python
class ModelIterable(BaseIterable):
"""Iterable that yields a model instance for each row."""
def __iter__(self):
queryset = self.queryset
db = queryset.db
compiler = queryset.query.get_compiler(using=db)
# Execute the query. This will also fill compiler.select,
klass_info,
# and annotations.
...
for row in compiler.results_iter(results):
obj = model_cls.from_db(db, init_list,
row[model_fields_start:model_fields_end])
for rel_populator in related_populators:
rel_populator.populate(row, obj)
...
}}}
}}}
Every object created will go through the following logic:
{{{
#!div style="font-size: 80%"
Code highlighting:
{{{#!python
class Model(metaclass=ModelBase):
def __init__(self, *args, **kwargs):
# Alias some things as locals to avoid repeat global lookups
cls = self.__class__
opts = self._meta
_setattr = setattr
_DEFERRED = DEFERRED
...
if not kwargs:
fields_iter = iter(opts.concrete_fields)
# The ordering of the zip calls matter - zip throws
StopIteration
# when an iter throws it. So if the first iter throws it, the
second
# is *not* consumed. We rely on this, so don't change the
order
# without changing the logic.
for val, field in zip(args, fields_iter):
if val is _DEFERRED:
continue
_setattr(self, field.attname, val)
...
}}}
}}}
If the field is a foreign key, then it will call the overrided `__set__`.
For one API call, we had a `filter` query on a table with three foreign
keys that returns 11k rows. This small change did not scale well with a
query.
== Verification
We ran the same API call by disabling this overriding:
{{{
#!div style="font-size: 80%"
Code highlighting:
{{{#!python
class ForeignKeyDeferredAttribute(DeferredAttribute):
def set_fake(self, instance, value):
if instance.__dict__.get(self.field.attname) != value and
self.field.is_cached(instance):
self.field.delete_cached_value(instance)
instance.__dict__[self.field.attname] = value
}}}
}}}
The latency of that API improved from ~0.41 seconds to ~0.31 seconds.
== Thoughts
This change does not seem scalable. Even with a moderately sized query and
three foreign-key relationships, the query increased the API latency by
50%. We understand the motivation behind the change:
[https://code.djangoproject.com/ticket/28147#no1 Ticket #28147]. We were
wondering if there is a more performant fix to that problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/34211>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: polarmt | Owner: nobody
Type: | Status: new
Cleanup/optimization |
(models, ORM) |
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
-------------------------------------+-------------------------------------
Old description:
New description:
== Overview
We recently upgraded from Django 2.2 to Django 3.2. After the upgrade,
several of our APIs slowed down significantly. For instance, our of our
APIs increased by 50% from ~0.28 seconds to ~0.41 seconds.
== Diagnosis
To diagnose the issue, I created Flamegraphs on the API with Django 2.2
--
--
Ticket URL: <https://code.djangoproject.com/ticket/34211#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: polarmt | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 3.2
(models, ORM) |
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
-------------------------------------+-------------------------------------
Description changed by polarmt:
Old description:
> == Overview
> We recently upgraded from Django 2.2 to Django 3.2. After the upgrade,
> several of our APIs slowed down significantly. For instance, our of our
> APIs increased by 50% from ~0.28 seconds to ~0.41 seconds.
>
> == Diagnosis
New description:
keys that returns 11k rows. The new `__set__` was called 33k times, and
the latency accumulated.
== Verification
We ran the same API call by disabling this overriding:
{{{
#!div style="font-size: 80%"
Code highlighting:
{{{#!python
class ForeignKeyDeferredAttribute(DeferredAttribute):
def set_fake(self, instance, value):
if instance.__dict__.get(self.field.attname) != value and
self.field.is_cached(instance):
self.field.delete_cached_value(instance)
instance.__dict__[self.field.attname] = value
}}}
}}}
The latency of that API improved from ~0.41 seconds to ~0.31 seconds.
== Thoughts
This change does not seem scalable. Even with a moderately sized query and
three foreign-key relationships, the query increased the API latency by
50%. We understand the motivation behind the change:
[https://code.djangoproject.com/ticket/28147#no1 Ticket #28147]. We were
wondering if there is a more performant fix to that problem.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/34211#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: polarmt | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
regression | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* keywords: => performance, regression
--
Ticket URL: <https://code.djangoproject.com/ticket/34211#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: polarmt | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: performance, | Triage Stage:
regression | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> == Overview
> We recently upgraded from Django 2.2 to Django 3.2. After the upgrade,
> several of our APIs slowed down significantly. For instance, our of our
> APIs increased by 50% from ~0.28 seconds to ~0.41 seconds.
>
> == Diagnosis
> To diagnose the issue, I created Flamegraphs on the API with Django 2.2
New description:
[https://github.com/django/django/blob/stable/3.2.x/django/db/models/query.py#L42
django/db/models/query.py in Django 3.2]
[https://github.com/django/django/blob/stable/3.2.x/django/db/models/base.py#L404
django/db/models/base.py in Django 3.2]
If the field is a foreign key, then it will call the overrided `__set__`.
For one API call, we had a `filter` query on a table with three foreign
keys that returns 11k rows. The new `__set__` was called 33k times, and
the latency accumulated.
== Verification
We ran the same API call by disabling this overriding:
{{{
#!div style="font-size: 80%"
Code highlighting:
{{{#!python
class ForeignKeyDeferredAttribute(DeferredAttribute):
def set_fake(self, instance, value):
if instance.__dict__.get(self.field.attname) != value and
self.field.is_cached(instance):
self.field.delete_cached_value(instance)
instance.__dict__[self.field.attname] = value
}}}
}}}
The latency of that API improved from ~0.41 seconds to ~0.31 seconds.
== Thoughts
This change does not seem scalable. Even with a moderately sized query and
three foreign-key relationships, the query increased the API latency by
50%. We understand the motivation behind the change:
[https://code.djangoproject.com/ticket/28147#no1 Ticket #28147]. We were
wondering if there is a more performant fix to that problem.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/34211#comment:4>
Django
Old description:
New description:
== Overview
We recently upgraded from Django 2.2 to Django 3.2. After the upgrade,
several of our APIs slowed down significantly. For instance, our of our
APIs increased by 50% from ~0.28 seconds to ~0.41 seconds.
== Diagnosis
To diagnose the issue, we created Flamegraphs on the API with Django 2.2
--
--
Ticket URL: <https://code.djangoproject.com/ticket/34211#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: polarmt | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: performance, | Triage Stage:
regression | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
I see three ways to address this issue
1. Revert 4122d9d3f1983eea612f236e941d937bd8589a0d
2. Optimize `ForeignKeyDeferredAttribute.__set__` to reduce its overheard
on model initialization
3. Introduce `Field.initialize_value(instance: Model, value: Any)` that
`Model.__init__` would use. It would default to `setattr(instance,
self.attname, value)` and could be overridden in `ForeignKey` to do
`self.__dict__[self.attname] = value`
------
**Revert 4122d9d3f1983eea612f236e941d937bd8589a0d**
Pros
- Avoid performance regression
Cons
- Re-introduce confusing cache invalidation logic
------
**Optimize `ForeignKeyDeferredAttribute.__set__`**
We'd need to test it out but I'd be curious to see how better the
following performs
{{{#!python
diff --git a/django/db/models/fields/related_descriptors.py
b/django/db/models/fields/related_descriptors.py
index 422b08e6ca..e3ba443b13 100644
--- a/django/db/models/fields/related_descriptors.py
+++ b/django/db/models/fields/related_descriptors.py
@@ -84,9 +84,10 @@ class Child(Model):
class ForeignKeyDeferredAttribute(DeferredAttribute):
def __set__(self, instance, value):
- if instance.__dict__.get(self.field.attname) != value and
self.field.is_cached(
- instance
- ):
+ setvalue = instance.__dict__.setdefault(self.field.attname,
value)
+ if setvalue == value:
+ return
+ if self.field.is_cached(instance):
self.field.delete_cached_value(instance)
instance.__dict__[self.field.attname] = value
}}}
Pros
- Keep #28147 fixed with a (possibly) more performant initialization
approach
Cons
- The initialization optimization is at the cost of an update tax which
might be ok since we more often initialize a large set of model instances
than update their attributes
-----
Introduce `Field.initialize_value(instance: Model, value: Any)` that would
also need to be tested but given the `_setattr = setattr` caching I think
this might be a dead end since `field.initialize` would incur an extra
attribute lookup as well as a function call **for all fields** not only
`ForeignKey` and its subclasses
-----
My take on it is that we should investigate the feasibility and impact of
2. but I'd be 0 on reverting 122d9d3f1983eea612f236e941d937bd8589a0d given
materializing thousands of model instances instead of using
`QuerySet.values` is always going to be slow.
--
Ticket URL: <https://code.djangoproject.com/ticket/34211#comment:6>