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.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. The only thing that might be useful is some sort of signifier meaning "async method only"

Copy link
Member

@VSadov VSadov Jan 9, 2025

Choose a reason for hiding this comment

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

Await cannot be formally an async method, since it would need to return Task.

}
}
```

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it worth putting something like a modreq(AsyncOnly) on it?

Copy link
Member

Choose a reason for hiding this comment

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

We could, though I'm not sure that would actually help. It's still technically a language change to forbid calling the method.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it's a language change -- the compiler is free to provide more errors than mandated by the spec. For example, "this program is too complex to compile" or the errors provided around usage of the ParamArrayAttribute and similar.

@agocke
Copy link
Member Author

agocke commented Jan 8, 2025

@jakobbotsch Could you take a look at this and give your thoughts? Hoping to move forward or close this out soon.

@VSadov VSadov requested a review from davidwrighton January 8, 2025 23:38
@VSadov
Copy link
Member

VSadov commented Jan 8, 2025

The primary benefit in this case is that it makes the IL valid according to previous versions of the ECMA specification.

I think this is not exactly what is achieved here. Current IL at the call sites is actually "valid" - as in "we call a method that returns T and get result typed as T".

The part that the called method (that returns modopt[Task'1] int) is not found in IL is not entirely new. That happens. The method may be runtime-provided (like Get/Set/Address methods on arrays) on we may not know all the referenced assemblies at IL analysis time.
It becomes a matter of binding/JIT to figure what is actually called and how to call it, but for the purpose of IL correctness the current encoding of call sites is ok.

Besides, we will still need to special case async method bodies to require that returned values be compatible with unwarapped return type in the signature. That seems acceptable, but also means that IL tools will still need to learn a few things about async methods.

I think the main benefit of the proposed call site encoding is that the entire modopt[Task'1] int thing becomes an implementation detail internal to VM/JIT.
We would still need thunks - for implementing/overriding purposes, but would not need to leak any part of their representation into public API.

FWIW it would be possible to choose different ways to represent helper/thunks internally if needed.

@VSadov
Copy link
Member

VSadov commented Jan 8, 2025

Now, if we agree on benefits, what are the disadvantages?

My main concerns are:

  1. is it reasonable to rely on JIT/Interpreter to recognize a pattern. Are there precedents.
    Is it not some kind of unnatural requirement for the JIT?
    @jakobbotsch, @davidwrighton - any insights on this?

  2. how strict is the pattern?
    Is it just

call <Task-returning method>
call <Await>

What about

call <Task-returning method>
nop
call <Await>
  1. What happens when Await is used out of pattern? - Like Await(someRandomTask)
    I think Await could have an actual implementation that is return arg.Result.
    The sync-over-async could be observable in some subtle ways, but considering that noone should emit such code, it might be acceptable.

Or better - it could have the same implementation as what we emit for thuncs (re: EmitAsync2MethodThunk)

Something like:

        // Marked intrinsic since this needs to be
        // recognizes as an async2 call.
        [Intrinsic]
        [BypassReadyToRun]
        [MethodImpl(MethodImplOptions.NoInlining)]
        public static T Await<T>(Task<T> task)
        {
            TaskAwaiter <T> awaiter = task.GetAwaiter();
            ref RuntimeAsyncAwaitState state = ref t_runtimeAsyncAwaitState;
            Continuation? sentinelContinuation = state.SentinelContinuation;
            if (sentinelContinuation == null)
                state.SentinelContinuation = sentinelContinuation = new Continuation();

            state.Notifier = awaiter;
            SuspendAsync2(sentinelContinuation);
            return awaiter.Result;
        }

public static Task AwaitAwaiterFromRuntimeAsync<TAwaiter>(TAwaiter awaiter) where TAwaiter : INotifyCompletion { ... }
public static void AwaitAwaiterFromRuntimeAsync<TAwaiter>(TAwaiter awaiter) where TAwaiter : INotifyCompletion { ... }
[MethodImpl(MethodImplOptions.Async)]
public static void UnsafeAwaitAwaiterFromRuntimeAsync<TAwaiter>(TAwaiter awaiter) where TAwaiter : ICriticalNotifyCompletion
Copy link
Member

@VSadov VSadov Jan 9, 2025

Choose a reason for hiding this comment

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

I assume the purpose of public AwaitAwaiterFromRuntimeAsync would be just for custom awaitables?

Previously, in async2 methods, Roslyn would generate a thunk call in cases when the await argument has Task/ValueTask type and is a method invocation.
Otherwise it would emit

                     {
                       var awaiter = arg.GetAwaiter();
                       if (awaiter.IsComplete())
                       {
                           UnsafeAwaitAwaiterFromRuntimeAsync(awaiter)
                       }
                       awaiter.GetResult()
                     }

Now awaiting anything that has Task type can be lowered into Await helper call.
I.E. await obj.taskField ==> Await(obj.taskField)

public static void UnsafeAwaitAwaiterFromRuntimeAsync<TAwaiter>(TAwaiter awaiter) where TAwaiter : ICriticalNotifyCompletion

[MethodImpl(MethodImplOptions.Async)]
public static void Await(Task task);
Copy link
Contributor

@pentp pentp Jan 9, 2025

Choose a reason for hiding this comment

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

One extremely common use case is await task.ConfigureAwait(false) - with the current set of methods this would need to use the more verbose (and presumably less efficient) methods above, but could consider adding overloads for these Await methods that take a ConfigureAwaitOptions parameter.

Copy link
Member

Choose a reason for hiding this comment

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

Right, it was discussed a bit above. We definitely will need a way to handle these.

@jakobbotsch
Copy link
Member

jakobbotsch commented Jan 9, 2025

@jakobbotsch Could you take a look at this and give your thoughts? Hoping to move forward or close this out soon.

I don't have a strong opinion on this representation over the existing one. I like that the existing one does not have any ambiguities in its handling. Conversely I like that the new one hides away some of the internal details as @VSadov points out above.

  1. is it reasonable to rely on JIT/Interpreter to recognize a pattern. Are there precedents.
    Is it not some kind of unnatural requirement for the JIT?
    @jakobbotsch, @davidwrighton - any insights on this?

There are some precedents to strict pattern matching on IL sequences, like various boxing sequences or array initialization via RuntimeHelpers.InitializeArray.
I'm not sure the pattern here needs to be strictly recognized, i.e. even when unrecognized we can fall back to a default implementation that works.

2. how strict is the pattern?
Is it just

call <Task-returning method>
call <Await>

What about

call <Task-returning method>
nop
call <Await>
  1. What happens when Await is used out of pattern? - Like Await(someRandomTask)
    I think Await could have an actual implementation that is return arg.Result.
    The sync-over-async could be observable in some subtle ways, but considering that noone should emit such code, it might be acceptable.

Or better - it could have the same implementation as what we emit for thuncs (re: EmitAsync2MethodThunk)

Something like:

        // Marked intrinsic since this needs to be
        // recognizes as an async2 call.
        [Intrinsic]
        [BypassReadyToRun]
        [MethodImpl(MethodImplOptions.NoInlining)]
        public static T Await<T>(Task<T> task)
        {
            TaskAwaiter <T> awaiter = task.GetAwaiter();
            ref RuntimeAsyncAwaitState state = ref t_runtimeAsyncAwaitState;
            Continuation? sentinelContinuation = state.SentinelContinuation;
            if (sentinelContinuation == null)
                state.SentinelContinuation = sentinelContinuation = new Continuation();

            state.Notifier = awaiter;
            SuspendAsync2(sentinelContinuation);
            return awaiter.Result;
        }

I think the implementation would just forward to UnsafeAwaitAwaiterFromRuntimeAsync. But it does require some intrinsic recognition in the JIT regardless, since Await and UnsafeAwaitAwaiterFromRuntimeAsync are not going to be "normal" shaped async2 awaited calls. (We are in the same boat today where UnsafeAwaitAwaiterFromRuntimeAsync is specially recognized as an async2 call.)

That intrinsic recognition does seem to complicate things significantly for this representation, however, as we will need Await itself to become a state machine of the right form.

@agocke
Copy link
Member Author

agocke commented Jan 9, 2025

I'm not sure the pattern here needs to be strictly recognized, i.e. even when unrecognized we can fall back to a default implementation that works.

Agreed.

But it does require some intrinsic recognition in the JIT regardless, since Await and UnsafeAwaitAwaiterFromRuntimeAsync are not going to be "normal" shaped async2 awaited calls.

Also agreed, but I think UnsafeAwaitAwaiterFromRuntimeAsync is going to need generalized handling since it will be emitted by Roslyn in various cases where there's an actual task object that needs to be awaited.

That intrinsic recognition does seem to complicate things significantly for this representation, however, as we will need Await itself to become a state machine of the right form.

Can you elaborate? My thought would be that the exact double sequence would be optimized -- a two call instruction that ends with Await would be what we have today with the special signature.

But I imagined that Await could be implemented as a simple transformation. Since we would only actually invoke the body in a rare de-opt situation, it could be as slow as we want.

@jakobbotsch
Copy link
Member

jakobbotsch commented Jan 10, 2025

Also agreed, but I think UnsafeAwaitAwaiterFromRuntimeAsync is going to need generalized handling since it will be emitted by Roslyn in various cases where there's an actual task object that needs to be awaited.

I don't think the handling of UnsafeAwaitAwaiterFromRuntimeAsync is going to change. It will keep being recognized as an async2 call and its implementation will always suspend.

But I imagined that Await could be implemented as a simple transformation. Since we would only actually invoke the body in a rare de-opt situation, it could be as slow as we want.

Well, the question is how we implement the fallback version of Await. It does not look that simple to me since it is going to have to be a state machine but it has none of the standard metadata markers of one.

@VSadov
Copy link
Member

VSadov commented Jan 17, 2025

Well, the question is how we implement the fallback version of Await. It does not look that simple to me since it is going to have to be a state machine but it has none of the standard metadata markers of one.

Await would not have any awaits inside it, so it does not need to be a state machine.
I think it could follow the same plan as UnsafeAwaitAwaiterFromRuntimeAsync.

For a decently performing implementation it would need a value-returning equivalent of SuspendAsync2 (i.e. ReturnAsync2), but even with just SuspendAsync2 it seems doable, just not as efficient due to unconditional suspend.

@VSadov
Copy link
Member

VSadov commented Jan 17, 2025

Since JIT recognizing/optimizing the pattern is optional, it is possible to prototype a rough version of the helper without involving JIT or Roslyn and see if there are any details that we did not see.

I tried to prototype the Await helper for the Task<T>.
Here is a PR with changes (I do not intend to merge it, just to experiment with how it would look like).
dotnet/runtimelab#2941

The change basically implements await as a helper that can be used inside runtime async methods. Like the following:

            long result =
#if AWAIT
                RuntimeHelpers.Await(Run(depth - 1));      // this is what Roslyn would emit with the new API.
#else
                await Run(depth - 1);
#endif

I can’t comment on how easy it is to JIT-optimize the pattern, but so far I hear the optimization should not be too hard.

At runtime/VM side it looks like overall the approach seems feasible. Perhaps it is time to conclude the discussion and move to implementing the proposal.

A few observations:

Magic helpers with nonconforming signatures.

Just like AwaitAwaiterFromRuntimeAsync, the Await is a “magic” helper that breaks some rules.
From the signature it looks like a blocking call that unwraps the Task. Indeed, there is no way to await asynchronously when not returning a promise. How would you signal to the caller that suspend is needed?

That works because the signatures of AwaitAwaiterFromRuntimeAsync and Await helpers do not match their actual behavior. They are in fact async functions that can return continuations to signal that the call chain must be suspended. (they are not resumable though, they do not need to be).

There are other API options that would not require to fit async behavior into helpers with synchronous signature. For example Await could implement only “awaiting” part, but not the “unwrapping” part. In such case it could return the same Task<T> as it takes, but with added guarantee that the returned task is always complete.

I.E.

    // returned task is guaranteed to be completed.
    Task<T> Await<T>(Task<T> t)

Used as

   int x = Await(ReturnsTaskOfInt()).Result;

While this approach avoids the issues with signatures, there are downsides. Primarily - the “Call+Await+Result” is obviously longer and thus more fragile.

There is also a thing with tasks that complete in a faulted state. If Await only guarantees the completion of the task, without observing the result, it would be responsibility of the trailing .Result to throw. Leaving a faulted task in unobserved state does not seem a good idea, so any reasonable use of this Await would better immediately observe the result.

How would one observe a result of a void/nongeneric Await? .Wait()?

Would that actually throw the right exception? (not the AggregateException). We might need to actually do Await(ReturnsTaskOfInt()).GetAwaiter().GetResult(), which is even more verbose.

If awaiting and observing/unwrapping should always come together, then why not just have a helper that does both?
Thus we come back to the original T Await<T>(Task<T> t), whose signature is technically misleading, but might be ok for a "magic" method.

Naming

I wonder if, following the precedent of AwaitAwaiterFromRuntimeAsync, the naming should be

T AwaitTaskFromRuntimeAsync<T>(Task<T>);
T AwaitValueTaskFromRuntimeAsync<T>(ValueTask<T>);
void AwaitTaskFromRuntimeAsync(Task);
void AwaitValueTaskFromRuntimeAsync(ValueTask);

Or just rely on overloading:

T AwaitFromRuntimeAsync<T>(Task<T>);
T AwaitFromRuntimeAsync<T>(ValueTask<T>);
void AwaitFromRuntimeAsync(Task);
void AwaitFromRuntimeAsync(ValueTask);

And perhaps even rename the awaiter version -

void AwaitFromRuntimeAsync<TAwaiter>(TAwaiter awaiter);

and
(why do we have this actually? It seems doing exactly same thing as the other helper)

void UnsafeAwaitFromRuntimeAsync<TAwaiter>(TAwaiter awaiter) ;

@jakobbotsch
Copy link
Member

jakobbotsch commented Jan 17, 2025

Await would not have any awaits inside it, so it does not need to be a state machine.

I think Await will be awaiting UnsafeAwaitAwaiterFromRuntimeAsync. It is going to be most natural for it to be a state machine. In dotnet/runtimelab#2941 you basically just wrote the state machine manually, but I don't see why we shouldn't leave that up to the JIT.

I can’t comment on how easy it is to JIT-optimize the pattern, but so far I hear the optimization should not be too hard.

I would guess the optimization is actually going to be the hardest part of all of this. The VM side logic to figure out how to call a function happens in CEEInfo::getCallInfo, and it is very complex since there are lots of cases to handle (GVMs, shared generics, DIMs, abstract statics, ...). It is passed a token today, and in some cases we leave persistent data around based on that token (e.g. a generic context or a signature for runtime lookups). When we try this optimization we are not going to have a token, so I think getCallInfo will need some non-trivial changes. It will probably start with adding a flag that says "we actually mean the runtime-async variant of the function this token represents".

There is also a thing with tasks that complete in a faulted state. If Await only guarantees the completion of the task, without observing the result, it would be responsibility of the trailing .Result to throw. Leaving a faulted task in unobserved state does not seem a good idea, so any reasonable use of this Await would better immediately observe the result.

How would one observe a result of a void/nongeneric Await? .Wait()?

I think Await should be exactly similar to what Roslyn produces today when awaiting a task. That is, its implementation should be

T Await<T>(Task<T> t)
{
  TaskAwaiter<T> awaiter = t.GetAwaiter();
  if (!awaiter.IsCompleted)
    UnsafeAwaitAwaiterFromRuntimeAsync(awaiter); // this is a runtime-async call

  return awaiter.GetResult();
}

For void tasks this is still going to call awaiter.GetResult().

And perhaps even rename the awaiter version -

void AwaitFromRuntimeAsync(TAwaiter awaiter);
and
(why do we have this actually? It seems doing exactly same thing as the other helper)

One is for awaiters implementing INotifyCompletion and the other is for awaiters implementing ICriticalNotifyCompletion. (Edit -- we might have some bugs around this, AwaitableProxy likely needs some extra state to ensure it uses the right variant of OnCompleted when both interfaces are implemented)

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.

7 participants