Allow parameterized arrays to be differentiated using value comparison (Fixes #2838)

[User0332]

Solves a larger problem dealing with array comparison that fixes #2838

Previous Behavior

In the following example,

public class ArrayOnDifferentMethods
{
	public IEnumerable<int[]> GetArrays()
	{
		yield return new int[] { 1, 2, 3 };
		yield return new int[] { 2, 3, 4 };
	}

	[Benchmark(Baseline = true)]
	[ArgumentsSource(nameof(GetArrays))]
	public void AcceptsArrays(int[] arr)
	{
		int[] x;

		if (arr[0] == 1) x = [.. arr.Order()];
		else x = [];

		if (x.Length != 3 && x.Length != 0)
			throw new ArgumentException("Incorrect length");
	}

	[Benchmark]
	[ArgumentsSource(nameof(GetArrays))]
	public void AcceptsArrays2(int[] arr)
	{
		if (arr.Length != 3)
			throw new ArgumentException("Incorrect length");
	}
}

the arguments int[] { 1, 2, 3 } and int[] { 2, 3, 4 } would not be recognized as different because their ParameterInstances.ValueInfo property--which DefaultOrderer uses to generate logical groups--depended on object.ToString(), which for an array just provides the type and length of the array (in this case, they were equal, so the library interpreted them as the same argument). Because they were seen as the same, the cases where arr = { 1, 2, 3 } and arr = { 2, 3, 4 } were not separated into different logical groups and the runtime ratio for all cases was calculated with respect to the first case AcceptsArrays({ 1, 2, 3 }). The desired behavior would be for the cases with corresponding arguments to be grouped properly and have the baseline runtime calculated once for arr = { 1, 2, 3 } and another time for arr = { 2, 3, 4 }.

Solution

Have ParameterInstances.ValueInfo depend on a customizable ToValueText (which eventually propagates down to a new property IParam.ValueText) rather than ToString() . For most param types, IParam.ValueText will point to IParam.ValueText, but for arrays, it will contain extra information like Array.Rank as well as a hash computed based off of the array's elements to properly distinguish between different-valued arrays of the same size.

Downsides of this approach

• As the array size grows, this approach to differentiating different arrays might grow expensive.
• This approach is prone to hash collisions; it uses hashes because it is unreasonable to add every array element to a string representation (especially if the array is large). This issue is mitigated by the fact that the array length, rank, and value hash must all be equivalent for two arrays to be seen as equivalent.

New Behavior

Benchmark cases accepting an array parameter or argument will be grouped by array values rather than just length.

The new test case ArrayArgumentsCanBeGrouped tests for this behavior.

[timcassell]

2 issues I have with this.

Hashes should not be used to determine equality, as you mentioned they can collide. If we really want to go down this road, we should probably refactor the system to get it correct.

Users may be surprised to see it work for arrays, but not lists and other collection types.

I wonder if we should just leave it up to the user somehow (I'm not sure if the current system lets users override it already).

[User0332]

I agree with the hashing equality thing and that we probably refactor to not use ValueText

On the docs there is a snippet on wrapping things like arrays and other collections which don't have signatures built in to their ToString in a custom class that provides a unique ToString, so maybe this is a wontfix?

https://benchmarkdotnet.org/articles/features/parameterization.html#another-example:~:text=ns%20%7C%200.3063%20ns%20%7C-,Another%20example,-If%20the%20values

[User0332]

On the other hand I could bring back my old ParameterComparer code that worked for arrays via StructuralComparer (so we could probably get it to work with most collections through LINQ conversions) and refactor the way logical groups are organized (to go for object comparison rather than ValueInfo comparsion)

[timcassell]

Well, looking at the OP again, it looks like the display value of the text is not being taken into account for the array comparisons, so that should be fixed at least (so it will at least take length into account). I also see that it shows multi dimensional arrays as single dimension, so that should also be fixed. But probably yeah we leave the true equality comparisons as won't fix, and the user can wrap it in a struct that implements IComparable and do it themself.

[timcassell]

On the other hand I could bring back my old ParameterComparer code that worked for arrays via StructuralComparer (so we could probably get it to work with most collections through LINQ conversions) and refactor the way logical groups are organized (to go for object comparison rather than ValueInfo comparsion)

I'm open to trying that out.

[User0332]

On the other hand I could bring back my old ParameterComparer code that worked for arrays via StructuralComparer (so we could probably get it to work with most collections through LINQ conversions) and refactor the way logical groups are organized (to go for object comparison rather than ValueInfo comparsion)

I'm open to trying that out.

I'll try this first and otherwise will just go for fixing #2838

Closing this PR for now since both changes would go in a different direction.