-
Notifications
You must be signed in to change notification settings - Fork 286
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
Simplify internal async callback code #2091
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2091 +/- ##
==========================================
+ Coverage 70.67% 70.69% +0.01%
==========================================
Files 306 306
Lines 61995 62071 +76
==========================================
+ Hits 43816 43878 +62
- Misses 18179 18193 +14
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
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.
I will take a closer look sometime soon, but just sharing a few thoughts I had when I started looking into it.
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs
Show resolved
Hide resolved
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.
This definitely improves code readability. Thanks for the submission!
In previous PR's I optimized async paths to use state objects and cached delegates to static functions to avoid instance delegate invocations. The c# compiler now implements delegate caching directly for static lambdas and produces easier to read code. This PR converts the existing manual delegate caching to the compiler provided method.
It converts ExecuteXmlReaderAsync to use the cached context object pattern.
It converts ExecuteNonQueryAsync to use a cached context object, it was already using a context object so this just adds caching.
I suggest reviewing each commit individually. Side by side diffs make the changes look more confusing than they really are. Netcore only because when sqlcommand is merged I expect the netcore versions to be used since they're generally better for perf.