I’m a CPython core developer working on free-threading. Would the Django team be open to reviewing patches aimed at improving Django’s performance on free-threaded Python (e.g., 3.14t)?
For context, I’ve been running some multithreading benchmarks with Zulip, which uncovered a number of scaling issues. I’m fixing things in upstream CPython when possible, but some are specific to Django. Here are a few examples to give a sense of the possible changes:
django.db.models.fields.Field maintains a class-level creation_counter attribute that tracks each time a Field instance is created. For the most part, that’s fine because Field instances are typically created at class definition time, but there are a few code paths that create temporary Field instances per request. That leads to contention on creation_counter and invalidation of CPython’s internal attribute/method lookup cache for Field subclasses. For example, AutoField.rel_db_type creates a temporary IntegerField. I think this can be avoided with some refactoring.- Some of the bounded
@lru_cache(maxsize=N) decorators lead to scaling bottlenecks. This is because strict LRU policy requires updating metadata on every hit. I’m not entirely sure what the best fix is here, but we could consider using per-thread caches for free threading, using a mostly-static pre-computed cache for language codes, or something else along these lines.
- In 3.14t (but not 3.15t), there’s lock contention on the set lookup in
HttpHeaders.UNPREFIX_HEADERS. Using a frozenset avoids this.
If this kind of contribution would be welcome, I’m happy to start submitting PRs. I haven’t explored all the scaling issues, but I expect most changes will be small. For reference, I’ve also opened #36983 (Improve free-threading performance) – Django on Trac.
Thanks,
Sam
8 Likes
Hello @colesbury!
I’d be interested in supporting your efforts in reviewing such patches (the field creation counter one is of particular interest to me) but I was curious to hear if you have any strategy in mind to prevent regression as we merge those or some rule of thumb to help reviewers catch those early?
I assume anything that uses threading.Lock and friends should be something we keep an eye on?
We do have an ASV setup running these tests if that can be used to capture some of the progress made along the way.
Cheers,
Simon
Thinking of Field.creation_counter it really only matter during model creation time so I think we could move the assignment and locking to contribute_to_class (kind of a predecessor to __set_name__) defaulting the field to 0 for instances that are not bound to a models.Model subclass.
It was mainly required prior to Python preserving dict insertion order and some of this code is pretty old; looks like Field.auto_creation_counter is completely unused for example.
This will likely require us changing Field.__hash__ but it seems to be already broken in some cases.
Thanks Simon!
Moving Field.creation_counter assignment to contribute_to_class sounds like a simpler fix than what I tried.
I don’t have a particular regression testing strategy in mind. I’ll take a look at the ASV setup. I think the GitHub actions runners only have a few cores, so it might be hard to catch scaling regressions with them. For CPython, we have a script (ftscalingbench.py) that you can run locally that runs some code snippets from multiple threads vs. one thread.
Along with threading.Lock, I think the big thing is to watch out for shared mutable data (like Field.creation_counter). Then there are more subtle things like functools.lru_cache and random.random() (the global random generator has an internal lock protecting it).
FWIW I gave a shot at implementing the contribute_to_class approach to avoid looking up Field.creation_counter on every field creation and here’s what I came up with
diff --git a/django/db/models/fields/__init__.py b/django/db/models/fields/__init__.py
index feb0441bab..b04e9c2b01 100644
--- a/django/db/models/fields/__init__.py
+++ b/django/db/models/fields/__init__.py
@@ -124,11 +124,13 @@ class Field(RegisterLookupMixin):
empty_strings_allowed = True
empty_values = list(validators.EMPTY_VALUES)
- # These track each time a Field instance is created. Used to retain order.
- # The auto_creation_counter is used for fields that Django implicitly
- # creates, creation_counter is used for all user-specified fields.
+ # These track each time a Field instance is attached to a model to retain
+ # field definition order through model inheritance. The auto_creation_counter
+ # is used for fields that Django implicitly creates, creation_counter is used
+ # for all user-specified fields.
creation_counter = 0
auto_creation_counter = -1
+
default_validators = [] # Default set of validators
default_error_messages = {
"invalid_choice": _("Value %(value)r is not a valid choice."),
@@ -231,14 +233,6 @@ class Field(RegisterLookupMixin):
self._db_tablespace = db_tablespace
self.auto_created = auto_created
- # Adjust the appropriate creation counter, and save our local copy.
- if auto_created:
- self.creation_counter = Field.auto_creation_counter
- Field.auto_creation_counter -= 1
- else:
- self.creation_counter = Field.creation_counter
- Field.creation_counter += 1
-
self._validators = list(validators) # Store for deconstruction later
self._error_messages = error_messages # Store for deconstruction later
@@ -700,7 +694,9 @@ class Field(RegisterLookupMixin):
return NotImplemented
def __hash__(self):
- return hash(self.creation_counter)
+ if (creation_counter := self.__dict__.get("creation_counter")) is not None:
+ return hash(creation_counter)
+ return super().__hash__()
def __deepcopy__(self, memodict):
# We don't have to deepcopy very much here, since most things are not
@@ -948,6 +944,15 @@ class Field(RegisterLookupMixin):
If private_only is True, create a separate instance of this field
for every subclass of cls, even if cls is not an abstract model.
"""
+ # Preserve existing creation counter to ensure the proper ordering of
+ # fields inherited from abstract bases.
+ if "creation_counter" not in self.__dict__:
+ if self.auto_created:
+ self.creation_counter = Field.auto_creation_counter
+ Field.auto_creation_counter -= 1
+ else:
+ self.creation_counter = Field.creation_counter
+ Field.creation_counter += 1
self.set_attributes_from_name(name)
self.model = cls
cls._meta.add_field(self, private=private_only)
diff --git a/tests/model_fields/tests.py b/tests/model_fields/tests.py
index 3d856d36c5..6fca94275a 100644
--- a/tests/model_fields/tests.py
+++ b/tests/model_fields/tests.py
@@ -4,6 +4,7 @@ from django import forms
from django.core.exceptions import ValidationError
from django.db import models
from django.test import SimpleTestCase, TestCase
+from django.test.utils import isolate_apps
from django.utils.choices import CallableChoiceIterator
from django.utils.functional import lazy
@@ -89,11 +90,18 @@ class BasicFieldTests(SimpleTestCase):
f = Foo._meta.get_field("a")
self.assertEqual(str(f), "model_fields.Foo.a")
+ @isolate_apps("model_fields")
def test_field_ordering(self):
"""Fields are ordered based on their creation."""
f1 = models.Field()
f2 = models.Field(auto_created=True)
f3 = models.Field()
+
+ class Model(models.Model):
+ field1 = f1
+ field2 = f2
+ field3 = f3
+
self.assertLess(f2, f1)
self.assertGreater(f3, f1)
self.assertIsNotNone(f1)
Apart from the test adjusted in the patch in I ran into a test added to ensures Field.__hash__ remains stable before and after assignment to a model which is problematic for this approach as Field.__hash__ is based of creation_counter. We’ll likely have to revisit this decision if we want to go this route.
Would assigning creation_counter on first access make sense? For example, in addition to assigning it in contribute_to_class, we could assign it whenever __hash__, __eq__ or anything else touches it.
I didn’t see any test failures with this.
diff --git a/django/db/models/fields/__init__.py b/django/db/models/fields/__init__.py
index d98319ef00..6ecd62b873 100644
--- a/django/db/models/fields/__init__.py
+++ b/django/db/models/fields/__init__.py
@@ -128,7 +128,7 @@ class Field(RegisterLookupMixin):
# These track each time a Field instance is created. Used to retain order.
# The auto_creation_counter is used for fields that Django implicitly
# creates, creation_counter is used for all user-specified fields.
- creation_counter = 0
+ next_creation_counter = 0
auto_creation_counter = -1
default_validators = [] # Default set of validators
default_error_messages = {
@@ -257,15 +257,7 @@ class Field(RegisterLookupMixin):
self.db_comment = db_comment
self._db_tablespace = db_tablespace
self.auto_created = auto_created
-
- # Adjust the appropriate creation counter, and save our local copy.
- if auto_created:
- self.creation_counter = Field.auto_creation_counter
- Field.auto_creation_counter -= 1
- else:
- self.creation_counter = Field.creation_counter
- Field.creation_counter += 1
-
+ self._creation_counter = None
self._validators = list(validators) # Store for deconstruction later
self._error_messages = error_messages # Store for deconstruction later
@@ -586,6 +578,21 @@ class Field(RegisterLookupMixin):
return Col(self.model._meta.db_table, self)
+ def _assign_creation_counter(self):
+ # Adjust the appropriate creation counter, and save our local copy.
+ if self.auto_created:
+ self._creation_counter = Field.auto_creation_counter
+ Field.auto_creation_counter -= 1
+ else:
+ self._creation_counter = Field.next_creation_counter
+ Field.next_creation_counter += 1
+
+ @property
+ def creation_counter(self):
+ if self._creation_counter is None:
+ self._assign_creation_counter()
+ return self._creation_counter
+
def select_format(self, compiler, sql, params):
"""
Custom format for select clauses. For example, GIS columns need to be
@@ -975,6 +982,8 @@ class Field(RegisterLookupMixin):
If private_only is True, create a separate instance of this field
for every subclass of cls, even if cls is not an abstract model.
"""
+ if self._creation_counter is None:
+ self._assign_creation_counter()
self.set_attributes_from_name(name)
self.model = cls
cls._meta.add_field(self, private=private_only)
Would assigning creation_counter on first access make sense?
It might pass the suite but I think it would circumvent the intent of the logic.
For example, if you do something like
first = Field()
second = Field()
assert sorted([second, first]) == [first, second]
second.creation_counter would be accessed before first.creation_counter resulting in unexpected ordering.
I do believe we should use this opportunity to get rid of creation_counter entirely and only define specialized ordering and hashing on model bound fields as it’s where it only matters.
Field.creation_counter comes from a distant time when type.__new__(attrs) wasn’t preserving insertion order. When 3.6+ released a decade ago we should have considered getting rid of it by moving it to a binding ordering taking advantage of the preserved insertion of type.__new__(attrs).
A potential deprecation plan could be
- Keep
creation_counter (and its auto friend) around for now but raise a deprecation warning and follow the normal deprecation process. We could make it increment only at attribute access like you proposed doing here.
- Two alternative for a replacement counter
a. Introduce a _model_index property assigned at field bounding time (contribute_to_class) that keeps a per-model local index. I could see this being handy for other purposes to determine the field index without checking Model._meta.local_fields and would be easy to implement by making it self._model_index = len(model._meta.fields)
b. Keep using a global counter, just rename it _bind_counter and only increment at bounding time.
- Adjust ordering, equality, and hashing methods to use the new bound counter and return / raise not implemented on non-bound instances.
I’d be curious to hear your thoughts on the above @adamchainz given your concerns on maintaining ordering and hashability on unbound field instances and your interest in speeding up query construction by avoiding models.Field construction.
1 Like
I’m abusing the creation_counter field in django-translated-fields to ensure that fields are ordered according to my expectations. The comment in the commit isn’t correct – without creation_counter the generated fields would be grouped at the end instead of being inserted at the correct location in the model’s local fields list even with Python >3.6. I’m doing this because being able to depend on the default ordering of fields is more convenient than having to define fieldsets or fields on all forms and model admin classes.
I’m aware that this is definitely not an intended use and not supported. I want to follow developments here and also throw in a vote for a ordering which is at least locally deterministic. A local _model_index would definitely work for me as would a global _bind_counter. Assigning the value at access time might work too but sounds a bit fragile.
1 Like