From 5477e28b474f912affcb55ddf1b940e95256cd20 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 24 Feb 2022 14:40:57 -0800 Subject: [PATCH 1/4] Speed up contended HTTP/2 frame writing --- .../src/Internal/Http2/Http2Connection.cs | 53 +++++++++++++------ 1 file changed, 36 insertions(+), 17 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs index e9444502d89b..56c375919963 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs @@ -36,7 +36,9 @@ internal partial class Http2Connection : IHttp2StreamLifetimeHandler, IHttpStrea private readonly HttpConnectionContext _context; private readonly Http2FrameWriter _frameWriter; private readonly Pipe _input; + private readonly Pipe _output; private readonly Task _inputTask; + private readonly Task _outputTask; private readonly int _minAllocBufferSize; private readonly HPackDecoder _hpackDecoder; private readonly InputFlowControl _inputFlowControl; @@ -83,8 +85,34 @@ public Http2Connection(HttpConnectionContext context) // Capture the ExecutionContext before dispatching HTTP/2 middleware. Will be restored by streams when processing request _context.InitialExecutionContext = ExecutionContext.Capture(); + var inputPipeOptions = new PipeOptions(pool: context.MemoryPool, + readerScheduler: context.ServiceContext.Scheduler, + writerScheduler: PipeScheduler.Inline, + pauseWriterThreshold: 1, + resumeWriterThreshold: 1, + minimumSegmentSize: context.MemoryPool.GetMinimumSegmentSize(), + useSynchronizationContext: false); + + // Never write inline because we do not want to hold Http2FramerWriter._writeLock for potentially expensive TLS + // write operations. This essentially doubles the MaxResponseBufferSize for HTTP/2 connections compared to + // HTTP/1.x. This seems reasonable given HTTP/2's support for many concurrent streams per connection. We don't + // want every write to return an incomplete ValueTasK now that we're dispatching TLS write operations which + // would likely happen with a pauseWriterThreashold of 1, but we still need to respect connection backpressure. + var maxWriteBufferSize = httpLimits.MaxResponseBufferSize ?? 0; + + var outputPipeOptions = new PipeOptions(pool: context.MemoryPool, + readerScheduler: PipeScheduler.ThreadPool, + writerScheduler: PipeScheduler.Inline, + pauseWriterThreshold: maxWriteBufferSize, + resumeWriterThreshold: maxWriteBufferSize / 2, + minimumSegmentSize: context.MemoryPool.GetMinimumSegmentSize(), + useSynchronizationContext: false); + + _input = new Pipe(inputPipeOptions); + _output = new Pipe(outputPipeOptions); + _frameWriter = new Http2FrameWriter( - context.Transport.Output, + _output.Writer, context.ConnectionContext, this, _outputFlowControl, @@ -94,15 +122,6 @@ public Http2Connection(HttpConnectionContext context) context.MemoryPool, context.ServiceContext); - var inputOptions = new PipeOptions(pool: context.MemoryPool, - readerScheduler: context.ServiceContext.Scheduler, - writerScheduler: PipeScheduler.Inline, - pauseWriterThreshold: 1, - resumeWriterThreshold: 1, - minimumSegmentSize: context.MemoryPool.GetMinimumSegmentSize(), - useSynchronizationContext: false); - - _input = new Pipe(inputOptions); _minAllocBufferSize = context.MemoryPool.GetMinimumAllocSize(); _hpackDecoder = new HPackDecoder(http2Limits.HeaderTableSize, http2Limits.MaxRequestHeaderFieldSize); @@ -129,7 +148,8 @@ public Http2Connection(HttpConnectionContext context) _scheduleInline = context.ServiceContext.Scheduler == PipeScheduler.Inline; - _inputTask = ReadInputAsync(); + _inputTask = CopyPipe(_context.Transport.Input, _input.Writer); + _outputTask = CopyPipe(_output.Reader, _context.Transport.Output); } public string ConnectionId => _context.ConnectionId; @@ -381,8 +401,10 @@ public async Task ProcessRequestsAsync(IHttpApplication appl finally { Input.Complete(); + _output.Writer.Complete(); _context.Transport.Input.CancelPendingRead(); await _inputTask; + await _outputTask; } } } @@ -1655,16 +1677,13 @@ public void DecrementActiveClientStreamCount() Interlocked.Decrement(ref _clientActiveStreamCount); } - private async Task ReadInputAsync() + private async Task CopyPipe(PipeReader reader, PipeWriter writer) { Exception? error = null; try { while (true) { - var reader = _context.Transport.Input; - var writer = _input.Writer; - var readResult = await reader.ReadAsync(); if ((readResult.IsCompleted && readResult.Buffer.Length == 0) || readResult.IsCanceled) @@ -1699,8 +1718,8 @@ private async Task ReadInputAsync() } finally { - await _context.Transport.Input.CompleteAsync(); - _input.Writer.Complete(error); + await reader.CompleteAsync(); + await writer.CompleteAsync(error); } } From 4c2f19fdb26f5b1de1ac2e07ded3062a53a01a4a Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 24 Feb 2022 20:26:21 -0800 Subject: [PATCH 2/4] Fix tests --- .../src/Internal/Http2/Http2Connection.cs | 72 ++++++++++++------- 1 file changed, 45 insertions(+), 27 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs index 56c375919963..ebd0ae246c2c 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs @@ -85,31 +85,8 @@ public Http2Connection(HttpConnectionContext context) // Capture the ExecutionContext before dispatching HTTP/2 middleware. Will be restored by streams when processing request _context.InitialExecutionContext = ExecutionContext.Capture(); - var inputPipeOptions = new PipeOptions(pool: context.MemoryPool, - readerScheduler: context.ServiceContext.Scheduler, - writerScheduler: PipeScheduler.Inline, - pauseWriterThreshold: 1, - resumeWriterThreshold: 1, - minimumSegmentSize: context.MemoryPool.GetMinimumSegmentSize(), - useSynchronizationContext: false); - - // Never write inline because we do not want to hold Http2FramerWriter._writeLock for potentially expensive TLS - // write operations. This essentially doubles the MaxResponseBufferSize for HTTP/2 connections compared to - // HTTP/1.x. This seems reasonable given HTTP/2's support for many concurrent streams per connection. We don't - // want every write to return an incomplete ValueTasK now that we're dispatching TLS write operations which - // would likely happen with a pauseWriterThreashold of 1, but we still need to respect connection backpressure. - var maxWriteBufferSize = httpLimits.MaxResponseBufferSize ?? 0; - - var outputPipeOptions = new PipeOptions(pool: context.MemoryPool, - readerScheduler: PipeScheduler.ThreadPool, - writerScheduler: PipeScheduler.Inline, - pauseWriterThreshold: maxWriteBufferSize, - resumeWriterThreshold: maxWriteBufferSize / 2, - minimumSegmentSize: context.MemoryPool.GetMinimumSegmentSize(), - useSynchronizationContext: false); - - _input = new Pipe(inputPipeOptions); - _output = new Pipe(outputPipeOptions); + _input = new Pipe(GetInputPipeOptions()); + _output = new Pipe(GetOutputPipeOptions()); _frameWriter = new Http2FrameWriter( _output.Writer, @@ -1677,6 +1654,46 @@ public void DecrementActiveClientStreamCount() Interlocked.Decrement(ref _clientActiveStreamCount); } + private PipeOptions GetInputPipeOptions() => new PipeOptions(pool: _context.MemoryPool, + readerScheduler: _context.ServiceContext.Scheduler, + writerScheduler: PipeScheduler.Inline, + pauseWriterThreshold: 1, + resumeWriterThreshold: 1, + minimumSegmentSize: _context.MemoryPool.GetMinimumSegmentSize(), + useSynchronizationContext: false); + + private PipeOptions GetOutputPipeOptions() + { + // Never write inline because we do not want to hold Http2FramerWriter._writeLock for potentially expensive TLS + // write operations. This essentially doubles the MaxResponseBufferSize for HTTP/2 connections compared to + // HTTP/1.x. This seems reasonable given HTTP/2's support for many concurrent streams per connection. We don't + // want every write to return an incomplete ValueTask now that we're dispatching TLS write operations which + // would likely happen with a pauseWriterThreashold of 1, but we still need to respect connection back pressure. + var pauseWriterThreshold = _context.ServiceContext.ServerOptions.Limits.MaxResponseBufferSize switch + { + // null means that we have no back pressure + null => 0, + // 0 = no buffering so we need to configure the pipe so the writer waits on the reader directly + 0 => 1, + long limit => limit, + }; + + var resumeWriterThreshold = pauseWriterThreshold switch + { + // The resumeWriterThreashould must be at least 1 to ever resume after pausing. + 1 => 1, + long limit => limit / 2, + }; + + return new PipeOptions(pool: _context.MemoryPool, + readerScheduler: _context.ServiceContext.Scheduler, + writerScheduler: PipeScheduler.Inline, + pauseWriterThreshold: pauseWriterThreshold, + resumeWriterThreshold: resumeWriterThreshold, + minimumSegmentSize: _context.MemoryPool.GetMinimumSegmentSize(), + useSynchronizationContext: false); + } + private async Task CopyPipe(PipeReader reader, PipeWriter writer) { Exception? error = null; @@ -1699,11 +1716,12 @@ private async Task CopyPipe(PipeReader reader, PipeWriter writer) bufferSlice.CopyTo(outputBuffer.Span); - reader.AdvanceTo(bufferSlice.End); writer.Advance(copyAmount); var result = await writer.FlushAsync(); + reader.AdvanceTo(bufferSlice.End); + if (result.IsCompleted || result.IsCanceled) { // flushResult should not be canceled. @@ -1718,7 +1736,7 @@ private async Task CopyPipe(PipeReader reader, PipeWriter writer) } finally { - await reader.CompleteAsync(); + await reader.CompleteAsync(error); await writer.CompleteAsync(error); } } From ff79d09aa868824305ae36f220102a0f60cb0314 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Thu, 24 Feb 2022 21:00:22 -0800 Subject: [PATCH 3/4] CopyPipeAsync --- .../Kestrel/Core/src/Internal/Http2/Http2Connection.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs index ebd0ae246c2c..8ccf52b10407 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs @@ -125,8 +125,8 @@ public Http2Connection(HttpConnectionContext context) _scheduleInline = context.ServiceContext.Scheduler == PipeScheduler.Inline; - _inputTask = CopyPipe(_context.Transport.Input, _input.Writer); - _outputTask = CopyPipe(_output.Reader, _context.Transport.Output); + _inputTask = CopyPipeAsync(_context.Transport.Input, _input.Writer); + _outputTask = CopyPipeAsync(_output.Reader, _context.Transport.Output); } public string ConnectionId => _context.ConnectionId; @@ -1694,7 +1694,7 @@ private PipeOptions GetOutputPipeOptions() useSynchronizationContext: false); } - private async Task CopyPipe(PipeReader reader, PipeWriter writer) + private async Task CopyPipeAsync(PipeReader reader, PipeWriter writer) { Exception? error = null; try From f90389071fea0baf6d6d29b888ff95bb68140843 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Fri, 25 Feb 2022 16:22:41 -0800 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: Aditya Mandaleeka --- .../Kestrel/Core/src/Internal/Http2/Http2Connection.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs index 8ccf52b10407..88aed68720cf 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs @@ -1668,7 +1668,7 @@ private PipeOptions GetOutputPipeOptions() // write operations. This essentially doubles the MaxResponseBufferSize for HTTP/2 connections compared to // HTTP/1.x. This seems reasonable given HTTP/2's support for many concurrent streams per connection. We don't // want every write to return an incomplete ValueTask now that we're dispatching TLS write operations which - // would likely happen with a pauseWriterThreashold of 1, but we still need to respect connection back pressure. + // would likely happen with a pauseWriterThreshold of 1, but we still need to respect connection back pressure. var pauseWriterThreshold = _context.ServiceContext.ServerOptions.Limits.MaxResponseBufferSize switch { // null means that we have no back pressure @@ -1680,7 +1680,7 @@ private PipeOptions GetOutputPipeOptions() var resumeWriterThreshold = pauseWriterThreshold switch { - // The resumeWriterThreashould must be at least 1 to ever resume after pausing. + // The resumeWriterThreshold must be at least 1 to ever resume after pausing. 1 => 1, long limit => limit / 2, };