Optimise async/await and ValueTask

[petroemil]

Version Used:

.NET Core 2.1

Steps to Reproduce:

I created a small test application to test the pure overhead of using Task/ValueTask, async keyword in an otherwise synchronous methods and await-ing them or using .Result or .GetAwaiter().GetResult().

As a method signature I had 5 (including the reference) different options:
1.1. bool Reference() => true;
1.2. Task<bool> SyncTask() => Task.FromResult(true);
1.3. async Task<bool> AsyncTask() => true;
1.4. ValueTask<bool> SyncValueTask() => new ValueTask<bool>(true);
1.5. async ValueTask<bool> AsyncValueTask() => true;

And on the caller side (for the Task/ValueTask returning options) I had:
2.1. x.Result
2.2. x.GetAwaiter().GetResult()
2.3. await x
2.4. x.IsCompleted ? x.Result : await x
2.5. x.IsCompleted ? x.GetAwaiter().GetResult() : await x

Test results:

As for the Task options, using a Task in a synchronous way (1.2.), the results were very similar, no matter how I was calling it, and it had (on my machine) a fairly stable ~20x execution time compared to the reference implementation.

Just by adding an async keyword to the method (1.3.), the execution time of the method increased to ~60x, compared to the reference implementation, or ~3x, compared to the non-async version of the method.

The async version of ValueTask method (1.5.) was surprisingly slow, the execution time of such method took - regardless of the calling mechanism - around ~100x, compared to the reference implementation.

And last but not least, the actually interesting bit, the non-async version of the ValueTask method (1.4.).
For most call the execution time was somewhere around ~30x, but when using the .Result (2.1. / 2.4.), the execution time dropped down to just ~2-5x, compared to the reference implementation.

Expected Behavior:

One of the things that I would like to see is some extra smartness in the compiler to detect methods with async keyword in the signature but no await in the body and treat them as simple, synchronous methods.

Another potential optimisation could be to detect (in compile time) if we are trying to await an actually synchronous method in which case the await could be just turned into a .Result call (or more generically a .GetAwaiter().GetResult()).

And last but not least, specifically for ValueTask a simple optimisation in the code, like (2.4.) could bring significant performance improvements, minimising the overhead of the method call/execution.

PS:

I know that these overheads fade away when the method actually has something more complicated in the body, but with things like System.IO.Pipelines, the upcoming IAsyncEnumerable or even Orleans Grains, we have lots of cases where the contract has to be "asynchronous" but the actual implementation won't be and in many cases those methods will be very short and simple so even this kind of optimisation could bring some improvement in the overall performance/throughput of these systems.

[CyrusNajmabadi]

Tagging @stephentoub

[stephentoub]

1.2. Task SyncTask() => Task.FromResult(true);

This allocates a new task each time. You'd be better off using a cached task.

One of the things that I would like to see is some extra smartness in the compiler to detect methods with async keyword in the signature but no await in the body and treat them as simple, synchronous methods.

What does "simple, synchronous method" mean? What would a Task<T>-returning method return? How would it handle exceptions? How would it handle changes made to execution context? Etc. It would take a fair amount of code to provide the same semantics, at which point you will have effectively duplicated the code that's currently used by the compiler today... you would need to demonstrate a significant win for it to thus be a worthwhile change by the compiler. Regardless, this is basically the same as #1981.

Another potential optimisation could be to detect (in compile time) if we are trying to await an actually synchronous method

How? That could only be done reliably in a small minority of cases. I don't see the ROI.

And last but not least, specifically for ValueTask a simple optimisation in the code, like (2.4.) could bring significant performance improvements, minimising the overhead of the method call/execution.

You're likely just seeing https://github.com/dotnet/corefx/issues/30655, which is really a dup of https://github.com/dotnet/coreclr/issues/18542. It'd be much better to "simply" fix the underlying codegen issue rather than trying to paper over it in the C# compiler by special-casing this in this manner.

[petroemil]

What does "simple, synchronous method" mean?

Basically going from 1.3 to 1.2 or from 1.5 to 1.4. In which case we can also be sure that the method really does run synchronously. What I can just see is that putting an async keyword on an otherwise synchronous method just immediately triples the overhead of calling it. There's even a compiler warning for it, so it's clearly a known "issue".

Probably the detection of "awaiting a synchronous method" really is a tough question, not at all obvious to reliably detect such thing.

The ValueTask.Result performance is just a weird case and I didn't exactly know why it worked like that, but nonetheless felt like a big improvement so I thought it's worth mentioning. If it can be fixed with an even more generic solution, even better.

[stephentoub]

There's even a compiler warning for it, so it's clearly a known "issue".

That warning has nothing to do with perf.

What I can just see is that putting an async keyword on an otherwise synchronous method just immediately triples the overhead of calling it.

Primarily because of the other behaviors I mentioned: protecting the execution context, ensuring exception don't escape, etc. You would need those same behaviors in any valid compiler transformation here.

[petroemil]

Oooh now I feel a bit stupid. It's probably because I never really interact with Tasks other than await-ing them, so I never thought about these extra behaviors an async method has.

I just always hear that don't use async if my method is synchronous, just use Task.FromResult at the end instead, but no one ever mentions that I should also cover exceptions with Task.FromException... and of course as long as the call is await-ed and wrapped in a try-catch block it will make little-to-no difference, but if I use ContinueWith or Rx to build asynchronous "pipelines" this behavior is absolutely essential, but I'm pretty sure a lot of code breaks it.

Thank you for the answers, I think everything got answered, so the issue can probably be closed.

[stephentoub]

Oooh now I feel a bit stupid

No, this stuff is subtle. 😄