search

benjamin-hodgson / Pidgin

A lightweight and fast parsing library for C#.
https://www.benjamin.pizza/Pidgin/
MIT License
920 stars 71 forks source link

Support trimming and AOT #146

Closed stevemonaco closed 1 year ago

stevemonaco commented 1 year ago

I was experimenting with AOT in an app that uses Pidgin which seems to not support AOT or trimming. While one can successfully skip trimming via <TrimmerRootAssembly Include="Pidgin" />, it seems to cause the AOT build process (ilc.exe) to never finish. At least not in the 5-10 minutes I let it run. If I remove the TrimmerRootAssembly, this small project AOT builds in about 40 seconds. However, it will crash due to the trimming issues. Please add docs to address the expected level of support for trimming/AOT.

Trimming alone is sufficient for my project and Pidgin is small enough even untrimmed, so the lack of AOT is not a deal breaker. .NET 5 might not have all of the attributes to properly annotate trimming with though. Some are on .NET 5 while others are .NET 7/8.

Version: Pidgin 3.2.1 / .NET 7 Console App

Trimming warnings (3):

Pidgin.EnumerableExtensions.FastCompareInternal<T>.GetCallFastCompare(): Call to 'System.Reflection.MethodInfo.MakeGenericMethod(Type[])' can not be statically analyzed. It's not possible to guarantee the availability of requirements of the generic method.
Pidgin.EnumerableExtensions.FastEqualInternal<T>.GetCallFastEqual(): Call to 'System.Reflection.MethodInfo.MakeGenericMethod(Type[])' can not be statically analyzed. It's not possible to guarantee the availability of requirements of the generic method.
Pidgin.SequenceTokenParser<TToken,TEnumerable>.GetCreateParser(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicParameterlessConstructor' in call to 'System.Type.MakeGenericType(Type[])'. The generic parameter 'TToken' of 'Pidgin.SequenceTokenParser<TToken,TEnumerable>' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

AOT crash:

Unhandled Exception: System.TypeInitializationException: A type initializer threw an exception. To determine which type, inspect the InnerException's StackTrace property.
 ---> System.TypeInitializationException: A type initializer threw an exception. To determine which type, inspect the InnerException's StackTrace property.
 ---> System.NotSupportedException: 'Pidgin.SequenceTokenParserFast`2[System.Char, System.String]' is missing native code or metadata. This can happen for code that is not compatible with trimming or AOT. Inspect and fix trimming and AOT related warnings that were generated when the app was published. For more information see https://aka.ms/nativeaot-compatibility
   at System.Reflection.Runtime.General.TypeUnifier.WithVerifiedTypeHandle(RuntimeConstructedGenericTypeInfo, RuntimeTypeInfo[]) + 0x88
   at Pidgin.SequenceTokenParser`2.GetCreateParser() + 0xc7
   at Pidgin.SequenceTokenParser`2..cctor() + 0x31
   at System.Runtime.CompilerServices.ClassConstructorRunner.EnsureClassConstructorRun(StaticClassConstructionContext*) + 0xcf
   --- End of inner exception stack trace ---
   at System.Runtime.CompilerServices.ClassConstructorRunner.EnsureClassConstructorRun(StaticClassConstructionContext*) + 0x167
   at System.Runtime.CompilerServices.ClassConstructorRunner.CheckStaticClassConstructionReturnGCStaticBase(StaticClassConstructionContext*, Object) + 0xd
   at Pidgin.SequenceTokenParser`2.Create(TEnumerable) + 0x19
   at TableLib.Parsing.TableParsers..cctor() + 0x179
   at System.Runtime.CompilerServices.ClassConstructorRunner.EnsureClassConstructorRun(StaticClassConstructionContext*) + 0xc6
   --- End of inner exception stack trace ---
   at System.Runtime.CompilerServices.ClassConstructorRunner.EnsureClassConstructorRun(StaticClassConstructionContext*) + 0x167
   at System.Runtime.CompilerServices.ClassConstructorRunner.CheckStaticClassConstructionReturnNonGCStaticBase(StaticClassConstructionContext*, IntPtr) + 0xd
   at TableLib.TableReader.ParseEntry(String, Nullable`1) + 0x62
benjamin-hodgson commented 1 year ago

To the extent that it's possible I wanna support trimming/AOT. For context I simply haven't tried them, they're new(ish) features and the library was written before they existed. I'm certainly not philosophically against it or anything like that - just never tried it before. You're the first person to have asked me about it.

These error messages all come from runtime codegen parts of the codebase (unsurprising since runtime codegen is not supported in AOT obviously). But the runtime codegen is there only as a microoptimisation (devirtualising generic code) and I think it's likely that performance would be acceptable for most users without the microoptimisation. The fallback code isn't super reflection heavy or anything like that, it just has some interface calls in loops. (I know that there've been some devirtualisation improvements in the jit in recent years so I wonder whether I even need this codegen any more.)

So ideally I'd be able simply to disable those code paths (at runtime?) and just use the existing fallback implementation if we're in a trimming/AOT scenario.

I had a skim through the docs and it doesn't seem like there's an obvious way to do that, at least nothing jumped off the page at me. Sounds like you know more about this than I do - is there a way for a lib to detect whether it's being deployed trimmed/AOT? Something in RuntimeHelpers perhaps?

I'm also open to multi-targeting the library, as long as Nuget/PackageReference can do asset selection for AOT. I don't want to publish two different packages.

benjamin-hodgson commented 1 year ago

I don't know why the AOT compiler spins. Sounds like it could be a bug in the AOT system.

stevemonaco commented 1 year ago

I'm not experienced with AOT. This was my first real exploration attempt.

So ideally I'd be able simply to disable those code paths (at runtime?) and just use the existing fallback implementation if we're in a trimming/AOT scenario.

It seems possible using RuntimeHelpers.IsDynamicCodeSupported with .NET 5 (check bottom example). However, the suppression attribute (RequiresDynamicCodeAttribute) is .NET 7+. As an aside, the <IsAotCompatible>true</IsAotCompatible> csproj property is .NET 8.

I don't know why the AOT compiler spins. Sounds like it could be a bug in the AOT system.

I think so too, but I would need to install and check .NET 8 preview as it may have been fixed already.

benjamin-hodgson commented 1 year ago

Happily it seems like the runtime codegen bits are no longer needed with recent jits. I looked at the asm from a current (net7) runtime and it seems like Comparer.Default/EqualityComparer.Default got devirtualised+inlined just fine. So I just deleted the code with the warnings.

Fixed in https://github.com/benjamin-hodgson/Pidgin/commit/63426564398b8b8fdf48ae6d7b11e40e9c01c8eb and shipped in v3.2.2.

Thanks for bringing this to my attention!

jmlane commented 1 year ago

Thanks!