Skip to content

Addressing code review suggestions #1975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adamsitnik merged 7 commits into from
Apr 4, 2022
Merged

Addressing code review suggestions #1975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d147a2a
remove redundant ctor
adamsitnik Mar 30, 2022
098882d
use rootAllApplicationAssemblies to determine whether TrimmerDefault…
adamsitnik Mar 30, 2022
f7f6107
set TrimMode to link in explicit way as for older versions it might h…
adamsitnik Mar 30, 2022
e8ff47b
Merge remote-tracking branch 'origin/master' into fewFixes
adamsitnik Apr 4, 2022
818c357
remove NativeAot50, nobody should be using it
adamsitnik Apr 4, 2022
7409d12
some minor pedantic changes
adamsitnik Apr 4, 2022
8da82d1
fix the tests
adamsitnik Apr 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions src/BenchmarkDotNet.Annotations/Attributes/GenericTypeArgumentsAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using JetBrains.Annotations;
using System;
using System;
using System.Diagnostics.CodeAnalysis;

namespace BenchmarkDotNet.Attributes
Expand All @@ -9,9 +8,6 @@ public class GenericTypeArgumentsAttribute : Attribute
{
public Type[] GenericTypeArguments { get; }

// CLS-Compliant Code requires a constructor without an array in the argument list
[PublicAPI] public GenericTypeArgumentsAttribute() => GenericTypeArguments = new Type[0];

public GenericTypeArgumentsAttribute([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type type)
=> GenericTypeArguments = new Type[] { type };

Expand Down
5 changes: 0 additions & 5 deletions src/BenchmarkDotNet.Annotations/Jobs/RuntimeMoniker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,6 @@ public enum RuntimeMoniker
/// </summary>
Net70,

/// <summary>
/// NativeAOT compiled as net5.0
/// </summary>
NativeAot50,

/// <summary>
/// NativeAOT compiled as net6.0
/// </summary>
Expand Down
1 change: 0 additions & 1 deletion src/BenchmarkDotNet/ConsoleArguments/ConfigParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,6 @@ private static Job CreateJobForGivenRuntime(Job baseJob, string runtimeId, Comma
.WithToolchain(CsProjCoreToolchain.From(new NetCoreAppSettings(runtimeId, null, runtimeId, options.CliPath?.FullName, options.RestorePath?.FullName)));
case RuntimeMoniker.Mono:
return baseJob.WithRuntime(new MonoRuntime("Mono", options.MonoPath?.FullName));
case RuntimeMoniker.NativeAot50:
case RuntimeMoniker.NativeAot60:
return CreateAotJob(baseJob, options, runtimeMoniker, "6.0.0-*", "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-experimental/nuget/v3/index.json");
case RuntimeMoniker.NativeAot70:
Expand Down
5 changes: 0 additions & 5 deletions src/BenchmarkDotNet/Environments/Runtimes/NativeAotRuntime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,6 @@ namespace BenchmarkDotNet.Environments
{
public class NativeAotRuntime : Runtime
{
/// <summary>
/// NativeAOT compiled as net5.0
/// </summary>
public static readonly NativeAotRuntime Net50 = new NativeAotRuntime(RuntimeMoniker.NativeAot50, "net5.0", "NativeAOT 5.0");
/// <summary>
/// NativeAOT compiled as net6.0
/// </summary>
Expand Down Expand Up @@ -40,7 +36,6 @@ public static NativeAotRuntime GetCurrentVersion()

switch (version)
{
case Version v when v.Major == 5 && v.Minor == 0: return Net50;
case Version v when v.Major == 6 && v.Minor == 0: return Net60;
case Version v when v.Major == 7 && v.Minor == 0: return Net70;
default:
Expand Down
2 changes: 0 additions & 2 deletions src/BenchmarkDotNet/Extensions/RuntimeMonikerExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,6 @@ internal static Runtime GetRuntime(this RuntimeMoniker runtimeMoniker)
return CoreRuntime.Core70;
case RuntimeMoniker.Mono:
return MonoRuntime.Default;
case RuntimeMoniker.NativeAot50:
return NativeAotRuntime.Net50;
case RuntimeMoniker.NativeAot60:
return NativeAotRuntime.Net60;
case RuntimeMoniker.NativeAot70:
Expand Down
23 changes: 7 additions & 16 deletions src/BenchmarkDotNet/Toolchains/NativeAot/Generator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ internal Generator(string ilCompilerVersion,
string runtimeFrameworkVersion, string targetFrameworkMoniker, string cliPath,
string runtimeIdentifier, IReadOnlyDictionary<string, string> feeds, bool useNuGetClearTag,
bool useTempFolderForRestore, string packagesRestorePath,
bool rootAllApplicationAssemblies, bool ilcGenerateCompleteTypeMetadata, bool ilcGenerateStackTraceData, string trimmerDefaultAction)
bool rootAllApplicationAssemblies, bool ilcGenerateCompleteTypeMetadata, bool ilcGenerateStackTraceData)
: base(targetFrameworkMoniker, cliPath, GetPackagesDirectoryPath(useTempFolderForRestore, packagesRestorePath), runtimeFrameworkVersion)
{
this.ilCompilerVersion = ilCompilerVersion;
Expand All @@ -38,7 +38,6 @@ internal Generator(string ilCompilerVersion,
this.rootAllApplicationAssemblies = rootAllApplicationAssemblies;
this.ilcGenerateCompleteTypeMetadata = ilcGenerateCompleteTypeMetadata;
this.ilcGenerateStackTraceData = ilcGenerateStackTraceData;
this.trimmerDefaultAction = trimmerDefaultAction;
}

private readonly string ilCompilerVersion;
Expand All @@ -51,7 +50,6 @@ internal Generator(string ilCompilerVersion,
private readonly bool rootAllApplicationAssemblies;
private readonly bool ilcGenerateCompleteTypeMetadata;
private readonly bool ilcGenerateStackTraceData;
private readonly string trimmerDefaultAction;

private bool IsNuGet => feeds.ContainsKey(NativeAotNuGetFeed) && !string.IsNullOrWhiteSpace(ilCompilerVersion);

Expand Down Expand Up @@ -189,24 +187,17 @@ private string GenerateProjectForLocalBuild(BuildPartition buildPartition, Artif
</Project>";

private string GetTrimmingSettings()
{
StringBuilder sb = new StringBuilder();

sb.AppendLine(rootAllApplicationAssemblies ? "<PublishTrimmed>false</PublishTrimmed>" : "<TrimMode>link</TrimMode>");

if (!string.IsNullOrEmpty(trimmerDefaultAction))
{
sb.Append($"<TrimmerDefaultAction>{trimmerDefaultAction}</TrimmerDefaultAction>");
}

return sb.ToString();
}
=> rootAllApplicationAssemblies
? "" // use the defaults
// TrimMode is set in explicit way as for older versions it might have different default value
Copy link
Member

@MichalStrehovsky MichalStrehovsky Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TrimMode=link by default in .NET 6 and NativeAOT doesn't support .NET 5. It's probably unnecessary.

Copy link
Member Author

@adamsitnik adamsitnik Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@MichalStrehovsky MichalStrehovsky Mar 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@kant2002 kant2002 Apr 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this one #1978 does --runtimes nativeaot5.0 needed?

Copy link
Member Author

@adamsitnik adamsitnik Apr 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.0

With <TrimMode>link</TrimMode> it takes 9s to publish the generated app. Without it it takes 110s. So I am going to keep it.

: "<TrimMode>link</TrimMode><TrimmerDefaultAction>link</TrimmerDefaultAction>";

public IEnumerable<string> GetRdXmlFiles(Type benchmarkTarget, ILogger logger)
{
yield return GeneratedRdXmlFileName;

var projectFile = GetProjectFilePath(benchmarkTarget, logger);
var projectFileFolder = projectFile.DirectoryName;
yield return GeneratedRdXmlFileName;
var rdXml = Path.Combine(projectFileFolder, "rd.xml");
if (File.Exists(rdXml))
{
Expand Down
25 changes: 12 additions & 13 deletions src/BenchmarkDotNet/Toolchains/NativeAot/NativeAotToolchain.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,32 +7,32 @@ namespace BenchmarkDotNet.Toolchains.NativeAot
{
public class NativeAotToolchain : Toolchain
{
/// <summary>
/// compiled as net5.0, targets experimental 6.0.0-* NativeAOT build from the https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-experimental/nuget/v3/index.json
/// </summary>
public static readonly IToolchain Net50 = GetBuilderForOldExperimentalFeed().TargetFrameworkMoniker("net5.0").ToToolchain();
/// <summary>
/// compiled as net6.0, targets experimental 6.0.0-* NativeAOT build from the https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-experimental/nuget/v3/index.json
/// </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")
Copy link
Contributor

@kant2002 kant2002 Apr 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@adamsitnik adamsitnik Apr 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need it as I want to benchmark ILCompiler 6.0 vs 7.0 to see if there are any regressions.

Copy link
Contributor

@kant2002 kant2002 Apr 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense then.

Copy link
Member

@MichalStrehovsky MichalStrehovsky Apr 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.TargetFrameworkMoniker("net6.0")
.ToToolchain();

/// <summary>
/// compiled as net7.0, targets latest (7.0.0-*) NativeAOT build from the .NET 7 feed: https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet7/nuget/v3/index.json
/// </summary>
public static readonly IToolchain Net70 = CreateBuilder().UseNuGet().TargetFrameworkMoniker("net7.0").ToToolchain();

private static NativeAotToolchainBuilder GetBuilderForOldExperimentalFeed()
=> CreateBuilder().UseNuGet("6.0.0-*", "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-experimental/nuget/v3/index.json");
public static readonly IToolchain Net70 = CreateBuilder()
.UseNuGet("7.0.0-*", "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet7/nuget/v3/index.json")
.TargetFrameworkMoniker("net7.0")
.ToToolchain();

internal NativeAotToolchain(string displayName,
string ilCompilerVersion, string ilcPath,
string runtimeFrameworkVersion, string targetFrameworkMoniker, string runtimeIdentifier,
string customDotNetCliPath, string packagesRestorePath,
Dictionary<string, string> feeds, bool useNuGetClearTag, bool useTempFolderForRestore,
bool rootAllApplicationAssemblies, bool ilcGenerateCompleteTypeMetadata, bool ilcGenerateStackTraceData, string trimmerDefaultAction)
bool rootAllApplicationAssemblies, bool ilcGenerateCompleteTypeMetadata, bool ilcGenerateStackTraceData)
: base(displayName,
new Generator(ilCompilerVersion, runtimeFrameworkVersion, targetFrameworkMoniker, customDotNetCliPath,
runtimeIdentifier, feeds, useNuGetClearTag, useTempFolderForRestore, packagesRestorePath,
rootAllApplicationAssemblies, ilcGenerateCompleteTypeMetadata, ilcGenerateStackTraceData, trimmerDefaultAction),
rootAllApplicationAssemblies, ilcGenerateCompleteTypeMetadata, ilcGenerateStackTraceData),
new DotNetCliPublisher(customDotNetCliPath, GetExtraArguments(runtimeIdentifier), GetEnvironmentVariables(ilcPath)),
new Executor())
{
Expand All @@ -43,8 +43,7 @@ internal NativeAotToolchain(string displayName,

public static NativeAotToolchainBuilder CreateBuilder() => NativeAotToolchainBuilder.Create();

public static string GetExtraArguments(string runtimeIdentifier)
=> $"-r {runtimeIdentifier}";
public static string GetExtraArguments(string runtimeIdentifier) => $"-r {runtimeIdentifier}";

// https://github.com/dotnet/corert/blob/7f902d4d8b1c3280e60f5e06c71951a60da173fb/Documentation/how-to-build-and-run-ilcompiler-in-console-shell-prompt.md#compiling-source-to-native-code-using-the-ilcompiler-you-built
// we have to pass IlcPath env var to get it working
Expand Down
17 changes: 1 addition & 16 deletions src/BenchmarkDotNet/Toolchains/NativeAot/NativeAotToolchainBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ public class NativeAotToolchainBuilder : CustomDotNetCliToolchainBuilder
private bool ilcGenerateStackTraceData = true;

private bool isIlCompilerConfigured;
private string trimmerDefaultAction = "link";

/// <summary>
/// creates a NativeAOT toolchain targeting NuGet build of Microsoft.DotNet.ILCompiler
Expand Down Expand Up @@ -111,19 +110,6 @@ public NativeAotToolchainBuilder IlcGenerateStackTraceData(bool value)
return this;
}

/// <summary>
/// By using the default value ("link") this ensures that the trimmer only analyzes the parts of the library's dependencies that are used.
/// It tells the trimmer that any code that is not part of a "root" can be trimmed if it is unused.
/// </summary>
/// <remarks>Pass null or empty string to NOT set TrimmerDefaultAction to any value.</remarks>
[PublicAPI]
public NativeAotToolchainBuilder SeTrimmerDefaultAction(string value = "link")
{
trimmerDefaultAction = value;

return this;
}

[PublicAPI]
public override IToolchain ToToolchain()
{
Expand All @@ -144,8 +130,7 @@ public override IToolchain ToToolchain()
useTempFolderForRestore: useTempFolderForRestore,
rootAllApplicationAssemblies: rootAllApplicationAssemblies,
ilcGenerateCompleteTypeMetadata: ilcGenerateCompleteTypeMetadata,
ilcGenerateStackTraceData: ilcGenerateStackTraceData,
trimmerDefaultAction: trimmerDefaultAction
ilcGenerateStackTraceData: ilcGenerateStackTraceData
);
}
}
Expand Down
2 changes: 0 additions & 2 deletions src/BenchmarkDotNet/Toolchains/ToolchainExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,6 @@ private static IToolchain GetToolchain(RuntimeMoniker runtimeMoniker)
return CsProjCoreToolchain.NetCoreApp60;
case RuntimeMoniker.Net70:
return CsProjCoreToolchain.NetCoreApp70;
case RuntimeMoniker.NativeAot50:
return NativeAotToolchain.Net50;
case RuntimeMoniker.NativeAot60:
return NativeAotToolchain.Net60;
case RuntimeMoniker.NativeAot70:
Expand Down
2 changes: 1 addition & 1 deletion tests/BenchmarkDotNet.IntegrationTests/BuildTimeoutTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public void WhenBuildTakesMoreTimeThanTheTimeoutTheBuildIsCancelled()
var config = ManualConfig.CreateEmpty()
.WithBuildTimeout(timeout)
.AddJob(Job.Dry
.WithRuntime(NativeAotRuntime.Net50)
.WithRuntime(NativeAotRuntime.Net60)
.WithToolchain(NativeAotToolchain.CreateBuilder()
.UseNuGet(
"6.0.0-rc.1.21420.1", // we test against specific version to keep this test stable
Expand Down
2 changes: 1 addition & 1 deletion tests/BenchmarkDotNet.Tests/ConfigParserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ public void ClrVersionParsedCorrectly()
public void IlCompilerPathParsedCorrectly()
{
var fakePath = new FileInfo(typeof(ConfigParserTests).Assembly.Location).Directory;
var config = ConfigParser.Parse(new[] { "-r", "nativeaot50", "--ilcPath", fakePath.FullName }, new OutputLogger(Output)).config;
var config = ConfigParser.Parse(new[] { "-r", "nativeaot60", "--ilcPath", fakePath.FullName }, new OutputLogger(Output)).config;

Assert.Single(config.GetJobs());
NativeAotToolchain toolchain = config.GetJobs().Single().GetToolchain() as NativeAotToolchain;
Expand Down