From 9fe9df76d135ed5a9fce2a6d9aa46dcf38019068 Mon Sep 17 00:00:00 2001 From: pedrobsaila Date: Sat, 21 May 2022 00:39:55 +0200 Subject: [PATCH 1/4] optimize BlockingCollection.GetConsumingEnumerable iterator --- .../Concurrent/BlockingCollection.cs | 29 +++++++++++++++---- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/BlockingCollection.cs b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/BlockingCollection.cs index e4ec98502a72f5..7e254f44a64a2e 100644 --- a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/BlockingCollection.cs +++ b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/BlockingCollection.cs @@ -603,6 +603,14 @@ public bool TryTake([MaybeNullWhen(false)] out T item) public bool TryTake([MaybeNullWhen(false)] out T item, TimeSpan timeout) { ValidateTimeout(timeout); + + //If the collection is completed then there is no need remove an item. + if (IsCompleted) + { + item = default(T); + return false; + } + return TryTakeWithNoTimeValidation(out item, (int)timeout.TotalMilliseconds, CancellationToken.None, null); } @@ -624,6 +632,14 @@ public bool TryTake([MaybeNullWhen(false)] out T item, TimeSpan timeout) public bool TryTake([MaybeNullWhen(false)] out T item, int millisecondsTimeout) { ValidateMillisecondsTimeout(millisecondsTimeout); + + //If the collection is completed then there is no need remove an item. + if (IsCompleted) + { + item = default(T); + return false; + } + return TryTakeWithNoTimeValidation(out item, millisecondsTimeout, CancellationToken.None, null); } @@ -649,6 +665,14 @@ public bool TryTake([MaybeNullWhen(false)] out T item, int millisecondsTimeout) public bool TryTake([MaybeNullWhen(false)] out T item, int millisecondsTimeout, CancellationToken cancellationToken) { ValidateMillisecondsTimeout(millisecondsTimeout); + + //If the collection is completed then there is no need remove an item. + if (IsCompleted) + { + item = default(T); + return false; + } + return TryTakeWithNoTimeValidation(out item, millisecondsTimeout, cancellationToken, null); } @@ -675,11 +699,6 @@ private bool TryTakeWithNoTimeValidation([MaybeNullWhen(false)] out T item, int cancellationToken.ThrowIfCancellationRequested(); - //If the collection is completed then there is no need to wait. - if (IsCompleted) - { - return false; - } bool waitForSemaphoreWasSuccessful = false; // set the combined token source to the combinedToken parameter if it is not null (came from GetConsumingEnumerable) From 94037cffba3473d78aa4e7326fd41720c26e1047 Mon Sep 17 00:00:00 2001 From: pedrobsaila Date: Sat, 21 May 2022 12:50:56 +0200 Subject: [PATCH 2/4] assert on non disposed/completed collection --- .../src/System/Collections/Concurrent/BlockingCollection.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/BlockingCollection.cs b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/BlockingCollection.cs index 7e254f44a64a2e..07a6be2c33b523 100644 --- a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/BlockingCollection.cs +++ b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/BlockingCollection.cs @@ -694,7 +694,8 @@ public bool TryTake([MaybeNullWhen(false)] out T item, int millisecondsTimeout, /// If the collection has been disposed. private bool TryTakeWithNoTimeValidation([MaybeNullWhen(false)] out T item, int millisecondsTimeout, CancellationToken cancellationToken, CancellationTokenSource? combinedTokenSource) { - CheckDisposed(); + Debug.Assert(!_isDisposed); + Debug.Assert(!IsCompleted); item = default(T)!; cancellationToken.ThrowIfCancellationRequested(); From fb26b8f726b6109b13a1a323dfb41af0832cfc08 Mon Sep 17 00:00:00 2001 From: pedrobsaila Date: Sat, 21 May 2022 15:04:42 +0200 Subject: [PATCH 3/4] do not assert on completeness --- .../src/System/Collections/Concurrent/BlockingCollection.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/BlockingCollection.cs b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/BlockingCollection.cs index 07a6be2c33b523..a1aa9d24de5bba 100644 --- a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/BlockingCollection.cs +++ b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/BlockingCollection.cs @@ -695,7 +695,6 @@ public bool TryTake([MaybeNullWhen(false)] out T item, int millisecondsTimeout, private bool TryTakeWithNoTimeValidation([MaybeNullWhen(false)] out T item, int millisecondsTimeout, CancellationToken cancellationToken, CancellationTokenSource? combinedTokenSource) { Debug.Assert(!_isDisposed); - Debug.Assert(!IsCompleted); item = default(T)!; cancellationToken.ThrowIfCancellationRequested(); From 7605f77428d2a6e3bd0359dada078149c6935070 Mon Sep 17 00:00:00 2001 From: pedrobsaila Date: Tue, 24 May 2022 21:23:11 +0200 Subject: [PATCH 4/4] throw exception when cancelling --- .../Collections/Concurrent/BlockingCollection.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/BlockingCollection.cs b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/BlockingCollection.cs index a1aa9d24de5bba..eba5ed1b6b8dcb 100644 --- a/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/BlockingCollection.cs +++ b/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/BlockingCollection.cs @@ -604,7 +604,7 @@ public bool TryTake([MaybeNullWhen(false)] out T item, TimeSpan timeout) { ValidateTimeout(timeout); - //If the collection is completed then there is no need remove an item. + // If the collection is completed then there is no need remove an item. if (IsCompleted) { item = default(T); @@ -633,7 +633,7 @@ public bool TryTake([MaybeNullWhen(false)] out T item, int millisecondsTimeout) { ValidateMillisecondsTimeout(millisecondsTimeout); - //If the collection is completed then there is no need remove an item. + // If the collection is completed then there is no need remove an item. if (IsCompleted) { item = default(T); @@ -666,7 +666,8 @@ public bool TryTake([MaybeNullWhen(false)] out T item, int millisecondsTimeout, { ValidateMillisecondsTimeout(millisecondsTimeout); - //If the collection is completed then there is no need remove an item. + cancellationToken.ThrowIfCancellationRequested(); + // If the collection is completed then there is no need remove an item. if (IsCompleted) { item = default(T); @@ -697,8 +698,6 @@ private bool TryTakeWithNoTimeValidation([MaybeNullWhen(false)] out T item, int Debug.Assert(!_isDisposed); item = default(T)!; - cancellationToken.ThrowIfCancellationRequested(); - bool waitForSemaphoreWasSuccessful = false; // set the combined token source to the combinedToken parameter if it is not null (came from GetConsumingEnumerable) @@ -1664,6 +1663,7 @@ public IEnumerable GetConsumingEnumerable() /// If the is canceled. public IEnumerable GetConsumingEnumerable(CancellationToken cancellationToken) { + cancellationToken.ThrowIfCancellationRequested(); CancellationTokenSource? linkedTokenSource = null; try {