Opened 18 months ago
Closed 18 months ago
#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 , 18 months ago
Cc: | added |
---|
comment:2 by , 18 months ago
Cc: | added |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Cleanup/optimization |
comment:4 by , 18 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.
comment:5 by , 18 months ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Triage Stage: | Accepted → Unreviewed |
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.
Tentatively accepted. However, this only makes sense on hot paths, I don't want to added dozens of
identity
properties/attributes for micro optimization.