Repository navigation
optimize BlockingCollection<T>.GetConsumingEnumerable iterator - #69631
pedrobsaila wants to merge 4 commits into
Conversation
ghost
added
area-System.Collections
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-collections Issue DetailsFixes #69320 I tried to search why this double-check had been done. In history, it exists since .NETCore 1.0 RC. Could not find what version of .NET Framework introduced this change (just found that it exists in .NET 4.8)
|
|
Hi @pedrobsaila! Please take a look at this recent comment, regarding the behavior of the |
| cancellationToken.ThrowIfCancellationRequested(); | ||
|
|
||
| //If the collection is completed then there is no need to wait. | ||
| if (IsCompleted) |
There was a problem hiding this comment.
Should be replaced with Debug.Assert(IsCompleted);
There was a problem hiding this comment.
Should be replaced with
Debug.Assert(IsCompleted);
IsCompleted can be changed from antoher thread , the assert would not be always true https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-69631-merge-a83f55ae749f443aa3/System.Collections.Concurrent.Tests/1/console.b9f2d628.log?helixlogtype=result
| @@ -675,11 +699,6 @@ private bool TryTakeWithNoTimeValidation([MaybeNullWhen(false)] out T item, int | |||
|
|
|||
There was a problem hiding this comment.
Can the CheckDisposed(); call in line 673 also be removed in favor of an assert? It seems like IsCompleted is also making that check.
eiriktsarpalis
left a comment
eiriktsarpalis
left a comment
There was a problem hiding this comment.
I'm not sure if we could call this change an "optimization" unless there are demonstrable performance improvements in benchmarks.
Here's the source code of the benchmark dotnet/performance@eb01ff1. I do benchmark for the worst case which is when collection is not complete. The opposite case will have the same results I guess, my change is not supposed to impact it
|
| item = default(T); | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
@pedrobsaila this alters the behavior of the Take method in case the collection is completed and the cancellationToken is canceled. The existing behavior is to throw an OperationCanceledException. The new behavior will be to return false. Altering the behavior of the method is unlikely to be desirable. A demonstration of the current behavior can be found in this comment.
| { | ||
| ValidateTimeout(timeout); | ||
|
|
||
| //If the collection is completed then there is no need remove an item. |
There was a problem hiding this comment.
Maybe:
| //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. |
Can you clarify what is meant by a "double check" in this case? Judging by the diff it seems that you have moved the check out of |
Is the code in pedrobsaila:69320 branch
Is the code in main branch |
|
@pedrobsaila I get that, but I fail to see which code path avoids checking |
It is the |
|
I see, so it basically only concerns this callsite? Not necessarily convinced shaving a few percentage points is worth the potential risk of accidental regression. If anything, BlockingCollection has been superseded by more modern patterns such as System.Threading.Channels so we should encourage users to adopt that instead. |
Exactly
I understand. There is not a substantial improvement and if we take into consideration the error, maybe there is any of all. I just wanted to try doing something related to perf for the first time. You can close it. |
@pedrobsaila when I made the initial proposal, my idea was about a localized change inside the |
@eiriktsarpalis my understanding is that the Channels are intended for asynchronous scenarios. Using them is synchronous scenarios will require blocking on asynchronous methods that return |
I think you are talking about this : I did not follow the initial proposal, because (correct me if I'm wrong) your proposal assumes that when there is no element to take, |
|
@pedrobsaila yes, this is the change I had in mind. Surprisingly the |
|
@pedrobsaila in that case, I'm going to close this PR. Thank you for the contribution! |
|
@eiriktsarpalis, the original issue should be closed as well then? |
|
Per @theodorzoulias's comment in #69631 (comment) the original issue was proposing a different change, although I haven't validated the feasibility or usefulness of the proposal. |
The initial proposal changes the behaviour of |
@pedrobsaila have you confirmed this experimentally, or you infer it by reading the code? |
yes I experimented it, try : |
|
@pedrobsaila please take a look this online demo. I implemented the |
Yes the test blocks in your proposal also (sorry for that false alarm). It's because we block also when collection is empty with |
|
@pedrobsaila be aware than my proposal changes the behavior of the |
ghost
locked as resolved and limited conversation to collaborators
5 participants
Fixes #69320
I tried to search why this double-check had been done. It exists since the first RC of .NETCore 1.0. Could not find what version of .NET Framework introduced this change (just found that it exists in .NET 4.8)