From d7190193ec9ac0735b457b33de671c1f11fc680a Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 24 Jun 2024 16:10:38 +0200 Subject: [PATCH 1/5] Remove unnecessary state machine allocation --- .../System.Net.Quic/src/System/Net/Quic/QuicStream.cs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs index 55058958a2141..8d02f7edaba99 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs @@ -349,7 +349,7 @@ public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationTo /// The region of memory to write data from. /// The token to monitor for cancellation requests. The default value is . /// Notifies the peer about gracefully closing the write side, i.e.: sends FIN flag with the data. - public async ValueTask WriteAsync(ReadOnlyMemory buffer, bool completeWrites, CancellationToken cancellationToken = default) + public ValueTask WriteAsync(ReadOnlyMemory buffer, bool completeWrites, CancellationToken cancellationToken = default) { ObjectDisposedException.ThrowIf(_disposed == 1, this); @@ -379,8 +379,7 @@ public async ValueTask WriteAsync(ReadOnlyMemory buffer, bool completeWrit // No need to call anything since we already have a result, most likely an exception. if (valueTask.IsCompleted) { - await valueTask.ConfigureAwait(false); - return; + return valueTask; } // For an empty buffer complete immediately, close the writing side of the stream if necessary. @@ -391,8 +390,7 @@ public async ValueTask WriteAsync(ReadOnlyMemory buffer, bool completeWrit { CompleteWrites(); } - await valueTask.ConfigureAwait(false); - return; + return valueTask; } // We own the lock, abort might happen, but exception will get stored instead. @@ -428,7 +426,7 @@ public async ValueTask WriteAsync(ReadOnlyMemory buffer, bool completeWrit } } - await valueTask.ConfigureAwait(false); + return valueTask; } /// From 4b8db0e7a71b7d7738a154ad5b2aa4836d347abc Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 24 Jun 2024 16:38:19 +0200 Subject: [PATCH 2/5] Optimize hot path on QuicStream.ReadAsync --- .../src/System/Net/Quic/QuicStream.cs | 75 +++++++++++-------- 1 file changed, 45 insertions(+), 30 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs index 8d02f7edaba99..f17ca174a1150 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs @@ -262,7 +262,7 @@ internal ValueTask StartAsync(CancellationToken cancellationToken = default) } /// - public override async ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) + public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) { ObjectDisposedException.ThrowIf(_disposed == 1, this); @@ -283,21 +283,41 @@ public override async ValueTask ReadAsync(Memory buffer, Cancellation cancellationToken.ThrowIfCancellationRequested(); } - // The following loop will repeat at most twice depending whether some data are readily available in the buffer (one iteration) or not. - // In which case, it'll wait on RECEIVE or any of PEER_SEND_(SHUTDOWN|ABORTED) event and attempt to copy data in the second iteration. - int totalCopied = 0; - do + if (TryReadAsyncInternal(buffer, out int totalCopied, out ValueTask valueTask, cancellationToken)) + { + // hot path: there was data available. + Debug.Assert(valueTask.IsCompletedSuccessfully, "No pending read expected."); + valueTask.GetAwaiter().GetResult(); + + FinalizeRead(totalCopied); + return ValueTask.FromResult(totalCopied); + } + + // cold path: no data available, await pending read + return ReadAsyncCold(buffer, valueTask, cancellationToken); + + async ValueTask ReadAsyncCold(Memory buffer, ValueTask pendingTask, CancellationToken cancellationToken) + { + await pendingTask.ConfigureAwait(false); + + bool success = TryReadAsyncInternal(buffer, out int totalCopied, out pendingTask, cancellationToken); + Debug.Assert(success, "TryReadAsyncInternal should succeed after the await."); + Debug.Assert(pendingTask.IsCompletedSuccessfully, "No pending read expected."); + pendingTask.GetAwaiter().GetResult(); + + FinalizeRead(totalCopied); + return totalCopied; + } + + bool TryReadAsyncInternal(Memory buffer, out int read, out ValueTask valueTask, CancellationToken cancellationToken) { // Concurrent call, this one lost the race. - if (!_receiveTcs.TryGetValueTask(out ValueTask valueTask, this, cancellationToken)) + if (!_receiveTcs.TryGetValueTask(out valueTask, this, cancellationToken)) { throw new InvalidOperationException(SR.Format(SR.net_io_invalidnestedcall, "read")); } - // Copy data from the buffer, reduce target and increment total. - int copied = _receiveBuffers.CopyTo(buffer, out bool complete, out bool empty); - buffer = buffer.Slice(copied); - totalCopied += copied; + read = _receiveBuffers.CopyTo(buffer, out bool complete, out bool empty); // Make sure the task transitions into final state before the method finishes. if (complete) @@ -306,38 +326,33 @@ public override async ValueTask ReadAsync(Memory buffer, Cancellation } // Unblock the next await to end immediately, i.e. there were/are any data in the buffer. - if (totalCopied > 0 || !empty) + if (read > 0 || !empty) { _receiveTcs.TrySetResult(); } - // This will either wait for RECEIVE event (no data in buffer) or complete immediately and reset the task. - await valueTask.ConfigureAwait(false); + return read > 0 || !empty || complete; + } - // This is the last read, finish even despite not copying anything. - if (complete) + void FinalizeRead(int totalCopied) + { + if (totalCopied > 0 && Interlocked.CompareExchange(ref _receivedNeedsEnable, 0, 1) == 1) { - break; + unsafe + { + ThrowHelper.ThrowIfMsQuicError(MsQuicApi.Api.StreamReceiveSetEnabled( + _handle, + 1), + "StreamReceivedSetEnabled failed"); + } } - } while (!buffer.IsEmpty && totalCopied == 0); // Exit the loop if target buffer is full we at least copied something. - if (totalCopied > 0 && Interlocked.CompareExchange(ref _receivedNeedsEnable, 0, 1) == 1) - { - unsafe + if (NetEventSource.Log.IsEnabled()) { - ThrowHelper.ThrowIfMsQuicError(MsQuicApi.Api.StreamReceiveSetEnabled( - _handle, - 1), - "StreamReceivedSetEnabled failed"); + NetEventSource.Info(this, $"{this} Stream read '{totalCopied}' bytes."); } } - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(this, $"{this} Stream read '{totalCopied}' bytes."); - } - - return totalCopied; } /// From 330c82484de9e9eefb975816d84587f9e22f9945 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 24 Jun 2024 16:48:45 +0200 Subject: [PATCH 3/5] Revert "Optimize hot path on QuicStream.ReadAsync" This reverts commit 4b8db0e7a71b7d7738a154ad5b2aa4836d347abc. --- .../src/System/Net/Quic/QuicStream.cs | 75 ++++++++----------- 1 file changed, 30 insertions(+), 45 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs index f17ca174a1150..8d02f7edaba99 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs @@ -262,7 +262,7 @@ internal ValueTask StartAsync(CancellationToken cancellationToken = default) } /// - public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) + public override async ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) { ObjectDisposedException.ThrowIf(_disposed == 1, this); @@ -283,41 +283,21 @@ public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken.ThrowIfCancellationRequested(); } - if (TryReadAsyncInternal(buffer, out int totalCopied, out ValueTask valueTask, cancellationToken)) - { - // hot path: there was data available. - Debug.Assert(valueTask.IsCompletedSuccessfully, "No pending read expected."); - valueTask.GetAwaiter().GetResult(); - - FinalizeRead(totalCopied); - return ValueTask.FromResult(totalCopied); - } - - // cold path: no data available, await pending read - return ReadAsyncCold(buffer, valueTask, cancellationToken); - - async ValueTask ReadAsyncCold(Memory buffer, ValueTask pendingTask, CancellationToken cancellationToken) - { - await pendingTask.ConfigureAwait(false); - - bool success = TryReadAsyncInternal(buffer, out int totalCopied, out pendingTask, cancellationToken); - Debug.Assert(success, "TryReadAsyncInternal should succeed after the await."); - Debug.Assert(pendingTask.IsCompletedSuccessfully, "No pending read expected."); - pendingTask.GetAwaiter().GetResult(); - - FinalizeRead(totalCopied); - return totalCopied; - } - - bool TryReadAsyncInternal(Memory buffer, out int read, out ValueTask valueTask, CancellationToken cancellationToken) + // The following loop will repeat at most twice depending whether some data are readily available in the buffer (one iteration) or not. + // In which case, it'll wait on RECEIVE or any of PEER_SEND_(SHUTDOWN|ABORTED) event and attempt to copy data in the second iteration. + int totalCopied = 0; + do { // Concurrent call, this one lost the race. - if (!_receiveTcs.TryGetValueTask(out valueTask, this, cancellationToken)) + if (!_receiveTcs.TryGetValueTask(out ValueTask valueTask, this, cancellationToken)) { throw new InvalidOperationException(SR.Format(SR.net_io_invalidnestedcall, "read")); } - read = _receiveBuffers.CopyTo(buffer, out bool complete, out bool empty); + // Copy data from the buffer, reduce target and increment total. + int copied = _receiveBuffers.CopyTo(buffer, out bool complete, out bool empty); + buffer = buffer.Slice(copied); + totalCopied += copied; // Make sure the task transitions into final state before the method finishes. if (complete) @@ -326,33 +306,38 @@ bool TryReadAsyncInternal(Memory buffer, out int read, out ValueTask value } // Unblock the next await to end immediately, i.e. there were/are any data in the buffer. - if (read > 0 || !empty) + if (totalCopied > 0 || !empty) { _receiveTcs.TrySetResult(); } - return read > 0 || !empty || complete; - } + // This will either wait for RECEIVE event (no data in buffer) or complete immediately and reset the task. + await valueTask.ConfigureAwait(false); - void FinalizeRead(int totalCopied) - { - if (totalCopied > 0 && Interlocked.CompareExchange(ref _receivedNeedsEnable, 0, 1) == 1) + // This is the last read, finish even despite not copying anything. + if (complete) { - unsafe - { - ThrowHelper.ThrowIfMsQuicError(MsQuicApi.Api.StreamReceiveSetEnabled( - _handle, - 1), - "StreamReceivedSetEnabled failed"); - } + break; } + } while (!buffer.IsEmpty && totalCopied == 0); // Exit the loop if target buffer is full we at least copied something. - if (NetEventSource.Log.IsEnabled()) + if (totalCopied > 0 && Interlocked.CompareExchange(ref _receivedNeedsEnable, 0, 1) == 1) + { + unsafe { - NetEventSource.Info(this, $"{this} Stream read '{totalCopied}' bytes."); + ThrowHelper.ThrowIfMsQuicError(MsQuicApi.Api.StreamReceiveSetEnabled( + _handle, + 1), + "StreamReceivedSetEnabled failed"); } } + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Info(this, $"{this} Stream read '{totalCopied}' bytes."); + } + + return totalCopied; } /// From 885ef9cebf4257bc94e950addcdfd4b6cec16a8b Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 25 Jun 2024 15:48:26 +0200 Subject: [PATCH 4/5] No explicit throwing --- .../src/System/Net/Quic/QuicStream.cs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs index 8d02f7edaba99..e13bbed6e8e21 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs @@ -351,11 +351,14 @@ public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationTo /// Notifies the peer about gracefully closing the write side, i.e.: sends FIN flag with the data. public ValueTask WriteAsync(ReadOnlyMemory buffer, bool completeWrites, CancellationToken cancellationToken = default) { - ObjectDisposedException.ThrowIf(_disposed == 1, this); + if (_disposed == 1) + { + return ValueTask.FromException(new ObjectDisposedException(nameof(QuicStream))); + } if (!_canWrite) { - throw new InvalidOperationException(SR.net_quic_writing_notallowed); + return ValueTask.FromException(new InvalidOperationException(SR.net_quic_writing_notallowed)); } if (NetEventSource.Log.IsEnabled()) @@ -363,17 +366,17 @@ public ValueTask WriteAsync(ReadOnlyMemory buffer, bool completeWrites, Ca NetEventSource.Info(this, $"{this} Stream writing memory of '{buffer.Length}' bytes while {(completeWrites ? "completing" : "not completing")} writes."); } - if (_sendTcs.IsCompleted) + if (_sendTcs.IsCompleted && cancellationToken.IsCancellationRequested) { // Special case exception type for pre-canceled token while we've already transitioned to a final state and don't need to abort write. // It must happen before we try to get the value task, since the task source is versioned and each instance must be awaited. - cancellationToken.ThrowIfCancellationRequested(); + return ValueTask.FromCanceled(cancellationToken); } // Concurrent call, this one lost the race. if (!_sendTcs.TryGetValueTask(out ValueTask valueTask, this, cancellationToken)) { - throw new InvalidOperationException(SR.Format(SR.net_io_invalidnestedcall, "write")); + return ValueTask.FromException(new InvalidOperationException(SR.Format(SR.net_io_invalidnestedcall, "write"))); } // No need to call anything since we already have a result, most likely an exception. From 45083203f2d8b174094df31b29e9271e7b05c505 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 25 Jun 2024 17:12:50 +0200 Subject: [PATCH 5/5] ExceptionDispatchInfo.SetCurrentStackTrace --- .../System.Net.Quic/src/System/Net/Quic/QuicStream.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs index e13bbed6e8e21..07b5ae342be72 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs @@ -4,6 +4,7 @@ using System.Diagnostics; using System.IO; using System.Runtime.CompilerServices; +using System.Runtime.ExceptionServices; using System.Runtime.InteropServices; using System.Threading; using System.Threading.Tasks; @@ -353,12 +354,12 @@ public ValueTask WriteAsync(ReadOnlyMemory buffer, bool completeWrites, Ca { if (_disposed == 1) { - return ValueTask.FromException(new ObjectDisposedException(nameof(QuicStream))); + return ValueTask.FromException(ExceptionDispatchInfo.SetCurrentStackTrace(new ObjectDisposedException(nameof(QuicStream)))); } if (!_canWrite) { - return ValueTask.FromException(new InvalidOperationException(SR.net_quic_writing_notallowed)); + return ValueTask.FromException(ExceptionDispatchInfo.SetCurrentStackTrace(new InvalidOperationException(SR.net_quic_writing_notallowed))); } if (NetEventSource.Log.IsEnabled()) @@ -376,7 +377,7 @@ public ValueTask WriteAsync(ReadOnlyMemory buffer, bool completeWrites, Ca // Concurrent call, this one lost the race. if (!_sendTcs.TryGetValueTask(out ValueTask valueTask, this, cancellationToken)) { - return ValueTask.FromException(new InvalidOperationException(SR.Format(SR.net_io_invalidnestedcall, "write"))); + return ValueTask.FromException(ExceptionDispatchInfo.SetCurrentStackTrace(new InvalidOperationException(SR.Format(SR.net_io_invalidnestedcall, "write")))); } // No need to call anything since we already have a result, most likely an exception.