From 857529d5eb5f58e70ad7ffda0676d9e866a67d81 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Fri, 21 Feb 2020 13:08:13 -0500 Subject: [PATCH] Remove CreditManager from Http2Stream (#32624) The CreditManager implementation supports multiple awaiters, but Http2Stream's CreditManager never has more than one waiter at a time. We can instead just encode similar logic into Http2Stream, and make its waiter a reusable singleton, such that if we have to allocate it, we can just keep reusing it for all subsequent waits. This means we avoid the CreditManager allocation per Http2Stream as well as the Waiter allocation per wait (other than the first). --- .../System.Net.Http/src/System.Net.Http.csproj | 22 ++--- .../Net/Http/SocketsHttpHandler/CreditManager.cs | 109 +++------------------ .../Net/Http/SocketsHttpHandler/CreditWaiter.cs | 107 ++++++++++++++++++++ .../Net/Http/SocketsHttpHandler/Http2Stream.cs | 63 ++++++++++-- 4 files changed, 185 insertions(+), 116 deletions(-) create mode 100644 src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/CreditWaiter.cs diff --git a/src/libraries/System.Net.Http/src/System.Net.Http.csproj b/src/libraries/System.Net.Http/src/System.Net.Http.csproj index 3cd8695..9f24963 100644 --- a/src/libraries/System.Net.Http/src/System.Net.Http.csproj +++ b/src/libraries/System.Net.Http/src/System.Net.Http.csproj @@ -12,7 +12,6 @@ $(DefineConstants);SYSNETHTTP_NO_OPENSSL - @@ -44,11 +43,6 @@ - - - - - @@ -94,7 +88,6 @@ - Common\System\IO\StreamHelpers.CopyValidation.cs @@ -140,15 +133,22 @@ + + + + + + + @@ -162,12 +162,12 @@ - - - - + + + + Common\System\Net\NTAuthentication.Common.cs diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/CreditManager.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/CreditManager.cs index a1f08a2..2812241 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/CreditManager.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/CreditManager.cs @@ -3,10 +3,8 @@ // See the LICENSE file in the project root for more information. using System.Diagnostics; -using System.Runtime.ExceptionServices; using System.Threading; using System.Threading.Tasks; -using System.Threading.Tasks.Sources; namespace System.Net.Http { @@ -18,7 +16,7 @@ namespace System.Net.Http private bool _disposed; /// Circular singly-linked list of active waiters. /// If null, the list is empty. If non-null, this is the tail. If the list has one item, its Next is itself. - private Waiter _waitersTail; + private CreditWaiter _waitersTail; public CreditManager(IHttpTrace owner, string name, int initialCredit) { @@ -33,12 +31,9 @@ namespace System.Net.Http private object SyncObject { - get - { - // Generally locking on "this" is considered poor form, but this type is internal, - // and it's unnecessary overhead to allocate another object just for this purpose. - return this; - } + // Generally locking on "this" is considered poor form, but this type is internal, + // and it's unnecessary overhead to allocate another object just for this purpose. + get => this; } public ValueTask RequestCreditAsync(int amount, CancellationToken cancellationToken) @@ -64,9 +59,10 @@ namespace System.Net.Http if (NetEventSource.IsEnabled) _owner.Trace($"{_name}. requested={amount}, no credit available."); // Otherwise, create a new waiter. - Waiter waiter = cancellationToken.CanBeCanceled ? - new CancelableWaiter(amount, SyncObject, cancellationToken) : - new Waiter(amount); + CreditWaiter waiter = cancellationToken.CanBeCanceled ? + new CancelableCreditWaiter(SyncObject, cancellationToken) : + new CreditWaiter(); + waiter.Amount = amount; // Add the waiter at the tail of the queue. if (_waitersTail is null) @@ -106,7 +102,7 @@ namespace System.Net.Http while (_current > 0 && _waitersTail != null) { // Get the waiter from the head of the queue. - Waiter waiter = _waitersTail.Next; + CreditWaiter waiter = _waitersTail.Next; int granted = Math.Min(waiter.Amount, _current); // Remove the waiter from the list. @@ -143,12 +139,12 @@ namespace System.Net.Http _disposed = true; - Waiter waiter = _waitersTail; + CreditWaiter waiter = _waitersTail; if (waiter != null) { do { - Waiter next = waiter.Next; + CreditWaiter next = waiter.Next; waiter.Next = null; waiter.Dispose(); waiter = next; @@ -159,88 +155,5 @@ namespace System.Net.Http } } } - - /// Represents a waiter for credit. - /// All of the public members on the instance must only be accessed while holding the CreditManager's lock. - private class Waiter : IValueTaskSource - { - public readonly int Amount; - public Waiter Next; - protected ManualResetValueTaskSourceCore _source; - - public Waiter(int amount) - { - Amount = amount; - _source.RunContinuationsAsynchronously = true; - } - - public ValueTask AsValueTask() => new ValueTask(this, _source.Version); - - public bool IsPending => _source.GetStatus(_source.Version) == ValueTaskSourceStatus.Pending; - - public bool TrySetResult(int result) - { - if (IsPending) - { - _source.SetResult(result); - return true; - } - - return false; - } - - public virtual void Dispose() - { - if (IsPending) - { - _source.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(new ObjectDisposedException(nameof(CreditManager), SR.net_http_disposed_while_in_use))); - } - } - - int IValueTaskSource.GetResult(short token) => - _source.GetResult(token); - ValueTaskSourceStatus IValueTaskSource.GetStatus(short token) => - _source.GetStatus(token); - void IValueTaskSource.OnCompleted(Action continuation, object state, short token, ValueTaskSourceOnCompletedFlags flags) => - _source.OnCompleted(continuation, state, token, flags); - } - - private sealed class CancelableWaiter : Waiter - { - private readonly object _syncObj; - private CancellationTokenRegistration _registration; - - public CancelableWaiter(int amount, object syncObj, CancellationToken cancellationToken) : base(amount) - { - _syncObj = syncObj; - _registration = cancellationToken.UnsafeRegister(s => - { - CancelableWaiter thisRef = (CancelableWaiter)s!; - lock (thisRef._syncObj) - { - if (thisRef.IsPending) - { - thisRef._source.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(new OperationCanceledException(thisRef._registration.Token))); - thisRef._registration = default; // benign race with setting in the ctor - - // We don't remove it from the list as we lack a prev pointer that would enable us to do so correctly, - // and it's not worth adding a prev pointer for the rare case of cancellation. We instead just - // check when completing a waiter whether it's already been canceled. As such, we also do not - // dispose it here. - } - } - }, this); - } - - public override void Dispose() - { - Monitor.IsEntered(_syncObj); - - _registration.Dispose(); - _registration = default; - - base.Dispose(); - } - } } } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/CreditWaiter.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/CreditWaiter.cs new file mode 100644 index 0000000..9520eaf --- /dev/null +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/CreditWaiter.cs @@ -0,0 +1,107 @@ +// 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.Diagnostics; +using System.Runtime.ExceptionServices; +using System.Threading; +using System.Threading.Tasks; +using System.Threading.Tasks.Sources; + +namespace System.Net.Http +{ + /// Represents a waiter for credit. + internal class CreditWaiter : IValueTaskSource + { + public int Amount; + public CreditWaiter Next; + protected ManualResetValueTaskSourceCore _source; + + public CreditWaiter() => _source.RunContinuationsAsynchronously = true; + + public ValueTask AsValueTask() => new ValueTask(this, _source.Version); + + public bool IsPending => _source.GetStatus(_source.Version) == ValueTaskSourceStatus.Pending; + + public bool TrySetResult(int result) + { + if (IsPending) + { + _source.SetResult(result); + return true; + } + + return false; + } + + public virtual void CleanUp() { } + + public void Dispose() + { + CleanUp(); + if (IsPending) + { + _source.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(new ObjectDisposedException(nameof(CreditManager), SR.net_http_disposed_while_in_use))); + } + } + + int IValueTaskSource.GetResult(short token) => + _source.GetResult(token); + ValueTaskSourceStatus IValueTaskSource.GetStatus(short token) => + _source.GetStatus(token); + void IValueTaskSource.OnCompleted(Action continuation, object state, short token, ValueTaskSourceOnCompletedFlags flags) => + _source.OnCompleted(continuation, state, token, flags); + } + + /// Represents a cancelable waiter for credit. + internal sealed class CancelableCreditWaiter : CreditWaiter + { + private readonly object _syncObj; + private CancellationTokenRegistration _registration; + + public CancelableCreditWaiter(object syncObj, CancellationToken cancellationToken) + { + _syncObj = syncObj; + RegisterCancellation(cancellationToken); + } + + public void ResetForAwait(CancellationToken cancellationToken) + { + Debug.Assert(Monitor.IsEntered(_syncObj)); + Debug.Assert(!IsPending); + Debug.Assert(Next is null); + Debug.Assert(_registration == default); + + _source.Reset(); + RegisterCancellation(cancellationToken); + } + + public override void CleanUp() + { + Monitor.IsEntered(_syncObj); + _registration.Dispose(); + _registration = default; + } + + private void RegisterCancellation(CancellationToken cancellationToken) + { + _registration = cancellationToken.UnsafeRegister(s => + { + CancelableCreditWaiter thisRef = (CancelableCreditWaiter)s!; + lock (thisRef._syncObj) + { + if (thisRef.IsPending) + { + thisRef._registration = default; // benign race with setting in the ctor + thisRef._source.SetException(ExceptionDispatchInfo.SetCurrentStackTrace(new OperationCanceledException(thisRef._registration.Token))); + + // We don't remove it from the list as we lack a prev pointer that would enable us to do so correctly, + // and it's not worth adding a prev pointer for the rare case of cancellation. We instead just + // check when completing a waiter whether it's already been canceled. As such, we also do not + // dispose it here. + } + } + }, this); + } + } +} diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs index 5746f09..88fb400 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs @@ -31,7 +31,6 @@ namespace System.Net.Http private readonly Http2Connection _connection; private readonly int _streamId; - private readonly CreditManager _streamWindow; private readonly HttpRequestMessage _request; private HttpResponseMessage _response; /// Stores any trailers received after returning the response content to the caller. @@ -39,6 +38,8 @@ namespace System.Net.Http private ArrayBuffer _responseBuffer; // mutable struct, do not make this readonly private int _pendingWindowUpdate; + private CancelableCreditWaiter _creditWaiter; + private int _availableCredit; private StreamCompletionState _requestCompletionState; private StreamCompletionState _responseCompletionState; @@ -104,8 +105,7 @@ namespace System.Net.Http _responseBuffer = new ArrayBuffer(InitialStreamBufferSize, usePool: true); _pendingWindowUpdate = 0; - - _streamWindow = new CreditManager(this, nameof(_streamWindow), initialWindowSize); + _availableCredit = initialWindowSize; _headerBudgetRemaining = connection._pool.Settings._maxResponseHeadersLength * 1024; @@ -135,7 +135,7 @@ namespace System.Net.Http if (NetEventSource.IsEnabled) Trace($"{request}, {nameof(initialWindowSize)}={initialWindowSize}"); } - private object SyncObject => _streamWindow; + private object SyncObject => this; // this isn't handed out to code that may lock on it public int StreamId => _streamId; @@ -326,7 +326,15 @@ namespace System.Net.Http _connection.RemoveStream(this); - _streamWindow.Dispose(); + lock (SyncObject) + { + CreditWaiter w = _creditWaiter; + if (w != null) + { + w.Dispose(); + _creditWaiter = null; + } + } } private void Cancel() @@ -397,7 +405,21 @@ namespace System.Net.Http return (signalWaiter, sendReset); } - public void OnWindowUpdate(int amount) => _streamWindow.AdjustCredit(amount); + public void OnWindowUpdate(int amount) + { + lock (SyncObject) + { + _availableCredit = checked(_availableCredit + amount); + if (_availableCredit > 0 && _creditWaiter != null && _creditWaiter.IsPending) + { + int granted = Math.Min(_availableCredit, _creditWaiter.Amount); + if (_creditWaiter.TrySetResult(granted)) + { + _availableCredit -= granted; + } + } + } + } void IHttpHeadersHandler.OnStaticIndexedHeader(int index) { @@ -1031,7 +1053,34 @@ namespace System.Net.Http { while (buffer.Length > 0) { - int sendSize = await _streamWindow.RequestCreditAsync(buffer.Length, cancellationToken).ConfigureAwait(false); + int sendSize = -1; + lock (SyncObject) + { + if (_availableCredit > 0) + { + sendSize = Math.Min(buffer.Length, _availableCredit); + _availableCredit -= sendSize; + } + else + { + if (_creditWaiter is null) + { + _creditWaiter = new CancelableCreditWaiter(SyncObject, cancellationToken); + } + else + { + Debug.Assert(!_creditWaiter.IsPending); + _creditWaiter.ResetForAwait(cancellationToken); + } + _creditWaiter.Amount = buffer.Length; + } + } + + if (sendSize == -1) + { + sendSize = await _creditWaiter.AsValueTask().ConfigureAwait(false); + _creditWaiter.CleanUp(); + } ReadOnlyMemory current; (current, buffer) = SplitBuffer(buffer, sendSize); -- 2.7.4