Skip to content
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.

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

Make a bit of BenchmarkDotNet trimmable #2046

Merged
merged 2 commits into from
Jul 20, 2022

Conversation

MichalStrehovsky
Copy link
Member

Another attempt at fixing the problem I tried to fix in #2020.

In that pull request, I reflection-rooted all of BenchmarkDotNet, but that looks like it increases the size of the closure too much (dotnet/performance#2532). So here I'm rolling that part back and annotating enough of the Characteristic APIs to make it so that we shouldn't run into the original problem.

I basically added <EnableTrimAnalyzer>true</EnableTrimAnalyzer> to the BechmarkDotNet project, looked at the trimming warning in

private static IReadOnlyList<Characteristic> GetThisTypeCharacteristicsCore(Type characteristicObjectType)
{
var fieldValues = characteristicObjectType.GetTypeInfo()
.GetFields(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.FlattenHierarchy | BindingFlags.Static)
.Where(f => IsCharacteristicSubclass(f.FieldType))
.Select(f => AssertHasValue(f, (Characteristic)f.GetValue(null)));
var propertyValues = characteristicObjectType.GetTypeInfo()
.GetProperties(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.FlattenHierarchy | BindingFlags.Static)
.Where(p => p.GetMethod != null && IsCharacteristicSubclass(p.PropertyType))
.Select(p => AssertHasValue(p, (Characteristic)p.GetValue(null)));
// DONTTOUCH: DO NOT change the order of characteristic as it may break logic of some operations.
return fieldValues
.Concat(propertyValues)
.Distinct()
.OrderBy(k => k.HasChildCharacteristics ? 1 : 0)
.ThenBy(k => k.Id)
.ToArray();
}
where we were getting the crash and kept adding annotations until it stopped generating new warnings.

We now have an annotation in a spot that should make sure the Job class has all fields and properties kept (it's used with one of the annotated Create methods).

There are more trimming warnings that I didn't try to solve because they're unrelated to the problem at hand.

Cc @adamsitnik
Cc @dotnet/ilc-contrib FYI

Another attempt at fixing the problem I tried to fix in dotnet#2020.

In that pull request, I reflection-rooted all of BenchmarkDotNet, but that looks like it increases the size of the closure too much (dotnet/performance#2532). So here I'm rolling that part back and annotating enough of the `Characteristic` APIs to make it so that we shouldn't run into the original problem.

I basically added `<EnableTrimAnalyzer>true</EnableTrimAnalyzer>` to the BechmarkDotNet project, looked at the trimming warning in https://github.com/dotnet/BenchmarkDotNet/blob/63e28c100a42a6492d09a0b93a8a4c141061bb0d/src/BenchmarkDotNet/Characteristics/CharacteristicHelper.cs#L49-L68 where we were getting the crash and kept getting annotations until it stopped generating new warnings.

We now have an annotation in a spot that should make sure the `Job` class has all fields and properties kept (it's used with one of the annotated `Create` methods).

There are more trimming warnings that I didn't try to solve because they're unrelated to the problem at hand.
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks a lot @MichalStrehovsky !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants