[aspnetcore-corert] Upgrade to .NET 6.0-RC2 #6862
Conversation
| @@ -0,0 +1,51 @@ | |||
| <Directives> | |||
| <Application> | |||
| <Assembly Name="Npgsql"> | |||
There was a problem hiding this comment.
@MichalStrehovsky I'm surprised this is still needed in the latest versions of Npgsql - we did quite a bit of work on trimming friendliness (though not 100% there yet for 6.0).
There was a problem hiding this comment.
RC1 of Npgsql has an infinite generic recursion somewhere that I didn't look into. It doesn't work with NativeAOT at all. This is downgrading to preview 3 because preview 3 at least works. I had the rd.xml for it laying around from some internal investigation a couple months ago.
There was a problem hiding this comment.
OK. This is definitely something I'd like to look into at some point, if you get some spare time and can point me in the right direction I'd be happy to.
There was a problem hiding this comment.
@roji I looked into the generic recursion. It's pretty trivial. NpgsqlTypeHandler<TDefault> has a CreateRangeHandler method that creates new RangeHandler<TDefault>(pgRangeType, this);. But RangeHandler derives from NpgsqlTypeHandler<NpgsqlRange<TElement>>.
So NpgsqlTypeHandler<Foo> forces the existence of RangeHandler<Foo>, which forces the existence of NpgsqlTypeHandler<NpgsqlRange<Foo>>, which in turn requires RangeHandler<NpgsqlRange<Foo>> that forces NpgsqlTypeHandler<NpgsqlRange<NpgsqlRange<Foo>>>... until we run out of memory. If NpgsqlRange wasn't a valuetype, this wouldn't require new codegen, but it is a valuetype and new code is needed. This pattern is AOT kryptonite.
There was a problem hiding this comment.
FWIW, looks like npgsql/npgsql#2060 was an earlier fix for this exact issue in Npgsql. Not sure how applicable it is now.
There was a problem hiding this comment.
Thanks @MichalStrehovsky... Looking at npgsql/npgsql#2060, and at 6.0.0-preview3, it seems that there as well, RangeHandler<TElement> extends NpgsqlTypeHandler<Range<TElement>>, and that NpgsqlTypeHandler<TDefault> has CreateRangeHandler which instantiates a new RangeHandler<TDefault> - so I'm still not sure what the critical difference is...
BTW what would be a simple way for me to track this down? I followed the NativeAOT steps for a hello world program, and did end up with something that builds Npgsql 5.0, and fails when building the latest 6.0. However the failure is cryptic and doesn't point me in the direction of what's actually wrong (e.g. RangeHandler):
/home/roji/.nuget/packages/microsoft.dotnet.ilcompiler/6.0.0-rc.1.21420.1/build/Microsoft.NETCore.Native.targets(283,5): error MSB3073: The command ""/home/roji/.nuget/packages/runtime.linux-x64.microsoft.dotnet.ilcompiler/6.0.0-rc.1.21420.1/tools/ilc" @"obj/Release/net6.0/linux-x64/native/ClassLibrary1.ilc.rsp"" exited with code 137. [/tmp/weird/ClassLibrary1/ClassLibrary1/ClassLibrary1.csproj]
I'd ideally add a step in our CI to make sure this doesn't come back yet again.
There was a problem hiding this comment.
It's very likely it was just unreachable. The Techempower benchmark csproj turns on more aggressive trimming that trims Npgsql too. If we didn't trim it it would probably fail too (I think).
Exit code 137 is very likely the OOM killer. Linux is designed in a way that a process cannot give you useful feedback if the system runs out of memory. You would see OutOfMemoryException on Windows.
NativeAOT still doesn't have detection of generic cycles so it just does the naive thing and runs out of memory without telling in detail where the cycle is. It's in my wishlist and we have an issue for it.
There was a problem hiding this comment.
@MichalStrehovsky this is fixed for 6.0 (see npgsql/npgsql#4095).
BTW when testing just now, I did see warnings around generic infinite recursion (see below), rather than OOM, maybe you implemented the check since we talked?
ILC : AOT analysis warning IL1005: Npgsql.Internal.TypeHandlers.MultirangeHandler`1<NpgsqlRange`1<NpgsqlRange`1<NpgsqlRange`1<NpgsqlRange`1<__Canon>>>>>.ValidateAndGetLengthMultirange<NpgsqlRange`1<NpgsqlRange`1<NpgsqlRange`1<NpgsqlRange`1<__Canon>>>>>(IList`1<NpgsqlRange`1<NpgsqlR
ange`1<NpgsqlRange`1<NpgsqlRange`1<NpgsqlRange`1<__Canon>>>>>>,NpgsqlLengthCache&,NpgsqlParameter): Method will always throw because: Failed to load type 'Npgsql.Internal.TypeHandling.NpgsqlTypeHandler' from assembly 'Npgsql, Version=6.0.0.0, Culture=neutral, PublicKeyToken=5d8b90d
52f46fda7' [/home/roji/projects/test/Test.csproj]
There was a problem hiding this comment.
That's great to hear, thank you!
Yup, I did: dotnet/runtimelab#1681. Having the compiler just run out of memory was annoying.
Getting rid of the recursions should hopefully speed up the compilation of anything using Npgsql and make the AOT compiled output a lot smaller. (The recursions detection makes the compiler not crash, but the result is still not great because there's not much that can be done with it.)
There was a problem hiding this comment.
I did notice the compilation speed difference - but it's even better to be able to get an explicit warning! Thanks!
| { | ||
| // Is semantically identical to RawDbMySqlConnector.cs. | ||
| // If you are changing RawDbNpgsql.cs, also consider changing RawDbMySqlConnector.cs. | ||
| public class RawDb |
There was a problem hiding this comment.
@MichalStrehovsky is there an alternative to duplicating all these? Maybe multiple csprojs referencing the same source files, with some conditional compilation where needed?
There was a problem hiding this comment.
I don't want to condition the shipping benchmarks on getting NativeAOT passing with it. I'm pretty sure sharing this code would mean NativeAOT benchmark getting disabled whenever it's in the way.
|
@nbrady-techempower this is now ready, thanks! |
No description provided.