#34631 closed Cleanup/optimization (wontfix)

Expression.identity() performance

Reported by: Blaž Šnuderl Owned by: nobody
Component: Database layer (models, ORM) Version: 4.2
Severity: Normal Keywords:
Cc: David Sanders, Simon Charette Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Expression class defines an identity uses that relies heavily on reflection/inspect to generate identity of its objects. This is generally gonna be slow and does a lot of extra work compared to hand writing these identity expressions.

Based on some simple profiling in our project, I saw a lot of query building time being spent in this identity function. Atleast for simple queries vast majority of Expression objects are Col, one per column in the model.

My proposal would be to optimize atleast this case, but potentially we can also explore whether we need such complicated identity and all and if we could avoid the inspect calls in general.

I have opened a very simple PR demonstrating a possible improvement here https://github.com/django/django/pull/16940 and it passes all django tests and I also ran it on our projects test suite without issues.

Change History (5)

comment:1 by David Sanders, 17 months ago

Cc: David Sanders added

comment:2 by Mariusz Felisiak, 17 months ago

Cc: Simon Charette added
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

Tentatively accepted. However, this only makes sense on hot paths, I don't want to added dozens of identity properties/attributes for micro optimization.

comment:3 by Mariusz Felisiak, 17 months ago

Have you found other candidates besides Col?

comment:4 by Blaž Šnuderl, 17 months ago

Yes I definetly agree. I can try profiling our code a bit more if anything else pops out but honestly I don't expect anything else to have a big impact unless we specifically design test cases for a particular optimization.

Last edited 17 months ago by Blaž Šnuderl (previous) (diff)

comment:5 by Mariusz Felisiak, 17 months ago

Resolution: wontfix
Status: newclosed
Triage Stage: AcceptedUnreviewed

It seems that the performance gain is no longer significant after fixing #34580. It's probably not worth adding a custom identity to Col(). Closing as "wontfix", unless someone proves it's worth adding.

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