Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

@benaadams
Copy link
Member

@benaadams benaadams commented Aug 24, 2016

Fail faster for Wait(0) when no counts available.

Don't allocate in the Wait(0) and WaitAsync(0) fail paths.

1M iters Server GC

method pre (ms) post (ms) improvement
Wait(0) 33,788.25 12.28 x 2751.5
WaitAsync(0) 360.79 24.38 x 14.8

Memory allocations, saves 360MB over the 1M ops


cancellationToken.ThrowIfCancellationRequested();

if (millisecondsTimeout == 0 && m_currentCount == 0)
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment about why it's safe to be checking this outside of the lock.

@stephentoub
Copy link
Member

Have you run the System.Threading tests from corefx with this?

@benaadams
Copy link
Member Author

Just doing it now (within build time definitions of now)

@benaadams benaadams force-pushed the semaphore-timeout-0 branch from 51501fb to d8bef37 Compare August 24, 2016 22:02
@benaadams
Copy link
Member Author

@stephentoub tested with
msbuild System.Threading.Tests.builds /p:BUILDTOOLS_OVERRIDE_RUNTIME=I:\Work\GitHub\coreclr\bin\Product\Windows_NT.x64.Release

Failure in AsyncMethodNotifications which I assume is unrealated

  Discovering: System.Threading.Tests
  Discovered:  System.Threading.Tests
  Starting:    System.Threading.Tests
     System.Threading.Tests.AsyncLocalTests.AsyncMethodNotifications [FAIL]
        System.TypeLoadException : Could not load type 'YieldAwaiter' from assembly 'System.Threading.Tasks, Version=4.0.12.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'.
        Stack Trace:
              at System.Threading.Tests.AsyncLocalTests.<>c__DisplayClass7_0.<AsyncMethodNotifications>b__1()
           I:\Work\GitHub\corefx\src\System.Threading\tests\AsyncLocalTests.cs(251,0): at System.Threading.Tests.AsyncLocalTests.Run(Func`1 func)
           I:\Work\GitHub\corefx\src\System.Threading\tests\AsyncLocalTests.cs(275,0): at System.Threading.Tests.AsyncLocalTests.<AsyncMethodNotifications>d__7.MoveNext()
           --- End of stack trace from previous location where exception was thrown ---
              at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
              at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
           --- End of stack trace from previous location where exception was thrown ---
              at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
              at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
           --- End of stack trace from previous location where exception was thrown ---
              at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
              at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
  Finished:    System.Threading.Tests

  === TEST EXECUTION SUMMARY ===
I:\Work\GitHub\corefx\Tools\tests.targets(260,5): warning :    System.Threading.Tests  Total: 167, Errors: 0, Failed: 1, Skipped: 0, Time: 5.414s [I:\Work\GitHub\corefx\src\System.Threading\tests\System.Threading.Tests.csproj]

Also assume I have override correct, it doesn't shout that its using the coreclr repo in a way I can identify

@benaadams benaadams force-pushed the semaphore-timeout-0 branch from d8bef37 to b5bb3c3 Compare August 24, 2016 22:10
@benaadams
Copy link
Member Author

Fixed spelling of volatile in comments and squashed

@stephentoub
Copy link
Member

Thanks, Ben. LGTM.

Have you measured to see what the throughput improvement is? (And verified that indeed there aren't any allocations?

@benaadams
Copy link
Member Author

benaadams commented Aug 24, 2016

As aside, a use case example is choice on whether to go async

private bool IsReadyToSend => _outgoingSends.Wait(0);
private Task ReadyToSend => _outgoingSends.WaitAsync();
private bool MaxOutstandingSendsReached => _outgoingSends.CurrentCount == 0;

private Task SendAsync(BufferSpan span, bool endOfMessage)
{
    if (!IsReadyToSend)
    {
        return SendAsyncAwaited(span, endOfMessage);
    }

    var flushSends = endOfMessage || MaxOutstandingSendsReached;

    Send(GetSegmentFromSpan(span), flushSends);

    if (flushSends && !endOfMessage)
    {
        return AwaitReadyToSend();
    }

    return _completedTask;
}

private async Task AwaitReadyToSend()
{
    await ReadyToSend;
}

private async Task SendAsyncAwaited(BufferSpan span, bool endOfMessage)
{
    await ReadyToSend;

    var flushSends = endOfMessage || MaxOutstandingSendsReached;

    Send(GetSegmentFromSpan(span), flushSends);

    if (flushSends && !endOfMessage)
    {
        await ReadyToSend;
    }
}

@benaadams
Copy link
Member Author

benaadams commented Aug 25, 2016

@stephentoub the non-async path fared quite badly before (async blocked: code)

1M iters Server GC

method pre (ms) post (ms) improvement
Wait(0) 33,788.25 12.28 x 2751.5
WaitAsync(0) 360.79 24.38 x 14.8

Just testing allocations


cancellationToken.ThrowIfCancellationRequested();

// Pref: Check the stack timeout parameter before checking the volatile count
Copy link
Member

Choose a reason for hiding this comment

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

typo

@benaadams benaadams force-pushed the semaphore-timeout-0 branch from b5bb3c3 to 21c89ff Compare August 25, 2016 08:30
@benaadams
Copy link
Member Author

benaadams commented Aug 25, 2016

Memory allocations, saves 720MB over the 2M ops

Pre
pre-allocs

Post
post-allocs

@benaadams
Copy link
Member Author

Looks like its working 😄

@benaadams
Copy link
Member Author

@dotnet-bot test Linux ARM Emulator Cross Debug Build

@stephentoub
Copy link
Member

Thanks, @benaadams. LGTM.

@stephentoub stephentoub merged commit 8e794b5 into dotnet:master Aug 25, 2016
@davidfowl
Copy link
Member

👏

migueldeicaza added a commit to migueldeicaza/mono that referenced this pull request Sep 1, 2016
This brings the patch: dotnet/coreclr#6898

Fail faster for `Wait(0)` when no counts available.

Don't allocate in the `Wait(0)` and `WaitAsync(0)` fail paths.

1M iters Server GC

method | pre (ms) | post (ms) | improvement
-------- |------- |------- |------ |
Wait(0) | 33,788.25 | 12.28 | x 2751.5
WaitAsync(0) | 360.79 | 24.38 | x 14.8

Memory allocations, saves 360MB over the 1M ops
@benaadams benaadams deleted the semaphore-timeout-0 branch March 27, 2018 05:11
marek-safar pushed a commit to mono/mono that referenced this pull request Apr 13, 2018
This brings the patch: dotnet/coreclr#6898

Fail faster for `Wait(0)` when no counts available.

Don't allocate in the `Wait(0)` and `WaitAsync(0)` fail paths.

1M iters Server GC

method | pre (ms) | post (ms) | improvement
-------- |------- |------- |------ |
Wait(0) | 33,788.25 | 12.28 | x 2751.5
WaitAsync(0) | 360.79 | 24.38 | x 14.8

Memory allocations, saves 360MB over the 1M ops
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…ut-0

Semaphoreslim, fail faster for timeout 0

Commit migrated from dotnet/coreclr@8e794b5
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants