Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Simplify some non-generic CompareTo methods #101545

Merged
merged 6 commits into from
Jun 4, 2024
Merged

Simplify some non-generic CompareTo methods #101545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6647c6f
Simplify some CompareTo methods
lilinus Apr 25, 2024
5dab47d
Simplify some more CompareTo methods
lilinus Apr 25, 2024
73a1418
Revert "Simplify some more CompareTo methods"
lilinus Apr 25, 2024
f9c41b8
Merge branch 'main' into simplify-some-compareto-methods
lilinus Apr 25, 2024
957cb10
Use DateTime.CompareTo in DatetimeOffset
lilinus May 8, 2024
99c5bd1
Merge branch 'main' into simplify-some-compareto-methods
lilinus May 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 4 additions & 14 deletions src/libraries/System.Private.CoreLib/src/System/DateTimeOffset.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,26 +571,16 @@ public static int Compare(DateTimeOffset first, DateTimeOffset second) =>
int IComparable.CompareTo(object? obj)
{
if (obj == null) return 1;
if (!(obj is DateTimeOffset))
if (obj is not DateTimeOffset other)
{
throw new ArgumentException(SR.Arg_MustBeDateTimeOffset);
}

DateTime objUtc = ((DateTimeOffset)obj).UtcDateTime;
DateTime utc = UtcDateTime;
if (utc > objUtc) return 1;
if (utc < objUtc) return -1;
return 0;
return DateTime.Compare(UtcDateTime, other.UtcDateTime);
}

public int CompareTo(DateTimeOffset other)
{
DateTime otherUtc = other.UtcDateTime;
DateTime utc = UtcDateTime;
if (utc > otherUtc) return 1;
if (utc < otherUtc) return -1;
return 0;
}
public int CompareTo(DateTimeOffset other) =>
DateTime.Compare(UtcDateTime, other.UtcDateTime);

// Checks if this DateTimeOffset is equal to a given object. Returns
// true if the given object is a boxed DateTimeOffset and its value
Expand Down
61 changes: 2 additions & 59 deletions src/libraries/System.Private.CoreLib/src/System/Guid.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -958,68 +958,11 @@ public int CompareTo(object? value)
{
return 1;
}
if (!(value is Guid))
if (value is not Guid other)
{
throw new ArgumentException(SR.Arg_MustBeGuid, nameof(value));
}
Guid g = (Guid)value;

if (g._a != _a)
{
return GetResult((uint)_a, (uint)g._a);
}

if (g._b != _b)
{
return GetResult((uint)_b, (uint)g._b);
}

if (g._c != _c)
{
return GetResult((uint)_c, (uint)g._c);
}

if (g._d != _d)
{
return GetResult(_d, g._d);
}

if (g._e != _e)
{
return GetResult(_e, g._e);
}

if (g._f != _f)
{
return GetResult(_f, g._f);
}

if (g._g != _g)
{
return GetResult(_g, g._g);
}

if (g._h != _h)
{
return GetResult(_h, g._h);
}

if (g._i != _i)
{
return GetResult(_i, g._i);
}

if (g._j != _j)
{
return GetResult(_j, g._j);
}

if (g._k != _k)
{
return GetResult(_k, g._k);
}

return 0;
return CompareTo(other);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change going to regress performance of CompareTo(object? 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.

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 |

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please share the source code for the benchmark?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codegen seems to inline the other call, looks more compact

sharplab

}

public int CompareTo(Guid value)
Expand Down
14 changes: 1 addition & 13 deletions src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NFloat.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -780,19 +780,7 @@ public int CompareTo(object? obj)
{
if (obj is NFloat other)
{
if (_value < other._value) return -1;
if (_value > other._value) return 1;
if (_value == other._value) return 0;

// At least one of the values is NaN.
if (NativeType.IsNaN(_value))
{
return NativeType.IsNaN(other._value) ? 0 : -1;
}
else
{
return 1;
}
return CompareTo(other);
}
else if (obj is null)
{
Expand Down
Loading