Optimize SqlClient connection pool performance #33580
Conversation
change SSPI login buffers to arraypool allocations
src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
|
There is one test failure in SqlClient on Windows.10.Amd64.ClientRS4.Open-x64-Debug: Is it related? |
|
@dotnet-bot test this please |
|
Yes it is related. There's a very peculiar case in managed only mode (so not reachable on windows desktop) where it reassigns the buffer about 4 calls below where it's allocated. It could just have returned a 0 length as far as I can see but I really don't want to try and rework the state object interface at this point so I've worked around by keeping a separate reference to the rental and releasing that. |
|
@dotnet-bot test Linux arm64 release please |
|
@dotnet-bot test this please |
|
@dotnet-bot test UWP CoreCLR x64 Debug Build please |
| string sidString = identity.User.Value; | ||
| SecurityIdentifier user = identity.User; | ||
| bool isNetwork = user.IsWellKnown(WellKnownSidType.NetworkSid); | ||
| string sidString = user.ToString(); |
There was a problem hiding this comment.
Why use ToString()? The docs mention that Value provides the upper case value of SID and ToString() has no such guarantees.
It makes sense to leave this part of assigning sidString as is.
There was a problem hiding this comment.
For the sake on consistency, I think it makes more sense to use the Value instead of ToString()
There was a problem hiding this comment.
I rewrote the code for ToString() to be faster in #33579 In fact it is guaranteed that the ToString() return value has an uppercase S in it because it's a literal.
Calling ToUpperCase() on it is a waste of time. There is no need for the returned value to be uppercased only that it be consistent with every call, the only use is as part of the pool key so case is irrelevant. I judged it better not to waste time making a call that can be proven to have no effect.
There was a problem hiding this comment.
The part that is concerning to me is that knowing that ToString() implementation has an uppercase S is an internal detail vs there is an API called .Value which guarantees upper case string.
In terms of API consistency, if this code is to execute on another framework which doesn't have the same guarantees of internals, that you are describing, then this connection pool code may fall apart.
You improved the performance of ToString(), which means that there should be an overall improvement with .Value retrieval as well on .Net Core
There was a problem hiding this comment.
It doesn't matter about the case though because we only use the value as part of the pool key, upper or mixed is irrelevant as long as it's the same every time we ask for it from SecurityIdentifier.
I can change it back if you really want and we won't lose much it's a really tiny improvement compared to the other changes, it just bothers me to do something I know is worthless.
There was a problem hiding this comment.
Done, the perf change isn't worth worrying about.
* change pool to fetch user object only once and use direct tostirng change SSPI login buffers to arraypool allocations * change rental to keep track of rented array in case the caller reassigns * address feedback Commit migrated from dotnet/corefx@9b6618f
7 participants
When using trusted authentication with connection pooling profiling has shown that getting the connection from the pool and getting the security information from the unmanaged library are hot paths.
This PR slightly rearranges the code in the connection pool key identification code to be more efficient about use of the user SecurityIdentifier object fetching it only once because it is expensive and calling ToString() directly instead of using Value because Value will make an unneeded ToUpperInvariant() call.
It also changes the allocation of SSPI buffers to use the ArrayPool, in testing the sspi buffers were being requested at 55K each time which caused a lot of memory churn.
Combined with the changes in #33579 this should allow connections to be retrieved ~20% faster allowing better short frequent query throughput. I'm still having trouble with benching but the profile run output is reasonable, all tests pass and the changes are very small.
/CC @keeratsingh, @afsanehr, @saurabh500, @divega