SNIPacket memory allocation perf improvement (revised) #27758

geleems · 2018-03-06T02:13:44Z

Previous version of SNIPacket change (#27187) contains couple of issues, and caused several test failures (https://github.com/dotnet/corefx/issues/27587, https://github.com/dotnet/corefx/issues/27574).
One of the failures caused by missing locks, and other ones were caused by unreleased byte array on SNIPacket.Release(). This revised fix excludes dealing with locks, and also properly deallocates byte array when SNIPacket.Release() is called.

corivera · 2018-03-06T02:18:53Z

There are a couple of changes from #27743 that might conflict with these changes.

Hold off on this until that gets merged in.

geleems · 2018-03-06T02:27:36Z

All Manual Tests including System.Data.SqlClient.ManualTesting.Tests.CommandCancelTest were passed in Release mode in Linux. (TvpTest was skipped since it is failed in master)

stephentoub · 2018-03-06T03:11:37Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.cs

        {
-            _capacity = capacity;
-            _data = new Byte[capacity];
+            if (_data == null || _data.Length != bufferSize)


I asked this last time, but I don't remember the answer. Why can't this be _data.Length < bufferSize?

There were 2 reasons:

I changed to _data.Length < bufferSize, and faced several test failures during network operation. It seemed they needed exact packet length as a parameter the tests handed over to Allocate() method. So I rolled back to original implementation.

The packet size is initially 4,096 for login process, and then becomes 8,000 constantly until connection is closed. So, in this situation, there is not much benefit for changing to _data.Length < bufferSize.

stephentoub · 2018-03-06T03:13:14Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.cs

@@ -217,9 +211,8 @@ public int TakeData(byte[] buffer, int dataOffset, int size)
        /// </summary>
        public void Release()
        {
-            _length = 0;
-            _capacity = 0;
            _data = null;


What is the purpose of Release? If the object is just going to get dropped, there shouldn't be a need to null out the byte[], and if it's going to get reused, it'd be nice to be able to reuse the same byte[] if it's large enough. Is the issue that someone else still holds a reference to _data and we don't want to reuse it, or something like that?

That was exactly what I thought. But it turned out the Manual Test https://github.com/dotnet/corefx/issues/27574 were failed due to non-nulled out byte[] after Release() method called. Release() is now only used for disposing SNIPacket object when an error occurred.

stephentoub · 2018-03-06T03:18:15Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNITcpHandle.cs

                }
-            });
+            );


This should pass , CancellationToken.None, TaskContinuationOptions.DenyChildAttach, TaskScheduler.Default, and ideally it would also pass ExecuteSynchronously |'d into the continuation options, and ideally pass the necessary state in as the object state rather than closing over it. But all of that goes away when this is changed to use await.

saurabh500

I ran these changes through the tests in Manual Tests with connection string Encrypt=True;TrustServerCertificate=True for one of our test servers and I see that the tests hang with this change. If I use Encrypt=True and TrustServerCertificate=True with EF tests, I see that the tests hang there as well.
Please test encryption.
Please also test MARS with encryption

saurabh500 · 2018-03-09T20:52:00Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.cs

@@ -298,6 +343,11 @@ public void WriteToStream(Stream stream)
            stream.Write(_data, 0, _length);
        }

+        public Task WriteToStreamAsync(Stream stream)


This seems out of sync with master. Please sync it

saurabh500 · 2018-03-09T20:54:17Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.cs

@@ -67,6 +72,14 @@ public int Length
            }
        }

+        public int Capacity


Nit: public int Capacity => _capacity;

saurabh500 · 2018-03-09T22:19:48Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.cs

+            _isBufferFromArrayPool = isArrayFromArrayPool;
+        }
+
+        public void SetData(byte[] data, int length, bool isArrayFromArrayPool)


This seems like an unused method.

saurabh500 · 2018-03-09T22:20:25Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.cs

@@ -146,12 +176,18 @@ public void GetData(byte[] buffer, ref int dataSize)
        /// </summary>
        /// <param name="data">Data</param>
        /// <param name="length">Length</param>
-        public void SetData(byte[] data, int length)
+        public void SetData(byte[] data, int length, int capaticy = -1, bool isArrayFromArrayPool = false)


Type: int capacity = -1

The SetData below, which is unused seems like the only method calling this overload with isArrayFromArrayPool. If the method below is unused, the method parameter isArrayFromArrayPool can be removed.

Also the parameter int capacity can be removed since no other method passes in these parameters

saurabh500 · 2018-03-09T22:23:58Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.cs

        {
-            _offset = 0;
+            _capacity = -1;


Why can't this be 0 instead of -1 ?

capacity = 0 means _data = new byte[0];
capacity = -1 means _data = null;

It doesnt make sense to overload the value of negative to signify what _data looks like. Can't you simply check if _data = null instead of relying on a negative value of capacity?

saurabh500 · 2018-03-09T22:29:32Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.cs

@@ -240,11 +240,11 @@ public uint WritePacket(SNIHandle handle, SNIPacket packet, bool sync)
        {
            if (sync)
            {
-                return handle.Send(packet.Clone());
+                return handle.Send(packet);


This doesn't seem right. Earlier, we were setting the _outBuff into the SNIPacket and then sending a Clone of the SNI packet over the network. Now we are sending the reference to _outBuff over the wire.
This can cause issues if _outBuff in TdsParserStateObject is changed and the data transmission is still in progress.

if _outBuff in TdsParserStateObject is changed and the data transmission is still in progress.

Can that situation actually happen?
If so, handle.Send(packet.Clone()); should be atomic.

Why should it be atomic? There is a buffer that State Object operates on, and when that buffer is sent to network, it should be dissociated, since the state object can continue to modify it. I am not talking about multiple threads calling this method, but data modifications at multiple locations.

saurabh500 · 2018-03-09T23:19:36Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNITcpHandle.cs

@@ -533,6 +533,7 @@ public override uint SendAsync(SNIPacket packet, SNIAsyncCallback callback = nul
            }
            return TdsEnums.SNI_SUCCESS_IO_PENDING;
        }
+


Nit: Extra spaces.

saurabh500 · 2018-03-09T23:19:53Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNITcpHandle.cs

-            packet.Allocate(_bufferSize);
-
+            packet = new SNIPacket(_bufferSize);
+


Nit: Extra spaces

saurabh500 · 2018-03-09T23:41:52Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.cs

@@ -238,14 +238,18 @@ public uint GetConnectionId(SNIHandle handle, ref Guid clientConnectionId)
        /// <returns>SNI error status</returns>
        public uint WritePacket(SNIHandle handle, SNIPacket packet, bool sync)
        {
+            SNIPacket clonedPacket = packet.Clone();


This can be changed to

using(SNIPacket clonedPacket = packet.Clone()) { // send logic }

This way even if an exception occurs in Send or SendAsync will result in clonedPacket being disposed

On Second thoughts, should the Cloned packet be disposed here? SendAsync will return immideately, which means that the I/O may not complete and the Dispose will set the _data to null and allow it to be GCed before I/O completes.

saurabh500 · 2018-03-09T23:44:39Z

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

-            }
-            finally
-            {
+            sniError = WritePacket(packet, sync);


Why remove try{} finally {} ?

saurabh500 · 2018-03-11T08:09:33Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.cs

@@ -109,11 +91,27 @@ public void InvokeCompletionCallback(uint sniErrorCode)
        /// <summary>
        /// Allocate space for data
        /// </summary>
-        /// <param name="capacity">Bytes to allocate</param>
+        /// <param name="bufferSize">Length of byte array to be allocated</param>


buffersize from comment doesn't exist
It's capacity

saurabh500 · 2018-03-13T06:46:22Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.cs

            }
            else
            {
-                return handle.SendAsync(packet.Clone());
+                clonedPacket.DisposeAfterWriteAsync = true;


Instead of maintaining another state inside SNIPacket, wouldn't it be better to tell the SNIHandle to Dispose the packet after SendAsync completes? You could put an optional parameter in handle.SendAsync(SNIPacket packet, bool disposePacket) and it can be set to true from this call. This way the packet doesn't need to maintain a state.

@saurabh500
I am not very clear about what you meant.

corefx/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNITcpHandle.cs

Lines 527 to 535 in 68cfdde

public override uint SendAsync(SNIPacket packet, SNIAsyncCallback callback = null)

{

SNIAsyncCallback cb = callback ?? _sendCallback;

lock(this)

{

packet.WriteToStreamAsync(_stream, cb, SNIProviders.TCP_PROV);

}

return TdsEnums.SNI_SUCCESS_IO_PENDING;

}

Line#532 runs asynchronously, so I cannot put packet.Dispose() after the line.
Otherwise, packet will be disposed immediately after calling WriteToStreamAsync().
Dispose() should happen in the async context, not the main context.
I could not find anywhere for Dispose() other than inside of SNIPacket.WriteToStreamAsync()
Which code spot did you mean exactly for putting Dispose()?

tell the SNIHandle to Dispose the packet after SendAsync completes

Did you mean main thread waits at line#532 until packet.WriteToStreamAsync() is finished, and then begins to dispose the packet?

I was saying, pass the disposePacket=true from SNIHandle.SendAsync -> packet.writeToStreamAsync() so that that writeToStreamAsync can dispose the packet in continuation when this parameter is set to true.

saurabh500 · 2018-03-13T06:47:27Z

@keeratsingh Can you run this thru the Sql Tests ?

keeratsingh · 2018-03-13T19:38:50Z

@saurabh500
Code changes look good from SQLTests perspective.
Tests Run:
System.Data.SqlClient\tests\FunctionalTests
System.Data.SqlClient\tests\ManualTests

geleems · 2018-03-14T00:36:26Z

This is how MARS packet is sent.

SNIProxy.WritePacket() <== MARS SNIPacket is cloned here
--> SNIHandle.SendAsync() 
--> SNIMarsHandle.SendAsync()
--> SNIMarsHandle.SendPeningPackets()
--> SNIMarsHandle.InternalSendAsync()
--> SNIMarsConnection.SendAsync()
--> SNITcpHandle.SendAsync()

saurabh500 · 2018-03-14T02:12:55Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNINpHandle.cs

@@ -213,7 +211,7 @@ public override uint Send(SNIPacket packet)
            }
        }

-        public override uint SendAsync(SNIPacket packet, SNIAsyncCallback callback = null)
+        public override uint SendAsync(SNIPacket packet, bool disposePacketAfterSendAsync, SNIAsyncCallback callback = null)
        {
            SNIAsyncCallback cb = callback ?? _sendCallback;
            packet.WriteToStreamAsync(_stream, cb, SNIProviders.NP_PROV);


Pass the disposePacketAfterSendAsync to the packet.WriteToStreamAsync(... ) in SNINpHandle.cs

saurabh500

Approved after the SNINPHandle.cs modification.

saurabh500 · 2018-03-14T02:13:51Z

@geleems please make sure that we get a test pass for EF Core with this change for the test matrix that we have been running.

…#27758) Commit migrated from dotnet/corefx@1de2b37

geleems requested review from saurabh500 and corivera March 6, 2018 02:13

geleems self-assigned this Mar 6, 2018

stephentoub reviewed Mar 6, 2018

View reviewed changes

geleems added the area-System.Data.SqlClient label Mar 6, 2018

saurabh500 suggested changes Mar 6, 2018

View reviewed changes

geleems force-pushed the newpub1 branch from 87eac77 to 359e81b Compare March 8, 2018 00:13

saurabh500 changed the title ~~SNIPacket memeory allocation perf improvement (revised)~~ SNIPacket memory allocation perf improvement (revised) Mar 9, 2018

saurabh500 reviewed Mar 9, 2018

View reviewed changes

geleems force-pushed the newpub1 branch from 4c1d387 to b3aa65b Compare March 9, 2018 21:50

saurabh500 suggested changes Mar 9, 2018

View reviewed changes

saurabh500 reviewed Mar 9, 2018

View reviewed changes

geleems force-pushed the newpub1 branch 2 times, most recently from 05649b3 to 29c881e Compare March 11, 2018 09:41

SNIPacket optimization with ArrayPool

48a3d58

geleems force-pushed the newpub1 branch from 5a91dcf to 48a3d58 Compare March 12, 2018 21:12

minor fix

093e170

saurabh500 reviewed Mar 13, 2018

View reviewed changes

fix as comment

e6e4d9a

fix as comment

1da4b0f

geleems force-pushed the newpub1 branch from 0c17197 to 1da4b0f Compare March 14, 2018 01:01

fix as comment

4dd9d48

saurabh500 reviewed Mar 14, 2018

View reviewed changes

saurabh500 approved these changes Mar 14, 2018

View reviewed changes

fix as comment

f562831

geleems merged commit 1de2b37 into dotnet:master Mar 14, 2018

karelz added this to the 2.1.0 milestone Mar 18, 2018

ericstj pushed a commit to ericstj/corefx that referenced this pull request Mar 28, 2018

SNIPacket memory allocation perf improvement (revised) (dotnet#27758)

8ee335c

Ruikuan mentioned this pull request Nov 28, 2018

.Net Core 2.1 性能改进 Ruikuan/blog#26

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

SNIPacket memory allocation perf improvement (revised) (dotnet/corefx…

120c15d

…#27758) Commit migrated from dotnet/corefx@1de2b37

		packet.Allocate(_bufferSize);

		packet = new SNIPacket(_bufferSize);

	public override uint SendAsync(SNIPacket packet, SNIAsyncCallback callback = null)
	{
	SNIAsyncCallback cb = callback ?? _sendCallback;
	lock(this)
	{
	packet.WriteToStreamAsync(_stream, cb, SNIProviders.TCP_PROV);
	}
	return TdsEnums.SNI_SUCCESS_IO_PENDING;
	}

SNIPacket memory allocation perf improvement (revised) #27758

SNIPacket memory allocation perf improvement (revised) #27758

Conversation

geleems commented Mar 6, 2018 • edited Loading

corivera commented Mar 6, 2018

geleems commented Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saurabh500 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geleems Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saurabh500 commented Mar 13, 2018

keeratsingh commented Mar 13, 2018

geleems commented Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

saurabh500 left a comment

Choose a reason for hiding this comment

saurabh500 commented Mar 14, 2018

geleems commented Mar 6, 2018 •

edited

Loading

geleems commented Mar 6, 2018 •

edited

Loading

stephentoub Mar 6, 2018 •

edited

Loading

saurabh500 left a comment •

edited

Loading

geleems Mar 13, 2018 •

edited

Loading

geleems commented Mar 14, 2018 •

edited

Loading