Fix NativeAOT with minimal actions #35167

davidfowl · 2021-08-09T00:22:44Z

Generic methods for value types need to be visible at compile time so that the AOT compiler can generate code for the right instantiations (closed generic types). Since Enum.TryParse<T> is used purely via reflection is gets removed from the final AOT binary. This causes our logic that looks for Enum.TryParse<T> to fail. Instead, we call back to the non-generic overload which doesn't get trimmed and can at runtime lookup the metadata for the enum type in order to allow parsing.
Made TryParseMethodCache creatable so it could be tested more easily.
Added tests

PS: To get a better sense of what the exception looks like for not having the code for a specific generic instantiation:

Unhandled Exception: EETypeRva:0x01CA62A8(System.Reflection.MissingRuntimeArtifactException): MakeGenericMethod() cannot create this generic method instantiation because no code was generated for it: 'System.Enum.TryParse<Choice>(System.String,System.Boolean,out Choice&)'.
   at System.Reflection.Runtime.MethodInfos.RuntimeNamedMethodInfo`1.GetUncachedMethodInvoker(RuntimeTypeInfo[], MemberInfo) + 0x1dc
   at System.Reflection.Runtime.MethodInfos.RuntimeConstructedGenericMethodInfo.get_UncachedMethodInvoker() + 0x3b
   at System.Reflection.Runtime.MethodInfos.RuntimeMethodInfo.get_MethodInvoker() + 0x1c0
   at System.Reflection.Runtime.MethodInfos.RuntimeNamedMethodInfo`1.MakeGenericMethod(Type[]) + 0x3e9
   at Microsoft.AspNetCore.Http.TryParseMethodCache.<FindTryParseMethod>g__Finder|4_0(Type) + 0xfd
   at WebApplication520!<BaseAddress>+0x104bc58
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey, Func`2) + 0x124
   at Microsoft.AspNetCore.Http.TryParseMethodCache.FindTryParseMethod(Type) + 0x56
   at Microsoft.AspNetCore.Http.TryParseMethodCache.HasTryParseMethod(ParameterInfo) + 0x87
   at Microsoft.AspNetCore.Http.RequestDelegateFactory.CreateArgument(ParameterInfo, RequestDelegateFactory.FactoryContext) + 0xa31
   at Microsoft.AspNetCore.Http.RequestDelegateFactory.CreateArguments(ParameterInfo[], RequestDelegateFactory.FactoryContext) + 0xd6
   at Microsoft.AspNetCore.Http.RequestDelegateFactory.CreateTargetableRequestDelegate(MethodInfo, RequestDelegateFactoryOptions, Expression) + 0x1bc
   at Microsoft.AspNetCore.Http.RequestDelegateFactory.Create(Delegate, RequestDelegateFactoryOptions) + 0x188
   at Microsoft.AspNetCore.Builder.MinimalActionEndpointRouteBuilderExtensions.Map(IEndpointRouteBuilder, RoutePattern, Delegate) + 0x253
   at Microsoft.AspNetCore.Builder.MinimalActionEndpointRouteBuilderExtensions.MapMethods(IEndpointRouteBuilder, String, IEnumerable`1, Delegate) + 0xa1
   at Microsoft.AspNetCore.Builder.MinimalActionEndpointRouteBuilderExtensions.MapGet(IEndpointRouteBuilder, String, Delegate) + 0x42
   at Program.<Main>$(String[]) + 0x703
   at WebApplication520!<BaseAddress>+0x10ad997
   at WebApplication520!<BaseAddress>+0x10ada20

Also, to see how this all of this expression compilation works with native AOT, the expression tree interpreter is used:

var builder = WebApplication.CreateBuilder(args);

var app = builder.Build();

app.MapGet("/", () => Environment.StackTrace);

app.Run();

   at System.Environment.get_StackTrace() + 0x32
   at Program.<>c.<<Main>$>b__0_0() + 0x19
   at WebApplication520!<BaseAddress>+0x13d245e
   at System.InvokeUtils.CallDynamicInvokeMethod(Object, IntPtr, IntPtr, IntPtr, Object, Object[], BinderBundle, Boolean, Boolean) + 0x1ac
   at Internal.Runtime.Augments.RuntimeAugments.CallDynamicInvokeMethod(Object, IntPtr, IntPtr, IntPtr, Object, Object[], BinderBundle, Boolean, Boolean) + 0x68
   at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.Invoke(Object, Object[], BinderBundle, Boolean) + 0x108
   at Internal.Reflection.Core.Execution.MethodInvoker.Invoke(Object, Object[], Binder, BindingFlags, CultureInfo) + 0x83
   at System.Reflection.Runtime.MethodInfos.RuntimeMethodInfo.Invoke(Object, BindingFlags, Binder, Object[], CultureInfo) + 0x86
   at System.Reflection.MethodBase.Invoke(Object, Object[]) + 0x4d
   at System.Linq.Expressions.Interpreter.MethodInfoCallInstruction.Run(InterpretedFrame) + 0x170
   at System.Linq.Expressions.Interpreter.Interpreter.Run(InterpretedFrame) + 0x69
   at System.Linq.Expressions.Interpreter.LightLambda.Run(Object[]) + 0xac
   at WebApplication520!<BaseAddress>+0x1084590
   at Microsoft.AspNetCore.Http.RequestDelegateFactory.<>c__DisplayClass33_0.<Create>b__0(HttpContext) + 0x62
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.Invoke(HttpContext) + 0x1f4
   at Microsoft.AspNetCore.Routing.EndpointRoutingMiddleware.SetRoutingAndContinue(HttpContext) + 0xf3
   at Microsoft.AspNetCore.Routing.EndpointRoutingMiddleware.Invoke(HttpContext) + 0x16a
   at Microsoft.AspNetCore.HostFiltering.HostFilteringMiddleware.Invoke(HttpContext) + 0x8d
   at Microsoft.AspNetCore.Hosting.HostingApplication.ProcessRequestAsync(HostingApplication.Context) + 0x53
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.<ProcessRequests>d__223`1.MoveNext() + 0x497
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1.ExecutionContextCallback(Object) + 0x6a
   at WebApplication520!<BaseAddress>+0x10a353a
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread, ExecutionContext, ContextCallback, Object) + 0x73
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1.MoveNext(Thread) + 0x124
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1.ExecuteFromThreadPool(Thread) + 0x1f
   at System.Threading.ThreadPoolWorkQueue.DispatchWorkItem(Object, Thread) + 0x52
   at System.Threading.ThreadPoolWorkQueue.Dispatch() + 0x177
   at System.Threading.ThreadPool.DispatchCallback(IntPtr, IntPtr, IntPtr) + 0x6e

- Generic methods for value types need to be visible at compile time so that the AOT compiler can generate code for the right instantiations (closed generic types). Since Enum.TryParse<T> is used purely via reflection is gets removed from the final AOT binary. This causes our logic that looks for Enum.TryParse<T> to fail. Instead we call back to the non-generic overload which doesn't get trimmed and can at runtime lookup the metadata for the enum type in order to allow parsing. - Made TryParseMethodCache creatable so it could be tested more easily. - Added tests

src/Shared/TryParseMethodCache.cs

Co-authored-by: Martin Costello <martin@martincostello.com>

davidfowl · 2021-08-09T08:46:43Z

/azp run

azure-pipelines · 2021-08-09T08:47:01Z

Azure Pipelines successfully started running 2 pipeline(s).

src/Shared/TryParseMethodCache.cs

pranavkm · 2021-08-09T14:48:22Z

src/Shared/TryParseMethodCache.cs

        {
            var staticEnumMethods = typeof(Enum).GetMethods(BindingFlags.Public | BindingFlags.Static);

+            // With NativeAOT, if there's no static usage of Enum.TryParse<T>, it will be removed
+            // we fallback to the non-generic version if that is the case


Is there a reason the non-generic one does not get trimmed?

I assume it's because something else uses it. I have no idea what.

Should we force it to be kept (e.g using DynamicDependency)

I'm not sure if it's needed TBH, I think typeof(Enum).GetMethods(BindingFlags.Public | BindingFlags.Static) is visible to the linker but I'd need to make sure.

cc @MichalStrehovsky @eerhardt

This is surprising to me.

var staticEnumMethods = typeof(Enum).GetMethods(BindingFlags.Public | BindingFlags.Static);

That should "mark" all public static methods on Enum, and the AOT compiler shouldn't trim any of them.

Is it possible to build up the GetMethod query correctly so you only select the one method you are looking for?

typeof(Enum).GetMethods(BindingFlags.Public | BindingFlags.Static) is visible to the linker but I'd need to make sure.

Yes. It is a bug in NativeAOT: dotnet/runtimelab#1402

Once this bug gets fixed, this will be broken again.

FWIW, I think fallbacks like this are wrong way to fix trimming and AOT friendliness issues.

I didn't realize this is going to return TryParse<T> if that method got reflection-rooted through other means. The fact that it's not just rooted from GetMethod is kind of a bug but it only happens because the compiler figured there's not much it can do there - by default the AOT compiler will try to enable metadata for the method and generate a shared instantiation (over reference types), but the constraints on TryParse<T> rule out reference types. Right now, because it can't make a usable method body, it will not generate the metadata either. But we can fix it to create the metadata.

Yes, this is likely going to fail at MakeGenericMethod later.

Once this bug gets fixed, this will be broken again.

Thanks. I'll look into that.

FWIW, I think fallbacks like this are wrong way to fix trimming and AOT friendliness issues.

It just needs to work well enough (or barely work) to enable people to try it out and give feedback. Seems like we need something a bit more creative here (like a check for IsDynamicCodeCompiled maybe). Remember it's using the expression tree interpreter here so the bar is low 😄

IsDynamicCodeSupported is what I did in the DataContractSerializer as a temporary workaround while we figure things out: https://github.com/dotnet/runtimelab/pull/692/files#diff-5c36d3b2cc664a8e51d53ac77390c98eca051ca63b13443233afc38ccee5e65fR60-R64

Yep, this is what I'll do. FWIW I have a pretty good handle on most of the AOT friendly problems that will plague ASP.NET Core (like JSON 😄 ) and have prototypes for how to tackle most of them.

src/Shared/TryParseMethodCache.cs

Co-authored-by: Pranav K <prkrishn@hotmail.com>

davidfowl requested review from BrennanConroy, Pilchie and Tratcher as code owners August 9, 2021 00:22

davidfowl requested a review from halter73 August 9, 2021 00:24

davidfowl added feature-minimal-actions Controller-like actions for endpoint routing old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Aug 9, 2021

davidfowl added 2 commits August 8, 2021 17:25

Fixed comment

0f70ba4

Fixed shared source

d8a09ca

davidfowl requested review from javiercn and pranavkm as code owners August 9, 2021 00:44

martincostello reviewed Aug 9, 2021

View reviewed changes

src/Shared/TryParseMethodCache.cs Outdated Show resolved Hide resolved

Update src/Shared/TryParseMethodCache.cs

638ecd0

Co-authored-by: Martin Costello <martin@martincostello.com>

martincostello mentioned this pull request Aug 9, 2021

Support binding to form and form file parameters in minimal actions #35158

Merged

6 tasks

pranavkm approved these changes Aug 9, 2021

View reviewed changes

davidfowl commented Aug 9, 2021

View reviewed changes

src/Shared/TryParseMethodCache.cs Outdated Show resolved Hide resolved

davidfowl commented Aug 9, 2021

View reviewed changes

src/Shared/TryParseMethodCache.cs Show resolved Hide resolved

Apply suggestions from code review

1fdc35e

Co-authored-by: Pranav K <prkrishn@hotmail.com>

davidfowl merged commit 83927d9 into main Aug 9, 2021

davidfowl deleted the davidfowl/aot-friendly-minimal-actions branch August 9, 2021 19:57

ghost added this to the 6.0-rc1 milestone Aug 9, 2021

MichalStrehovsky mentioned this pull request Aug 9, 2021

Consider supporting shared code over enums dotnet/runtimelab#1400

Closed

rafikiassumani-msft assigned davidfowl Aug 9, 2021

davidfowl mentioned this pull request Aug 10, 2021

Slightly better support for NativeAOT and trimming #35211

Merged

amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix NativeAOT with minimal actions #35167

Fix NativeAOT with minimal actions #35167

Uh oh!

davidfowl commented Aug 9, 2021 •

edited

Loading

Uh oh!

Uh oh!

davidfowl commented Aug 9, 2021

Uh oh!

azure-pipelines bot commented Aug 9, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pranavkm Aug 9, 2021

Uh oh!

davidfowl Aug 9, 2021

Uh oh!

pranavkm Aug 9, 2021

Uh oh!

davidfowl Aug 9, 2021

Uh oh!

eerhardt Aug 9, 2021

Uh oh!

jkotas Aug 10, 2021

Uh oh!

MichalStrehovsky Aug 10, 2021

Uh oh!

davidfowl Aug 10, 2021 •

edited

Loading

Uh oh!

MichalStrehovsky Aug 10, 2021

Uh oh!

davidfowl Aug 10, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fix NativeAOT with minimal actions #35167

Fix NativeAOT with minimal actions #35167

Uh oh!

Conversation

davidfowl commented Aug 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

davidfowl commented Aug 9, 2021

Uh oh!

azure-pipelines bot commented Aug 9, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davidfowl commented Aug 9, 2021 •

edited

Loading

davidfowl Aug 10, 2021 •

edited

Loading