Opened 3 months ago

Closed 2 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 Adam Johnson)

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 Adam Johnson, 3 months ago

Description: modified (diff)
Has patch: set

comment:2 by Adam Johnson, 3 months ago

Description: modified (diff)

comment:3 by Simon Charette, 3 months ago

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.

comment:4 by Natalia Bidart, 3 months ago

Triage Stage: UnreviewedAccepted

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 Adam Johnson, 3 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 Adam Johnson, 3 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.

Last edited 3 months ago by Adam Johnson (previous) (diff)

comment:7 by GitHub <noreply@…>, 2 months ago

Resolution: fixed
Status: assignedclosed

In e65deb7:

Fixed #35246 -- Made Field.unique a cached property.

Co-authored-by: Natalia <124304+nessita@…>

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