Skip to content
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

Recycle relational and ADO.NET objects in query execution #24206

Closed
roji opened this issue Feb 20, 2021 · 4 comments
Closed

Recycle relational and ADO.NET objects in query execution #24206

roji opened this issue Feb 20, 2021 · 4 comments
Assignees
Labels
area-perf closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@roji
Copy link
Member

roji commented Feb 20, 2021

While doing some memory profiling on Fortunes, a lot of objects are being allocated which seem like they should be recycled (numbers are for a run with 30k iterations):

RecycleRelationalObjects

Note especially all the Stopwatches getting allocated - 6th place global allocations. Also, the above screenshot is just a simple of the top objects, here are some more: NpgsqlParameterCollection, List<NpgsqlParameter>, NpgsqlParameter[], RelationalDataReader, NpgsqlStatement, List<NpgsqlStatement>.

We have a small architectural blocker for recycling these... RelationalCommand gets generated as part of compilation, and cached globally in RelationalCommandCache. In effect, it's an immutable, shared type, which prevents it from caching resources such as Stopwatch, DbCommand, RelationalReader, etc. PR #24207 is a hacky prototype for caching a RelationalCommand on RelationalConnection, and populating it from the RelationalCommand stored in RelationalCommandCache; this allow caching most of the resources needed for executing a query. With this PR, instance allocations go down by 27%, byte allocations go down by 18%, and Fortunes RPS goes up by around 2.3%.

A better design would be to split RelationalCommand and have, e.g. RelationalCommandInfo, which is the cached result of compilation; it cannot be executed, is immutable and just holds the CommandText and Parameters. RelationalCommand would then be initialized from this and executed as above. This would be a provider-facing breaking change though.

On a related note, we currently dispose ADO.NET objects (NpgsqlConnection, NpgsqlCommand) when a query is complete, when we could be reusing them. This PR also does that.

@Giorgi
Copy link
Contributor

Giorgi commented Feb 20, 2021

I might be totally wrong but as Stopwatch.Elapsed is used for logging and interceptor events I think you can avoid allocating StopWatch by checking for available interceptors and moving the diagnostics.ShouldLog(definition) check from LogCommandExecuted and other methods to Execute*** methods.

@roji
Copy link
Member Author

roji commented Feb 21, 2021

After this PR, all Stopwatch instances should be cached in any case. You're right that it may be possible to avoid restarting the Stopwatches if there's no need for it, but it's a bit more complicated than that (BroadcastCommandExecuted also reports the Stopwatch-based duration to BroadcastcommandExecute based on the results of NeedsEventData). It may be possible to do something, but restarting a Stopwatch (as opposed to allocating one) is probably not something worth optimizing away.

I'll keep an eye out for this while analyzing traces; if I see that restarting the Stopwatch (i.e. getting the timestamp) is expensive, we can definitely look into it.

@ajcvickers
Copy link
Member

@roji See also #16197

@ajcvickers ajcvickers added this to the 6.0.0 milestone Feb 22, 2021
@roji
Copy link
Member Author

roji commented Apr 21, 2021

Done in #24207

@roji roji closed this as completed Apr 21, 2021
@roji roji added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed needs-design labels Apr 21, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0, 6.0.0-preview4 Apr 26, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0-preview4, 6.0.0 Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-perf closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
Development

No branches or pull requests

3 participants