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

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

Merged
merged 5 commits into from
Feb 2, 2022

Conversation

adamsitnik
Copy link
Member

The story behind this fix is quite long.

In the dotnet/performance repo we have a single type that allows for partitioning benchmarks. The idea is quite simple: it's a filter that takes partition index and count and performs modulo operation to check whether current benchmark belongs to given partition (and should be executed).

Entire logic:

public bool Predicate(BenchmarkCase benchmarkCase)
    => _counter++ % _partitionsCount == _partitionIndex; // will return true only for benchmarks that belong to it’s partition

Some time ago @DrewScoggins has realized that some of the benchmarks in our Reporting System seem to be missing data from time to time. In the example below you can see no new uploads since december:

image

I believe that it's most likely caused by lack of deterministic order in the reflection APIs (dotnet/runtime#46272). There is no guarantee that the value of _counter will be the same for the same benchmark if we compile and run exactly the same source code multiple times.

Example: let's say that we have 4 benchmarks: A, B, C and D.
Usually they are provided to the filter (which uses their index for filtering) as A, B, C, D.
But sometimes they are ordered as A, B, D, C.

So if we have 2 partitions, and run the process with:
--partition-count 2 --partition-index 0: for ABCD it decides to run benchmark A and C
--partition-count 2 --partition-index 1: for ABDC it decides to run benchmark B and C.

As you can see, benchmark D was never executed because the index changed between two runs of exactly same code.

The types are already sorted by name:

internal static Type[] GetRunnableBenchmarks(this Assembly assembly)
=> assembly
.GetTypes()
.Where(type => type.ContainsRunnableBenchmarks())
.OrderBy(t => t.Namespace)
.ThenBy(t => t.Name)
.ToArray();

But we can't sort the methods by names as our users are used to the fact that BDN by default runs the benchmarks in the declaration order.

My proposal is to include the source code line provided by the compiler in the [BenchmarkAttribute] and just use the value for ordering.

I have not added any tests as it's super hard to repro this bug.
I am aware of the fact that the partition filter referenced above is fragile.

…ethods by source code line number so the order is always the same
@adamsitnik adamsitnik added this to the v0.13.2 milestone Feb 1, 2022
Copy link
Member

@AndreyAkinshin AndreyAkinshin left a comment

Choose a reason for hiding this comment

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

In general, I like this approach. However, I guess it makes sense to also use CallerFilePath in order to correctly support benchmark ordering in partial classes.

@adamsitnik
Copy link
Member Author

@AndreyAkinshin good point, I've addressed your feedback, PTAL

@@ -108,7 +115,6 @@ private static ImmutableConfig GetFullMethodConfig(MethodInfo method, ImmutableC
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

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