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

Propose new async API #110420

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Propose new async API #110420

wants to merge 3 commits into from

Conversation

agocke
Copy link
Member

@agocke agocke commented Dec 4, 2024

I'd appreciate everyone's thoughts on this.

This PR replaces the 'method signature' overloading with a new 'Await' method that JIT should treat as an intrinsic. The primary benefit in this case is that it makes the IL valid according to previous versions of the ECMA specification. Ideally, this should cause fewer failures in tooling that examines IL.

I'm still considering something to deal with the type-mismatch in the return type.

This PR replaces the 'method signature' overloading with a new
'Await' method that JIT should treat as an intrinsic. The primary
benefit in this case is that it makes the IL valid according to previous
versions of the ECMA specification. Ideally, this should cause fewer
failures in tooling that examines IL.
Copy link
Contributor

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

```C#
namespace System.Runtime.CompilerServices
{
public static class RuntimeHelpers
Copy link
Member

Choose a reason for hiding this comment

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

What assembly will define these? Should we restrict it to just system.private.corelib?

Copy link
Member Author

Choose a reason for hiding this comment

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

In practice, this class already exists and is in corelib, so we will put these in corelib. As far as ECMA is concerned, I see no reason to mandate it.

Copy link
Member

Choose a reason for hiding this comment

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

If there's no reason to mandate it, that must mean that the runtime needs to accept a user defining these helpers themselves, rather than the ones the runtime defines. That seems... Unlikely to be what you want.


Callers may retrieve a Task/ValueTask return type from an async method via calling its primary, definitional signature. This functionality is available in both sync and async methods.
These methods are also only legal to call inside async methods. These methods perform suspension like the `AwaitAwaiter...` methods, but are optimized for calling on the return value of a call to an async method. To achieve maximum performance, the IL sequence of two `call` instructions -- one to the async method and immediately one to the `Await` method -- should be preferred.
Copy link
Member

Choose a reason for hiding this comment

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

How does this interact with ConfigureAwait? It seems like 99.999% of all use of await in our core libraries would not use these specialty methods?

Copy link
Member

Choose a reason for hiding this comment

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

One of the things that the group proposed is that the runtime may be able to understand the pattern of RuntimeHelpers.Await(taskMethod().ConfigureAwait(false)) and avoid the Task materialization in that case.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but that doesn't quite fit with the proposed APIs.

Copy link
Member Author

@agocke agocke Dec 5, 2024

Choose a reason for hiding this comment

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

The main problem is that what comes out of ConfigureAwait isn't a task, it's a ConfiguredTaskAwaitable. The only way we have of dealing with this in the current design is to use the AwaitAwaiter... API.

I agree that ConfigureAwait is something we should handle, it's just that it's not a 'new' problem with our API structure.

One option is to add an Await for ConfigureTaskAwaitable as above, and then recognize the triple sequence above.

Copy link
Member

Choose a reason for hiding this comment

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

It seems to raise a question about what the value of this Await function is, over recognizing RuntimeHelpers.AwaitAwaiterFromRuntimeAsync(Async2Function().GetAwaiter()) and RuntimeHelpers.AwaitAwaiterFromRuntimeAsync(Async2Function().ConfigureAwait(constant).GetAwaiter()).
Size comes to mind.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose AwaitAwaiterFromRuntimeAsync does not really have the right shape to be used in the way we'd like here, and it's not easy to generalize it to have a shape that would be usable either.

Copy link
Member Author

@agocke agocke Dec 5, 2024

Choose a reason for hiding this comment

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

Yes, good point. This was implicit, but let's make it explicit. I see the value of Await as:

  • Size of IL
  • Complexity (a clearer sequence to recognize. If we open up GetAwaiter we need to also think about entire blocks like if (!awaiter.IsCompleted) { awaiter.UnsafeAwaitAwaiterFromRuntimeAsync(); }. Await is comparatively simpler)
  • Shorter/simpler sequence for the JIT

Copy link
Member

Choose a reason for hiding this comment

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

That second point is particularly relevant. I expect that's the general pattern that Roslyn will emit for such locations.

@teo-tsirpanis
Copy link
Contributor

the type-mismatch in the return type

Maybe also recognize the FromResult and CompletedTask APIs as intrinsics?

@agocke
Copy link
Member Author

agocke commented Dec 5, 2024

the type-mismatch in the return type

Maybe also recognize the FromResult and CompletedTask APIs as intrinsics?

Two things here:

  1. I'm not sure how valuable it is. This change makes it so that current IL tools will be able to correctly bind the call instructions to async methods and presumably keep control flow going. The failure case of having a "missing" method seems like it could be problematic. Not because scanners shouldn't be able to handle it -- unresolvable calls are not unusual in C# -- but because not resolving the calls could have weird behavior. On the other hand, having a mismatched return type seems like it has a smaller breakage risk. It's not clear to me what failure we would be preventing. It's also a very simple change for any IL logic.

  2. FromResult is a little shaky. The nice part about Await above is that we can actually make the semantics correct even in the case where intrinsification fails. It will just be slower. So if an IL compiler reorders the calls and breaks up the sequence, it should be fine. FromResult is not the same. The actual IL convention for async methods is to return the value directly. If someone took the result, called FromResult on it, stashed it in a local, then returned the value, that would be incorrect async method IL.

I think we should understand the case where we are actually helping an unaware IL rewriter keep working, vs creating a situation that makes things worse and the only fix, regardless of what we do, is for the IL rewriter to fix their code.

[MethodImpl(MethodImplOptions.Async)]
public static T Await<T>(Task<T> task);
[MethodImpl(MethodImplOptions.Async)]
public static T Await<T>(ValueTask<T> task);
Copy link
Member

Choose a reason for hiding this comment

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

I think the MethodImpl part is uninteresting for the spec – it really is an implementation detail of SPC whether or not this bit is set for these, and consumers should not be using it to make any decisions. Similarly for AwaitAwaiterFromRuntimeAsync.

}
}
```

Each of the above methods will have semantics analogous to the current AsyncTaskMethodBuilder.AwaitOnCompleted/AwaitUnsafeOnCompleted methods. After calling this method, it can be presumed that the task has completed. These methods are only legal to call from inside async methods.
These methods are only legal to call inside async methods. The `...AwaitAwaiter...` methods will have semantics analogous to the current `AsyncTaskMethodBuilder.AwaitOnCompleted/AwaitUnsafeOnCompleted` methods. After calling either method, it can be presumed that the task or awaiter has completed. The `Await` methods perform suspension like the `AwaitAwaiter...` methods, but are optimized for calling on the return value of a call to an async method. To achieve maximum performance, the IL sequence of two `call` instructions -- one to the async method and immediately one to the `Await` method -- should be preferred.
Copy link
Member

Choose a reason for hiding this comment

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

These methods are only legal to call inside async methods

From a language perspective, we'd probably have to make an actual change to C# enforce this as a compiler error. We could do a warning without any issue though.

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.

5 participants