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.

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

Conversation

YegorStepanov
Copy link
Contributor

fixes #791

Look at the third commit to see what the PR changes.

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.

It's almost ready. PTAL at my comments and again thank you @YegorStepanov !

[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

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?

{
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

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:

: 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

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

Co-authored-by: Adam Sitnik <[email protected]>
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.

Benchmark(Baseline = true) doesn't work when using byte[] arrays in Params()
2 participants