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

[RuntimeAsync] Make Async marker a bit in MethodImpl flags #110274

Merged
merged 5 commits into from
Dec 4, 2024
Merged

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Nov 29, 2024

Addresses one of design issues in: #109632
As discussed in: #104063 (comment)

This would change the Async marker to be a MethodImpl bit instead of an ordinary RuntimeAsyncMethodAttribute.

  • There are similarities with prior use of MethodImpl bits that direct the JIT/AOT/Interpreter to implement enhanced behavior while using the IL method body as a "blueprint" - I.E. Synchronized

  • There are certain conveniences for the runtime checks for whether a method is Async if the marker is a MethodImpl - no need to worry about attribute parsing, the location of the attribute definition, deal with attribute absence or presence of multiple attributes.

Copy link
Contributor

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

@@ -11,7 +11,7 @@ These are proposed modifications to the ECMA-335 specification for runtime-async

### I.8.4.5 Sync and Async Methods

Methods may be either 'sync' or 'async'. Async method definitions are methods with a return type of `System.Threading.Task`, `System.Threading.ValueTask`, `System.Threading.Task<T>`, or `System.Threading.ValueTask<T>` attributed with `[System.Runtime.CompilerServices.RuntimeAsyncMethodAttribute]`. Async method definitions are only valid inside async-capable assemblies. An async-capable assembly is one which references a corlib containing an `abstract sealed class RuntimeFeature` with a `public const string` field member named `Async`, or a corelib meeting these requirements. The `RuntimeAsyncMethodAttribute` should only be applied to methods, and may be defined in any assembly. Inside async method bodies, certain methods are also invokable by a special signature encoding, described in [### I.8.6.1.5 Method signatures].
Methods may be either 'sync' or 'async'. Async method definitions are methods with CIL implementation and with a return type of `System.Threading.Task`, `System.Threading.ValueTask`, `System.Threading.Task<T>`, or `System.Threading.ValueTask<T>` attributed with `[MethodImpl(MethodImplOptions.Async)]`. The `[MethodImpl(MethodImplOptions.Async)]` has no effect outside of this pattern. Async method definitions are only valid inside async-capable assemblies. An async-capable assembly is one which references a corlib containing an `abstract sealed class RuntimeFeature` with a `public const string` field member named `Async`, or a corelib meeting these requirements. Inside async method bodies, certain methods are also invokable by a special signature encoding, described in [### I.8.6.1.5 Method signatures].
Copy link
Member Author

@VSadov VSadov Nov 29, 2024

Choose a reason for hiding this comment

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

  • We may want to exclude property/event accessors from this. (anything else?)
  • Many questionable things like .ctor , ..cctor, ~Finalize() are effectively excluded, since they would not fit the Task-returning pattern.
  • Async appears incompatible with Synchronized, or at least a quite terrible combination. (anything else?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Also note that CIL implementation requirement would exclude delegate Invoke method, since it is runtime-provided.

I do not think it is a problem, semantically.
An async method (runtime or not) should be able to call another async method (runtime or not) via a delegate that returns a Task.

Avoiding materializing a Task when having runtime async on both sides of the delegate invocation could be an interesting challenge though.
I think it is a separate discussion. It is one of the items in #109632

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've added obvious restrictions on use of Async - such as not valid with Synchronized and varargs. We may add more as we get into details of the feature.

I've added a follow up item about event and property accessors. We may want to forbid those to be async, but I am not sure if that is easily enforceable. It feels like it goes out of the scope of a change whose main purpose is to switch Async marker to be a MethodImpl

@MichalPetryka
Copy link
Contributor

I wonder if RuntimeAsync would be a better name here, kindof feels like just Async is not descriptive enough for differentiating between legacy and new async.

@hez2010
Copy link
Contributor

hez2010 commented Nov 30, 2024

kindof feels like just Async is not descriptive enough for differentiating between legacy and new async.

I don't think this attribute is meant to be used by users directly, instead async methods returning a Task/Task<T> or ValueTask/ValueTask<T> will be automatically marked with MethodImpl(MethodImplOptions.Async) by the compiler. The runtime here is an implementation detail, and the compiler can still choose to emit a state machine if the target framework doesn't have runtime async support. So I would choose the term Async instead of RuntimeAsync.

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@VSadov
Copy link
Member Author

VSadov commented Dec 4, 2024

Re: Async vs. RuntimeAsync. I think adding "Runtime" is redundant since the marker clearly affects runtime behavior.
This is not the final form of the spec though, and many things, especially names may change.

For now I will merge the PR as it is, so that we could make further progress on the broader picture before refining various details.

@VSadov VSadov merged commit 48e3f13 into main Dec 4, 2024
13 checks passed
@VSadov VSadov deleted the AsyncMethodImpl branch December 4, 2024 01:01
eduardo-vp pushed a commit to eduardo-vp/runtime that referenced this pull request Dec 5, 2024
…0274)

* Make Async marker a bit in MethodImpl flags

* Linter error

* PR feedback

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

* Update runtime-async.md

* style tweaks

---------

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
mikelle-rogers pushed a commit to mikelle-rogers/runtime that referenced this pull request Dec 10, 2024
…0274)

* Make Async marker a bit in MethodImpl flags

* Linter error

* PR feedback

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

* Update runtime-async.md

* style tweaks

---------

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
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