Can't use latest version of BenchmarkDotNet

[radical]

I tried to use the latest bdn master (73f8fd1) here, but trying to run benchmarks fails with:

Unhandled exception. System.InvalidOperationException: Failed to compare two elements in the array.
 ---> System.ArgumentException: At least one object must implement IComparable.
   at System.Collections.Comparer.Compare(Object a, Object b)
   at System.ValueTuple`3.CompareTo(ValueTuple`3 other)
   at System.ValueTuple`3.System.IComparable.CompareTo(Object other)
   at BenchmarkDotNet.Parameters.ParameterComparer.CompareValues(Object x, Object y)
   at BenchmarkDotNet.Parameters.ParameterComparer.Compare(ParameterInstances x, ParameterInstances y)
   at BenchmarkDotNet.Order.DefaultOrderer.BenchmarkComparer.Compare(BenchmarkCase x, BenchmarkCase y)
   at System.Collections.Generic.ArraySortHelper`1.SwapIfGreater(Span`1 keys, Comparison`1 comparer, Int32 i, Int32 j)
   at System.Collections.Generic.ArraySortHelper`1.PickPivotAndPartition(Span`1 keys, Comparison`1 comparer)
   at System.Collections.Generic.ArraySortHelper`1.IntroSort(Span`1 keys, Int32 depthLimit, Comparison`1 comparer)
   at System.Collections.Generic.ArraySortHelper`1.IntrospectiveSort(Span`1 keys, Comparison`1 comparer)
   at System.Collections.Generic.GenericArraySortHelper`1.Sort(Span`1 keys, IComparer`1 comparer)
   --- End of inner exception stack trace ---
   at System.Collections.Generic.GenericArraySortHelper`1.Sort(Span`1 keys, IComparer`1 comparer)
   at System.Array.Sort[T](T[] array, Int32 index, Int32 length, IComparer`1 comparer)
   at System.Collections.Generic.List`1.Sort(Int32 index, Int32 count, IComparer`1 comparer)
   at BenchmarkDotNet.Order.DefaultOrderer.GetExecutionOrder(ImmutableArray`1 benchmarkCases, IEnumerable`1 order)
   at BenchmarkDotNet.Running.BenchmarkConverter.MethodsToBenchmarksWithFullConfig(Type type, MethodInfo[] benchmarkMethods, IConfig config)
   at BenchmarkDotNet.Running.BenchmarkConverter.TypeToBenchmarks(Type type, IConfig config)
   at BenchmarkDotNet.Running.TypeFilter.<>c__DisplayClass1_0.<Filter>b__0(Type type)
   at System.Linq.Enumerable.SelectListIterator`2.MoveNext()
   at System.Linq.Enumerable.WhereEnumerableIterator`1.ToArray()
   at BenchmarkDotNet.Running.TypeFilter.Filter(IConfig effectiveConfig, IEnumerable`1 types)
   at BenchmarkDotNet.Running.BenchmarkSwitcher.RunWithDirtyAssemblyResolveHelper(String[] args, IConfig config, Boolean askUserForInput)
   at BenchmarkDotNet.Running.BenchmarkSwitcher.Run(String[] args, IConfig config)
   at MicroBenchmarks.Program.Main(String[] args) in /Users/ankj/dev/performance/src/benchmarks/micro/Program.cs:line 43

This seems to be failing for

System.ValueTuple`3[System.Globalization.CultureInfo,System.Globalization.CompareOptions,System.Boolean], (en-US, Ordinal, False)
System.ValueTuple`3[System.Globalization.CultureInfo,System.Globalization.CompareOptions,System.Boolean], (pl-PL, None, False)`

.. , because CultureInfo does not implement IComparable.

IIUC, this is a new code path introduced as part of #2304 .

And it fails for https://github.com/dotnet/performance/blob/dd14c7d44444a0211035af8464b743a55b4dd55e/src/benchmarks/micro/libraries/System.Globalization/StringSearch.cs#L46 .

cc @mrahhal @adamsitnik @LoopedBard3

[radical]

Should this be in dotnet/BenchmarkDotNet instead?

[mrahhal]

The following reproduces the problem:

var t1 = (new CultureInfo("en-US"), CompareOptions.Ordinal, false);
var t2 = (new CultureInfo("pl-PL"), CompareOptions.None, false);

Assert.IsTrue(typeof(IComparable).IsAssignableFrom(t1.GetType())); // Succeeds

var result = t1.CompareTo(t2); // Throws System.ArgumentException: At least one object must implement IComparable.

This is because ValueTuple implements IComparable regardless of whether or not the inner types inside imeplement it, so its CompareTo implementation is fallible and might throw in that case.

In this particular case, CultureInfo doesn't, and so it throws.

One potential solution is to have specific support for detecting ValueTuples, and do an alternative check of whether all the types inside are IComparable to consider the whole tuple IComparable. Or the easy way out, if it's a ValueTuple then just skip the IComparable support and go straight to the ToString() comparison.

For reference, the ValueTuple's CompareTo implementation, and this is where we might solve this in BenchmarkDotNet.

[mrahhal]

Working on this fix. Let me know which of the following two solutions I listed above do you prefer:

1. For ValueTuples, check all inner types if they implement IComparable before considering it such
2. For ValueTuples, just skip IComparable support and go straight to string comparison (emulates old behavior for ValueTuples)

Any other suggested solution?

[timcassell]

@mrahhal Maybe just catch the exception and fall back to string comparison?

[mrahhal]

Could do that, but that might hide other unrelated exceptions we didn't intend to hide. Might be better to properly handle this particular case in my opinion. ValueTuple seems to be a special case, IComparable doesn't usually throw.

[cincuranet]

@mrahhal How is the fix looking? Any ETA? Thanks.

[mrahhal]

Was waiting for a reply as I didn't want to work on something and then change it right after a review. And honestly forgot about it while waiting. Ready to roll out a fix when a maintainer confirms one of my two suggestions above..

Or I think I'll just go with whatever I think is better to start with. Will submit a PR soon.

[timcassell]

I think if you want to handle ValueTuple specially, it's fine.

check all inner types if they implement IComparable before considering it such

Just be aware that they can have variable number of types and nested ValueTuples.

[cincuranet]

Just be aware that they can have variable number of types and nested ValueTuples.

Going through ITuple might make that easier.

[mrahhal]

Going through ITuple might make that easier.

That's an internal interface (and it only has a Size prop, I don't think it helps much anyway).

Just be aware that they can have variable number of types and nested ValueTuples.

Yes. A simple recursive check will do for nesting. The problem is actually how to detect ValueTuples, since it's a series of (unlimited number of?) structs depending on the number of items. And I don't feel either supporting up to some fixed number or doing something fancy with Type.FullName is a good idea.

Starting to think handling exceptions when calling the IComparable impl and falling back to .ToString() as @timcassell suggested is just the easiest as a start to get the old behavior back. Mayhe this can be handled better in the future, but let's just roll out a fix for now.

[cincuranet]

That's an internal interface (and it only has a Size prop, I don't think it helps much anyway).

It's public interface. And has also indexer.

[mrahhal]

It's public interface. And has also indexer.

I think it's internal in netstandard. But more importantly, isn't ITuple for Tuple (the older class tuples)? I don't think it's related to ValueTuple.

[cincuranet]

Yeah, you need at least netstandard2.1. :(

Both Tuple and ValueTuple.

[timcassell]

BDN has a transitive reference to the System.ValueTuple nuget package, so you can use the ITuple interface. In fact, you should use that interface to check for Tuple also, as it also has the same IComparable functionality.

public interface ITuple
{
    int Length { get; }
    object this[int index] { get; }
}

[mrahhal]

Doesn't seem usable in netstandard2.0.

And yes, Tuple has the same problem. Looking at the ITuple interface above, indeed it would have been the best way to solve this, but I guess we're stuck because of netstandard2.0.

[timcassell]

Oh weird, it's not public from the nuget package. You may still be able to use it with reflection, though.

[mrahhal]

Would going this far really be worth it here? 😅

I think it's better to just solve it the reliable way for now. Catch the exception and fallback.

[timcassell]

Catch the exception and fallback.

I also find that acceptable. Just catch the exact exception type and compare the message to avoid suppressing erroneous exceptions.

System.ArgumentException: At least one object must implement IComparable.