Repository navigation
Can the BlockingCollection<T>.GetConsumingEnumerable iterator be optimized? #69320
Description
Activity
Is the
TryTakeWithNoTimeValidationmethod invoked elsewhere? If not, the check might exist for historical reasons that no longer apply, and replacing it with aDebug.Assert(IsCompleted);assertion is a good idea. Would you be interested in prototyping such an improvement?@eiriktsarpalis yes, the
TryTakeWithNoTimeValidationis also used by these three methods:public bool TryTake(out T item, TimeSpan timeout); public bool TryTake(out T item, int millisecondsTimeout); public bool TryTake(out T item, int millisecondsTimeout, CancellationToken cancellationToken);
The check is needed for these methods. Removing it would most likely cause performance regression. It's just that the
GetConsumingEnumerableiterator does also this check just before calling this method, so it seems like the check in the iteratorwhile (!IsCompleted)is superfluous.Regarding prototyping an improvement, I am not aware of the procedure. Is there any document that I could read about it?
Take a look at the workflow guide to get started on building and testing the repo.
@eiriktsarpalis thanks for the link. I am seeing that building the repository is a quite involved process, so I'll skip it for now. Feel free to close this issue if you think that it's not actionable. The impact of removing the superfluous check of a
volatilefield should be quite minuscule after all. 😃I think that I found the answer why the "superfluous" second check exists. In the case of a completed
BlockingCollection<T>, theGetConsumingEnumerableiterator has different behavior than theTake/TryTakemethods when cancellation is involved. The iterator ignores the cancellation and completes normally, while theTake/TryTakehonor the cancellation and complete with exception. Here is a minimal demonstration of this behavior:var blockingCollection = new BlockingCollection<object>(); blockingCollection.CompleteAdding(); var token = new CancellationToken(true); // Canceled Test("Take", () => blockingCollection.Take(token)); Test("TryTake", () => blockingCollection.TryTake(out _, Timeout.Infinite, token)); Test("GetConsumingEnumerable", () => { foreach (var item in blockingCollection.GetConsumingEnumerable(token)) { } }); static void Test(string title, Action action) { try { action(); Console.WriteLine($"{title}, OK!"); } catch (Exception ex) { Console.WriteLine($"{title}, Error: {ex.GetType().Name}"); } }
Output:
Take, Error: OperationCanceledException TryTake, Error: OperationCanceledException GetConsumingEnumerable, OK!So the optimization that I suggested is problematic, because it results in a behavioral change: The iterator will start throwing exceptions in conditions that previously didn't.
I am not closing this issue, because it might be possible to eliminate the double check without altering the current behavior.
I am not closing this issue, because it might be possible to eliminate the double check without altering the current behavior.
I guess my question would be, why is it important to eliminate the double check? Is it demonstrably impacting performance? I wouldn't think so, this is a concurrent collection so I find it unlikely that a redundant check would be a performance bottleneck.
@eiriktsarpalis the
BlockingCollection<T>.IsCompletedproperty (source code) calls theSemaphoreSlim.CurrentCountproperty (source code), which is backed by avolatilefield. My understanding is that accessing avolatilefield results in emitting a half fence, which is not something completely trivial. From what I know it amounts to a few dozens CPU cycles, but I might be wrong.- ghost locked as resolved and limited conversation to collaborators
on Feb 9, 2023
Hi! I noticed that the implementation of the
BlockingCollection<T>.GetConsumingEnumerablemethod checks twice if the collectionIsCompletedon each iteration. It is checked in thewhileloop:...and also inside the
TryTakeWithNoTimeValidationmethod:I would like to ask if there is a technical reason for this double check, or if it's something that could be optimized. Like this for example:
Thanks!