Optimize SqlClient tds state to remove handle boxing #34044

Wraith2 · 2018-12-12T22:15:20Z

These changes remove constant boxing of session and packet IntPtr when using the native implementation of TdsParserStateObject (native is used in windows non-uap builds) which improves memory performance. Making these changes also allows assertion of the handle types in debug builds which adds safety and testability.

I have used ref structs to ensure that the types can only be used to pass information through the abstract methods that define the interface between the core parser logic and the native/managed specific elements, these handles can never be accidentally stored or take part in async activity. There are separate unnix and windows implementations of these types to handle the differences in members and simplify development using them.

Performance profiling results were posted in #33155 and show approximately ~13% speed increase on small write performance and lower GC frequency.

This PR was split from #33155 for easier review.
/cc @keeratsingh @afsanehr @saurabh500 @David-Engel

…id boxing of IntPtr in native mode

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserSafeHandles.cs

Wraith2 · 2018-12-14T10:32:47Z

Fixed up merge conflicts from the other optimizing PR merged yesterday.

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObject.cs

…tations

Wraith2 · 2018-12-18T00:06:49Z

@dotnet test OSX x64 Debug Build please

src/System.Data.SqlClient/src/System/Data/SqlClient/PacketHandle.Windows.cs

src/System.Data.SqlClient/src/System/Data/SqlClient/SessionHandle.Windows.cs

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectFactory.Windows.cs

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectNative.cs

saurabh500 · 2018-12-18T18:37:57Z

@Wraith2 can you update the description of the PR since you got rid of FEATURE_INTEROPSNI

remove unused packethandle variable in IsConnectionAlive remove identidal overridden implementations of EmptyReadHandle

Wraith2 · 2018-12-18T21:09:40Z

Description updated and everything addressed.

Wraith2 · 2018-12-18T21:23:04Z

Getting test failures about mismatched packets, i'll push and let you know when it's passing.

saurabh500 · 2018-12-18T21:28:43Z

@Wraith2 Do you happen to have the test failure link ?

Wraith2 · 2018-12-18T21:45:53Z

It's a manual test, stack trace is:

  unexpected readPacket without corresponding SNIPacketRelease

     at System.Data.SqlClient.TdsParserStateObject.ReadSniSyncOverAsync() in E:\Programming\csharp7\corefx\src\System.Data.SqlClient\src\System\Data\SqlClient\TdsParserStateObject.cs:line 2111
     at System.Data.SqlClient.TdsParserStateObject.TryReadNetworkPacket() in E:\Programming\csharp7\corefx\src\System.Data.SqlClient\src\System\Data\SqlClient\TdsParserStateObject.cs:line 2038
     at System.Data.SqlClient.TdsParserStateObject.TryPrepareBuffer() in E:\Programming\csharp7\corefx\src\System.Data.SqlClient\src\System\Data\SqlClient\TdsParserStateObject.cs:line 1083
     at System.Data.SqlClient.TdsParserStateObject.TryReadByteArray(Span`1 buff, Int32 len, Int32& totalRead) in E:\Programming\csharp7\corefx\src\System.Data.SqlClient\src\System\Data\SqlClient\TdsParserStateObject.cs:line 1279
     at System.Data.SqlClient.TdsParserStateObject.TryReadPlpBytes(Byte[]& buff, Int32 offset, Int32 len, Int32& totalBytesRead) in E:\Programming\csharp7\corefx\src\System.Data.SqlClient\src\System\Data\SqlClient\TdsParserStateObject.cs:line 1916
     at System.Data.SqlClient.TdsParser.TryReadSqlValue(SqlBuffer value, SqlMetaDataPriv md, Int32 length, TdsParserStateObject stateObj) in E:\Programming\csharp7\corefx\src\System.Data.SqlClient\src\System\Data\SqlClient\TdsParser.cs:line 4512
     at System.Data.SqlClient.SqlDataReader.TryReadColumnInternal(Int32 i, Boolean readHeaderOnly) in E:\Programming\csharp7\corefx\src\System.Data.SqlClient\src\System\Data\SqlClient\SqlDataReader.cs:line 3417
     at System.Data.SqlClient.SqlDataReader.TryReadColumn(Int32 i, Boolean setTimeout, Boolean allowPartiallyReadColumn) in E:\Programming\csharp7\corefx\src\System.Data.SqlClient\src\System\Data\SqlClient\SqlDataReader.cs:line 3270
     at System.Data.SqlClient.SqlDataReader.GetValueInternal(Int32 i) in E:\Programming\csharp7\corefx\src\System.Data.SqlClient\src\System\Data\SqlClient\SqlDataReader.cs:line 2513
     at System.Data.SqlClient.SqlDataReader.GetValue(Int32 i) in E:\Programming\csharp7\corefx\src\System.Data.SqlClient\src\System\Data\SqlClient\SqlDataReader.cs:line 2455
     at System.Data.SqlClient.ManualTesting.Tests.CommandCancelTest.<>c__DisplayClass18_0.<TimeOutDuringRead>b__0() in E:\Programming\csharp7\corefx\src\System.Data.SqlClient\tests\ManualTests\SQL\CommandCancelTest\CommandCancelTest.cs:line 262

saurabh500 · 2018-12-18T21:47:47Z

OK. Keep me posted.

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObject.cs

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectFactory.Windows.cs

…kets in implementations define IsValidPacket implementations more stringently with type checks

saurabh500 · 2018-12-19T21:23:58Z

@Wraith2 Just to be sure, is this ready for review again? Can the WIP be removed?

saurabh500 · 2018-12-19T21:25:35Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectManaged.cs

-        internal override bool IsValidPacket(PacketHandle packet) => packet.ManagedPacket?.IsInvalid ?? false;
+        internal override bool IsValidPacket(PacketHandle packet)
+        {
+            return (


Interesting. Why would we run into the this packet type not being ManagedPacketType? Can we solve this problem where it begins? Rather than having the check here about the type of packet being valid?
There should be no reason that IsValidPacket in TdsParserStateObjectManaged should get a native packet.

if you use PacketHandle packet = default' you'll get type 0 which is not managed or native, it's just unknown. The answer is that you should never use default but I can't make the compiler enforce that so this is the next best thing. If you do use default you will get a fault during testing.
It isn't possible to get managed and native mixed up.

In that case, I think this method deserves a Debug.Assert as well just the way you added in TdsParserStateObjectNative.IsVaildPacket

Wraith2 · 2018-12-19T21:57:25Z

Ready to review and test, thanks for the help and patience.

saurabh500 · 2018-12-19T22:54:06Z

@dotnet-bot Test Linux x64 Release Build

Setting status of 4801eb3f327f826e1ebadb8101ed93665dcbd071 to FAILURE with url https://ci3.dot.net/job/dotnet_corefx/job/master/job/linux-TGroup_netcoreapp+CGroup_Release+AGroup_x64+TestOuter_false_prtest/19895/ and message: 'Build finished. '
Using context: Linux x64 Release Build
java.lang.NullPointerException
	at java.util.Collections$UnmodifiableCollection.<init>(Collections.java:1026)
	at java.util.Collections$UnmodifiableList.<init>(Collections.java:1302)
	at java.util.Collections.unmodifiableList(Collections.java:1287)
	at hudson.model.ParametersAction.getAllParameters(ParametersAction.java:352)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.jenkinsci.plugins.workflow.cps.ParamsVariable.getValue(ParamsVariable.java:61)
	at org.jenkinsci.plugins.workflow.cps.CpsScript.getProperty(CpsScript.java:121)
	at org.codehaus.groovy.runtime.InvokerHelper.getProperty(InvokerHelper.java:174)
	at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.getProperty(ScriptBytecodeAdapter.java:456)
	at org.kohsuke.groovy.sandbox.impl.Checker$6.call(Checker.java:284)
	at org.kohsuke.groovy.sandbox.GroovyInterceptor.onGetProperty(GroovyInterceptor.java:68)
	at org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SandboxInterceptor.onGetProperty(SandboxInterceptor.java:326)
	at org.kohsuke.groovy.sandbox.impl.Checker$6.call(Checker.java:282)
	at org.kohsuke.groovy.sandbox.impl.Checker.checkedGetProperty(Checker.java:286)
	at org.kohsuke.groovy.sandbox.impl.Checker.checkedGetProperty(Checker.java:262)
	at com.cloudbees.groovy.cps.sandbox.SandboxInvoker.getProperty(SandboxInvoker.java:29)
	at com.cloudbees.groovy.cps.impl.PropertyAccessBlock.rawGet(PropertyAccessBlock.java:20)
Caused: java.lang.reflect.InvocationTargetException
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.jenkinsci.plugins.workflow.cps.ParamsVariable.getValue(ParamsVariable.java:61)
	at org.jenkinsci.plugins.workflow.cps.CpsScript.getProperty(CpsScript.java:121)
	at org.codehaus.groovy.runtime.InvokerHelper.getProperty(InvokerHelper.java:174)
	at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.getProperty(ScriptBytecodeAdapter.java:456)
	at org.kohsuke.groovy.sandbox.impl.Checker$6.call(Checker.java:284)
	at org.kohsuke.groovy.sandbox.GroovyInterceptor.onGetProperty(GroovyInterceptor.java:68)
	at org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SandboxInterceptor.onGetProperty(SandboxInterceptor.java:326)
	at org.kohsuke.groovy.sandbox.impl.Checker$6.call(Checker.java:282)
	at org.kohsuke.groovy.sandbox.impl.Checker.checkedGetProperty(Checker.java:286)
	at org.kohsuke.groovy.sandbox.impl.Checker.checkedGetProperty(Checker.java:262)
	at com.cloudbees.groovy.cps.sandbox.SandboxInvoker.getProperty(SandboxInvoker.java:29)
	at com.cloudbees.groovy.cps.impl.PropertyAccessBlock.rawGet(PropertyAccessBlock.java:20)
	at WorkflowScript.run(WorkflowScript:17)
	at simpleDockerNode.call(/jenkins/jobs/dotnet_corefx/jobs/master/jobs/linux-TGroup_netcoreapp+CGroup_Release+AGroup_x64+TestOuter_false_prtest/builds/19895/libs/dotnet-ci/vars/simpleDockerNode.groovy:103)
	at org.jenkinsci.plugins.docker.workflow.Docker$Image.inside(jar:file:/jenkins/plugins/docker-workflow/WEB-INF/lib/docker-workflow.jar!/org/jenkinsci/plugins/docker/workflow/Docker.groovy:129)
	at ___cps.transform___(Native Method)
	at com.cloudbees.groovy.cps.impl.PropertyishBlock$ContinuationImpl.get(PropertyishBlock.java:74)
	at com.cloudbees.groovy.cps.LValueBlock$GetAdapter.receive(LValueBlock.java:30)
	at com.cloudbees.groovy.cps.impl.PropertyishBlock$ContinuationImpl.fixName(PropertyishBlock.java:66)
	at sun.reflect.GeneratedMethodAccessor170.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at com.cloudbees.groovy.cps.impl.ContinuationPtr$ContinuationImpl.receive(ContinuationPtr.java:72)
	at com.cloudbees.groovy.cps.impl.ConstantBlock.eval(ConstantBlock.java:21)
	at com.cloudbees.groovy.cps.Next.step(Next.java:83)
	at com.cloudbees.groovy.cps.Continuable$1.call(Continuable.java:174)
	at com.cloudbees.groovy.cps.Continuable$1.call(Continuable.java:163)
	at org.codehaus.groovy.runtime.GroovyCategorySupport$ThreadCategoryInfo.use(GroovyCategorySupport.java:122)
	at org.codehaus.groovy.runtime.GroovyCategorySupport.use(GroovyCategorySupport.java:261)
	at com.cloudbees.groovy.cps.Continuable.run0(Continuable.java:163)
	at org.jenkinsci.plugins.workflow.cps.SandboxContinuable.access$001(SandboxContinuable.java:19)
	at org.jenkinsci.plugins.workflow.cps.SandboxContinuable$1.call(SandboxContinuable.java:35)
	at org.jenkinsci.plugins.workflow.cps.SandboxContinuable$1.call(SandboxContinuable.java:32)
	at org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.GroovySandbox.runInSandbox(GroovySandbox.java:108)
	at org.jenkinsci.plugins.workflow.cps.SandboxContinuable.run0(SandboxContinuable.java:32)
	at org.jenkinsci.plugins.workflow.cps.CpsThread.runNextChunk(CpsThread.java:174)
	at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup.run(CpsThreadGroup.java:330)
	at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup.access$100(CpsThreadGroup.java:82)
	at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup$2.call(CpsThreadGroup.java:242)
	at org.jenkinsci.plugins.workflow.cps.CpsThreadGroup$2.call(CpsThreadGroup.java:230)
	at org.jenkinsci.plugins.workflow.cps.CpsVmExecutorService$2.call(CpsVmExecutorService.java:64)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at hudson.remoting.SingleLaneExecutorService$1.run(SingleLaneExecutorService.java:112)
	at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
Finished: FAILURE

saurabh500 · 2018-12-19T22:54:53Z

@dotnet-bot Test Linux-musl x64 Debug Build
Test Linux arm64 Release Build

saurabh500 · 2018-12-20T18:39:13Z

@dotnet-bot Test this

saurabh500 · 2018-12-20T18:41:10Z

Ready to merge once the CI is green.

saurabh500 · 2018-12-20T19:38:54Z

@dotnet-bot Test Packaging All Configurations x64 Debug Build

D:\j\workspace\windows-TGrou---0d2c9ac4\.dotnet\sdk\2.1.401\Roslyn\Microsoft.CSharp.Core.targets(52,5): error MSB6006: "csc.dll" exited with code -1073741819. [D:\j\workspace\windows-TGrou---0d2c9ac4\src\System.Net.Http\src\System.Net.Http.csproj]

saurabh500 · 2018-12-21T07:02:52Z

@dotnet-bot Test Linux arm Release Build

Wraith2 · 2018-12-21T14:57:38Z

@dotnetbo test Tizen armel Debug Build please

…4044) * change TdsParserStateObject to pass packets using a ref struct to avoid boxing of IntPtr in native mode * add project define for FEATURE_INTEROPSNI on windows non uap builds * update interop to use SniPacketHandle type name * rename SNIPacketHandle to SNIPacket * split PacketHandle and SessionHandle into separate files and implementations * add comments to PacketHandle and SessionHandle remove unused packethandle variable in IsConnectionAlive remove identidal overridden implementations of EmptyReadHandle * move lazy bool into debug region * re-add EmptyReadPackt and provide correctly types valid but empty packets in implementations define IsValidPacket implementations more stringently with type checks * change implementation switch name to make more sense * add packet type assertion in IsValiePacket Commit migrated from dotnet/corefx@d3d8c74

Wraith2 added 2 commits December 12, 2018 21:54

change TdsParserStateObject to pass packets using a ref struct to avo…

ba78a61

…id boxing of IntPtr in native mode

add project define for FEATURE_INTEROPSNI on windows non uap builds

1a8f447

Wraith2 changed the title ~~Sqlperf packetstruct~~ Change SqlClient TdsParserStateObject to remove IntPtr boxing Dec 12, 2018

Wraith2 mentioned this pull request Dec 12, 2018

Change SqlClient to use strongly typed packet and session handles #33155

Closed

Wraith2 changed the title ~~Change SqlClient TdsParserStateObject to remove IntPtr boxing~~ Optimize SqlClient tds state to remove handle boxing Dec 12, 2018

update interop to use SniPacketHandle type name

7ac5553

saurabh500 added the area-System.Data.SqlClient label Dec 13, 2018

saurabh500 reviewed Dec 13, 2018

View reviewed changes

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserSafeHandles.cs Outdated Show resolved Hide resolved

Wraith2 added 2 commits December 14, 2018 01:19

rename SNIPacketHandle to SNIPacket

1d6235d

fix merge conflicts

f98c9e5

saurabh500 reviewed Dec 17, 2018

View reviewed changes

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObject.cs Outdated Show resolved Hide resolved

Wraith2 added 2 commits December 17, 2018 20:32

fix merge conflict

76b688e

split PacketHandle and SessionHandle into separate files and implemen…

1e1f540

…tations

saurabh500 reviewed Dec 18, 2018

View reviewed changes

Wraith2 added 2 commits December 18, 2018 20:52

add comments to PacketHandle and SessionHandle

69a8337

remove unused packethandle variable in IsConnectionAlive remove identidal overridden implementations of EmptyReadHandle

move lazy bool into debug region

6b1900c

saurabh500 reviewed Dec 18, 2018

View reviewed changes

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObject.cs Outdated Show resolved Hide resolved

saurabh500 reviewed Dec 18, 2018

View reviewed changes

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectFactory.Windows.cs Outdated Show resolved Hide resolved

Wraith2 changed the title ~~Optimize SqlClient tds state to remove handle boxing~~ WIP Optimize SqlClient tds state to remove handle boxing Dec 19, 2018

Wraith2 changed the title ~~WIP Optimize SqlClient tds state to remove handle boxing~~ [WIP] Optimize SqlClient tds state to remove handle boxing Dec 19, 2018

Wraith2 added 2 commits December 19, 2018 21:10

re-add EmptyReadPackt and provide correctly types valid but empty pac…

dc24084

…kets in implementations define IsValidPacket implementations more stringently with type checks

change implementation switch name to make more sense

4801eb3

saurabh500 reviewed Dec 19, 2018

View reviewed changes

Wraith2 changed the title ~~[WIP] Optimize SqlClient tds state to remove handle boxing~~ Optimize SqlClient tds state to remove handle boxing Dec 19, 2018

add packet type assertion in IsValiePacket

a76208f

karelz assigned Wraith2, keeratsingh and AfsanehR-zz Dec 19, 2018

saurabh500 merged commit d3d8c74 into dotnet:master Dec 21, 2018

Wraith2 deleted the sqlperf-packetstruct branch December 21, 2018 16:19

karelz added this to the 3.0 milestone Dec 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize SqlClient tds state to remove handle boxing #34044

Optimize SqlClient tds state to remove handle boxing #34044

Wraith2 commented Dec 12, 2018 •

edited

Loading

Wraith2 commented Dec 14, 2018

Wraith2 commented Dec 18, 2018

saurabh500 commented Dec 18, 2018

Wraith2 commented Dec 18, 2018

Wraith2 commented Dec 18, 2018

saurabh500 commented Dec 18, 2018

Wraith2 commented Dec 18, 2018

saurabh500 commented Dec 18, 2018

saurabh500 commented Dec 19, 2018

saurabh500 Dec 19, 2018

Wraith2 Dec 19, 2018

saurabh500 Dec 19, 2018

Wraith2 Dec 19, 2018

Wraith2 commented Dec 19, 2018

saurabh500 commented Dec 19, 2018

saurabh500 commented Dec 19, 2018

saurabh500 commented Dec 20, 2018

saurabh500 commented Dec 20, 2018

saurabh500 commented Dec 20, 2018

saurabh500 commented Dec 21, 2018

Wraith2 commented Dec 21, 2018

Optimize SqlClient tds state to remove handle boxing #34044

Optimize SqlClient tds state to remove handle boxing #34044

Conversation

Wraith2 commented Dec 12, 2018 • edited Loading

Wraith2 commented Dec 14, 2018

Wraith2 commented Dec 18, 2018

saurabh500 commented Dec 18, 2018

Wraith2 commented Dec 18, 2018

Wraith2 commented Dec 18, 2018

saurabh500 commented Dec 18, 2018

Wraith2 commented Dec 18, 2018

saurabh500 commented Dec 18, 2018

saurabh500 commented Dec 19, 2018

saurabh500 Dec 19, 2018

Choose a reason for hiding this comment

Wraith2 Dec 19, 2018

Choose a reason for hiding this comment

saurabh500 Dec 19, 2018

Choose a reason for hiding this comment

Wraith2 Dec 19, 2018

Choose a reason for hiding this comment

Wraith2 commented Dec 19, 2018

saurabh500 commented Dec 19, 2018

saurabh500 commented Dec 19, 2018

saurabh500 commented Dec 20, 2018

saurabh500 commented Dec 20, 2018

saurabh500 commented Dec 20, 2018

saurabh500 commented Dec 21, 2018

Wraith2 commented Dec 21, 2018

Wraith2 commented Dec 12, 2018 •

edited

Loading