Opened 23 months ago
Last modified 8 months ago
#34211 new Cleanup/optimization
Performance regression in ForeignKeyDeferredAttribute changes.
Reported by: | polarmt | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 3.2 |
Severity: | Normal | Keywords: | performance, regression |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
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 and Django 3.2. The results were nearly identical except for one part:
Code highlighting:
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
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:
Code highlighting:
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) ...
django/db/models/query.py in Django 3.2
Every object created will go through the following logic:
Code highlighting:
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) ...
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:
Code highlighting:
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: Ticket #28147. We were wondering if there is a more performant fix to that problem.
Change History (7)
comment:1 by , 23 months ago
Description: | modified (diff) |
---|
comment:2 by , 23 months ago
Description: | modified (diff) |
---|
comment:3 by , 23 months ago
Keywords: | performance regression added |
---|
comment:4 by , 23 months ago
Description: | modified (diff) |
---|
comment:5 by , 23 months ago
Description: | modified (diff) |
---|
comment:7 by , 23 months ago
Summary: | Performance Regression After Upgrade to Django 3.2 → Performance regression in ForeignKeyDeferredAttribute changes. |
---|---|
Triage Stage: | Unreviewed → Accepted |
Accepting for investigation. I would also prefer to avoid reverting 4122d9d3f1983eea612f236e941d937bd8589a0d.
I see three ways to address this issue
ForeignKeyDeferredAttribute.__set__
to reduce its overheard on model initializationField.initialize_value(instance: Model, value: Any)
thatModel.__init__
would use. It would default tosetattr(instance, self.attname, value)
and could be overridden inForeignKey
to doself.__dict__[self.attname] = value
Revert 4122d9d3f1983eea612f236e941d937bd8589a0d
Pros
Cons
Optimize
ForeignKeyDeferredAttribute.__set__
We'd need to test it out but I'd be curious to see how better the following performs
Pros
Cons
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 sincefield.initialize
would incur an extra attribute lookup as well as a function call for all fields not onlyForeignKey
and its subclassesMy take on it is that we should investigate the feasibility and impact of 2. but I'd be 0 on reverting 4122d9d3f1983eea612f236e941d937bd8589a0d given materializing thousands of model instances instead of using
QuerySet.values
is always going to be slow.