From 1cd3e96b00268872f2b63e2592fddb4f41295ba9 Mon Sep 17 00:00:00 2001 From: Sebastien Ros Date: Wed, 25 Aug 2021 11:51:01 -0700 Subject: [PATCH 1/8] Use ArrayPool as SlabMemoryPool fallback Fixes #30364 --- .../Core/src/Internal/Http/Http1OutputProducer.cs | 13 ++++++++++++- .../Core/src/Internal/Http2/Http2OutputProducer.cs | 13 +++++++++++-- .../Core/src/Internal/Http3/Http3OutputProducer.cs | 12 +++++++++++- 3 files changed, 34 insertions(+), 4 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs index 1e5d98d6887c..15e4668ca1cc 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs @@ -654,8 +654,19 @@ private Memory GetFakeMemory(int sizeHint) { if (_fakeMemoryOwner == null) { - _fakeMemoryOwner = _memoryPool.Rent(sizeHint); + // + if (sizeHint <= _memoryPool.MaxBufferSize) + { + // Use the specified pool as it fits. + _fakeMemoryOwner = _memoryPool.Rent(sizeHint); + } + else + { + // Use the array pool. It's MaxBufferSize is int.MaxValue. + _fakeMemoryOwner = MemoryPool.Shared.Rent(sizeHint); + } } + return _fakeMemoryOwner.Memory; } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs index 6a17db8934e3..d0b8a481aeb8 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs @@ -490,12 +490,21 @@ private Memory GetFakeMemory(int sizeHint) { if (_fakeMemoryOwner == null) { - _fakeMemoryOwner = _memoryPool.Rent(sizeHint); + // + if (sizeHint <= _memoryPool.MaxBufferSize) + { + // Use the specified pool as it fits. + _fakeMemoryOwner = _memoryPool.Rent(sizeHint); + } + else + { + // Use the array pool. It's MaxBufferSize is int.MaxValue. + _fakeMemoryOwner = MemoryPool.Shared.Rent(sizeHint); + } } return _fakeMemoryOwner.Memory; } - [StackTraceHidden] private void ThrowIfSuffixSentOrCompleted() { diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs index 9bc96871f8a5..a586f0b7a81c 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs @@ -212,7 +212,17 @@ private Memory GetFakeMemory(int sizeHint) { if (_fakeMemoryOwner == null) { - _fakeMemoryOwner = _memoryPool.Rent(sizeHint); + // + if (sizeHint <= _memoryPool.MaxBufferSize) + { + // Use the specified pool as it fits. + _fakeMemoryOwner = _memoryPool.Rent(sizeHint); + } + else + { + // Use the array pool. It's MaxBufferSize is int.MaxValue. + _fakeMemoryOwner = MemoryPool.Shared.Rent(sizeHint); + } } return _fakeMemoryOwner.Memory; From f8db646178cdb9780a0a741282bd90009bb6fc71 Mon Sep 17 00:00:00 2001 From: Sebastien Ros Date: Wed, 25 Aug 2021 11:56:19 -0700 Subject: [PATCH 2/8] Fix comments --- .../Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs | 2 +- .../Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs | 2 +- .../Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs index 15e4668ca1cc..2d9da412aa80 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs @@ -654,7 +654,7 @@ private Memory GetFakeMemory(int sizeHint) { if (_fakeMemoryOwner == null) { - // + // Requesting a bigger buffer could throw. if (sizeHint <= _memoryPool.MaxBufferSize) { // Use the specified pool as it fits. diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs index d0b8a481aeb8..56483c3766e8 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs @@ -490,7 +490,7 @@ private Memory GetFakeMemory(int sizeHint) { if (_fakeMemoryOwner == null) { - // + // Requesting a bigger buffer could throw. if (sizeHint <= _memoryPool.MaxBufferSize) { // Use the specified pool as it fits. diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs index a586f0b7a81c..e6ef30177b29 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs @@ -212,7 +212,7 @@ private Memory GetFakeMemory(int sizeHint) { if (_fakeMemoryOwner == null) { - // + // Requesting a bigger buffer could throw. if (sizeHint <= _memoryPool.MaxBufferSize) { // Use the specified pool as it fits. From a17efe5687420e98c33e366cf1296fae20b6927e Mon Sep 17 00:00:00 2001 From: Sebastien Ros Date: Wed, 25 Aug 2021 12:06:11 -0700 Subject: [PATCH 3/8] Typos [ci skip] --- .../Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs | 2 +- .../Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs | 2 +- .../Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs index 2d9da412aa80..2284b335c64d 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs @@ -662,7 +662,7 @@ private Memory GetFakeMemory(int sizeHint) } else { - // Use the array pool. It's MaxBufferSize is int.MaxValue. + // Use the array pool. Its MaxBufferSize is int.MaxValue. _fakeMemoryOwner = MemoryPool.Shared.Rent(sizeHint); } } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs index 56483c3766e8..195da799a079 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs @@ -498,7 +498,7 @@ private Memory GetFakeMemory(int sizeHint) } else { - // Use the array pool. It's MaxBufferSize is int.MaxValue. + // Use the array pool. Its MaxBufferSize is int.MaxValue. _fakeMemoryOwner = MemoryPool.Shared.Rent(sizeHint); } } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs index e6ef30177b29..67caa216af5a 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs @@ -220,7 +220,7 @@ private Memory GetFakeMemory(int sizeHint) } else { - // Use the array pool. It's MaxBufferSize is int.MaxValue. + // Use the array pool. Its MaxBufferSize is int.MaxValue. _fakeMemoryOwner = MemoryPool.Shared.Rent(sizeHint); } } From a1358d065ecbd74ee2bfffe5d4ae0c32fa9bfb12 Mon Sep 17 00:00:00 2001 From: Sebastien Ros Date: Fri, 27 Aug 2021 10:15:35 -0700 Subject: [PATCH 4/8] Support increase buffer size --- .../src/Internal/Http/Http1OutputProducer.cs | 18 ++-- .../src/Internal/Http2/Http2OutputProducer.cs | 19 ++--- .../src/Internal/Http3/Http3OutputProducer.cs | 18 ++-- .../src/Internal/HttpOutputProducerHelper.cs | 23 +++++ .../Kestrel/Core/test/OutputProducerTests.cs | 83 +++++++++++++++++++ 5 files changed, 122 insertions(+), 39 deletions(-) create mode 100644 src/Servers/Kestrel/Core/src/Internal/HttpOutputProducerHelper.cs diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs index 2284b335c64d..3332e508843e 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs @@ -650,21 +650,13 @@ private void WriteCurrentChunkMemoryToPipeWriter(ref BufferWriter wr _advancedBytesForChunk = 0; } - private Memory GetFakeMemory(int sizeHint) + internal Memory GetFakeMemory(int sizeHint) { - if (_fakeMemoryOwner == null) + if (_fakeMemoryOwner == null || _fakeMemoryOwner.Memory.Length < sizeHint) { - // Requesting a bigger buffer could throw. - if (sizeHint <= _memoryPool.MaxBufferSize) - { - // Use the specified pool as it fits. - _fakeMemoryOwner = _memoryPool.Rent(sizeHint); - } - else - { - // Use the array pool. Its MaxBufferSize is int.MaxValue. - _fakeMemoryOwner = MemoryPool.Shared.Rent(sizeHint); - } + _fakeMemoryOwner?.Dispose(); + + _fakeMemoryOwner = HttpOutputProducerHelper.ReserveFakeMemory(_memoryPool, sizeHint); } return _fakeMemoryOwner.Memory; diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs index 195da799a079..d8b80398c47f 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs @@ -486,25 +486,18 @@ static void ThrowUnexpectedState() } } - private Memory GetFakeMemory(int sizeHint) + internal Memory GetFakeMemory(int sizeHint) { - if (_fakeMemoryOwner == null) + if (_fakeMemoryOwner == null || _fakeMemoryOwner.Memory.Length < sizeHint) { - // Requesting a bigger buffer could throw. - if (sizeHint <= _memoryPool.MaxBufferSize) - { - // Use the specified pool as it fits. - _fakeMemoryOwner = _memoryPool.Rent(sizeHint); - } - else - { - // Use the array pool. Its MaxBufferSize is int.MaxValue. - _fakeMemoryOwner = MemoryPool.Shared.Rent(sizeHint); - } + _fakeMemoryOwner?.Dispose(); + + _fakeMemoryOwner = HttpOutputProducerHelper.ReserveFakeMemory(_memoryPool, sizeHint); } return _fakeMemoryOwner.Memory; } + [StackTraceHidden] private void ThrowIfSuffixSentOrCompleted() { diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs index 67caa216af5a..90d9a045d933 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs @@ -208,21 +208,13 @@ public Span GetSpan(int sizeHint = 0) } } - private Memory GetFakeMemory(int sizeHint) + internal Memory GetFakeMemory(int sizeHint) { - if (_fakeMemoryOwner == null) + if (_fakeMemoryOwner == null || _fakeMemoryOwner.Memory.Length < sizeHint) { - // Requesting a bigger buffer could throw. - if (sizeHint <= _memoryPool.MaxBufferSize) - { - // Use the specified pool as it fits. - _fakeMemoryOwner = _memoryPool.Rent(sizeHint); - } - else - { - // Use the array pool. Its MaxBufferSize is int.MaxValue. - _fakeMemoryOwner = MemoryPool.Shared.Rent(sizeHint); - } + _fakeMemoryOwner?.Dispose(); + + _fakeMemoryOwner = HttpOutputProducerHelper.ReserveFakeMemory(_memoryPool, sizeHint); } return _fakeMemoryOwner.Memory; diff --git a/src/Servers/Kestrel/Core/src/Internal/HttpOutputProducerHelper.cs b/src/Servers/Kestrel/Core/src/Internal/HttpOutputProducerHelper.cs new file mode 100644 index 000000000000..4625ca439d4d --- /dev/null +++ b/src/Servers/Kestrel/Core/src/Internal/HttpOutputProducerHelper.cs @@ -0,0 +1,23 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace System.Buffers +{ + internal class HttpOutputProducerHelper + { + public static IMemoryOwner ReserveFakeMemory(MemoryPool memoryPool, int sizeHint) + { + // Requesting a bigger buffer could throw. + if (sizeHint <= memoryPool.MaxBufferSize) + { + // Use the specified pool as it fits. + return memoryPool.Rent(sizeHint); + } + else + { + // Use the array pool. Its MaxBufferSize is int.MaxValue. + return MemoryPool.Shared.Rent(sizeHint); + } + } + } +} diff --git a/src/Servers/Kestrel/Core/test/OutputProducerTests.cs b/src/Servers/Kestrel/Core/test/OutputProducerTests.cs index 3204cedd3407..c5eb71258b30 100644 --- a/src/Servers/Kestrel/Core/test/OutputProducerTests.cs +++ b/src/Servers/Kestrel/Core/test/OutputProducerTests.cs @@ -74,6 +74,89 @@ public void AbortsTransportEvenAfterDispose() mockConnectionContext.Verify(f => f.Abort(null), Times.Once()); } + [Fact] + public void AllocatesFakeMemorySmallerThanMaxBufferSize() + { + var pipeOptions = new PipeOptions + ( + pool: _memoryPool, + readerScheduler: Mock.Of(), + writerScheduler: PipeScheduler.Inline, + useSynchronizationContext: false + ); + + using (var socketOutput = CreateOutputProducer(pipeOptions)) + { + var bufferSize = 1; + var fakeMemory = socketOutput.GetFakeMemory(bufferSize); + + Assert.True(fakeMemory.Length >= bufferSize); + } + } + + [Fact] + public void AllocatesFakeMemoryBiggerThanMaxBufferSize() + { + var pipeOptions = new PipeOptions + ( + pool: _memoryPool, + readerScheduler: Mock.Of(), + writerScheduler: PipeScheduler.Inline, + useSynchronizationContext: false + ); + + using (var socketOutput = CreateOutputProducer(pipeOptions)) + { + var bufferSize = _memoryPool.MaxBufferSize * 2; + var fakeMemory = socketOutput.GetFakeMemory(bufferSize); + + Assert.True(fakeMemory.Length >= bufferSize); + } + } + + [Fact] + public void AllocatesIncreasingFakeMemory() + { + var pipeOptions = new PipeOptions + ( + pool: _memoryPool, + readerScheduler: Mock.Of(), + writerScheduler: PipeScheduler.Inline, + useSynchronizationContext: false + ); + + using (var socketOutput = CreateOutputProducer(pipeOptions)) + { + var bufferSize1 = 1024; + var bufferSize2 = 2048; + var fakeMemory = socketOutput.GetFakeMemory(bufferSize1); + fakeMemory = socketOutput.GetFakeMemory(bufferSize2); + + Assert.True(fakeMemory.Length >= bufferSize2); + } + } + + [Fact] + public void ReusesFakeMemory() + { + var pipeOptions = new PipeOptions + ( + pool: _memoryPool, + readerScheduler: Mock.Of(), + writerScheduler: PipeScheduler.Inline, + useSynchronizationContext: false + ); + + using (var socketOutput = CreateOutputProducer(pipeOptions)) + { + var bufferSize = 1024; + var fakeMemory1 = socketOutput.GetFakeMemory(bufferSize); + var fakeMemory2 = socketOutput.GetFakeMemory(bufferSize); + + Assert.Equal(fakeMemory1, fakeMemory2); + } + } + private TestHttpOutputProducer CreateOutputProducer( PipeOptions pipeOptions = null, ConnectionContext connectionContext = null) From 0fe35357d70dc18e1c6bf14d00c729324446651c Mon Sep 17 00:00:00 2001 From: Sebastien Ros Date: Fri, 27 Aug 2021 18:20:20 -0700 Subject: [PATCH 5/8] Add function tests, rename argument --- .../src/Internal/Http/Http1OutputProducer.cs | 6 +-- .../src/Internal/Http2/Http2OutputProducer.cs | 6 +-- .../src/Internal/Http3/Http3OutputProducer.cs | 6 +-- .../src/Internal/HttpOutputProducerHelper.cs | 8 ++-- .../PipeWriterHelpers/ConcurrentPipeWriter.cs | 8 ++-- ...erTests.cs => Http1OutputProducerTests.cs} | 4 +- .../Http2/Http2StreamTests.cs | 48 +++++++++++++++++++ .../Http3/Http3StreamTests.cs | 25 ++++++++++ 8 files changed, 92 insertions(+), 19 deletions(-) rename src/Servers/Kestrel/Core/test/{OutputProducerTests.cs => Http1OutputProducerTests.cs} (98%) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs index 3332e508843e..cf0ddafb203e 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs @@ -650,13 +650,13 @@ private void WriteCurrentChunkMemoryToPipeWriter(ref BufferWriter wr _advancedBytesForChunk = 0; } - internal Memory GetFakeMemory(int sizeHint) + internal Memory GetFakeMemory(int minSize) { - if (_fakeMemoryOwner == null || _fakeMemoryOwner.Memory.Length < sizeHint) + if (_fakeMemoryOwner == null || _fakeMemoryOwner.Memory.Length < minSize) { _fakeMemoryOwner?.Dispose(); - _fakeMemoryOwner = HttpOutputProducerHelper.ReserveFakeMemory(_memoryPool, sizeHint); + _fakeMemoryOwner = HttpOutputProducerHelper.ReserveFakeMemory(_memoryPool, minSize); } return _fakeMemoryOwner.Memory; diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs index d8b80398c47f..6333a615fc51 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs @@ -486,13 +486,13 @@ static void ThrowUnexpectedState() } } - internal Memory GetFakeMemory(int sizeHint) + internal Memory GetFakeMemory(int minSize) { - if (_fakeMemoryOwner == null || _fakeMemoryOwner.Memory.Length < sizeHint) + if (_fakeMemoryOwner == null || _fakeMemoryOwner.Memory.Length < minSize) { _fakeMemoryOwner?.Dispose(); - _fakeMemoryOwner = HttpOutputProducerHelper.ReserveFakeMemory(_memoryPool, sizeHint); + _fakeMemoryOwner = HttpOutputProducerHelper.ReserveFakeMemory(_memoryPool, minSize); } return _fakeMemoryOwner.Memory; diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs index 90d9a045d933..3fb7a782e16b 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs @@ -208,13 +208,13 @@ public Span GetSpan(int sizeHint = 0) } } - internal Memory GetFakeMemory(int sizeHint) + internal Memory GetFakeMemory(int minSize) { - if (_fakeMemoryOwner == null || _fakeMemoryOwner.Memory.Length < sizeHint) + if (_fakeMemoryOwner == null || _fakeMemoryOwner.Memory.Length < minSize) { _fakeMemoryOwner?.Dispose(); - _fakeMemoryOwner = HttpOutputProducerHelper.ReserveFakeMemory(_memoryPool, sizeHint); + _fakeMemoryOwner = HttpOutputProducerHelper.ReserveFakeMemory(_memoryPool, minSize); } return _fakeMemoryOwner.Memory; diff --git a/src/Servers/Kestrel/Core/src/Internal/HttpOutputProducerHelper.cs b/src/Servers/Kestrel/Core/src/Internal/HttpOutputProducerHelper.cs index 4625ca439d4d..a9ab66a7f160 100644 --- a/src/Servers/Kestrel/Core/src/Internal/HttpOutputProducerHelper.cs +++ b/src/Servers/Kestrel/Core/src/Internal/HttpOutputProducerHelper.cs @@ -5,18 +5,18 @@ namespace System.Buffers { internal class HttpOutputProducerHelper { - public static IMemoryOwner ReserveFakeMemory(MemoryPool memoryPool, int sizeHint) + public static IMemoryOwner ReserveFakeMemory(MemoryPool memoryPool, int minSize) { // Requesting a bigger buffer could throw. - if (sizeHint <= memoryPool.MaxBufferSize) + if (minSize <= memoryPool.MaxBufferSize) { // Use the specified pool as it fits. - return memoryPool.Rent(sizeHint); + return memoryPool.Rent(minSize); } else { // Use the array pool. Its MaxBufferSize is int.MaxValue. - return MemoryPool.Shared.Rent(sizeHint); + return MemoryPool.Shared.Rent(minSize); } } } diff --git a/src/Servers/Kestrel/Core/src/Internal/Infrastructure/PipeWriterHelpers/ConcurrentPipeWriter.cs b/src/Servers/Kestrel/Core/src/Internal/Infrastructure/PipeWriterHelpers/ConcurrentPipeWriter.cs index c74e5f33975b..ea652c1bc3e7 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Infrastructure/PipeWriterHelpers/ConcurrentPipeWriter.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Infrastructure/PipeWriterHelpers/ConcurrentPipeWriter.cs @@ -346,19 +346,19 @@ private void AllocateMemoryUnsynchronized(int sizeHint) } } - private BufferSegment AllocateSegmentUnsynchronized(int sizeHint) + private BufferSegment AllocateSegmentUnsynchronized(int minSize) { BufferSegment newSegment = CreateSegmentUnsynchronized(); - if (sizeHint <= _pool.MaxBufferSize) + if (minSize <= _pool.MaxBufferSize) { // Use the specified pool if it fits - newSegment.SetOwnedMemory(_pool.Rent(GetSegmentSize(sizeHint, _pool.MaxBufferSize))); + newSegment.SetOwnedMemory(_pool.Rent(GetSegmentSize(minSize, _pool.MaxBufferSize))); } else { // We can't use the recommended pool so use the ArrayPool - newSegment.SetOwnedMemory(ArrayPool.Shared.Rent(sizeHint)); + newSegment.SetOwnedMemory(ArrayPool.Shared.Rent(minSize)); } _tailMemory = newSegment.AvailableMemory; diff --git a/src/Servers/Kestrel/Core/test/OutputProducerTests.cs b/src/Servers/Kestrel/Core/test/Http1OutputProducerTests.cs similarity index 98% rename from src/Servers/Kestrel/Core/test/OutputProducerTests.cs rename to src/Servers/Kestrel/Core/test/Http1OutputProducerTests.cs index c5eb71258b30..272fca477865 100644 --- a/src/Servers/Kestrel/Core/test/OutputProducerTests.cs +++ b/src/Servers/Kestrel/Core/test/Http1OutputProducerTests.cs @@ -15,11 +15,11 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests { - public class OutputProducerTests : IDisposable + public class Http1OutputProducerTests : IDisposable { private readonly MemoryPool _memoryPool; - public OutputProducerTests() + public Http1OutputProducerTests() { _memoryPool = PinnedBlockMemoryPoolFactory.Create(); } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs index d90501723df9..aa4ec23ff085 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs @@ -5100,5 +5100,53 @@ await ExpectAsync(Http2FrameType.DATA, Assert.Contains(LogMessages, m => m.Message.Equals("One or more of the following response headers have been removed because they are invalid for HTTP/2 and HTTP/3 responses: 'Connection', 'Transfer-Encoding', 'Keep-Alive', 'Upgrade' and 'Proxy-Connection'.")); } + + [Theory] + [InlineData(1000)] + [InlineData(4096)] + [InlineData(8000)] // Greater than the default max pool size (4096) + public async Task GetMemory_AfterAbort_GetsFakeMemory(int sizeHint) + { + var headers = new[] + { + new KeyValuePair(HeaderNames.Method, "GET"), + new KeyValuePair(HeaderNames.Path, "/"), + new KeyValuePair(HeaderNames.Scheme, "http"), + }; + await InitializeConnectionAsync(async httpContext => + { + var response = httpContext.Response; + + await response.BodyWriter.FlushAsync(); + + httpContext.Abort(); + + var memory = response.BodyWriter.GetMemory(sizeHint); + Assert.True(memory.Length >= sizeHint); + + var fisrtPartOfResponse = Encoding.ASCII.GetBytes(new String('a', sizeHint)); + fisrtPartOfResponse.CopyTo(memory); + response.BodyWriter.Advance(sizeHint); + }); + + await StartStreamAsync(1, headers, endStream: true); + + var headersFrame = await ExpectAsync(Http2FrameType.HEADERS, + withLength: 32, + withFlags: (byte)Http2HeadersFrameFlags.END_HEADERS, + withStreamId: 1); + await ExpectAsync(Http2FrameType.RST_STREAM, + withLength: 4, + withFlags: (byte)Http2DataFrameFlags.NONE, + withStreamId: 1); + + await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); + + _hpackDecoder.Decode(headersFrame.PayloadSequence, endHeaders: false, handler: this); + + Assert.Equal(2, _decodedHeaders.Count); + Assert.Contains("date", _decodedHeaders.Keys, StringComparer.OrdinalIgnoreCase); + Assert.Equal("200", _decodedHeaders[HeaderNames.Status]); + } } } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs index e6f66f75b76c..6a82f44102c8 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs @@ -2927,5 +2927,30 @@ public async Task HEADERS_NoResponseBody_RequestEndsOnHeaders() var responseHeaders = await requestStream.ExpectHeadersAsync(expectEnd: true); Assert.Equal("200", responseHeaders[HeaderNames.Status]); } + + [Theory] + [InlineData(1000)] + [InlineData(4096)] + [InlineData(8000)] // Greater than the default max pool size (4096) + public async Task GetMemory_AfterAbort_GetsFakeMemory(int sizeHint) + { + var tcs = new TaskCompletionSource(); + var headers = new[] + { + new KeyValuePair(HeaderNames.Method, "GET"), + new KeyValuePair(HeaderNames.Path, "/"), + new KeyValuePair(HeaderNames.Scheme, "http"), + }; + var requestStream = await Http3Api.InitializeConnectionAndStreamsAsync(async context => + { + context.Abort(); + + var memory = context.Response.BodyWriter.GetMemory(sizeHint); + + Assert.True(memory.Length >= sizeHint); + await context.Response.CompleteAsync(); + context.Response.BodyWriter.Advance(memory.Length); + }); + } } } From c0ee4c09b6cb90c2132f39a5e3ce407df912b8ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Ros?= Date: Wed, 1 Sep 2021 14:41:51 -0700 Subject: [PATCH 6/8] Update src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs Co-authored-by: Stephen Halter --- .../Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs index 3fb7a782e16b..54f45a170f7a 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs @@ -208,7 +208,7 @@ public Span GetSpan(int sizeHint = 0) } } - internal Memory GetFakeMemory(int minSize) + private Memory GetFakeMemory(int minSize) { if (_fakeMemoryOwner == null || _fakeMemoryOwner.Memory.Length < minSize) { From 6348dc5f1929b5528f6e0b0c2a79222e2bef819d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Ros?= Date: Wed, 1 Sep 2021 14:41:59 -0700 Subject: [PATCH 7/8] Update src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs Co-authored-by: Stephen Halter --- .../Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs index 6333a615fc51..f4d7078a9b71 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs @@ -486,7 +486,7 @@ static void ThrowUnexpectedState() } } - internal Memory GetFakeMemory(int minSize) + private Memory GetFakeMemory(int minSize) { if (_fakeMemoryOwner == null || _fakeMemoryOwner.Memory.Length < minSize) { From a58d167ba4580d5cdc9df327ffb8584ff53f7410 Mon Sep 17 00:00:00 2001 From: Sebastien Ros Date: Thu, 2 Sep 2021 09:49:20 -0700 Subject: [PATCH 8/8] Reduce allocations --- .../src/Internal/Http/Http1OutputProducer.cs | 47 ++++++++++++++++-- .../src/Internal/Http2/Http2OutputProducer.cs | 49 +++++++++++++++++-- .../src/Internal/Http3/Http3OutputProducer.cs | 49 +++++++++++++++++-- .../src/Internal/HttpOutputProducerHelper.cs | 23 --------- 4 files changed, 131 insertions(+), 37 deletions(-) delete mode 100644 src/Servers/Kestrel/Core/src/Internal/HttpOutputProducerHelper.cs diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs index cf0ddafb203e..41abf1f6c53e 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs @@ -49,6 +49,7 @@ internal class Http1OutputProducer : IHttpOutputProducer, IDisposable private readonly ConcurrentPipeWriter _pipeWriter; private IMemoryOwner? _fakeMemoryOwner; + private byte[]? _fakeMemory; // Chunked responses need to be treated uniquely when using GetMemory + Advance. // We need to know the size of the data written to the chunk before calling Advance on the @@ -414,6 +415,12 @@ public void Dispose() _fakeMemoryOwner = null; } + if (_fakeMemory != null) + { + ArrayPool.Shared.Return(_fakeMemory); + _fakeMemory = null; + } + // Call dispose on any memory that wasn't written. if (_completedSegments != null) { @@ -652,14 +659,46 @@ private void WriteCurrentChunkMemoryToPipeWriter(ref BufferWriter wr internal Memory GetFakeMemory(int minSize) { - if (_fakeMemoryOwner == null || _fakeMemoryOwner.Memory.Length < minSize) + // Try to reuse _fakeMemoryOwner + if (_fakeMemoryOwner != null) { - _fakeMemoryOwner?.Dispose(); + if (_fakeMemoryOwner.Memory.Length < minSize) + { + _fakeMemoryOwner.Dispose(); + _fakeMemoryOwner = null; + } + else + { + return _fakeMemoryOwner.Memory; + } + } - _fakeMemoryOwner = HttpOutputProducerHelper.ReserveFakeMemory(_memoryPool, minSize); + // Try to reuse _fakeMemory + if (_fakeMemory != null) + { + if (_fakeMemory.Length < minSize) + { + ArrayPool.Shared.Return(_fakeMemory); + _fakeMemory = null; + } + else + { + return _fakeMemory; + } } - return _fakeMemoryOwner.Memory; + // Requesting a bigger buffer could throw. + if (minSize <= _memoryPool.MaxBufferSize) + { + // Use the specified pool as it fits. + _fakeMemoryOwner = _memoryPool.Rent(minSize); + return _fakeMemoryOwner.Memory; + } + else + { + // Use the array pool. Its MaxBufferSize is int.MaxValue. + return _fakeMemory = ArrayPool.Shared.Rent(minSize); + } } private Memory LeasedMemory(int sizeHint) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs index f4d7078a9b71..7be6cc652090 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2OutputProducer.cs @@ -36,6 +36,7 @@ internal class Http2OutputProducer : IHttpOutputProducer, IHttpOutputAborter, IV private readonly PipeReader _pipeReader; private readonly ManualResetValueTaskSource _resetAwaitable = new ManualResetValueTaskSource(); private IMemoryOwner? _fakeMemoryOwner; + private byte[]? _fakeMemory; private bool _startedWritingDataFrames; private bool _streamCompleted; private bool _suffixSent; @@ -120,6 +121,12 @@ public void Complete() _fakeMemoryOwner.Dispose(); _fakeMemoryOwner = null; } + + if (_fakeMemory != null) + { + ArrayPool.Shared.Return(_fakeMemory); + _fakeMemory = null; + } } } @@ -486,16 +493,48 @@ static void ThrowUnexpectedState() } } - private Memory GetFakeMemory(int minSize) + internal Memory GetFakeMemory(int minSize) { - if (_fakeMemoryOwner == null || _fakeMemoryOwner.Memory.Length < minSize) + // Try to reuse _fakeMemoryOwner + if (_fakeMemoryOwner != null) { - _fakeMemoryOwner?.Dispose(); + if (_fakeMemoryOwner.Memory.Length < minSize) + { + _fakeMemoryOwner.Dispose(); + _fakeMemoryOwner = null; + } + else + { + return _fakeMemoryOwner.Memory; + } + } - _fakeMemoryOwner = HttpOutputProducerHelper.ReserveFakeMemory(_memoryPool, minSize); + // Try to reuse _fakeMemory + if (_fakeMemory != null) + { + if (_fakeMemory.Length < minSize) + { + ArrayPool.Shared.Return(_fakeMemory); + _fakeMemory = null; + } + else + { + return _fakeMemory; + } } - return _fakeMemoryOwner.Memory; + // Requesting a bigger buffer could throw. + if (minSize <= _memoryPool.MaxBufferSize) + { + // Use the specified pool as it fits. + _fakeMemoryOwner = _memoryPool.Rent(minSize); + return _fakeMemoryOwner.Memory; + } + else + { + // Use the array pool. Its MaxBufferSize is int.MaxValue. + return _fakeMemory = ArrayPool.Shared.Rent(minSize); + } } [StackTraceHidden] diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs index 54f45a170f7a..52b05501688e 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3OutputProducer.cs @@ -34,6 +34,7 @@ internal class Http3OutputProducer : IHttpOutputProducer, IHttpOutputAborter private bool _disposed; private bool _suffixSent; private IMemoryOwner? _fakeMemoryOwner; + private byte[]? _fakeMemory; public Http3OutputProducer( Http3FrameWriter frameWriter, @@ -88,6 +89,12 @@ public void Dispose() _fakeMemoryOwner.Dispose(); _fakeMemoryOwner = null; } + + if (_fakeMemory != null) + { + ArrayPool.Shared.Return(_fakeMemory); + _fakeMemory = null; + } } } @@ -208,16 +215,48 @@ public Span GetSpan(int sizeHint = 0) } } - private Memory GetFakeMemory(int minSize) + internal Memory GetFakeMemory(int minSize) { - if (_fakeMemoryOwner == null || _fakeMemoryOwner.Memory.Length < minSize) + // Try to reuse _fakeMemoryOwner + if (_fakeMemoryOwner != null) { - _fakeMemoryOwner?.Dispose(); + if (_fakeMemoryOwner.Memory.Length < minSize) + { + _fakeMemoryOwner.Dispose(); + _fakeMemoryOwner = null; + } + else + { + return _fakeMemoryOwner.Memory; + } + } - _fakeMemoryOwner = HttpOutputProducerHelper.ReserveFakeMemory(_memoryPool, minSize); + // Try to reuse _fakeMemory + if (_fakeMemory != null) + { + if (_fakeMemory.Length < minSize) + { + ArrayPool.Shared.Return(_fakeMemory); + _fakeMemory = null; + } + else + { + return _fakeMemory; + } } - return _fakeMemoryOwner.Memory; + // Requesting a bigger buffer could throw. + if (minSize <= _memoryPool.MaxBufferSize) + { + // Use the specified pool as it fits. + _fakeMemoryOwner = _memoryPool.Rent(minSize); + return _fakeMemoryOwner.Memory; + } + else + { + // Use the array pool. Its MaxBufferSize is int.MaxValue. + return _fakeMemory = ArrayPool.Shared.Rent(minSize); + } } [StackTraceHidden] diff --git a/src/Servers/Kestrel/Core/src/Internal/HttpOutputProducerHelper.cs b/src/Servers/Kestrel/Core/src/Internal/HttpOutputProducerHelper.cs deleted file mode 100644 index a9ab66a7f160..000000000000 --- a/src/Servers/Kestrel/Core/src/Internal/HttpOutputProducerHelper.cs +++ /dev/null @@ -1,23 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -namespace System.Buffers -{ - internal class HttpOutputProducerHelper - { - public static IMemoryOwner ReserveFakeMemory(MemoryPool memoryPool, int minSize) - { - // Requesting a bigger buffer could throw. - if (minSize <= memoryPool.MaxBufferSize) - { - // Use the specified pool as it fits. - return memoryPool.Rent(minSize); - } - else - { - // Use the array pool. Its MaxBufferSize is int.MaxValue. - return MemoryPool.Shared.Rent(minSize); - } - } - } -}