Rename AsyncMethodDesc to AsyncMethodVariant, add "AsyncVariant" flag to MethodWithToken and MethodWithGCInfo #121218
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,138 @@ | ||||||
| // Licensed to the .NET Foundation under one or more agreements. | ||||||
| // The .NET Foundation licenses this file to you under the MIT license. | ||||||
|
|
||||||
| using System; | ||||||
| using System.Collections.Generic; | ||||||
| using System.Diagnostics; | ||||||
| using Internal.JitInterface; | ||||||
|
|
||||||
|
|
||||||
| namespace Internal.TypeSystem | ||||||
| { | ||||||
| /// <summary> | ||||||
| /// Either the AsyncMethodImplVariant or AsyncMethodThunkVariant of a method marked .IsAsync. | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| /// </summary> | ||||||
| public partial class AsyncMethodVariant : MethodDelegator, IJitHashableOnly | ||||||
|
There was a problem hiding this comment. We should make this the same between R2R and NAOT More context: #121295 (comment) and #121295 (comment) I think single MethodDesc type is fine, but it does not sound like that it should be JIT-lifetime only. There was a problem hiding this comment. Thinking about this, it might be better to do this in crossgen2 too. So take the CompilerTypeSystemContext.Async.cs from my PR and AsyncVariantImplMethod.cs too, but rename the method itself to AsyncMethodVariant like it's here (based on Jan's comments on my PR). In the end this is not about how long this lives, but how many components get exposed to this. Longer living means more components get exposed, but not necessarily - we can still hide the existence of these thunks from most of the system. There was a problem hiding this comment.
You probably meant "existence of async variants". |
||||||
| { | ||||||
| private readonly AsyncMethodVariantFactory _factory; | ||||||
| private readonly AsyncMethodKind _asyncMethodKind; | ||||||
| private readonly int _jitVisibleHashCode; | ||||||
| private MethodSignature _asyncSignature; | ||||||
|
|
||||||
| public AsyncMethodVariant(MethodDesc wrappedMethod, AsyncMethodVariantFactory factory, AsyncMethodKind kind) | ||||||
| : base(wrappedMethod) | ||||||
| { | ||||||
| Debug.Assert(wrappedMethod.IsTaskReturning); | ||||||
| Debug.Assert(kind switch | ||||||
| { | ||||||
| AsyncMethodKind.AsyncVariantThunk => !wrappedMethod.IsAsync, | ||||||
| AsyncMethodKind.AsyncVariantImpl => wrappedMethod.IsAsync, | ||||||
| _ => false, | ||||||
| }); | ||||||
|
Comment on lines
+26
to
+31
There was a problem hiding this comment. Do we need the There was a problem hiding this comment. Also, we should not need to cache the We probably do not need There was a problem hiding this comment.
Yeah, I was thinking about that too. We just need to be careful about: But from If we really need an enum, it could be an extension method and an enum stashed on the side, but honestly the names of these thunks still confuse me after looking at this for 2 days and when it's explicitly spelled out, it's more clear what we're checking. |
||||||
| _factory = factory; | ||||||
| _asyncMethodKind = kind; | ||||||
| _jitVisibleHashCode = HashCode.Combine(wrappedMethod.GetHashCode(), 0x310bb74f); | ||||||
| } | ||||||
|
|
||||||
| public MethodDesc Target => _wrappedMethod; | ||||||
|
|
||||||
| public override AsyncMethodKind AsyncMethodKind => _asyncMethodKind; | ||||||
|
|
||||||
| public override MethodSignature Signature | ||||||
| { | ||||||
| get | ||||||
| { | ||||||
| return _asyncSignature ??= _wrappedMethod.Signature.CreateAsyncSignature(); | ||||||
|
There was a problem hiding this comment. Inline |
||||||
| } | ||||||
| } | ||||||
|
|
||||||
| public override MethodDesc GetCanonMethodTarget(CanonicalFormKind kind) | ||||||
| { | ||||||
| return _factory.GetOrCreateAsyncMethodImplVariant(_wrappedMethod.GetCanonMethodTarget(kind), _asyncMethodKind); | ||||||
| } | ||||||
|
|
||||||
| public override MethodDesc GetMethodDefinition() | ||||||
| { | ||||||
| var real = _wrappedMethod.GetMethodDefinition(); | ||||||
| if (real == _wrappedMethod) | ||||||
| return this; | ||||||
|
|
||||||
| return _factory.GetOrCreateAsyncMethodImplVariant(real, _asyncMethodKind); | ||||||
| } | ||||||
|
|
||||||
| public override MethodDesc GetTypicalMethodDefinition() | ||||||
| { | ||||||
| var real = _wrappedMethod.GetTypicalMethodDefinition(); | ||||||
| if (real == _wrappedMethod) | ||||||
| return this; | ||||||
| return _factory.GetOrCreateAsyncMethodImplVariant(real, _asyncMethodKind); | ||||||
| } | ||||||
|
|
||||||
| public override MethodDesc InstantiateSignature(Instantiation typeInstantiation, Instantiation methodInstantiation) | ||||||
| { | ||||||
| var real = _wrappedMethod.InstantiateSignature(typeInstantiation, methodInstantiation); | ||||||
| if (real == _wrappedMethod) | ||||||
| return this; | ||||||
| return _factory.GetOrCreateAsyncMethodImplVariant(real, _asyncMethodKind); | ||||||
| } | ||||||
|
|
||||||
| public override string ToString() => $"Async variant ({_asyncMethodKind}): " + _wrappedMethod.ToString(); | ||||||
|
|
||||||
| protected override int ClassCode => throw new NotImplementedException(); | ||||||
|
|
||||||
| protected override int CompareToImpl(MethodDesc other, TypeSystemComparer comparer) | ||||||
| { | ||||||
| throw new NotImplementedException(); | ||||||
| } | ||||||
|
|
||||||
| protected override int ComputeHashCode() | ||||||
| { | ||||||
| throw new NotSupportedException("This method may not be stored as it is expected to only be used transiently in the JIT"); | ||||||
| } | ||||||
|
|
||||||
| int IJitHashableOnly.GetJitVisibleHashCode() => _jitVisibleHashCode; | ||||||
| } | ||||||
|
|
||||||
| public sealed class AsyncMethodVariantFactory : Dictionary<(MethodDesc, AsyncMethodKind), AsyncMethodVariant> | ||||||
| { | ||||||
| public AsyncMethodVariant GetOrCreateAsyncMethodImplVariant(MethodDesc wrappedMethod, AsyncMethodKind kind) | ||||||
| { | ||||||
| Debug.Assert(wrappedMethod.IsAsync); | ||||||
| if (!TryGetValue((wrappedMethod, kind), out AsyncMethodVariant variant)) | ||||||
| { | ||||||
| variant = new AsyncMethodVariant(wrappedMethod, this, kind); | ||||||
| this[(wrappedMethod, kind)] = variant; | ||||||
| } | ||||||
| return variant; | ||||||
| } | ||||||
|
|
||||||
| public AsyncMethodVariant GetOrCreateAsyncThunk(MethodDesc wrappedMethod) | ||||||
| { | ||||||
| return GetOrCreateAsyncMethodImplVariant(wrappedMethod, AsyncMethodKind.AsyncVariantThunk); | ||||||
| } | ||||||
|
|
||||||
| public AsyncMethodVariant GetOrCreateAsyncImpl(MethodDesc wrappedMethod) | ||||||
| { | ||||||
| return GetOrCreateAsyncMethodImplVariant(wrappedMethod, AsyncMethodKind.AsyncVariantImpl); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| public static class AsyncMethodVariantExtensions | ||||||
| { | ||||||
| /// <summary> | ||||||
| /// Returns true if this MethodDesc is an AsyncMethodVariant, which should not escape the jit interface. | ||||||
| /// </summary> | ||||||
| public static bool IsAsyncVariant(this MethodDesc method) | ||||||
| { | ||||||
| return method is AsyncMethodVariant; | ||||||
| } | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Gets the wrapped method of the AsyncMethodVariant. This method is Task-returning. | ||||||
| /// </summary> | ||||||
| public static MethodDesc GetAsyncVariantDefinition(this MethodDesc method) | ||||||
| { | ||||||
| return ((AsyncMethodVariant)method).Target; | ||||||
| } | ||||||
|
Comment on lines
+130
to
+136
|
||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.Diagnostics; | ||
MichalStrehovsky marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| using System.Threading; | ||
|
|
||
| namespace Internal.TypeSystem | ||
| { | ||
| public sealed partial class InstantiatedMethod | ||
| { | ||
| public override AsyncMethodKind AsyncMethodKind | ||
| { | ||
| get | ||
| { | ||
| return _methodDef.AsyncMethodKind; | ||
| } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be just
AsyncVariant(ie cover both Thunk or impl)?This enum is part of R2R file format. It is used to encode references to methods in other assemblies. This encoding should be resilient to compatible changes in the implementation of other assemblies. If we encode
AsyncThunkVarianthere, it does not sound like we will be resilient to the implementations in other assemblies changing between async v1 and async v2.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
READYTORUN_METHOD_SIG_SlotInsteadOfTokenused? Could we reuse it (bumping the R2R version of course)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant this to indicate the signature is for the Thunk variant, either the AsyncThunkVariant or RuntimeAsync kinds. It feels more natural to put the Impl in the MethodDefEntryPoints table, and the thunk into the InstanceMethodEntryPoints table, but maybe this should be AsyncVariant. It sounds like the flag being AsyncVariant would make it resilient to implementations in other assemblies changing?
I see it being read in a few places, but I don't see crossgen2 writing that flag out anywhere. We could potentially reuse that, but I don't know enough to be confident that it wouldn't cause issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it being set in number of places too: https://github.com/search?q=repo%3Adotnet%2Fruntime%20SlotInsteadOfToken&type=code