Opened 10 months ago
Closed 10 months ago
#35246 closed Cleanup/optimization (fixed)
Make Field.unique a plain attribute
Reported by: | Adam Johnson | Owned by: | Adam Johnson |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
Another candidate for caching, like #35230, #35232 and #35241, following the same system check profiling.
Field.unique
is a simple property that computes whether a field is unique from two inputs:
@property def unique(self): return self._unique or self.primary_key
The result is immutable because the two input attributes shouldn’t change.
I found this method was called 3543 times during system checks, taking ~0.7% (~0.3ms) of the total runtime on a Python 3.12 project with 118 models. After moving it to a plain attribute, this cost is eliminated. (cProfile’s overhead biases the cost of function calls upwards, so the actual saving may be smaller, but it still seems worth the minimal change.)
unique
is accessed in many other code paths so this change will help those paths too.
Change History (7)
comment:1 by , 10 months ago
Description: | modified (diff) |
---|---|
Has patch: | set |
comment:2 by , 10 months ago
Description: | modified (diff) |
---|
comment:3 by , 10 months ago
comment:4 by , 10 months ago
Triage Stage: | Unreviewed → Accepted |
---|
I agree with Simon on both items, that this optimization can be helpful and that we could use some metrics to do some follow up in the medium/long term.
I just checked the branch and wonder if converting the property into a cached one, instead of deleting it in favor of an attribute, might be a better idea. I'm thinking of potential usages out there in the wild overriding the property for <reasons> and the proposed change would break those scenarios.
comment:5 by , 10 months ago
See https://github.com/django/django-asv/pull/80 for a benchmark, plus the other general maintenance PRs I opened on django-asv :)
comment:6 by , 10 months ago
Sure, we can use cached_property; it has the same performance after the first call.
Edit: tried it, re-profiled and saw it had 570 calls (one for each field) running in neglible time, and have updated the PR.
Adam, I greatly appreciate the time you are spending investigating these but could we please create a benchmark so we can measure the impact of them sooner than later.
I understand that we can't create a benchmark for every performance improvement we make like we do when fixing bug with regression tests but I think the volume of these warrant creating a prior baseline so we can make sure all the work you are investing today doesn't disappear over time as we add more checks and features to Django.