SqlClient enviroment change optimization #34573
Conversation
add SqlEnvChangePool to store a small number of env change objects for re-use change SqlEnvChange to use rented arrays and uses to be aware of possible rental
|
@dotnet-bot test Windows x64 Debug Build please |
|
@afsanehr @tarikulsabbir @Gary-Zh @David-Engel can you please look at this PR too? This one is almost 2 months old :( |
|
@afsanehr @tarikulsabbir @Gary-Zh any update? |
| internal bool newBinRented; | ||
| internal bool oldBinRented; | ||
|
|
||
| public SqlEnvChange Next; |
There was a problem hiding this comment.
Let's make all these public APIs internal.
There was a problem hiding this comment.
Done, and I applied the class modifier guidelines to the file as well.
| @@ -1812,7 +1812,18 @@ internal void OnEnvChange(SqlEnvChange rec) | |||
| break; | |||
|
|
|||
| case TdsEnums.ENV_PROMOTETRANSACTION: | |||
| PromotedDTCToken = rec.newBinValue; | |||
| byte[] dtcToken = null; | |||
| if (rec.newBinRented) | |||
There was a problem hiding this comment.
This change is a bit unclear to me. Could you explain please? Why do we need to copy the newBinValue to dtcToken and then reference it to PromotedDTCToken? can't we directly copy to PromotedDTCToken?
There was a problem hiding this comment.
With this method PromotedDTCSession is never set until it has the correct contents. If we allocate and then copy into it directly there is a small window where it is set but the value is incorrect.
There was a problem hiding this comment.
I see. Thanks for the explanation. What is the scenario which would cause the value to be incorrect?
There was a problem hiding this comment.
If the array contents neds to be kept the contents is copied in then assigned so there are only two states, null or array containing the correct data. otherwise there's a third state which is non-null with zeroed data.
src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserHelperClasses.cs
Outdated
Show resolved
Hide resolved
src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserHelperClasses.cs
Outdated
Show resolved
Hide resolved
src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObject.cs
Outdated
Show resolved
Hide resolved
|
Thanks @Wraith2 . All the internal testing passes successfully. I re-ran the ci check on the PR and will merge it once green. |
|
@Wraith2 please get the latest changes here so the ci passes. Thanks. |
|
Done. |
SqlClient enviroment change optimization Commit migrated from dotnet/corefx@6977ddd
When connecting to the sql server and during various ambient events environment changes are signalled back from the sql server to the client. These events are infrequent after login and handled extremely quickly. The current implementation always allocates new arrays for any binary data even though only a small number are acted on and it uses arrays to carry the change set leading to dropped short term allocations.
This PR changes the
SqlEnvChangeclass to form it's own singly linked list, introduces a SqlEnvChange pool class based on a lock free pool from Roslyn and changes byte[] allocations to use the sharedArrayPoolfor backing arrays.auto and manual tests pass. cc the usual people @afsanehr @keeratsingh @saurabh500