Ticket #34631 Optimize Column expression identity #16940
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello! Thank you for your contribution 💪
As it's your first contribution be sure to check out the patch review checklist.
If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!
If you have any design or process questions then you can ask in the Django forum.
Welcome aboard ⛵️!
snuderl
changed the title
django/db/models/expressions.py
Outdated
| @@ -1142,6 +1142,10 @@ def __init__(self, alias, target, output_field=None): | |||
| super().__init__(output_field=output_field) | |||
| self.alias, self.target = alias, target | |||
|
|
|||
| @cached_property | |||
| def identity(self): | |||
| return (self.__class__, self.alias, self.target, self.output_field) | |||
There was a problem hiding this comment.
Had to move this back to a property as testing on django 4.2.2 showed no perf benefit otherwise anymore. I initially tested on 4.2.0
There was a problem hiding this comment.
That's suspicious. As far as I'm aware, we haven't done anything that could affect this optimization. Maybe it's not worth changing 🤔
There was a problem hiding this comment.
Yeah look I agree with felix here, especially seeing it's only a couple of dot releases. I think there needs be a second party confirming the profiling and not just throw in optimisations in a somewhat random looking manner :)
There was a problem hiding this comment.
https://code.djangoproject.com/ticket/34580 I think its related to this. Before the optimization in that PR we would always end up calling identity on these objects so the difference was much easily observed. Right now a very simple query without order_by will not call identity and thus there will be no difference.
When I add order_by to my query I see the difference. Let me provide some easy to reproduce examples.
There was a problem hiding this comment.
I think it's no longer worth if it's only micro-optimization.
Co-authored-by: Mariusz Felisiak <felisiak.mariusz@gmail.com>
|
Closing per ticket. |
https://code.djangoproject.com/ticket/34631#ticket
Based on some profiling I noticed a lot of time is spent doing reflection/inspect family of calls. The culprit looks like
identityproperty, especially that on Col class. We can easily avoid reflection here.Running a relatively simple query with a good number of columns in our codebase gives a nice boost.
Before:
440usAfter:
280us