Skip to content

Fix array comparison in params #2153

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix array comparison in params #2153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2a37a2f
add tests
YegorStepanov Oct 16, 2022
2366d72
Fix array comparison
YegorStepanov Oct 16, 2022
3f92bf9
Update tests
YegorStepanov Oct 16, 2022
2b06bbd
delete comment
YegorStepanov Oct 17, 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
22 changes: 22 additions & 0 deletions src/BenchmarkDotNet/Parameters/ParameterComparer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,35 @@ public int Compare(ParameterInstances x, ParameterInstances y)
if (x == null) return -1;
for (int i = 0; i < Math.Min(x.Count, y.Count); i++)
{
//todo: compare non-primitive types too
int compareTo = PrimitiveComparer.CompareTo(x[i]?.Value, y[i]?.Value);
if (compareTo != 0)
return compareTo;

int arrayCompareTo = CompareArrays(x[i]?.Value, y[i]?.Value);
Copy link
Member

Choose a reason for hiding this comment

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

I was surprised to see that it works, then I've checked and found that we are relying on the .ToString comparison:

BenchmarkDotNet/src/BenchmarkDotNet/Parameters/ParameterComparer.cs

Line 49 in 58d4bae

: string.CompareOrdinal(x?.ToString(), y?.ToString());

It would be more future proof to perform an explicit check:

int compareTo = x[i]?.Value is Array xArray && y[i]?.Value is Array yArray
   ? CompareArrays(xArra, yArray)
   : PrimitiveComparer.CompareTo(x[i]?.Value, y[i]?.Value);

Copy link
Contributor Author

@YegorStepanov YegorStepanov Oct 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The code works like this:

1) if type is primitive:
1.1) if (x > y or x < y) : return 1 or -1
2) if type is array:
1.1) if (x > y or x < y) : return 1 or -1
3) compare by string and return

So, if the x=0 and y=0, it exits on the last line:

return string.CompareOrdinal(0.ToString(), 0.ToString());

Future

Also, we need to add support for user IComparable types here, so the code will look like:

int primitiveCompareTo = PrimitiveComparer...;
if (primitiveCompareTo != 0)
   return primitiveCompareTo;

int arrayCompareTo = ArrayComparer...;
if (arrayCompareTo != 0)
   return arrayCompareTo;

int userCompareTo = IComparableComparer...;
if (userCompareTo != 0)
   return userCompareTo;

return string.CompareOrdinal(x?.ToString(), y?.ToString());

Stuns but looks elegant

if (arrayCompareTo != 0)
return arrayCompareTo;
}
return string.CompareOrdinal(x.DisplayInfo, y.DisplayInfo);
}

private static int CompareArrays(object x, object y)
{
if (x is Array xArray && y is Array yArray)
{
for (int i = 0; i < Math.Min(xArray.Length, yArray.Length); i++)
{
//todo: compare non-primitive types too
int compareTo = PrimitiveComparer.CompareTo(xArray.GetValue(i), yArray.GetValue(i));
if (compareTo != 0)
return compareTo;
}
if (xArray.Length != yArray.Length)
return xArray.Length.CompareTo(yArray.Length);
}
return 0;
}

private class Comparer
{
private readonly Dictionary<Type, Func<object, object, int>> comparers =
Expand Down
17 changes: 16 additions & 1 deletion src/BenchmarkDotNet/Parameters/ParameterInstances.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,25 @@ public void Dispose()

public string DisplayInfo => Items.Any() ? "[" + string.Join(", ", Items.Select(p => $"{p.Name}={p.ToDisplayText()}")) + "]" : "";

public string ValueInfo => Items.Any() ? "[" + string.Join(", ", Items.Select(p => $"{p.Name}={p.Value?.ToString() ?? ParameterInstance.NullParameterTextRepresentation}")) + "]" : "";
public string ValueInfo => Items.Any() ? "[" + string.Join(", ", Items.Select(p => $"{p.Name}={GetValueInfo(p.Value)}")) + "]" : "";

public string PrintInfo => printInfo ?? (printInfo = string.Join("&", Items.Select(p => $"{p.Name}={p.ToDisplayText()}")));

private static string GetValueInfo(object value)
{
if (value is not Array array)
return value?.ToString() ?? "null";

var strings = new List<string>(array.Length); //test array of array
for (int i = 0; i < array.Length; i++)
{
string str = GetValueInfo(array.GetValue(i));
strings.Add(str);
}

return string.Join(",", strings);
}

public ParameterInstance GetArgument(string name) => Items.Single(parameter => parameter.IsArgument && parameter.Name == name);

public bool Equals(ParameterInstances other)
Expand Down
20 changes: 20 additions & 0 deletions ...ers/ApprovedFiles/MarkdownExporterApprovalTests.GroupExporterTest.ParamArray.approved.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
=== ParamArray ===

BenchmarkDotNet=v0.10.x-mock, OS=Microsoft Windows NT 10.0.x.mock, VM=Hyper-V
MockIntel Core i7-6700HQ CPU 2.60GHz (Max: 3.10GHz), 1 CPU, 8 logical and 4 physical cores
Frequency=2531248 Hz, Resolution=395.0620 ns, Timer=TSC
[Host] : Clr 4.0.x.mock, 64mock RyuJIT-v4.6.x.mock CONFIGURATION
DefaultJob : extra output line


Method | Param | Mean | Error | StdDev | Ratio | RatioSD | LogicalGroup | Baseline |
------- |-------- |---------:|--------:|--------:|------:|--------:|--------------------- |--------- |
Foo | Byte[1] | 102.0 ns | 6.09 ns | 1.58 ns | 1.00 | 0.00 | [Param=0]-DefaultJob | Yes |
Foo | Byte[1] | 202.0 ns | 6.09 ns | 1.58 ns | 1.98 | 0.02 | [Param=0]-DefaultJob | Yes |
Bar | Byte[1] | 302.0 ns | 6.09 ns | 1.58 ns | 2.96 | 0.03 | [Param=0]-DefaultJob | No |
Bar | Byte[1] | 402.0 ns | 6.09 ns | 1.58 ns | 3.94 | 0.05 | [Param=0]-DefaultJob | No |
| | | | | | | | |
Foo | Byte[1] | 502.0 ns | 6.09 ns | 1.58 ns | 1.00 | 0.00 | [Param=1]-DefaultJob | Yes |
Bar | Byte[1] | 602.0 ns | 6.09 ns | 1.58 ns | 1.20 | 0.00 | [Param=1]-DefaultJob | No |

Errors: 0
20 changes: 20 additions & 0 deletions ...ovedFiles/MarkdownExporterApprovalTests.GroupExporterTest.ParamArrayOfArrays.approved.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
=== ParamArrayOfArrays ===

BenchmarkDotNet=v0.10.x-mock, OS=Microsoft Windows NT 10.0.x.mock, VM=Hyper-V
MockIntel Core i7-6700HQ CPU 2.60GHz (Max: 3.10GHz), 1 CPU, 8 logical and 4 physical cores
Frequency=2531248 Hz, Resolution=395.0620 ns, Timer=TSC
[Host] : Clr 4.0.x.mock, 64mock RyuJIT-v4.6.x.mock CONFIGURATION
DefaultJob : extra output line


Method | Param | Mean | Error | StdDev | Ratio | RatioSD | LogicalGroup | Baseline |
------- |----------- |---------:|--------:|--------:|------:|--------:|----------------------- |--------- |
Foo | Int32[][2] | 102.0 ns | 6.09 ns | 1.58 ns | 1.00 | 0.00 | [Param=0,0]-DefaultJob | Yes |
Foo | Int32[][2] | 302.0 ns | 6.09 ns | 1.58 ns | 2.96 | 0.03 | [Param=0,0]-DefaultJob | Yes |
Bar | Int32[][2] | 402.0 ns | 6.09 ns | 1.58 ns | 3.94 | 0.05 | [Param=0,0]-DefaultJob | No |
Bar | Int32[][2] | 602.0 ns | 6.09 ns | 1.58 ns | 5.90 | 0.08 | [Param=0,0]-DefaultJob | No |
| | | | | | | | |
Foo | Int32[][2] | 202.0 ns | 6.09 ns | 1.58 ns | 1.00 | 0.00 | [Param=0,1]-DefaultJob | Yes |
Bar | Int32[][2] | 502.0 ns | 6.09 ns | 1.58 ns | 2.49 | 0.01 | [Param=0,1]-DefaultJob | No |

Errors: 0
56 changes: 44 additions & 12 deletions tests/BenchmarkDotNet.Tests/Exporters/MarkdownExporterApprovalTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.Linq;
Expand Down Expand Up @@ -225,8 +226,8 @@ [Benchmark] public void Bar() { }
[SimpleJob(id: "Job1", baseline: true), SimpleJob(id: "Job2")]
public class MethodJobBaseline_MethodsJobs
{
[Benchmark(Baseline = true)] public void Foo() {}
[Benchmark] public void Bar() {}
[Benchmark(Baseline = true)] public void Foo() { }
[Benchmark] public void Bar() { }
}

[RankColumn, LogicalGroupColumn, BaselineColumn]
Expand All @@ -235,25 +236,25 @@ public class MethodJobBaseline_MethodsJobsParams
{
[Params(2, 10), UsedImplicitly] public int Param;

[Benchmark(Baseline = true)] public void Foo() {}
[Benchmark] public void Bar() {}
[Benchmark(Baseline = true)] public void Foo() { }
[Benchmark] public void Bar() { }
}

/* Invalid */

[RankColumn, LogicalGroupColumn, BaselineColumn]
public class Invalid_TwoMethodBaselines
{
[Benchmark(Baseline = true)] public void Foo() {}
[Benchmark(Baseline = true)] public void Bar() {}
[Benchmark(Baseline = true)] public void Foo() { }
[Benchmark(Baseline = true)] public void Bar() { }
}

[RankColumn, LogicalGroupColumn, BaselineColumn]
[SimpleJob(id: "Job1", baseline: true), SimpleJob(id: "Job2", baseline: true)]
public class Invalid_TwoJobBaselines
{
[Benchmark] public void Foo() {}
[Benchmark] public void Bar() {}
[Benchmark] public void Foo() { }
[Benchmark] public void Bar() { }
}

/* Escape Params */
Expand All @@ -262,10 +263,41 @@ public class Escape_ParamsAndArguments
{
[Params("\t", "\n"), UsedImplicitly] public string StringParam;

[Arguments('\t')] [Arguments('\n')]
[Benchmark] public void Foo(char charArg) {}
[Benchmark] public void Bar() {}
[Arguments('\t'), Arguments('\n')]
[Benchmark] public void Foo(char charArg) { }
[Benchmark] public void Bar() { }
}

/* Param Arrays */

[BaselineColumn]
[LogicalGroupColumn]
public class ParamArray
{
[Params(new byte[] { 0 }, new byte[] { 1 }, new byte[] { 0 })]
Copy link
Member

Choose a reason for hiding this comment

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

what do we gain by having duplicate params (new byte[] { 0 })? one day we might move https://github.com/dotnet/performance/blob/main/src/harness/BenchmarkDotNet.Extensions/UniqueArgumentsValidator.cs into BDN and this would be considered as a bug

Suggested change
[Params(new byte[] { 0 }, new byte[] { 1 }, new byte[] { 0 })]
[Params(new byte[] { 0 }, new byte[] { 1 })]

Copy link
Contributor Author

@YegorStepanov YegorStepanov Oct 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You want to remove the purpose of these tests and PR :)

These tests check that arrays can be grouped if they have the same primitive values (but they are actually different arrays).

We can disallow duplicate arguments for [Params] because Attribute supports only the built-in types. (IMO it should do Roslyn Analyzer only, not a validator)

But how can we disable it for [ParamSource]?

If the values are not primitive, it fallbacks to .ToString, which means that most of user classes are equal:

x.ToString().CompareTo(y.String()) == 0 //always if .ToString is not overloaded

UniqueArgumentsValidator

If I remember correctly, you can't distinguish between [Params] and [ParamSource] values, so UniqueArgumentsValidator falls for:

[ArgumentSource("GetValues")]
[Benchmark]
public void Benchmark(MyType instance) { }

public IEnumerable<MyType> GetValues()
{
    // fails because both ToString() are equal:
    yield return new MyType { Value = 10 };
    yield return new MyType { Value = 100 };
}

public class MyType { public int Value; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the type implements IComparable then we shouldn't jump to ToString() (because it can be overloaded).
So the code will look like:

if(IComparableComparer.TryCompareTo(x, y, var userCompareTo))
    return userCompareTo;

But it works for primitives/arrays.

Are you satisfied with the current implementation, or should I remake it?

public byte[] Param;

[Benchmark(Baseline = true)] public void Foo() { }
[Benchmark] public void Bar() { }
}

[BaselineColumn]
[LogicalGroupColumn]
public class ParamArrayOfArrays
{
[ParamsSource(nameof(GetValues))]
public int[][] Param;

public IEnumerable<int[][]> GetValues()
{
yield return new int[][] { new[] { 0 }, new[] { 0 }, };
yield return new int[][] { new[] { 0 }, new[] { 1 }, };
yield return new int[][] { new[] { 0 }, new[] { 0 }, };
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Suggested change
yield return new int[][] { new[] { 0 }, new[] { 0 }, };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

answered above

}

[Benchmark(Baseline = true)] public void Foo() { }
[Benchmark] public void Bar() { }
}
}
}
}
}