-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Use Task.WaitAsync in SemaphoreSlim.WaitAsync #55262
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
Conversation
stephentoub
commented
Jul 7, 2021
| Method | Toolchain | Mean | Ratio | Allocated |
|---|---|---|---|---|
| WithCT | \main\CoreRun.exe | 1.103 us | 1.00 | 496 B |
| WithCT | \pr\CoreRun.exe | 1.032 us | 0.94 | 440 B |
| WithTimeout | \main\CoreRun.exe | 1.492 us | 1.00 | 888 B |
| WithTimeout | \pr\CoreRun.exe | 1.103 us | 0.74 | 536 B |
| WithCTandTimeout | \main\CoreRun.exe | 1.589 us | 1.00 | 904 B |
| WithCTandTimeout | \pr\CoreRun.exe | 1.200 us | 0.75 | 536 B |
|
Tagging subscribers to this area: @mangod9 Issue Details
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Diagnosers;
using BenchmarkDotNet.Running;
using System;
using System.Threading.Tasks;
using System.Threading;
[MemoryDiagnoser]
public class Program
{
public static void Main(string[] args) => BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);
private SemaphoreSlim _sem = new SemaphoreSlim(0, 1);
private CancellationTokenSource _cts = new CancellationTokenSource();
[Benchmark]
public Task WithCT()
{
Task t = _sem.WaitAsync(_cts.Token);
_sem.Release();
return t;
}
[Benchmark]
public Task WithTimeout()
{
Task t = _sem.WaitAsync(TimeSpan.FromMinutes(1));
_sem.Release();
return t;
}
[Benchmark]
public Task WithCTandTimeout()
{
Task t = _sem.WaitAsync(TimeSpan.FromMinutes(1), _cts.Token);
_sem.Release();
return t;
}
}
|
|
Appears that there are some test failures though. |
|
Yes, there's one test deterministically failing. I know why. What I need to figure out is why it ever passed prior to this :-) |
The Cancel_WaitAsync_ContinuationInvokedAsynchronously test was failing, highlighting that we were no longer invoking the continuation asynchronously from the Cancel call. But in fact we were incompletely doing so in the past, such that we'd only force that asynchrony if no timeout was provided... if both a timeout and a token were provided, then we wouldn't. I've enhanced the test to validate both cases, and made sure we now pass.
|
Why not Task.Yield()? Do you care about avoiding the sync context? |
Always :) |
|
What about the extra delegate allocation tho 🙃 |
That occurs only if cancellation is requested? I'm not worried about it :) |