-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Recycling relational and ADO.NET objects in query pipeline #24207
Conversation
71c38d4
to
d4d73cf
Compare
d4d73cf
to
6a696a4
Compare
enumerator._relationalQueryContext.ParameterValues); | ||
|
||
var relationalCommand = enumerator._relationalCommand = enumerator._relationalQueryContext.Connection.RentCommand(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the split queries are still using old system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, this would have been a subtle bug.
I considered introducing a new type - maybe RelationalCommandTemplate - which cannot be executed, this way we immediately catch any bugs where we try to directly execute something that is not meant for execution (like here). I didn't do it yet because of the breaking change (and not strictly necessary), do you think that's valuable enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smitpatel regardless of RelationalCommandTemplate, I pushed a fix - please take a look (it's in ShaperProcessingExpressionVisitor 😨😨😨).
This slightly regresses split query performance, since we don't cache the related commands, and so allocate an additional RelationalCommand and populate it. This seems pretty negligible compared optimizations like #10878 - would you be OK with merging things as they area and opening an issue to optimize in the future?
To optimize this, related commands would need to be returned the the pool once their reader is consumed. For MARS, The SQL Server implementation of RentCommand could manage more than one RelationalCommand.
No caching for the update pipeline? |
For now I'm just concentrating on query :) I think we should definitely have an optimization push for update too, but probably better to do it separately (and see about prioritization...). How about I open a "user story" to at least start tracking possible optimizations to update? Or do you see a reason to do this now specifically? |
To follow the same pattern you are introducing. Less PR overhead. |
@AndriySvyryd I looked at this. The update pipeline is slightly different because there's no cached template command as in the query pipeline; that means that RelationalCommand itself needs to be fully mutable in order for us to reuse it (e.g. add parameters). At that point it seems like RelationalCommandBuilder becomes unnecessary, as RelationalCommands can be built directly; I'd therefore replace it with a very minimal RelationalCommandFactory. However, all that seems better done in a separate PR (which I can do). |
Reduces query allocations by around 10% (both instance numbers and total bytes).
This causes the following to be recycled:
... plus various other tidbits.
@AndriySvyryd @smitpatel would be good to get your eyes on this, some touchy changes.
See #24206