Compare parameters by object value rather than string representation (Related to #2838)

[User0332]

This is related to issue #2838 and PR #2886

Previous Behavior

Parameters and arguments passed to benchmarks were grouped into logical groups based on ParameterInstances.ValueInfo (a string representation), which failed for types like arrays (whose string representations just contain length and element type), (partially) causing #2838. Any type where objects are not equal but can have the same ToString() was affected by this behavior.

New Behavior

When using DefaultOrderer to group benchmark cases, parameters are compared by their actual value as opposed to their string representations, leading to correct benchmark case grouping. This is done through ParameterEqualityComparer, so any parameter or custom wrapping struct for a parameter overriding object.Equals will now be properly distinguished regardless of its ToString override. For any IEnumerables (both non-generic and generic), there is now built-in functionality in ParameterComparer which compares enumerables element-by-element for value-based equality.

[User0332]

The failing exporter checks seem to be because I changed the schema of the logical group keys, is there a way I could update what verify is expecting?

E.g. verify expects 'Only 1 job in a group can have "Baseline = true" applied to it, group Invalid_TwoJobBaselines.Foo in class Invalid_TwoJobBaselines has 2' where the logical group key is "Invalid_TwoJobBaselines.Foo" but with my new parameter-grouping keys it outputs 'Only 1 job in a group can have "Baseline = true" applied to it, group Distinct Param Set 0-Invalid_TwoJobBaselines.Foo in class Invalid_TwoJobBaselines has 2' where the logical group key is "Distinct Param Set 0-Invalid_TwoJobBaselines.Foo"

[User0332]

I guess I could have logical groups have a key for grouping and a display name for errors, but I feel this would just add unnecessary nonfunctional code

[timcassell]

You can just update the verified files.

[User0332]

You can just update the verified files.

Won't this not affect the CI though?
Or should I not worry about that

[timcassell]

You can just update the verified files.

Won't this not affect the CI though? Or should I not worry about that

I don't understand the question. The test compares the output of the run to the verified file.

[User0332]

You can just update the verified files.

Won't this not affect the CI though? Or should I not worry about that

I don't understand the question. The test compares the output of the run to the verified file.

Sorry, didn't realize that the GitHub actions pulled tests from my PR and not master.

Will implement these changes.

[User0332]

This effectively closes #2838 because it correctly groups parameters into different groups based on value. However, since the example in #2838 passes randomly generated parameters will improper seeding, the ratios won't come out as desired. If the parameters are properly seeded to be deterministic and effectively constant across calls of the ArgumentsSource, then the desired behavior will be exhibited.

With super large arrays like the ones in #2838, though, the report generation takes a few minutes (I think its because of a lot of repeated value comparison work when sorting for the summary table)

[timcassell]

With super large arrays like the ones in #2838, though, the report generation takes a few minutes (I think its because of a lot of repeated value comparison work when sorting for the summary table)

That's likely due to the boxing of every element. Perhaps it can be accelerated by using IStructuralComparable.CompareTo similar to the IComparable path, then special-case the common 2d and 3d arrays, and use the slow path for 4+d arrays. (And same thing for IStructuralEquatable.)

Btw, apparently md arrays throw from IStructural... (dotnet/runtime#66472), which we will need to handle.

[User0332]

I already flatten all enumerators (including md arrays) into an object[] before using StructuralComparer, but unfortunately since I just check for belonging IEnumerable and then perform a linq OfType query on them to convert it to object[], I'm not sure if there is a way to avoid the boxing (let me know if im not understanding your comment correctly).

Another solution I was thinking of was to id parameters with indices of the order they were returned from ArgumentsSource/Arguments attribute with; this would avoid having to do object comparison entirely as we could use the indices to track params

[timcassell]

I already flatten all enumerators (including md arrays) into an object[] before using StructuralComparer, but unfortunately since I just check for belonging IEnumerable and then perform a linq OfType query on them to convert it to object[], I'm not sure if there is a way to avoid the boxing (let me know if im not understanding your comment correctly).

Right, and that flattening is the expensive part, because it boxes every element. My idea is first check for IStructuralComparable, then simply run CompareTo. If the check fails, or if it throws the exception because md arrays don't support it, then check for md array and special-case 2d and 3d, then finally fallback to the code you already have. For the special-case, you can use reflection to create generic functions to avoid the boxing (MethodInfo.MakeGenericMethod).

Another solution I was thinking of was to id parameters with indices of the order they were returned from ArgumentsSource/Arguments attribute with; this would avoid having to do object comparison entirely as we could use the indices to track params

It's clever, but it won't work because 2 methods can have [Arguments(...)] with different order (or number of attrs).

[User0332]

Thanks for clarifying, will do!

[User0332]

I implemented the changes for ParameterComparer but realized that it alone is not actually the culprit, so I reverted a few of the things (like the special case methods) to maintain simplicity.

I'm currently investigating to see what else might be causing the slowdown

[User0332]

Ok, so it was a combination of ParameterComparer and ParameterEqualityComparer. It's sped up by magnitudes now. Thanks for the suggestion!

[User0332]

Thanks for restructuring the fallbacks

[User0332]

Is it guaranteed that the Orderer will always receive the same ImmutableArray of benchmark cases? Because then we could easily save the result of allBenchmarksCases.Select(benchmarkCase => benchmarkCase.Parameters).Distinct(ParameterEqualityComparer.Instance).ToArray() to avoid computing it multiple times.

[timcassell]

Is it guaranteed that the Orderer will always receive the same ImmutableArray of benchmark cases? Because then we could easily save the result of allBenchmarksCases.Select(benchmarkCase => benchmarkCase.Parameters).Distinct(ParameterEqualityComparer.Instance).ToArray() to avoid computing it multiple times.

No, the orderer is a singleton, unfortunately. It will receive the same cases per summary, but users can run multiple times to get multiple summaries.

[User0332]

Or we could overload GetLogicalGroupKey to accept a summary instead of ImmutableArray<BenchmarkCase> allBenchmarksCases and then use an internal mapping to map summaries to the distinct paramSets based on reference equality

It was a quick thing so I tried it out already and I can push it if you'd like

[timcassell]

Or we could overload GetLogicalGroupKey to accept a summary instead of ImmutableArray<BenchmarkCase> allBenchmarksCases and then use an internal mapping to map summaries to the distinct paramSets based on reference equality

It was a quick thing so I tried it out already and I can push it if you'd like

How? It's called from more than just Summary.

[User0332]

I left the old method signsture in place which is non-caching, and there is another one for summaries that is caching

[timcassell]

Let's just leave it for now. We can refactor the interface separately. Thanks.

[timcassell]

I optimized it in e0adbb9 to just scan the array once instead of building a set and then scanning the set.