Skip to content
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.

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

Add AsyncBarrier.SignalAndWait(CancellationToken) overload #1330

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

AArnott
Copy link
Member

@AArnott AArnott commented Jun 26, 2024

Also optimize the pre-existing code.
The new overload returns ValueTask to leave open the option of optimizing to zero-allocation use via IValueTaskSource.

Closes #1329

@AArnott AArnott added this to the v17.11 milestone Jun 26, 2024
Also optimize the pre-existing code.
The new overload returns `ValueTask` to leave open the option of optimizing to zero-allocation use via `IValueTaskSource`.
@AArnott AArnott modified the milestones: v17.11, v17.12 Jul 3, 2024
Copy link
Member

@lifengl lifengl left a comment

Choose a reason for hiding this comment

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

The PR looks good to me. It is interesting that we defined the signature of the new method to return ValueTask, instead of Task, which seems to make two overloads to have different return types, while it doesn't really save any memory allocation in this case, because we either return Task.CompletedTask or a TaskCompletionSource is always created in the implementation. Do we really have a strong reason to adopt ValueTask to make the two overloads different here?

@AArnott
Copy link
Member Author

AArnott commented Jul 5, 2024

Do we really have a strong reason to adopt ValueTask to make the two overloads different here?

Yielding Task returning methods must allocate a new Task object on every invocation. ValueTask does not have this restriction. This is how .NET has achieved amortized alloc-free yielding awaits. They use ValueTask for many of their new APIs.
Given this is an async primitive (albeit a very uncommon one), it seems prudent to prepare our API to allow for alloc-free patterns wherever we have the opportunity.
You're right that our implementation isn't alloc-free in the yielding case today. But we could fix that as an implementation detail in the future if we needed to, but only if the return type we choose now supports it.

@AArnott AArnott merged commit 678cc14 into microsoft:main Jul 5, 2024
5 checks passed
@AArnott AArnott deleted the fix1329 branch July 5, 2024 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support cancellation in AsyncBarrier
3 participants