[Django] #34631: Expression.identity() performance

[Django]

#34631: Expression.identity() performance
-------------------------------------+-------------------------------------
Reporter: Blaž | Owner: nobody
Šnuderl |
Type: | Status: new
Uncategorized |
Component: Database | Version: 4.2
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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.

--
Ticket URL: <https://code.djangoproject.com/ticket/34631>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#34631: Expression.identity() performance
-------------------------------------+-------------------------------------
Reporter: Blaž Šnuderl | Owner: nobody
Type: Uncategorized | Status: new
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by David Sanders):

* cc: David Sanders (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/34631#comment:1>

[Django]

#34631: Expression.identity() performance
-------------------------------------+-------------------------------------
Reporter: Blaž Šnuderl | Owner: nobody

Type: | Status: new
Cleanup/optimization |

Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* cc: Simon Charette (added)
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted

Comment:

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

--
Ticket URL: <https://code.djangoproject.com/ticket/34631#comment:2>

[Django]

#34631: Expression.identity() performance
-------------------------------------+-------------------------------------
Reporter: Blaž Šnuderl | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak):

Have you found other candidates besides `Col`?

--
Ticket URL: <https://code.djangoproject.com/ticket/34631#comment:3>

[Django]

#34631: Expression.identity() performance
-------------------------------------+-------------------------------------
Reporter: Blaž Šnuderl | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 4.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Blaž Šnuderl):

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 cases for it.

--
Ticket URL: <https://code.djangoproject.com/ticket/34631#comment:4>

[Django]

#34631: Expression.identity() performance
-------------------------------------+-------------------------------------
Reporter: Blaž Šnuderl | Owner: nobody

Type: | Status: closed

Cleanup/optimization |
Component: Database layer | Version: 4.2
(models, ORM) |

Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* status: new => closed
* resolution: => wontfix
* stage: Accepted => Unreviewed

Comment:

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/34631#comment:5>