From d35c70a8d459e2915d561b68f3f537b7ccf9d8d4 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Mon, 17 Dec 2018 13:22:38 +0000 Subject: [PATCH 1/5] Optimize SNIPacket async paths --- .../System/Data/SqlClient/SNI/SNIPacket.cs | 100 ++++++++++++------ 1 file changed, 69 insertions(+), 31 deletions(-) diff --git a/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.cs b/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.cs index f7ba249b066f..63420ce4cb63 100644 --- a/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.cs +++ b/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.cs @@ -23,6 +23,7 @@ internal class SNIPacket : IDisposable, IEquatable private ArrayPool _arrayPool = ArrayPool.Shared; private bool _isBufferFromArrayPool = false; + private ValueTask _asyncOperation; public SNIPacket() { } @@ -238,6 +239,7 @@ public void Reset() _offset = 0; _description = null; _completionCallback = null; + _asyncOperation = default; } /// @@ -247,37 +249,50 @@ public void Reset() /// Completion callback public void ReadFromStreamAsync(Stream stream, SNIAsyncCallback callback) { - bool error = false; - - stream.ReadAsync(_data, 0, _capacity, CancellationToken.None).ContinueWith(t => + // Treat local function as a static and pass all params otherwise as async will allocate + async ValueTask ReadFromStreamAsync(SNIPacket packet, SNIAsyncCallback cb, ValueTask valueTask) { - Exception e = t.Exception?.InnerException; - if (e != null) + bool error = false; + try { - SNILoadHandle.SingletonInstance.LastError = new SNIError(SNIProviders.TCP_PROV, SNICommon.InternalExceptionError, e); - error = true; - } - else - { - _length = t.Result; - - if (_length == 0) + packet._length = await valueTask.ConfigureAwait(false); + if (packet._length == 0) { SNILoadHandle.SingletonInstance.LastError = new SNIError(SNIProviders.TCP_PROV, 0, SNICommon.ConnTerminatedError, string.Empty); error = true; } } + catch (Exception ex) + { + SNILoadHandle.SingletonInstance.LastError = new SNIError(SNIProviders.TCP_PROV, SNICommon.InternalExceptionError, ex); + error = true; + } if (error) { - Release(); + packet.Release(); + } + + cb(packet, error ? TdsEnums.SNI_ERROR : TdsEnums.SNI_SUCCESS); + } + + ValueTask vt = stream.ReadAsync(new Memory(_data, 0, _capacity), CancellationToken.None); + + if (vt.IsCompletedSuccessfully) + { + _length = vt.Result; + // Zero length to go via async local function as is error condition + if (_length > 0) + { + callback(this, TdsEnums.SNI_SUCCESS); + + // Completed + return; } + } - callback(this, error ? TdsEnums.SNI_ERROR : TdsEnums.SNI_SUCCESS); - }, - CancellationToken.None, - TaskContinuationOptions.DenyChildAttach, - TaskScheduler.Default); + // Not complete or error call the async local function to complete + _asyncOperation = ReadFromStreamAsync(this, callback, vt); } /// @@ -302,24 +317,47 @@ public void WriteToStream(Stream stream) /// Write data to a stream asynchronously /// /// Stream to write to - public async void WriteToStreamAsync(Stream stream, SNIAsyncCallback callback, SNIProviders provider, bool disposeAfterWriteAsync = false) + public void WriteToStreamAsync(Stream stream, SNIAsyncCallback callback, SNIProviders provider, bool disposeAfterWriteAsync = false) { - uint status = TdsEnums.SNI_SUCCESS; - try + // Treat local function as a static and pass all params otherwise as async will allocate + async ValueTask WriteToStreamAsync(SNIPacket packet, SNIAsyncCallback cb, SNIProviders providers, bool disposeAfter, ValueTask valueTask) { - await stream.WriteAsync(_data, 0, _length, CancellationToken.None).ConfigureAwait(false); - } - catch (Exception e) - { - SNILoadHandle.SingletonInstance.LastError = new SNIError(provider, SNICommon.InternalExceptionError, e); - status = TdsEnums.SNI_ERROR; + uint status = TdsEnums.SNI_SUCCESS; + try + { + await valueTask.ConfigureAwait(false); + } + catch (Exception e) + { + SNILoadHandle.SingletonInstance.LastError = new SNIError(providers, SNICommon.InternalExceptionError, e); + status = TdsEnums.SNI_ERROR; + } + + cb(packet, status); + + if (disposeAfter) + { + packet.Dispose(); + } } - callback(this, status); - if (disposeAfterWriteAsync) + ValueTask vt = stream.WriteAsync(new Memory(_data, 0, _length), CancellationToken.None); + + if (vt.IsCompletedSuccessfully) { - Dispose(); + callback(this, TdsEnums.SNI_SUCCESS); + + if (disposeAfterWriteAsync) + { + Dispose(); + } + + // Completed + return; } + + // Not complete or error call the async local function to complete + _asyncOperation = WriteToStreamAsync(this, callback, provider, disposeAfterWriteAsync, vt); } /// From 9adbdcdefa042dbad7fea825ddb914cbcbae0346 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Thu, 20 Dec 2018 18:39:37 +0000 Subject: [PATCH 2/5] Feedback --- .../src/System/Data/SqlClient/SNI/SNIPacket.cs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.cs b/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.cs index 63420ce4cb63..60c361d2f103 100644 --- a/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.cs +++ b/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.cs @@ -23,7 +23,6 @@ internal class SNIPacket : IDisposable, IEquatable private ArrayPool _arrayPool = ArrayPool.Shared; private bool _isBufferFromArrayPool = false; - private ValueTask _asyncOperation; public SNIPacket() { } @@ -239,7 +238,6 @@ public void Reset() _offset = 0; _description = null; _completionCallback = null; - _asyncOperation = default; } /// @@ -250,7 +248,7 @@ public void Reset() public void ReadFromStreamAsync(Stream stream, SNIAsyncCallback callback) { // Treat local function as a static and pass all params otherwise as async will allocate - async ValueTask ReadFromStreamAsync(SNIPacket packet, SNIAsyncCallback cb, ValueTask valueTask) + async Task ReadFromStreamAsync(SNIPacket packet, SNIAsyncCallback cb, ValueTask valueTask) { bool error = false; try @@ -292,7 +290,7 @@ async ValueTask ReadFromStreamAsync(SNIPacket packet, SNIAsyncCallback cb, Value } // Not complete or error call the async local function to complete - _asyncOperation = ReadFromStreamAsync(this, callback, vt); + _ = ReadFromStreamAsync(this, callback, vt); } /// @@ -320,7 +318,7 @@ public void WriteToStream(Stream stream) public void WriteToStreamAsync(Stream stream, SNIAsyncCallback callback, SNIProviders provider, bool disposeAfterWriteAsync = false) { // Treat local function as a static and pass all params otherwise as async will allocate - async ValueTask WriteToStreamAsync(SNIPacket packet, SNIAsyncCallback cb, SNIProviders providers, bool disposeAfter, ValueTask valueTask) + async Task WriteToStreamAsync(SNIPacket packet, SNIAsyncCallback cb, SNIProviders providers, bool disposeAfter, ValueTask valueTask) { uint status = TdsEnums.SNI_SUCCESS; try @@ -345,6 +343,9 @@ async ValueTask WriteToStreamAsync(SNIPacket packet, SNIAsyncCallback cb, SNIPro if (vt.IsCompletedSuccessfully) { + // Read the result to register as complete for the ValueTask + vt.GetAwaiter().GetResult(); + callback(this, TdsEnums.SNI_SUCCESS); if (disposeAfterWriteAsync) @@ -357,7 +358,7 @@ async ValueTask WriteToStreamAsync(SNIPacket packet, SNIAsyncCallback cb, SNIPro } // Not complete or error call the async local function to complete - _asyncOperation = WriteToStreamAsync(this, callback, provider, disposeAfterWriteAsync, vt); + _ = WriteToStreamAsync(this, callback, provider, disposeAfterWriteAsync, vt); } /// From ceee9a78211b1a7e8acf54b24e51097115a66af2 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Thu, 20 Dec 2018 19:59:04 +0000 Subject: [PATCH 3/5] NET Core vs NET Std --- .../src/System.Data.SqlClient.csproj | 6 + .../SqlClient/SNI/SNIPacket.NetCoreApp.cs | 117 ++++++++++++++++++ .../SqlClient/SNI/SNIPacket.NetStandard.cs | 117 ++++++++++++++++++ .../System/Data/SqlClient/SNI/SNIPacket.cs | 105 +--------------- 4 files changed, 241 insertions(+), 104 deletions(-) create mode 100644 src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.NetCoreApp.cs create mode 100644 src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.NetStandard.cs diff --git a/src/System.Data.SqlClient/src/System.Data.SqlClient.csproj b/src/System.Data.SqlClient/src/System.Data.SqlClient.csproj index c60ca96e13c2..858bb76a8e7d 100644 --- a/src/System.Data.SqlClient/src/System.Data.SqlClient.csproj +++ b/src/System.Data.SqlClient/src/System.Data.SqlClient.csproj @@ -265,6 +265,12 @@ + + + + + + diff --git a/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.NetCoreApp.cs b/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.NetCoreApp.cs new file mode 100644 index 000000000000..6e5cab47e390 --- /dev/null +++ b/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.NetCoreApp.cs @@ -0,0 +1,117 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Buffers; +using System.IO; +using System.Threading; +using System.Threading.Tasks; + +namespace System.Data.SqlClient.SNI +{ + internal partial class SNIPacket + { + /// + /// Read data from a stream asynchronously + /// + /// Stream to read from + /// Completion callback + public void ReadFromStreamAsync(Stream stream, SNIAsyncCallback callback) + { + // Treat local function as a static and pass all params otherwise as async will allocate + async Task ReadFromStreamAsync(SNIPacket packet, SNIAsyncCallback cb, ValueTask valueTask) + { + bool error = false; + try + { + packet._length = await valueTask.ConfigureAwait(false); + if (packet._length == 0) + { + SNILoadHandle.SingletonInstance.LastError = new SNIError(SNIProviders.TCP_PROV, 0, SNICommon.ConnTerminatedError, string.Empty); + error = true; + } + } + catch (Exception ex) + { + SNILoadHandle.SingletonInstance.LastError = new SNIError(SNIProviders.TCP_PROV, SNICommon.InternalExceptionError, ex); + error = true; + } + + if (error) + { + packet.Release(); + } + + cb(packet, error ? TdsEnums.SNI_ERROR : TdsEnums.SNI_SUCCESS); + } + + ValueTask vt = stream.ReadAsync(new Memory(_data, 0, _capacity), CancellationToken.None); + + if (vt.IsCompletedSuccessfully) + { + _length = vt.Result; + // Zero length to go via async local function as is error condition + if (_length > 0) + { + callback(this, TdsEnums.SNI_SUCCESS); + + // Completed + return; + } + } + + // Not complete or error call the async local function to complete + _ = ReadFromStreamAsync(this, callback, vt); + } + + /// + /// Write data to a stream asynchronously + /// + /// Stream to write to + public void WriteToStreamAsync(Stream stream, SNIAsyncCallback callback, SNIProviders provider, bool disposeAfterWriteAsync = false) + { + // Treat local function as a static and pass all params otherwise as async will allocate + async Task WriteToStreamAsync(SNIPacket packet, SNIAsyncCallback cb, SNIProviders providers, bool disposeAfter, ValueTask valueTask) + { + uint status = TdsEnums.SNI_SUCCESS; + try + { + await valueTask.ConfigureAwait(false); + } + catch (Exception e) + { + SNILoadHandle.SingletonInstance.LastError = new SNIError(providers, SNICommon.InternalExceptionError, e); + status = TdsEnums.SNI_ERROR; + } + + cb(packet, status); + + if (disposeAfter) + { + packet.Dispose(); + } + } + + ValueTask vt = stream.WriteAsync(new Memory(_data, 0, _length), CancellationToken.None); + + if (vt.IsCompletedSuccessfully) + { + // Read the result to register as complete for the ValueTask + vt.GetAwaiter().GetResult(); + + callback(this, TdsEnums.SNI_SUCCESS); + + if (disposeAfterWriteAsync) + { + Dispose(); + } + + // Completed + return; + } + + // Not complete or error call the async local function to complete + _ = WriteToStreamAsync(this, callback, provider, disposeAfterWriteAsync, vt); + } + } +} diff --git a/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.NetStandard.cs b/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.NetStandard.cs new file mode 100644 index 000000000000..bfa48ac17b61 --- /dev/null +++ b/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.NetStandard.cs @@ -0,0 +1,117 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Buffers; +using System.IO; +using System.Threading; +using System.Threading.Tasks; + +namespace System.Data.SqlClient.SNI +{ + internal partial class SNIPacket + { + /// + /// Read data from a stream asynchronously + /// + /// Stream to read from + /// Completion callback + public void ReadFromStreamAsync(Stream stream, SNIAsyncCallback callback) + { + // Treat local function as a static and pass all params otherwise as async will allocate + async Task ReadFromStreamAsync(SNIPacket packet, SNIAsyncCallback cb, Task task) + { + bool error = false; + try + { + packet._length = await task.ConfigureAwait(false); + if (packet._length == 0) + { + SNILoadHandle.SingletonInstance.LastError = new SNIError(SNIProviders.TCP_PROV, 0, SNICommon.ConnTerminatedError, string.Empty); + error = true; + } + } + catch (Exception ex) + { + SNILoadHandle.SingletonInstance.LastError = new SNIError(SNIProviders.TCP_PROV, SNICommon.InternalExceptionError, ex); + error = true; + } + + if (error) + { + packet.Release(); + } + + cb(packet, error ? TdsEnums.SNI_ERROR : TdsEnums.SNI_SUCCESS); + } + + Task t = stream.ReadAsync(_data, 0, _capacity, CancellationToken.None); + + if ((t.Status & TaskStatus.RanToCompletion) != 0) + { + _length = t.Result; + // Zero length to go via async local function as is error condition + if (_length > 0) + { + callback(this, TdsEnums.SNI_SUCCESS); + + // Completed + return; + } + } + + // Not complete or error call the async local function to complete + _ = ReadFromStreamAsync(this, callback, t); + } + + /// + /// Write data to a stream asynchronously + /// + /// Stream to write to + public void WriteToStreamAsync(Stream stream, SNIAsyncCallback callback, SNIProviders provider, bool disposeAfterWriteAsync = false) + { + // Treat local function as a static and pass all params otherwise as async will allocate + async Task WriteToStreamAsync(SNIPacket packet, SNIAsyncCallback cb, SNIProviders providers, bool disposeAfter, Task task) + { + uint status = TdsEnums.SNI_SUCCESS; + try + { + await task.ConfigureAwait(false); + } + catch (Exception e) + { + SNILoadHandle.SingletonInstance.LastError = new SNIError(providers, SNICommon.InternalExceptionError, e); + status = TdsEnums.SNI_ERROR; + } + + cb(packet, status); + + if (disposeAfter) + { + packet.Dispose(); + } + } + + Task t = stream.WriteAsync(_data, 0, _length, CancellationToken.None); + + if ((t.Status & TaskStatus.RanToCompletion) != 0) + { + // Read the result to register as complete for the Task + t.GetAwaiter().GetResult(); + + callback(this, TdsEnums.SNI_SUCCESS); + + if (disposeAfterWriteAsync) + { + Dispose(); + } + + // Completed + return; + } + + // Not complete or error call the async local function to complete + _ = WriteToStreamAsync(this, callback, provider, disposeAfterWriteAsync, t); + } + } +} diff --git a/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.cs b/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.cs index 60c361d2f103..931d064b4cef 100644 --- a/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.cs +++ b/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIPacket.cs @@ -12,7 +12,7 @@ namespace System.Data.SqlClient.SNI /// /// SNI Packet /// - internal class SNIPacket : IDisposable, IEquatable + internal partial class SNIPacket : IDisposable, IEquatable { private byte[] _data; private int _length; @@ -240,59 +240,6 @@ public void Reset() _completionCallback = null; } - /// - /// Read data from a stream asynchronously - /// - /// Stream to read from - /// Completion callback - public void ReadFromStreamAsync(Stream stream, SNIAsyncCallback callback) - { - // Treat local function as a static and pass all params otherwise as async will allocate - async Task ReadFromStreamAsync(SNIPacket packet, SNIAsyncCallback cb, ValueTask valueTask) - { - bool error = false; - try - { - packet._length = await valueTask.ConfigureAwait(false); - if (packet._length == 0) - { - SNILoadHandle.SingletonInstance.LastError = new SNIError(SNIProviders.TCP_PROV, 0, SNICommon.ConnTerminatedError, string.Empty); - error = true; - } - } - catch (Exception ex) - { - SNILoadHandle.SingletonInstance.LastError = new SNIError(SNIProviders.TCP_PROV, SNICommon.InternalExceptionError, ex); - error = true; - } - - if (error) - { - packet.Release(); - } - - cb(packet, error ? TdsEnums.SNI_ERROR : TdsEnums.SNI_SUCCESS); - } - - ValueTask vt = stream.ReadAsync(new Memory(_data, 0, _capacity), CancellationToken.None); - - if (vt.IsCompletedSuccessfully) - { - _length = vt.Result; - // Zero length to go via async local function as is error condition - if (_length > 0) - { - callback(this, TdsEnums.SNI_SUCCESS); - - // Completed - return; - } - } - - // Not complete or error call the async local function to complete - _ = ReadFromStreamAsync(this, callback, vt); - } - /// /// Read data from a stream synchronously /// @@ -311,56 +258,6 @@ public void WriteToStream(Stream stream) stream.Write(_data, 0, _length); } - /// - /// Write data to a stream asynchronously - /// - /// Stream to write to - public void WriteToStreamAsync(Stream stream, SNIAsyncCallback callback, SNIProviders provider, bool disposeAfterWriteAsync = false) - { - // Treat local function as a static and pass all params otherwise as async will allocate - async Task WriteToStreamAsync(SNIPacket packet, SNIAsyncCallback cb, SNIProviders providers, bool disposeAfter, ValueTask valueTask) - { - uint status = TdsEnums.SNI_SUCCESS; - try - { - await valueTask.ConfigureAwait(false); - } - catch (Exception e) - { - SNILoadHandle.SingletonInstance.LastError = new SNIError(providers, SNICommon.InternalExceptionError, e); - status = TdsEnums.SNI_ERROR; - } - - cb(packet, status); - - if (disposeAfter) - { - packet.Dispose(); - } - } - - ValueTask vt = stream.WriteAsync(new Memory(_data, 0, _length), CancellationToken.None); - - if (vt.IsCompletedSuccessfully) - { - // Read the result to register as complete for the ValueTask - vt.GetAwaiter().GetResult(); - - callback(this, TdsEnums.SNI_SUCCESS); - - if (disposeAfterWriteAsync) - { - Dispose(); - } - - // Completed - return; - } - - // Not complete or error call the async local function to complete - _ = WriteToStreamAsync(this, callback, provider, disposeAfterWriteAsync, vt); - } - /// /// Get hash code /// From 92ade0402a3cc1cce1d9330063de2866e3843c49 Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Thu, 20 Dec 2018 20:26:17 +0000 Subject: [PATCH 4/5] csproj --- .../src/System.Data.SqlClient.csproj | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/System.Data.SqlClient/src/System.Data.SqlClient.csproj b/src/System.Data.SqlClient/src/System.Data.SqlClient.csproj index 858bb76a8e7d..0bced1b5e87c 100644 --- a/src/System.Data.SqlClient/src/System.Data.SqlClient.csproj +++ b/src/System.Data.SqlClient/src/System.Data.SqlClient.csproj @@ -265,17 +265,15 @@ - - - - - - + + + + From 1961cc48961877c64252e8ed66b0d250a38a56fd Mon Sep 17 00:00:00 2001 From: Ben Adams Date: Thu, 20 Dec 2018 20:55:27 +0000 Subject: [PATCH 5/5] Feedback --- src/System.Data.SqlClient/src/System.Data.SqlClient.csproj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Data.SqlClient/src/System.Data.SqlClient.csproj b/src/System.Data.SqlClient/src/System.Data.SqlClient.csproj index 0bced1b5e87c..d227f5494786 100644 --- a/src/System.Data.SqlClient/src/System.Data.SqlClient.csproj +++ b/src/System.Data.SqlClient/src/System.Data.SqlClient.csproj @@ -272,7 +272,7 @@ - +