Optimize SqlClient query memory allocation #34047
Conversation
… closure allocation unless needed
…necasary closure scope allocations
…ery EmptyReadPacket property access
changed multiple callsites to use new methods changed multiple callsites to extract closure where state method is not possible
src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
| } | ||
| }, TaskScheduler.Default); | ||
| }, | ||
| Tuple.Create(this, stateObj, taskReleaseConnectionLock, taskReleaseConnectionLock ? _connHandler : null), |
There was a problem hiding this comment.
taskReleaseConnectionLock ? _connHandler : null Why is this check needed? Why not simply pass in the _connHandler? The reason I ask for this is because, now to operate on _connHandler in the delegate, it will always need a check for taskReleaseConnectionLock. This makes it harder to enhance code.
There was a problem hiding this comment.
Good point. It isn't necesary to pass the boolean into the continuation at all, i can just null check the connHandle. Changed, take a look and see if that looks clearer.
src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObject.cs
Show resolved
Hide resolved
src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
|
This approach of splitting the code is way easier to review and digest. I personally could get through this much faster. I have left a few comments of which 2 are important in terms of behavior changes, one is about the PacketRelease having been removed and the other is the usage of _connHandler instead of connHandler property. @Wraith2 You mention that
I haven't faced this issue. I am using the Release build, built locally and I haven't had to build a Nuget package to consume this change. Honestly, I am building the dotnet/corefx code after a long time and I did setup everything from scratch except for installing Visual Studio. I understand that you haven't been able to test on Linux and you have been testing by flipping the ManagedSNI flag on Windows and that is causing issues as well. Testing with Managed SNI on Windows hasn't been great in terms of gaining confidence in Managed SNI code for Linux. (SNI is the networking interface part of SqlClient which is implemented in sni.dll for Windows and System.Data.SqlClient.SNI namespace for Unix) The reason for the lack of reliability is because the networking stack of .Net Core on Windows Unix and macOS are different. Ideally the author of SqlClient shouldn't have to worry about it, however in past while working on SqlClient on .Net Core Linux, we have uncovered problems with networking stack because of test execution on Linux. We have found 2 kinds of issues, the first kind (and many) in SqlClient where we should have handled certain failures and we were not, and the second is that we occasionally discovered issues in the networking stack of Core. I have been running EF tests (ran a few tests earlier this afternoon) with a Release build of SqlClient (build on Ubuntu 1804) against the Debug build of EFCore.SqlServer provider by replacing the DLL with the change in this PR and all worked well. I haven't hit any asserts and so shouldn't you or anyone else, with the Release build of SqlClient. I will post a bit more about these asserts and how the code has been structured, on the issue #33930 (perhaps in the next couple of days) Since you mentioned that there is a good amount of learning curve involved with Linux testing for you, I expect that you will take sometime to get this right. However we could work together on this. You could ping me on my email address on the Github profile and I could walk you through the steps there. Trying to keep the infrastructure issues separate from code review discussions. |
Asserts aren't compiled into release builds, you can't hit asserts if you use them. You'll only see them in Debug. |
Yes, I understand that. The point I was trying to make is we should still test the EF tests with the release build. Running EF tests with release build would give you and me the confidence that EF didn't break as a consumer of our Release builds. |
|
DO NOT MERGE. Pending offline verification with the product team. |
saurabh500
added
area-System.Data.SqlClient
* NO MERGE *
|
I have added the NO MERGE label. Just to make sure that the PR is not merged until it is verified with some more linux testing. I will update this PR when the verifications are done on Linux. |
|
Ok, all yours, let me know if anything more needs to be done. |
saurabh500
removed
the
* NO MERGE *
|
Tested Manual Tests on Ubuntu LGTM |
|
Excellent, thanks for the feedback. |
* add a pre-boxed invalid prepared handle constant to remove repeated boxing of -1 * change Task.Run to Task.Factory.StartNew(state) avoiding method scope closure allocation unless needed * move several continuation functions to separate functions to avoid unnecasary closure scope allocations * remove unused readPacket definition, allocation and cleanup * add pre-boxed empty read packet variable to avoid boxing an int on every EmptyReadPacket property access * refine state to remove this closure, minor formatting * added AsyncHelper.*WithState methods to avoid closure manually changed multiple callsites to use new methods changed multiple callsites to extract closure where state method is not possible * address feedback and remove further BeginExecuteNonQuery closure
* add a pre-boxed invalid prepared handle constant to remove repeated boxing of -1 * change Task.Run to Task.Factory.StartNew(state) avoiding method scope closure allocation unless needed * move several continuation functions to separate functions to avoid unnecasary closure scope allocations * remove unused readPacket definition, allocation and cleanup * add pre-boxed empty read packet variable to avoid boxing an int on every EmptyReadPacket property access * refine state to remove this closure, minor formatting * added AsyncHelper.*WithState methods to avoid closure manually changed multiple callsites to use new methods changed multiple callsites to extract closure where state method is not possible * address feedback and remove further BeginExecuteNonQuery closure Commit migrated from dotnet/corefx@c946a18
Many synchronous code paths contain lambda allocations for async usage which capture variables in function scope which cause the allocation and use of closure types to contain them. This set of changes attempts to prevent allocation of a closure by using a state tuple or by breaking the closure machinery into a separate method only called in the async case.
The CachedAsyncState field was being allocated in the sync code path, I have refined the logic slightly to avoid this unless it is needed.
I have introduced a couple of pre-boxed integer constants which prevent the repeated boxing of the default value, these are bof the invalid prepared handle value and the empty read packet.
I have added an
AsyncHelper.ContinueTaskWithStatefunction which uses the same logic as the existingContinueTaskand adds the ability to pass a state object to each of the delegate parameters, this helps to avoid closures in calling code.This PR was split from #32811 and into smaller commits for easier review. It has passed the manual and efcore tests in native mode, the tests cannot be successfully run in managed mode due to https://github.com/dotnet/corefx/issues/33930 . Performance improvements for this and related PR's are in the original discussion.
/cc @keeratsingh @afsanehr @saurabh500 @David-Engel