[RuntimeAsync] Use the newest API #3098
Conversation
|
CC: @agocke |
| return awaiter.GetResult(); | ||
| } | ||
| #else | ||
| public static void UnsafeAwaitAwaiter<TAwaiter>(TAwaiter awaiter) where TAwaiter : ICriticalNotifyCompletion { throw new NotImplementedException(); } |
There was a problem hiding this comment.
I am using this (as opposed to compat suppression) as suppressions do not seem to work with Mono.
If the enablement of the feature is tied to the presence of the API, then omitting helpers and suppressing compat failures could be a better option.
There was a problem hiding this comment.
The build and packaging system requires reference assemblies to be identical between CoreCLR vs. Mono. If you suppressed compat failures, you would still get the AsyncHelpers type in reference assembly. The only difference would be MissingMethodException vs. NotImplementedException at runtime if these APIs get called.
There was a problem hiding this comment.
Previously we had suppression for NativeAOT and helpers only for CoreCLR and clr partition could build. But not mono. The compat pass complains about diffs with ref and recommends to generate a suppression file, but generated file somehow does not have any effect and mono still fails to build.
I suspected the location of the file may be incorrect, but could not find another suppression file under mono.
There are some unimplemented public APIs on mono, like under GC, but they appear to throw NotSupported or similar.
There was a problem hiding this comment.
I just want mono to build at this point (and pass existing tests, of course).
In runtimelabs it is not a problem since we build/test locally, but in main repo we need no regressions in mono.
There was a problem hiding this comment.
I suspected the location of the file may be incorrect, but could not find another suppression file under mono.
There is no suppression file under mono since there is nothing to suppress. NativeAOT CoreLib is outlier for historic reasons.
I just want mono to build at this point (and pass existing tests, of course).
Right, the ifdef that you have in this change is the best way to do that.
| AsyncHelpers.AsyncSuspend(sentinelContinuation); | ||
| } | ||
|
|
||
| // Marked intrinsic since JIT recognises the helper by name when doing optimizations. |
There was a problem hiding this comment.
Nit: I do not think that these comments are needed. It is reworded copy&paste of the IntrinsicAttribute description. Several thousand of methods are marked with this attribute in CoreLib.
There was a problem hiding this comment.
We had some intrinsics because
- they had to be recognized by name to be non-Task
Asyncmethods, or - because they were participating in optimizations or were otherwise "magic" methods implemented by the runtime.
There were some explanations on which is which.
Now the #1 is no longer a case, since we use MethodImpl.Async instead of recognizing by name.
I.E. UnsafeAwaitAwaiter is not an intrinsic anymore - as in JIT has no special knowledge of it. It is a non-Task Async method, but there can be many. The "magic" part is done by AsyncSuspend, which is indeed intrinsic, but otherwise we could add more helpers that call AsyncSuspend without touching JIT.
So only #2 case remains, which is the most common purpose of [Intrinsic], thus indeed does not require a comment.
There was a problem hiding this comment.
Removed the redundant comment about Intrinsic
|
|
||
| // Marked intrinsic since JIT recognises the helper by name when doing optimizations. | ||
| [Intrinsic] | ||
| [BypassReadyToRun] |
There was a problem hiding this comment.
Comment on BypassReadyToRun would be more helpful. Is it a permanent thing or just a temporary measure since R2R support is not yet implemented?
There was a problem hiding this comment.
I think it is because R2R support is not yet implemented, but @jakobbotsch would know better.
If all Async methods are excluded from R2R anyways, maybe this is not necessary?
There was a problem hiding this comment.
The [NoInlining] on some methods is because the method suspends explicitly with a manually passed continuation.
No locals or other state would be automatically added to the continuation, thus it is better if the containing method is never inlined.
Using AsyncSuspend would generally require NoInlining for the containing method.
It seems it is worth a comment as well. I'll add a few words.
There was a problem hiding this comment.
Interestingly, all the uses of AsyncSuspend right now are with the sentinel continuation, which is the terminator of continuation chain and we never resume the sentinel.
It is possible to use AsyncSuspend with a non-sentinel continuation, you'd put enough info about your state in the continuation and assign Resume to something that effectively continues execution. We had some exploratory things that used such mechanism, but ended up not using it.
There was a problem hiding this comment.
BypassReadyToRun can likely be removed. Back when we had both prototypes and switched between them via environment variable UnsafeAwaitAwaiter/AwaitAwaiter had two different implementations depending on which one was selected, and this was just the simplest way to ensure we did not prejit those functions into one of them.
For Await I suspect prejitting will naturally fail once the JIT asks the EE runtime async questions, so it should also be unnecessary here.
There was a problem hiding this comment.
I've removed BypassReadyToRun and tests are passing.
We exclude Async variants from tiering for now, that would also make them not R2R able. But these are not variants, so I guess R2R just works. No reason not to, I think.
I did not verify for 100%, maybe it gets disabled for other reason. Seeing no failures is enough to remove BypassReadyToRun
There was a problem hiding this comment.
Added comments about NoInline
|
Thanks! |
VSadov
merged commit 7603366
into
dotnet:feature/async2-experiment
3 participants
As reviewed in dotnet/runtime#114310
This also updates Roslyn reference to a version that emits new helper names.