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.

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

Jump to bottom

ensure the default order of benchmarks is the same as declared in source code #1907

Merged
merged 5 commits into from
Feb 2, 2022
Merged

ensure the default order of benchmarks is the same as declared in source code #1907

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1143e34
before the benchmarks are created and filtered, order the benchmark m…
adamsitnik Feb 1, 2022
ceb724c
make TypeFilter a public class, so users can write unit tests for the…
adamsitnik Feb 2, 2022
104a863
address code review feedback: handle partial classes as well
adamsitnik Feb 2, 2022
4424412
remove redundant check (all provided methods already contain the attr…
adamsitnik Feb 2, 2022
aaff178
address code review feedback
adamsitnik Feb 2, 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
11 changes: 11 additions & 0 deletions src/BenchmarkDotNet.Annotations/Attributes/BenchmarkAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Runtime.CompilerServices;
using JetBrains.Annotations;

namespace BenchmarkDotNet.Attributes
Expand All @@ -7,10 +8,20 @@ namespace BenchmarkDotNet.Attributes
[MeansImplicitUse]
public class BenchmarkAttribute : Attribute
{
public BenchmarkAttribute([CallerLineNumber] int sourceCodeLineNumber = 0, [CallerFilePath] string sourceCodeFile = "")
{
SourceCodeLineNumber = sourceCodeLineNumber;
SourceCodeFile = sourceCodeFile;
}

public string Description { get; set; }

public bool Baseline { get; set; }

public int OperationsPerInvoke { get; set; } = 1;

public int SourceCodeLineNumber { get; }

public string SourceCodeFile { get; }
}
}
10 changes: 8 additions & 2 deletions src/BenchmarkDotNet/Running/BenchmarkConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,14 @@ public static BenchmarkRunInfo TypeToBenchmarks(Type type, IConfig config = null

// We should check all methods including private to notify users about private methods with the [Benchmark] attribute
var bindingFlags = BindingFlags.Static | BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic;
var benchmarkMethods = type.GetMethods(bindingFlags).Where(method => method.HasAttribute<BenchmarkAttribute>()).ToArray();
var benchmarkMethods = type
.GetMethods(bindingFlags)
.Select(method => (method, attribute: method.ResolveAttribute<BenchmarkAttribute>()))
.Where(pair => pair.attribute is not null)
.OrderBy(pair => pair.attribute.SourceCodeFile)
.ThenBy(pair => pair.attribute.SourceCodeLineNumber)
.Select(pair => pair.method)
.ToArray();

return MethodsToBenchmarksWithFullConfig(type, benchmarkMethods, config);
}
Expand Down Expand Up @@ -108,7 +115,6 @@ private static IEnumerable<Descriptor> GetTargets(
Tuple<MethodInfo, TargetedAttribute>[] iterationCleanupMethods)
{
return targetMethods
.Where(m => m.HasAttribute<BenchmarkAttribute>())
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a regression.

Currently, this code silently filters wrong methods. After this code will throw friendly message (and exit) if any method is not benchmarkMethod.

BenchmarkRunner.Run(typeof(BenchmarkClass), new MethodInfo[] { BenchmarkClass.PrivateMethod, string.Concat });

Copy link
Member Author

Choose a reason for hiding this comment

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

@YegorStepanov great catch, I've missed the fact that it's called by

public static BenchmarkRunInfo MethodsToBenchmarks(Type containingType, MethodInfo[] benchmarkMethods, IConfig config = null)

which contains user input

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm adding checks for BenchmarkRunner now:

Should be BenchmarkRunner fast exit if any MethodInfo is wrong or it should print error to the console and keep running?

The same question for Type[]. (Note BenchmarkRunner.Run with args != null switches to BenchmarkSwitcher and exit with error)

BenchmarkRunner.Run(new Type[] {typeof(BenchmarkClass), typeof(int)})

Copy link
Member Author

Choose a reason for hiding this comment

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

@YegorStepanov I think that for user provided MethodInfo it's OK to fail fast

.Select(methodInfo => CreateDescriptor(type,
GetTargetedMatchingMethod(methodInfo, globalSetupMethods),
methodInfo,
Expand Down
6 changes: 3 additions & 3 deletions src/BenchmarkDotNet/Running/TypeFilter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@

namespace BenchmarkDotNet.Running
{
internal static class TypeFilter
public static class TypeFilter
{
internal static (bool allTypesValid, IReadOnlyList<Type> runnable) GetTypesWithRunnableBenchmarks(IEnumerable<Type> types, IEnumerable<Assembly> assemblies, ILogger logger)
public static (bool allTypesValid, IReadOnlyList<Type> runnable) GetTypesWithRunnableBenchmarks(IEnumerable<Type> types, IEnumerable<Assembly> assemblies, ILogger logger)
{
var validRunnableTypes = new List<Type>();

Expand All @@ -37,7 +37,7 @@ internal static (bool allTypesValid, IReadOnlyList<Type> runnable) GetTypesWithR
return (true, validRunnableTypes);
}

internal static BenchmarkRunInfo[] Filter(IConfig effectiveConfig, IEnumerable<Type> types)
public static BenchmarkRunInfo[] Filter(IConfig effectiveConfig, IEnumerable<Type> types)
=> types
.Select(type => BenchmarkConverter.TypeToBenchmarks(type, effectiveConfig))
.Where(info => info.BenchmarksCases.Any())
Expand Down
12 changes: 12 additions & 0 deletions tests/BenchmarkDotNet.Tests/Running/BenchmarkConverterTests.BAC_Partial_DifferentFiles.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
using BenchmarkDotNet.Attributes;

namespace BenchmarkDotNet.Tests.Running
{
public partial class BenchmarkConverterTests
{
public partial class BAC_Partial_DifferentFiles
{
[Benchmark] public void B() { }
}
}
}
43 changes: 41 additions & 2 deletions tests/BenchmarkDotNet.Tests/Running/BenchmarkConverterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.Linq;
using System;
using System.Linq;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Configs;
using BenchmarkDotNet.Environments;
Expand All @@ -9,7 +10,7 @@

namespace BenchmarkDotNet.Tests.Running
{
public class BenchmarkConverterTests
public partial class BenchmarkConverterTests
{
/// <summary>
/// https://github.com/dotnet/BenchmarkDotNet/issues/495
Expand Down Expand Up @@ -198,5 +199,43 @@ public class WithFewMutators
{
[Benchmark] public void Method() { }
}

[Fact]
public void MethodDeclarationOrderIsPreserved()
{
foreach (Type type in new[] { typeof(BAC), typeof(BAC_Partial), typeof(BAC_Partial_DifferentFiles) })
{
var info = BenchmarkConverter.TypeToBenchmarks(type);

Assert.Equal(nameof(BAC.B), info.BenchmarksCases[0].Descriptor.WorkloadMethod.Name);
Assert.Equal(nameof(BAC.A), info.BenchmarksCases[1].Descriptor.WorkloadMethod.Name);
Assert.Equal(nameof(BAC.C), info.BenchmarksCases[2].Descriptor.WorkloadMethod.Name);
}
}

public class BAC
{
// BAC is not sorted in either desceding or ascending way
[Benchmark] public void B() { }
[Benchmark] public void A() { }
[Benchmark] public void C() { }
}

public partial class BAC_Partial
{
[Benchmark] public void B() { }
[Benchmark] public void A() { }
}

public partial class BAC_Partial
{
[Benchmark] public void C() { }
}

public partial class BAC_Partial_DifferentFiles
{
[Benchmark] public void A() { }
[Benchmark] public void C() { }
}
}
}