Opened 2 years ago

Last modified 9 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 polarmt)

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 polarmt, 2 years ago

Description: modified (diff)

comment:2 by polarmt, 2 years ago

Description: modified (diff)

comment:3 by polarmt, 2 years ago

Keywords: performance regression added

comment:4 by polarmt, 2 years ago

Description: modified (diff)

comment:5 by polarmt, 2 years ago

Description: modified (diff)

comment:6 by Simon Charette, 2 years ago

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

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 4122d9d3f1983eea612f236e941d937bd8589a0d given materializing thousands of model instances instead of using QuerySet.values is always going to be slow.

Last edited 2 years ago by Carlton Gibson (previous) (diff)

comment:7 by Mariusz Felisiak, 2 years 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.

Note: See TracTickets for help on using tickets.
Back to Top