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

perf: Add cached SqlReferenceCollection find delegate and context objects #380

Merged
merged 2 commits into from
Feb 28, 2020

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Jan 13, 2020

When checking to see if there is a live command or reader associated with a connection (which in any sane program is almost always a negative result) a delegate and in some cases a closure are allocated. These can be avoided most of the time by using a static cached delegate and allowing reuse of a single context object if it is available.

@cheenamalhotra cheenamalhotra added the 📈 Performance Issues that are targeted to performance improvements. label Feb 6, 2020
@Wraith2
Copy link
Contributor Author

Wraith2 commented Feb 24, 2020

Any feedback @cheenamalhotra @David-Engel ?

Copy link
Contributor

@David-Engel David-Engel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. Looks good to me. Can you merge the latest from master to refresh the build results?

@Wraith2 Wraith2 force-pushed the perf-refcollectionfind branch from d995624 to dcff145 Compare February 24, 2020 22:26
@cheenamalhotra cheenamalhotra added this to the 2.0.0-preview2 milestone Feb 26, 2020
@cheenamalhotra
Copy link
Member

cheenamalhotra commented Feb 26, 2020

The changes look good to me, but I don't think this code path is even tested currently.
Can you add tests for this class?

The code coverage shall drop otherwise.

Copy link
Member

@cheenamalhotra cheenamalhotra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just gathered:
Please also consider make these changes to NetFx code.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Feb 27, 2020

FindLiveCommand was never being used which is why there will be no code coverage for it. I've removed it. FindLiveReader should be covered through the cancellation tests.

@cheenamalhotra cheenamalhotra merged commit 9de0419 into dotnet:master Feb 28, 2020
@Wraith2 Wraith2 deleted the perf-refcollectionfind branch February 29, 2020 00:39
cheenamalhotra pushed a commit to cheenamalhotra/SqlClient that referenced this pull request Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📈 Performance Issues that are targeted to performance improvements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants