Simplify some non-generic CompareTo methods

[lilinus]

Reducing some duplicate code

[jkotas]

Is this change going to regress performance of CompareTo(object? value)?

[lilinus]

I actually get a performance improvement:

BenchmarkDotNet v0.13.13-nightly.20240311.145, Windows 10 (10.0.19044.3086/21H2/November2021Update)
AMD Ryzen 9 4900HS with Radeon Graphics, 1 CPU, 16 logical and 8 physical cores
.NET SDK 9.0.100-preview.3.24204.13
  [Host]     : .NET 9.0.0 (9.0.24.17209), X64 RyuJIT AVX2
  Job-DBVVUO : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2
  Job-KVLREW : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2

PowerPlanMode=00000000-0000-0000-0000-000000000000  IterationTime=250ms  MaxIterationCount=20
MinIterationCount=15  WarmupCount=1

| Method         | Job        | Toolchain                                                                                              | Mean     | Error     | StdDev    | Median   | Min      | Max      | Ratio | Gen0   | Allocated | Alloc Ratio |
|--------------- |----------- |------------------------------------------------------------------------------------------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|-------:|----------:|------------:|
| CompareTo_Guid | Job-DBVVUO | \dnrr\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe | 7.753 ns | 0.0870 ns | 0.0771 ns | 7.780 ns | 7.593 ns | 7.837 ns |  1.00 | 0.0153 |      32 B |        1.00 |
| CompareTo_Guid | Job-KVLREW | \dnrt\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe | 7.450 ns | 0.0677 ns | 0.0634 ns | 7.461 ns | 7.316 ns | 7.540 ns |  0.96 | 0.0153 |      32 B |        1.00 |

[jkotas]

Could you please share the source code for the benchmark?

[lilinus]

https://github.com/lilinus/performance/blob/8788b69d993716a009579fe11ce861de14956449/src/benchmarks/micro/libraries/System.Runtime/Perf.TestPr.cs

[jkotas]

https://github.com/lilinus/performance/blob/8788b69d993716a009579fe11ce861de14956449/src/benchmarks/micro/libraries/System.Runtime/Perf.TestPr.cs

This microbenchmark is likely going to be dominated by allocations from boxing the Guid. It is unlikely to be dominated by the code changed here.

What are the results for a microbenchmark like:

	public class Perf_TestPr
	{
		public static object GuidVal = (object)Guid.NewGuid();
		public static object OtherGuidVal = (object)Guid.NewGuid();
		[Benchmark]
		public int CompareTo_Guid() => GuidVal.CompareTo(OtherGuidVal);
	}

[lilinus]

Ah yes seems to be ~5% regression when I box it outside the benchmark. Surprisingly though, it is much more performant when Guids are different.

| Method                   | Job        | Toolchain                                                                                              | Mean     | Error     | StdDev    | Median   | Min      | Max      | Ratio | Allocated | Alloc Ratio |
|------------------------- |----------- |------------------------------------------------------------------------------------------------------- |---------:|----------:|----------:|---------:|---------:|---------:|------:|----------:|------------:|
| CompareTo_Guid_Same      | Job-IDPFCA | \dnrr\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe | 3.660 ns | 0.0137 ns | 0.0121 ns | 3.660 ns | 3.639 ns | 3.684 ns |  1.00 |         - |          NA |
| CompareTo_Guid_Same      | Job-ZINRGU | \dnrt\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe | 3.827 ns | 0.0141 ns | 0.0125 ns | 3.829 ns | 3.809 ns | 3.850 ns |  1.05 |         - |          NA |
|                          |            |                                                                                                        |          |           |           |          |          |          |       |           |             |
| CompareTo_Guid_Different | Job-IDPFCA | \dnrr\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe | 2.528 ns | 0.0238 ns | 0.0222 ns | 2.525 ns | 2.480 ns | 2.556 ns |  1.00 |         - |          NA |
| CompareTo_Guid_Different | Job-ZINRGU | \dnrt\artifacts\bin\testhost\net9.0-windows-Release-x64\shared\Microsoft.NETCore.App\9.0.0\CoreRun.exe | 1.661 ns | 0.0075 ns | 0.0066 ns | 1.660 ns | 1.648 ns | 1.676 ns |  0.66 |         - |          NA |

Updated source code for benchmarks are https://github.com/lilinus/performance/blob/af809c867ade897a293925d65003ed5c7b80ef0c/src/benchmarks/micro/libraries/System.Runtime/Perf.TestPr.cs

Is 5% is too much regression? If so, I can close the PR.

[jkotas]

Surprisingly though, it is much more performant when Guids are different.

That's not surprising. Comparing different guids is a lot less work - the method typically returns very quickly since the first byte is likely going to be different.

Note that comparing different items is the typical case in sort algorithms that is likely going to be the most common use of this method.

Is 5% is too much regression?

According to your results, it is much more than 5% for different guids that is the typical case.

[lilinus]

Surprisingly though, it is much more performant when Guids are different.

That's not surprising. Comparing different guids is a lot less work - the method typically returns very quickly since the first byte is likely going to be different.
Yes I understand, I mean it seems to be faster after the changes compared to main branch, given different guids.

Note that comparing different items is the typical case in sort algorithms that is likely going to be the most common use of this method.

Is 5% is too much regression?

According to your results, it is much more than 5% for different guids that is the typical case.

Sorry I should clarify how the results, top are for main branch, bottom ones are with changes.

Am I reading it wrong?
Same guids gives ratio 1.05 => ~5% more time (slower)
Different guids gives ratio 0.66 => 34% less time (faster)

[lilinus]

Codegen seems to inline the other call, looks more compact

sharplab

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.