Addressing code review suggestions #1975
Conversation
…ction should be set to link or not
…ave different default value
|
@MichalStrehovsky PTAL |
| } | ||
| => rootAllApplicationAssemblies | ||
| ? "" // use the defaults | ||
| // TrimMode is set in explicit way as for older versions it might have different default value |
There was a problem hiding this comment.
TrimMode=link by default in .NET 6 and NativeAOT doesn't support .NET 5. It's probably unnecessary.
There was a problem hiding this comment.
We are still supporting .NET 5 with runtimelab ILCompiler (--runtimes nativeaot5.0) and I've noticed that it has great impact on the build time for this particular config (which is used by our CI as we are still using .NET 5 SDK in BDN)
There was a problem hiding this comment.
The ILCompiler package brings it's own compiler, CoreLib and the entire BCL - I think if you use the runtimelab ILCompiler with --runtimes nativeaot5.0 or --runtimes nativeaot6.0, you won't see any difference in their performance. The only difference is that the reference assemblies that the C# compiles against won't let you use 6.0 APIs with runtimes=5.0. Either way you get RyuJIT, runtime, CoreLib and framework that is half way between 6.0 and 7.0.
The only difference might be if there's third party NuGets - it will pick up 5.0 versions if they multitarget.
There was a problem hiding this comment.
After this one #1978 does --runtimes nativeaot5.0 needed?
There was a problem hiding this comment.
I've removed nativeaot5.0. Moreover, I think that once we ship .NET 7 with official NativeAOT support we should also remove nativeaot6.0.
BTW I've tested the following command:
dotnet run -c Release -f net6.0 --filter *Basic* --job dry --runtimes nativeaot6.0With <TrimMode>link</TrimMode> it takes 9s to publish the generated app. Without it it takes 110s. So I am going to keep it.
# Conflicts: # src/BenchmarkDotNet/Toolchains/NativeAot/NativeAotToolchain.cs
| /// </summary> | ||
| public static readonly IToolchain Net60 = GetBuilderForOldExperimentalFeed().TargetFrameworkMoniker("net6.0").ToToolchain(); | ||
| public static readonly IToolchain Net60 = CreateBuilder() | ||
| .UseNuGet("6.0.0-*", "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-experimental/nuget/v3/index.json") |
There was a problem hiding this comment.
From what I know, .NET 6.0 works perfectly fine under 7.0 ILC and 6.0 is minimum target for ILC right now. Do we really need to tests 6.0 experimental? I do use .NET 6.0 + ILCompiler 7.0 and that's I believe what would be suggested by @MichalStrehovsky too.
There was a problem hiding this comment.
I need it as I want to benchmark ILCompiler 6.0 vs 7.0 to see if there are any regressions.
There was a problem hiding this comment.
ILCompiler 6.0 is really just an arbitrary snapshot close to when the branding in main changed from 6.0 to 7.0 (but not exactly because we didn't integrate from runtime to runtimelab commit by commit, but in ~weekly chunks). It doesn't have fixes that .NET 6.0 received after main snapped, (i.e. what got integrated from main to 6.0 after the snap), it doesn't have fixes close to when it snapped, and it's therefore not very comparable to .NET 6.0. As long as that's understood, it's fine.
#1973 (comment)
https://github.com/dotnet/BenchmarkDotNet/pull/1972/files#r837988801
cc @MichalStrehovsky