Skip to content

optimize BlockingCollection<T>.GetConsumingEnumerable iterator #69631

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pedrobsaila wants to merge 4 commits into from
Closed

optimize BlockingCollection<T>.GetConsumingEnumerable iterator #69631

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9fe9df7
optimize BlockingCollection<T>.GetConsumingEnumerable iterator
pedrobsaila May 20, 2022
94037cf
assert on non disposed/completed collection
pedrobsaila May 21, 2022
fb26b8f
do not assert on completeness
pedrobsaila May 21, 2022
7605f77
throw exception when cancelling
pedrobsaila May 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 27 additions & 8 deletions ...ies/System.Collections.Concurrent/src/System/Collections/Concurrent/BlockingCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -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);
}

Expand All @@ -649,6 +665,15 @@ public bool TryTake([MaybeNullWhen(false)] out T item, int millisecondsTimeout)
public bool TryTake([MaybeNullWhen(false)] out T item, int millisecondsTimeout, CancellationToken cancellationToken)
{
ValidateMillisecondsTimeout(millisecondsTimeout);

cancellationToken.ThrowIfCancellationRequested();
// If the collection is completed then there is no need remove an item.
if (IsCompleted)
{
item = default(T);
return false;
}

@theodorzoulias theodorzoulias May 24, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

return TryTakeWithNoTimeValidation(out item, millisecondsTimeout, cancellationToken, null);
}

Expand All @@ -670,16 +695,9 @@ public bool TryTake([MaybeNullWhen(false)] out T item, int millisecondsTimeout,
/// <exception cref="System.ObjectDisposedException">If the collection has been disposed.</exception>
private bool TryTakeWithNoTimeValidation([MaybeNullWhen(false)] out T item, int millisecondsTimeout, CancellationToken cancellationToken, CancellationTokenSource? combinedTokenSource)
{
CheckDisposed();
Debug.Assert(!_isDisposed);
item = default(T)!;

@eiriktsarpalis eiriktsarpalis May 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the CheckDisposed(); call in line 673 also be removed in favor of an assert? It seems like IsCompleted is also making that check.

cancellationToken.ThrowIfCancellationRequested();

//If the collection is completed then there is no need to wait.
if (IsCompleted)

@eiriktsarpalis eiriktsarpalis May 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be replaced with Debug.Assert(IsCompleted);

@pedrobsaila pedrobsaila May 21, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
return false;
}
bool waitForSemaphoreWasSuccessful = false;

// set the combined token source to the combinedToken parameter if it is not null (came from GetConsumingEnumerable)
Expand Down Expand Up @@ -1645,6 +1663,7 @@ public IEnumerable<T> GetConsumingEnumerable()
/// <exception cref="OperationCanceledException">If the <see cref="CancellationToken"/> is canceled.</exception>
public IEnumerable<T> GetConsumingEnumerable(CancellationToken cancellationToken)
{
cancellationToken.ThrowIfCancellationRequested();
CancellationTokenSource? linkedTokenSource = null;
try
{
Expand Down