From dc0b496ee45b0e33e73fe3e434d5240da1c24d1b Mon Sep 17 00:00:00 2001 From: David Fowler Date: Tue, 9 Mar 2021 01:46:00 -0800 Subject: [PATCH 1/8] Pool SocketSenders - SocketAsyncEventArgs have lots of state on them and as a result are quite big (~350) bytes at runtime. We can pool these since sends are usually very fast and we can reduce the per connection overhead as a result. - We also allocate one per IOQueue to reduce contention. - Fixed buffer list management - Disposed pool when the transport is disposed - Added project to slnf so running tests in VS was possible - Clear the buffer and buffer list before returning to the pool - This cleans up dumps as the pooled senders don't see references to buffers while pooled in the queue --- src/Servers/Kestrel/Kestrel.slnf | 3 +- .../src/Client/SocketConnectionFactory.cs | 4 ++ .../src/Internal/SocketAwaitableEventArgs.cs | 2 +- .../src/Internal/SocketConnection.cs | 21 ++++--- .../src/Internal/SocketReceiver.cs | 24 ++++---- .../src/Internal/SocketSender.cs | 52 +++++++++--------- .../src/Internal/SocketSenderPool.cs | 55 +++++++++++++++++++ .../src/Internal/SocketSenderReceiverBase.cs | 23 -------- .../src/SocketConnectionListener.cs | 17 +++++- 9 files changed, 129 insertions(+), 72 deletions(-) create mode 100644 src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs delete mode 100644 src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderReceiverBase.cs diff --git a/src/Servers/Kestrel/Kestrel.slnf b/src/Servers/Kestrel/Kestrel.slnf index 14be661c19a9..c23b30b99bed 100644 --- a/src/Servers/Kestrel/Kestrel.slnf +++ b/src/Servers/Kestrel/Kestrel.slnf @@ -35,7 +35,8 @@ "src\\Servers\\Kestrel\\test\\Sockets.BindTests\\Sockets.BindTests.csproj", "src\\Servers\\Kestrel\\test\\Sockets.FunctionalTests\\Sockets.FunctionalTests.csproj", "src\\Servers\\Kestrel\\tools\\CodeGenerator\\CodeGenerator.csproj", - "src\\ObjectPool\\src\\Microsoft.Extensions.ObjectPool.csproj" + "src\\ObjectPool\\src\\Microsoft.Extensions.ObjectPool.csproj", + "src\\Testing\\src\\Microsoft.AspNetCore.Testing.csproj" ] } } \ No newline at end of file diff --git a/src/Servers/Kestrel/Transport.Sockets/src/Client/SocketConnectionFactory.cs b/src/Servers/Kestrel/Transport.Sockets/src/Client/SocketConnectionFactory.cs index c0c6772be049..529d96a1ca0e 100644 --- a/src/Servers/Kestrel/Transport.Sockets/src/Client/SocketConnectionFactory.cs +++ b/src/Servers/Kestrel/Transport.Sockets/src/Client/SocketConnectionFactory.cs @@ -22,6 +22,7 @@ internal class SocketConnectionFactory : IConnectionFactory, IAsyncDisposable private readonly SocketsTrace _trace; private readonly PipeOptions _inputOptions; private readonly PipeOptions _outputOptions; + private readonly SocketSenderPool _socketSenderPool; public SocketConnectionFactory(IOptions options, ILoggerFactory loggerFactory) { @@ -46,9 +47,11 @@ public SocketConnectionFactory(IOptions options, ILogger // These are the same, it's either the thread pool or inline var applicationScheduler = _options.UnsafePreferInlineScheduling ? PipeScheduler.Inline : PipeScheduler.ThreadPool; var transportScheduler = applicationScheduler; + var awaiterScheduler = OperatingSystem.IsWindows() ? transportScheduler : PipeScheduler.Inline; _inputOptions = new PipeOptions(_memoryPool, applicationScheduler, transportScheduler, maxReadBufferSize, maxReadBufferSize / 2, useSynchronizationContext: false); _outputOptions = new PipeOptions(_memoryPool, transportScheduler, applicationScheduler, maxWriteBufferSize, maxWriteBufferSize / 2, useSynchronizationContext: false); + _socketSenderPool = new SocketSenderPool(awaiterScheduler); } public async ValueTask ConnectAsync(EndPoint endpoint, CancellationToken cancellationToken = default) @@ -72,6 +75,7 @@ public async ValueTask ConnectAsync(EndPoint endpoint, Cancel _memoryPool, _inputOptions.ReaderScheduler, // This is either threadpool or inline _trace, + _socketSenderPool, _inputOptions, _outputOptions, _options.WaitForDataBeforeAllocatingBuffer); diff --git a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketAwaitableEventArgs.cs b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketAwaitableEventArgs.cs index 50b8805d6c44..5ed561b0ed6e 100644 --- a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketAwaitableEventArgs.cs +++ b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketAwaitableEventArgs.cs @@ -11,7 +11,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets.Internal { - internal sealed class SocketAwaitableEventArgs : SocketAsyncEventArgs, ICriticalNotifyCompletion + internal class SocketAwaitableEventArgs : SocketAsyncEventArgs, ICriticalNotifyCompletion { private static readonly Action _callbackCompleted = () => { }; diff --git a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs index cf81d90e372b..1f9f4453b6e1 100644 --- a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs +++ b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs @@ -20,7 +20,8 @@ internal sealed class SocketConnection : TransportConnection private readonly Socket _socket; private readonly ISocketsTrace _trace; private readonly SocketReceiver _receiver; - private readonly SocketSender _sender; + private SocketSender? _sender; + private readonly SocketSenderPool _socketSenderPool; private readonly IDuplexPipe _originalTransport; private readonly CancellationTokenSource _connectionClosedTokenSource = new CancellationTokenSource(); @@ -36,6 +37,7 @@ internal SocketConnection(Socket socket, MemoryPool memoryPool, PipeScheduler transportScheduler, ISocketsTrace trace, + SocketSenderPool socketSenderPool, PipeOptions inputOptions, PipeOptions outputOptions, bool waitForData = true) @@ -48,8 +50,9 @@ internal SocketConnection(Socket socket, MemoryPool = memoryPool; _trace = trace; _waitForData = waitForData; + _socketSenderPool = socketSenderPool; - LocalEndPoint = _socket.LocalEndPoint; + LocalEndPoint = _socket.LocalEndPoint; RemoteEndPoint = _socket.RemoteEndPoint; ConnectionClosed = _connectionClosedTokenSource.Token; @@ -59,8 +62,7 @@ internal SocketConnection(Socket socket, // https://github.com/aspnet/KestrelHttpServer/issues/2573 var awaiterScheduler = OperatingSystem.IsWindows() ? transportScheduler : PipeScheduler.Inline; - _receiver = new SocketReceiver(_socket, awaiterScheduler); - _sender = new SocketSender(_socket, awaiterScheduler); + _receiver = new SocketReceiver(awaiterScheduler); var pair = DuplexPipe.CreateConnectionPair(inputOptions, outputOptions); @@ -93,7 +95,7 @@ private async Task StartAsync() await sendTask; _receiver.Dispose(); - _sender.Dispose(); + _sender?.Dispose(); } catch (Exception ex) { @@ -183,13 +185,13 @@ private async Task ProcessReceives() if (_waitForData) { // Wait for data before allocating a buffer. - await _receiver.WaitForDataAsync(); + await _receiver.WaitForDataAsync(_socket); } // Ensure we have some reasonable amount of buffer space var buffer = input.GetMemory(MinAllocBufferSize); - var bytesReceived = await _receiver.ReceiveAsync(buffer); + var bytesReceived = await _receiver.ReceiveAsync(_socket, buffer); if (bytesReceived == 0) { @@ -282,7 +284,10 @@ private async Task ProcessSends() var isCompleted = result.IsCompleted; if (!buffer.IsEmpty) { - await _sender.SendAsync(buffer); + _sender = _socketSenderPool.Rent(); + await _sender.SendAsync(_socket, buffer); + _socketSenderPool.Return(_sender); + _sender = null; } output.AdvanceTo(end); diff --git a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketReceiver.cs b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketReceiver.cs index c4b048fdf7af..91e1c8e5bd2d 100644 --- a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketReceiver.cs +++ b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketReceiver.cs @@ -7,34 +7,34 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets.Internal { - internal sealed class SocketReceiver : SocketSenderReceiverBase + internal sealed class SocketReceiver : SocketAwaitableEventArgs { - public SocketReceiver(Socket socket, PipeScheduler scheduler) : base(socket, scheduler) + public SocketReceiver(PipeScheduler ioScheduler) : base(ioScheduler) { } - public SocketAwaitableEventArgs WaitForDataAsync() + public SocketAwaitableEventArgs WaitForDataAsync(Socket socket) { - _awaitableEventArgs.SetBuffer(Memory.Empty); + SetBuffer(Memory.Empty); - if (!_socket.ReceiveAsync(_awaitableEventArgs)) + if (!socket.ReceiveAsync(this)) { - _awaitableEventArgs.Complete(); + Complete(); } - return _awaitableEventArgs; + return this; } - public SocketAwaitableEventArgs ReceiveAsync(Memory buffer) + public SocketAwaitableEventArgs ReceiveAsync(Socket socket, Memory buffer) { - _awaitableEventArgs.SetBuffer(buffer); + SetBuffer(buffer); - if (!_socket.ReceiveAsync(_awaitableEventArgs)) + if (!socket.ReceiveAsync(this)) { - _awaitableEventArgs.Complete(); + Complete(); } - return _awaitableEventArgs; + return this; } } } diff --git a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSender.cs b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSender.cs index 97cd0b685f77..220629a91f93 100644 --- a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSender.cs +++ b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSender.cs @@ -11,55 +11,56 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets.Internal { - internal sealed class SocketSender : SocketSenderReceiverBase + internal sealed class SocketSender : SocketAwaitableEventArgs { private List>? _bufferList; - public SocketSender(Socket socket, PipeScheduler scheduler) : base(socket, scheduler) + public SocketSender(PipeScheduler scheduler) : base(scheduler) { } - public SocketAwaitableEventArgs SendAsync(in ReadOnlySequence buffers) + public SocketAwaitableEventArgs SendAsync(Socket socket, in ReadOnlySequence buffers) { if (buffers.IsSingleSegment) { - return SendAsync(buffers.First); + return SendAsync(socket, buffers.First); } - if (!_awaitableEventArgs.MemoryBuffer.Equals(Memory.Empty)) - { - _awaitableEventArgs.SetBuffer(null, 0, 0); - } - - _awaitableEventArgs.BufferList = GetBufferList(buffers); + SetBufferList(buffers); - if (!_socket.SendAsync(_awaitableEventArgs)) + if (!socket.SendAsync(this)) { - _awaitableEventArgs.Complete(); + Complete(); } - return _awaitableEventArgs; + return this; } - private SocketAwaitableEventArgs SendAsync(ReadOnlyMemory memory) + public void Reset() { - // The BufferList getter is much less expensive then the setter. - if (_awaitableEventArgs.BufferList != null) - { - _awaitableEventArgs.BufferList = null; - } + // We clear the buffer and buffer list before we put it back into the pool + // it's a small performance hit but it removes the confusion when looking at dumps to see this still + // holder onto the buffer when it's back in the pool + BufferList = null; - _awaitableEventArgs.SetBuffer(MemoryMarshal.AsMemory(memory)); + SetBuffer(null, 0, 0); + + _bufferList?.Clear(); + } + + private SocketAwaitableEventArgs SendAsync(Socket socket, ReadOnlyMemory memory) + { + SetBuffer(MemoryMarshal.AsMemory(memory)); - if (!_socket.SendAsync(_awaitableEventArgs)) + if (!socket.SendAsync(this)) { - _awaitableEventArgs.Complete(); + Complete(); } - return _awaitableEventArgs; + return this; } - private List> GetBufferList(in ReadOnlySequence buffer) + private void SetBufferList(in ReadOnlySequence buffer) { Debug.Assert(!buffer.IsEmpty); Debug.Assert(!buffer.IsSingleSegment); @@ -79,7 +80,8 @@ private List> GetBufferList(in ReadOnlySequence buffer) _bufferList.Add(b.GetArray()); } - return _bufferList; + // The act of setting this list, sets the buffers in the internal buffer list + BufferList = _bufferList; } } } diff --git a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs new file mode 100644 index 000000000000..6e4495b659a8 --- /dev/null +++ b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs @@ -0,0 +1,55 @@ +using System; +using System.Collections.Concurrent; +using System.IO.Pipelines; + +namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets.Internal +{ + internal class SocketSenderPool : IDisposable + { + private const int MaxQueueSize = 1024; // REVIEW: Is this good enough? + + private readonly ConcurrentQueue _queue = new(); + private readonly PipeScheduler _scheduler; + private bool _disposed; + + public SocketSenderPool(PipeScheduler scheduler) + { + _scheduler = scheduler; + } + + public SocketSender Rent() + { + if (_queue.TryDequeue(out var sender)) + { + return sender; + } + return new SocketSender(_scheduler); + } + + public void Return(SocketSender sender) + { + if (_disposed || _queue.Count > MaxQueueSize) + { + sender.Dispose(); + return; + } + + sender.Reset(); + + _queue.Enqueue(sender); + } + + public void Dispose() + { + if (!_disposed) + { + _disposed = true; + while (_queue.TryDequeue(out var sender)) + { + sender.Dispose(); + } + } + } + + } +} diff --git a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderReceiverBase.cs b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderReceiverBase.cs deleted file mode 100644 index b28c7d798976..000000000000 --- a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderReceiverBase.cs +++ /dev/null @@ -1,23 +0,0 @@ -// Copyright (c) .NET Foundation. All rights reserved. -// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. - -using System; -using System.IO.Pipelines; -using System.Net.Sockets; - -namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets.Internal -{ - internal abstract class SocketSenderReceiverBase : IDisposable - { - protected readonly Socket _socket; - protected readonly SocketAwaitableEventArgs _awaitableEventArgs; - - protected SocketSenderReceiverBase(Socket socket, PipeScheduler scheduler) - { - _socket = socket; - _awaitableEventArgs = new SocketAwaitableEventArgs(scheduler); - } - - public void Dispose() => _awaitableEventArgs.Dispose(); - } -} diff --git a/src/Servers/Kestrel/Transport.Sockets/src/SocketConnectionListener.cs b/src/Servers/Kestrel/Transport.Sockets/src/SocketConnectionListener.cs index 0571ad4c8765..85d744fc37b7 100644 --- a/src/Servers/Kestrel/Transport.Sockets/src/SocketConnectionListener.cs +++ b/src/Servers/Kestrel/Transport.Sockets/src/SocketConnectionListener.cs @@ -50,18 +50,21 @@ internal SocketConnectionListener( for (var i = 0; i < _settingsCount; i++) { var transportScheduler = options.UnsafePreferInlineScheduling ? PipeScheduler.Inline : new IOQueue(); + var awaiterScheduler = OperatingSystem.IsWindows() ? transportScheduler : PipeScheduler.Inline; _settings[i] = new Settings { Scheduler = transportScheduler, InputOptions = new PipeOptions(_memoryPool, applicationScheduler, transportScheduler, maxReadBufferSize, maxReadBufferSize / 2, useSynchronizationContext: false), - OutputOptions = new PipeOptions(_memoryPool, transportScheduler, applicationScheduler, maxWriteBufferSize, maxWriteBufferSize / 2, useSynchronizationContext: false) + OutputOptions = new PipeOptions(_memoryPool, transportScheduler, applicationScheduler, maxWriteBufferSize, maxWriteBufferSize / 2, useSynchronizationContext: false), + SocketSenderPool = new SocketSenderPool(awaiterScheduler) }; } } else { var transportScheduler = options.UnsafePreferInlineScheduling ? PipeScheduler.Inline : PipeScheduler.ThreadPool; + var awaiterScheduler = OperatingSystem.IsWindows() ? transportScheduler : PipeScheduler.Inline; var directScheduler = new Settings[] { @@ -69,7 +72,8 @@ internal SocketConnectionListener( { Scheduler = transportScheduler, InputOptions = new PipeOptions(_memoryPool, applicationScheduler, transportScheduler, maxReadBufferSize, maxReadBufferSize / 2, useSynchronizationContext: false), - OutputOptions = new PipeOptions(_memoryPool, transportScheduler, applicationScheduler, maxWriteBufferSize, maxWriteBufferSize / 2, useSynchronizationContext: false) + OutputOptions = new PipeOptions(_memoryPool, transportScheduler, applicationScheduler, maxWriteBufferSize, maxWriteBufferSize / 2, useSynchronizationContext: false), + SocketSenderPool = new SocketSenderPool(awaiterScheduler) } }; @@ -155,6 +159,7 @@ void BindSocket() _memoryPool, setting.Scheduler, _trace, + setting.SocketSenderPool, setting.InputOptions, setting.OutputOptions, waitForData: _options.WaitForDataBeforeAllocatingBuffer); @@ -199,6 +204,13 @@ public ValueTask DisposeAsync() // Dispose the memory pool _memoryPool.Dispose(); + + // Dispose any pooled senders + foreach (var setting in _settings) + { + setting.SocketSenderPool.Dispose(); + } + return default; } @@ -207,6 +219,7 @@ private class Settings public PipeScheduler Scheduler { get; init; } = default!; public PipeOptions InputOptions { get; init; } = default!; public PipeOptions OutputOptions { get; init; } = default!; + public SocketSenderPool SocketSenderPool { get; init; } = default!; } } } From f4e75d2ea6f2f5991b2972d76aa4005ef3f6ac3c Mon Sep 17 00:00:00 2001 From: David Fowler Date: Tue, 9 Mar 2021 23:31:05 -0800 Subject: [PATCH 2/8] Update src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs Co-authored-by: James Newton-King --- .../Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs index 1f9f4453b6e1..1aa1f83e4776 100644 --- a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs +++ b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs @@ -52,7 +52,7 @@ internal SocketConnection(Socket socket, _waitForData = waitForData; _socketSenderPool = socketSenderPool; - LocalEndPoint = _socket.LocalEndPoint; + LocalEndPoint = _socket.LocalEndPoint; RemoteEndPoint = _socket.RemoteEndPoint; ConnectionClosed = _connectionClosedTokenSource.Token; From 1607af29da1684cad36d164a190474eb2655044f Mon Sep 17 00:00:00 2001 From: David Fowler Date: Wed, 10 Mar 2021 09:35:07 -0800 Subject: [PATCH 3/8] PR feedback --- .../Transport.Sockets/src/Internal/SocketConnection.cs | 2 ++ .../Kestrel/Transport.Sockets/src/Internal/SocketSender.cs | 7 +------ 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs index 1aa1f83e4776..4d8f6f27771b 100644 --- a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs +++ b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketConnection.cs @@ -286,6 +286,8 @@ private async Task ProcessSends() { _sender = _socketSenderPool.Rent(); await _sender.SendAsync(_socket, buffer); + // We don't return to the pool if there was an exception, and + // we keep the _sender assigned so that we can dispose it in StartAsync. _socketSenderPool.Return(_sender); _sender = null; } diff --git a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSender.cs b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSender.cs index 220629a91f93..d06076ef1519 100644 --- a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSender.cs +++ b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSender.cs @@ -40,7 +40,7 @@ public void Reset() { // We clear the buffer and buffer list before we put it back into the pool // it's a small performance hit but it removes the confusion when looking at dumps to see this still - // holder onto the buffer when it's back in the pool + // holds onto the buffer when it's back in the pool BufferList = null; SetBuffer(null, 0, 0); @@ -69,11 +69,6 @@ private void SetBufferList(in ReadOnlySequence buffer) { _bufferList = new List>(); } - else - { - // Buffers are pooled, so it's OK to root them until the next multi-buffer write. - _bufferList.Clear(); - } foreach (var b in buffer) { From 5067db400c8c1cf53a8745d11c24b757d6096ec3 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Sat, 13 Mar 2021 01:37:41 -0800 Subject: [PATCH 4/8] PR feedback --- .../Kestrel/Transport.Sockets/src/Internal/SocketSender.cs | 5 ++++- .../Transport.Sockets/src/Internal/SocketSenderPool.cs | 4 +++- .../Transport.Sockets/src/SocketConnectionListener.cs | 2 ++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSender.cs b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSender.cs index d06076ef1519..edcbe95e0f0b 100644 --- a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSender.cs +++ b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSender.cs @@ -41,7 +41,10 @@ public void Reset() // We clear the buffer and buffer list before we put it back into the pool // it's a small performance hit but it removes the confusion when looking at dumps to see this still // holds onto the buffer when it's back in the pool - BufferList = null; + if (BufferList != null) + { + BufferList = null; + } SetBuffer(null, 0, 0); diff --git a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs index 6e4495b659a8..06fbbe493675 100644 --- a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs +++ b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs @@ -1,3 +1,6 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + using System; using System.Collections.Concurrent; using System.IO.Pipelines; @@ -50,6 +53,5 @@ public void Dispose() } } } - } } diff --git a/src/Servers/Kestrel/Transport.Sockets/src/SocketConnectionListener.cs b/src/Servers/Kestrel/Transport.Sockets/src/SocketConnectionListener.cs index 85d744fc37b7..6b618e5d5f76 100644 --- a/src/Servers/Kestrel/Transport.Sockets/src/SocketConnectionListener.cs +++ b/src/Servers/Kestrel/Transport.Sockets/src/SocketConnectionListener.cs @@ -50,6 +50,7 @@ internal SocketConnectionListener( for (var i = 0; i < _settingsCount; i++) { var transportScheduler = options.UnsafePreferInlineScheduling ? PipeScheduler.Inline : new IOQueue(); + // https://github.com/aspnet/KestrelHttpServer/issues/2573 var awaiterScheduler = OperatingSystem.IsWindows() ? transportScheduler : PipeScheduler.Inline; _settings[i] = new Settings @@ -64,6 +65,7 @@ internal SocketConnectionListener( else { var transportScheduler = options.UnsafePreferInlineScheduling ? PipeScheduler.Inline : PipeScheduler.ThreadPool; + // https://github.com/aspnet/KestrelHttpServer/issues/2573 var awaiterScheduler = OperatingSystem.IsWindows() ? transportScheduler : PipeScheduler.Inline; var directScheduler = new Settings[] From 11b88425e70d7185322543f187b0464fb10dc69a Mon Sep 17 00:00:00 2001 From: David Fowler Date: Mon, 15 Mar 2021 21:37:58 -0700 Subject: [PATCH 5/8] PR Feedback - Keep track of items in the pool separately from the queue count. - Clear the buffer only if the buffer list is null --- .../src/Internal/SocketSender.cs | 6 +++-- .../src/Internal/SocketSenderPool.cs | 25 ++++++++++++++++--- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSender.cs b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSender.cs index edcbe95e0f0b..c2b0061f62b0 100644 --- a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSender.cs +++ b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSender.cs @@ -45,8 +45,10 @@ public void Reset() { BufferList = null; } - - SetBuffer(null, 0, 0); + else + { + SetBuffer(null, 0, 0); + } _bufferList?.Clear(); } diff --git a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs index 06fbbe493675..a09a38563f1f 100644 --- a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs +++ b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Concurrent; using System.IO.Pipelines; +using System.Threading; namespace Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets.Internal { @@ -12,6 +13,7 @@ internal class SocketSenderPool : IDisposable private const int MaxQueueSize = 1024; // REVIEW: Is this good enough? private readonly ConcurrentQueue _queue = new(); + private int _count; private readonly PipeScheduler _scheduler; private bool _disposed; @@ -24,6 +26,7 @@ public SocketSender Rent() { if (_queue.TryDequeue(out var sender)) { + Interlocked.Decrement(ref _count); return sender; } return new SocketSender(_scheduler); @@ -31,15 +34,31 @@ public SocketSender Rent() public void Return(SocketSender sender) { - if (_disposed || _queue.Count > MaxQueueSize) + if (Volatile.Read(ref _disposed)) { + // We disposed the queue sender.Dispose(); return; } - sender.Reset(); + // Add this sender back to the queue if we haven't crossed the max + var count = Volatile.Read(ref _count); + while (count < MaxQueueSize) + { + var prev = Interlocked.CompareExchange(ref _count, count + 1, count); + + if (prev == count) + { + sender.Reset(); + _queue.Enqueue(sender); + return; + } + + count = prev; + } - _queue.Enqueue(sender); + // Over the limit + sender.Dispose(); } public void Dispose() From fe2d74cd0f237cf50ffdf14d983c9c6959793148 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 16 Mar 2021 10:12:50 -0700 Subject: [PATCH 6/8] Update src/Servers/Kestrel/Transport.Sockets/src/Client/SocketConnectionFactory.cs Co-authored-by: Brennan --- .../Transport.Sockets/src/Client/SocketConnectionFactory.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Servers/Kestrel/Transport.Sockets/src/Client/SocketConnectionFactory.cs b/src/Servers/Kestrel/Transport.Sockets/src/Client/SocketConnectionFactory.cs index 529d96a1ca0e..7465ece15f98 100644 --- a/src/Servers/Kestrel/Transport.Sockets/src/Client/SocketConnectionFactory.cs +++ b/src/Servers/Kestrel/Transport.Sockets/src/Client/SocketConnectionFactory.cs @@ -47,6 +47,7 @@ public SocketConnectionFactory(IOptions options, ILogger // These are the same, it's either the thread pool or inline var applicationScheduler = _options.UnsafePreferInlineScheduling ? PipeScheduler.Inline : PipeScheduler.ThreadPool; var transportScheduler = applicationScheduler; + // https://github.com/aspnet/KestrelHttpServer/issues/2573 var awaiterScheduler = OperatingSystem.IsWindows() ? transportScheduler : PipeScheduler.Inline; _inputOptions = new PipeOptions(_memoryPool, applicationScheduler, transportScheduler, maxReadBufferSize, maxReadBufferSize / 2, useSynchronizationContext: false); From 704d9bc95f77cc66f03b2e906ef4651a60b0ac60 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Tue, 16 Mar 2021 10:23:26 -0700 Subject: [PATCH 7/8] More PR feedback --- .../src/Internal/SocketSender.cs | 4 ++-- .../src/Internal/SocketSenderPool.cs | 24 ++++--------------- 2 files changed, 7 insertions(+), 21 deletions(-) diff --git a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSender.cs b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSender.cs index c2b0061f62b0..666d71776087 100644 --- a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSender.cs +++ b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSender.cs @@ -44,13 +44,13 @@ public void Reset() if (BufferList != null) { BufferList = null; + + _bufferList?.Clear(); } else { SetBuffer(null, 0, 0); } - - _bufferList?.Clear(); } private SocketAwaitableEventArgs SendAsync(Socket socket, ReadOnlyMemory memory) diff --git a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs index a09a38563f1f..93aab36e2949 100644 --- a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs +++ b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs @@ -34,31 +34,17 @@ public SocketSender Rent() public void Return(SocketSender sender) { - if (Volatile.Read(ref _disposed)) + // This counting isn't accurate, but it's good enough for what we need to avoid using _queue.Count which could be expensive + if (_disposed || Interlocked.Increment(ref _count) > MaxQueueSize) { + Interlocked.Decrement(ref _count); // We disposed the queue sender.Dispose(); return; } - // Add this sender back to the queue if we haven't crossed the max - var count = Volatile.Read(ref _count); - while (count < MaxQueueSize) - { - var prev = Interlocked.CompareExchange(ref _count, count + 1, count); - - if (prev == count) - { - sender.Reset(); - _queue.Enqueue(sender); - return; - } - - count = prev; - } - - // Over the limit - sender.Dispose(); + sender.Reset(); + _queue.Enqueue(sender); } public void Dispose() From b5aca369bf92a1d967d2e53d1e4a6c098b3ecbb4 Mon Sep 17 00:00:00 2001 From: David Fowler Date: Tue, 16 Mar 2021 14:07:54 -0700 Subject: [PATCH 8/8] Update src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs Co-authored-by: Stephen Halter --- .../Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs index 93aab36e2949..558fe16480f2 100644 --- a/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs +++ b/src/Servers/Kestrel/Transport.Sockets/src/Internal/SocketSenderPool.cs @@ -38,7 +38,6 @@ public void Return(SocketSender sender) if (_disposed || Interlocked.Increment(ref _count) > MaxQueueSize) { Interlocked.Decrement(ref _count); - // We disposed the queue sender.Dispose(); return; }