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

Use Task.WaitAsync in SemaphoreSlim.WaitAsync #55262

Merged
merged 2 commits into from
Jul 8, 2021

Conversation

stephentoub
Copy link
Member

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
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;
    }
}

@stephentoub stephentoub added this to the 6.0.0 milestone Jul 7, 2021
@ghost
Copy link

ghost commented Jul 7, 2021

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details
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
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;
    }
}
Author: stephentoub
Assignees: -
Labels:

area-System.Threading, tenet-performance

Milestone: 6.0.0

@mangod9
Copy link
Member

mangod9 commented Jul 7, 2021

Appears that there are some test failures though.

@stephentoub
Copy link
Member Author

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.
@stephentoub stephentoub merged commit e30c200 into dotnet:main Jul 8, 2021
@stephentoub stephentoub deleted the semslimperf branch July 8, 2021 02:20
@davidfowl
Copy link
Member

Why not Task.Yield()? Do you care about avoiding the sync context?

@stephentoub
Copy link
Member Author

Do you care about avoiding the sync context?

Always :)

@davidfowl
Copy link
Member

What about the extra delegate allocation tho 🙃

@stephentoub
Copy link
Member Author

What about the extra delegate allocation tho

That occurs only if cancellation is requested? I'm not worried about it :)

@ghost ghost locked as resolved and limited conversation to collaborators Aug 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants