Unify Default Comparer logic #88006

MichalPetryka · 2023-06-24T19:57:13Z

Unifies (Equality)Comparer.Default logic so that it matches between CoreCLR, NativeAOT, Mono in runtime and intrinsic implementations.

Fixes devirt behaviour around Nullable types.

Also enables devirt for non final types in CoreCLR and NativeAOT.

Required for #87579.
Fixes #87391.

ghost · 2023-06-24T19:57:25Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Unifies (Equality)Comparer.Default logic so that it matches between CoreCLR, NativeAOT, Mono in runtime and intrinsic implementations.

Also enables devirt for non final types in CoreCLR and NativeAOT.

Required for #87579.

Author:	MichalPetryka
Assignees:	-
Labels:	`area-CodeGen-coreclr`, `community-contribution`
Milestone:	-

EgorBo · 2023-06-24T21:05:57Z

It should fix #87391

src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/ComparerHelpers.cs

src/coreclr/tools/Common/TypeSystem/IL/Stubs/ComparerIntrinsics.cs

...lr/nativeaot/System.Private.CoreLib/src/Internal/IntrinsicSupport/EqualityComparerHelpers.cs

src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/ComparerHelpers.cs

MichalPetryka · 2023-06-26T01:22:29Z

@jkotas Roslyn seems to be emitting some invalid IL for the enum tests I've added, going to report it. Should I remove them for now?

MichalPetryka · 2023-07-01T03:02:04Z

What is the type that it does not work for?

Some examples:

runtime/src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/CaptureCollection.cs

Line 118 in e80ef86

if (EqualityComparer<Capture>.Default.Equals(this[i], item))

This one seems okay?

runtime/src/libraries/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/Semantics/Types/TypeTable.cs

Line 25 in e80ef86

EqualityComparer<TKey1>.Default.Equals(_pKey1, other._pKey1)

This one doesn't however, since we don't have a type here since the type is shared:

; Assembly listing for method Microsoft.CSharp.RuntimeBinder.Semantics.TypeTable+KeyPair`2[System.__Canon,System.Nullable`1[int]]:Equals(System.Object):bool:this (FullOpts)

jkotas · 2023-07-01T05:09:37Z

I think you want to reorder the checks in getDefaultEqualityComparerClassHelper like this:

if (Nullable::IsNullableType(elemTypeHnd))
{
   ...CLASS__NULLABLE_EQUALITYCOMPARER...
}

if (elemTypeHnd.IsEnum())
{
   ...CLASS__ENUM_EQUALITYCOMPARER...
}

if (elemTypeHnd.IsCanonicalSubtype())
{
   Bail for shared generic types for now. We can improve this case later.
   return NULL;
}

if (elemTypeHnd.CanCastTo(...))
{
   ...CLASS__GENERIC_EQUALITYCOMPARER...
}

return ...CLASS__OBJECT_EQUALITYCOMPARER...;

jkotas · 2023-07-02T14:51:22Z

Would it make sense to split the JIT change into separate PR? The Mono, VM and BCL changes in this PR are good and they can go in. The JIT change may need some more work, and it may be a good idea to do it separately so that it is easier to look at its impact from jit-diffs.

MichalPetryka · 2023-07-03T22:28:08Z

@MihuBot

jkotas · 2023-07-04T04:28:32Z

src/coreclr/vm/jitinterface.cpp

-            TypeHandle resultTh = ((TypeHandle)targetClass->GetCanonicalMethodTable()).Instantiate(inst);
-            return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
-        }
+    if (elemTypeHnd.IsCanonicalSubtype())


The CanCastTo(IEquatable) case needs to be after this check, like I have suggested in my comment.

Or the IsCanonicalSubtype check needs to be the very first thing in the method - we would give up on more cases that way. (It would mirror what happens today.)

Or the CanCastTo(IEquatable) check needs to be implemented against generic type definition for the IsCanonicalSubtype case. This would be the most precise option, but there would be still some ambiguous cases where we would have to give up.

Here is example of a bug that would be hit by the code as written:

using System; using System.Threading; using System.Collections; using System.Collections.Generic; using System.Runtime.CompilerServices; for (int i = 0; i < 100; i++) { Internal.Console.WriteLine(Test<string, object>().ToString()); Thread.Sleep(1); } [MethodImpl(MethodImplOptions.NoInlining)] static bool Test<T,U>() { return EqualityComparer<G<T,U>>.Default.Equals(default, default); } struct G<T,U> : IEquatable<G<U,T>> { public bool Equals(G<U,T> x) => false; }

It will start printing true correctly, and then flips to printing false as the incorrect Tier1 optimization kicks in.

(It would mirror what happens today.)

It wouldn't, it'd then block Struct<_Canon> too, which you were concerned about before.
The issue right now is that this seems to still be devirtualizing __Canon as ObjectEqualityComparer for some reason (see those diffs). I have no idea what's going on here, could WellKnownArg be returning some weird handle here?

Right, it won't mirror the bugs. Notice that the bug demonstrated by my G<T,U> repro has been shipping for a while (just tried it on .NET 7).

Struct<_Canon> needs the third option (use generic type definition to check for IEquatable implementation). Otherwise, it is better to block it rather than to have functional bugs that manifest as observable behavior differences between Tier0 and Tier1 JIT.

see those diffs

This links to my comment. Did you meant to link to some diffs somewhere?

This links to my comment. Did you meant to link to some diffs somewhere?

Yeah, seems like Copy link on that GitHub message didn't work so I still had a link to your comment in my clipboard instead. Fixed that link.

There diffs look odd. Are you able to reproduce the behavior locally? It can be that the bot picked up stale bits from somewhere.

The new diffs look good to me, but I do not think the difference can be explained by the one commit that you have pushed.

The new diffs look good to me, but I do not think the difference can be explained by the one commit that you have pushed.

The bot seems to be having some issues with picking up VM changes, after a fix CoreLib changes look good but frameworks ones still have the weird diff, not sure if it's still an issue with the bot.

MichalPetryka · 2023-07-04T23:08:59Z

@MihuBot

jkotas · 2023-07-05T06:31:17Z

Could you please add a test case for the repro from Unify Default Comparer logic #88006 (comment)
I have implemented correct handling of shared types in jkotas@527e8c7 . Feel free to cherry-pick the commit into this PR if it looks good to you. It helps with devirtualization, but it does help with inlining. Inlining typically runs into the current shared generic code inlining limitations.

The change looks good to me otherwise. Thank you!

jkotas · 2023-07-05T12:07:08Z

Could you please add a test case for the repro from #88006 (comment)

It should be one more case in src/tests/JIT/opt/Devirtualization/Comparer_get_Default.cs like the cases that are there already. The JIT tests are run both with optimizations on and off, so we would find the bug that way.

MichalPetryka · 2023-07-05T12:14:07Z

I have implemented correct handling of shared types in jkotas@527e8c7 . Feel free to cherry-pick the commit into this PR if it looks good to you. It helps with devirtualization, but it does help with inlining. Inlining typically runs into the current shared generic code inlining limitations.

I'd prefer that to be in a separate PR after this gets merged to keep diffs separate and avoid any potential issues.

jkotas · 2023-07-05T19:12:48Z

I have implemented correct handling of shared types in jkotas@527e8c7 . Feel free to cherry-pick the commit into this PR if it looks good to you. It helps with devirtualization, but it does help with inlining. Inlining typically runs into the current shared generic code inlining limitations.

I'd prefer that to be in a separate PR after this gets merged to keep diffs separate and avoid any potential issues.

I am going to leave this for .NET 9.

stephentoub · 2023-07-06T00:58:21Z

I'm not sure why tests didn't fail here, but this appears to have broken some System.Runtime.Serialization.Formatters tests:
#88453

  Discovering: System.Runtime.Serialization.Formatters.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Runtime.Serialization.Formatters.Tests (found 79 of 80 test cases)
  Starting:    System.Runtime.Serialization.Formatters.Tests (parallel test collections = on, max threads = 2)
    System.Runtime.Serialization.Formatters.Tests.DisableBitTests.DisabledAlwaysInBrowser [SKIP]
      Condition(s) not met: "IsBinaryFormatterSuppressedOnThisPlatform"
    System.Runtime.Serialization.Formatters.Tests.BinaryFormatterTests.ValidateAgainstBlobs(obj: GenericEqualityComparer`1 { }, blobs: [System.Runtime.Serialization.Formatters.Tests.TypeSerializableValue, System.Runtime.Serialization.Formatters.Tests.TypeSerializableValue]) [FAIL]
      The stored blob for type System.Collections.Generic.GenericEqualityComparer`1[[System.Byte, System.Private.CoreLib, Version=8.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]] is outdated and needs to be updated.
      
      -------------------- Stored blob ---------------------
      Encoded: AAEAAAD/////AQAAAAAAAAAEAQAAAC9TeXN0ZW0uQ29sbGVjdGlvbnMuR2VuZXJpYy5CeXRlRXF1YWxpdHlDb21wYXJlcgAAAAAL
      Decoded: �����????�������������/System.Collections.Generic.ByteEqualityComparer�����
      
      --------------- Runtime generated blob ---------------
      Encoded: AAEAAAD/////AQAAAAAAAAAEAQAAAJABU3lzdGVtLkNvbGxlY3Rpb25zLkdlbmVyaWMuR2VuZXJpY0VxdWFsaXR5Q29tcGFyZXJgMVtbU3lzdGVtLkJ5dGUsIG1zY29ybGliLCBWZXJzaW9uPTQuMC4wLjAsIEN1bHR1cmU9bmV1dHJhbCwgUHVibGljS2V5VG9rZW49Yjc3YTVjNTYxOTM0ZTA4OV1dAAAAAAs=
      Decoded: �����????�������������?�System.Collections.Generic.GenericEqualityComparer`1[[System.Byte, mscorlib, Version=0.0.0.0 Culture=neutral, PublicKeyToken=b77a5c561934e089]]�����
      Expected: True
      Actual:   False
      Stack Trace:
        /_/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs(615,0): at System.Runtime.Serialization.Formatters.Tests.BinaryFormatterTests.SanityCheckBlob(Object obj, TypeSerializableValue[] blobs)
        /_/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs(111,0): at System.Runtime.Serialization.Formatters.Tests.BinaryFormatterTests.ValidateAndRoundtrip(Object obj, TypeSerializableValue[] blobs, Boolean isEqualityComparer)
        /_/src/libraries/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs(73,0): at System.Runtime.Serialization.Formatters.Tests.BinaryFormatterTests.ValidateAgainstBlobs(Object obj, TypeSerializableValue[] blobs)
           at InvokeStub_BinaryFormatterTests.ValidateAgainstBlobs(Object, Object, IntPtr*)
           at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

EgorBo · 2023-07-06T01:25:25Z

Presumably GenericEqualityComparer should have a backward compatibility patch for binary formatter, something like this:

runtime/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/EqualityComparer.cs

Lines 243 to 250 in 68b410f

    
           public void GetObjectData(SerializationInfo info, StreamingContext context) 
        
           { 
        
               // For back-compat we need to serialize the comparers for enums with underlying types other than int as ObjectEqualityComparer 
        
               if (Type.GetTypeCode(typeof(T)) != TypeCode.Int32) 
        
               { 
        
                   info.SetType(typeof(ObjectEqualityComparer<T>)); 
        
               } 
        
           }

for T being byte it should report ByteEqualityComparer

@MichalPetryka

MichalPetryka · 2023-07-06T01:36:36Z

for T being byte it should report ByteEqualityComparer

@jkotas do we want that or maybe changing the tests to expect GenericEqualityComparer would be preferred here?

stephentoub · 2023-07-06T01:41:34Z

I'm not particularly concerned about someone serializing the comparer on .NET 8 and when deserialized on .NET Framework having it be a different kind of comparer than it would otherwise have been, especially as BinaryFormatter is on its way out. @GrabYourPitchforks, do we need to fix anything here?

jkotas · 2023-07-06T01:57:42Z

Binary serialization of compares does not guarantee that you get the fastest possible comparer after deserialization. It only guarantees that you get a functionally equivalent comparer.

GenericEqualityComparer is going to work fine for bytes on .NET Framework. We do not need to do anything special for it.

It is not the case for EnumEqualityComparer. EnumEqualityComparer only works for int32 underlying type on .NET Framework, it does not work for other underlying types. It is why the code is going what it is doing.

MichalPetryka · 2023-07-06T02:22:22Z

I'm not sure why tests didn't fail here

Did anybody figure this out btw?

stephentoub · 2023-07-06T02:24:39Z

I'm not sure why tests didn't fail here

Did anybody figure this out btw?

The relevant test is part of the libraries tests, and I believe this PR ended up triggering legs for the libraries tests that ran either on mono or on a checked coreclr, and the particular test suite is disabled for checked coreclr because it's too slow.

Unify Default Comparer logic

7f617e2

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 24, 2023

ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 24, 2023

MichalPetryka added 2 commits June 24, 2023 22:25

Fix build

2536992

Fix build

5605e0e

EgorBo reviewed Jun 24, 2023

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/ComparerHelpers.cs Show resolved Hide resolved

MichalPetryka added 3 commits June 25, 2023 03:43

Update importercalls.cpp

72e548a

Update importercalls.cpp

9d9fc84

Update Comparer_get_Default.cs

f229039

MichalPetryka marked this pull request as ready for review June 25, 2023 12:11

MichalPetryka requested a review from MichalStrehovsky as a code owner June 25, 2023 12:11

MichalPetryka commented Jun 25, 2023

View reviewed changes

src/coreclr/tools/Common/TypeSystem/IL/Stubs/ComparerIntrinsics.cs Outdated Show resolved Hide resolved

MichalPetryka added 2 commits June 25, 2023 15:12

Update Comparer_get_Default.cs

70cb1c0

Update Comparer_get_Default.cs

e095936

jkotas reviewed Jun 25, 2023

View reviewed changes

...lr/nativeaot/System.Private.CoreLib/src/Internal/IntrinsicSupport/EqualityComparerHelpers.cs Outdated Show resolved Hide resolved

jkotas reviewed Jun 25, 2023

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/ComparerHelpers.cs Show resolved Hide resolved

jkotas reviewed Jun 25, 2023

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/ComparerHelpers.cs Outdated Show resolved Hide resolved

jkotas reviewed Jun 25, 2023

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/ComparerHelpers.cs Show resolved Hide resolved

build-analysis bot mentioned this pull request Jun 25, 2023

DataContractSerializerTests.DCS_MyPersonSurrogate_Stress failing in CI #35066

Open

Apply sugestions

d591efe

MichalPetryka requested a review from marek-safar as a code owner June 25, 2023 21:25

Add missed changes

7088336

MichalPetryka requested review from vargaz, lambdageek and SamMonoRT as code owners June 25, 2023 21:55

MichalPetryka added 2 commits June 26, 2023 00:06

Update ComparerIntrinsics.cs

0c0c84d

Update Comparer_get_Default.csproj

c516e7f

Update jitinterface.cpp

fc21b4f

MihuBot mentioned this pull request Jul 3, 2023

[MichalPetryka] Unify Default Comparer logic MihuBot/runtime-utils#73

Open

jkotas reviewed Jul 4, 2023

View reviewed changes

Update jitinterface.cpp

fff6180

MihuBot mentioned this pull request Jul 4, 2023

[MichalPetryka] Unify Default Comparer logic MihuBot/runtime-utils#75

Open

Update Comparer_get_Default.cs

1b1c1e7

MichalPetryka mentioned this pull request Jul 5, 2023

Print variable types when dumping the output #88420

Merged

jkotas approved these changes Jul 5, 2023

View reviewed changes

jkotas merged commit 18321c6 into dotnet:main Jul 5, 2023

MichalPetryka mentioned this pull request Jul 8, 2023

Convert RuntimeHelpers intrinsics to JIT ones #88543

Closed

cincuranet mentioned this pull request Jul 11, 2023

Regressions in Devirtualization.EqualityComparer #88667

Closed

EgorBo mentioned this pull request Jul 13, 2023

[Perf] Windows/arm64: 3 Improvements on 7/6/2023 2:30:01 AM dotnet/perf-autofiling-issues#19795

Closed

jakobbotsch mentioned this pull request Aug 1, 2023

Comparers don't work for messy generic corner cases #89767

Closed

ghost locked as resolved and limited conversation to collaborators Aug 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify Default Comparer logic #88006

Unify Default Comparer logic #88006

MichalPetryka commented Jun 24, 2023 •

edited

Loading

ghost commented Jun 24, 2023

EgorBo commented Jun 24, 2023

MichalPetryka commented Jun 26, 2023

MichalPetryka commented Jul 1, 2023

jkotas commented Jul 1, 2023

jkotas commented Jul 2, 2023

MichalPetryka commented Jul 3, 2023

jkotas Jul 4, 2023 •

edited

Loading

MichalPetryka Jul 4, 2023 •

edited

Loading

jkotas Jul 4, 2023

jkotas Jul 4, 2023

MichalPetryka Jul 4, 2023 •

edited

Loading

jkotas Jul 4, 2023

jkotas Jul 5, 2023

MichalPetryka Jul 5, 2023

MichalPetryka commented Jul 4, 2023

jkotas commented Jul 5, 2023

jkotas commented Jul 5, 2023

MichalPetryka commented Jul 5, 2023

jkotas commented Jul 5, 2023

stephentoub commented Jul 6, 2023 •

edited

Loading

EgorBo commented Jul 6, 2023 •

edited

Loading

MichalPetryka commented Jul 6, 2023

stephentoub commented Jul 6, 2023

jkotas commented Jul 6, 2023 •

edited

Loading

MichalPetryka commented Jul 6, 2023

stephentoub commented Jul 6, 2023

Unify Default Comparer logic #88006

Unify Default Comparer logic #88006

Conversation

MichalPetryka commented Jun 24, 2023 • edited Loading

ghost commented Jun 24, 2023

EgorBo commented Jun 24, 2023

MichalPetryka commented Jun 26, 2023

MichalPetryka commented Jul 1, 2023

jkotas commented Jul 1, 2023

jkotas commented Jul 2, 2023

MichalPetryka commented Jul 3, 2023

jkotas Jul 4, 2023 • edited Loading

Choose a reason for hiding this comment

MichalPetryka Jul 4, 2023 • edited Loading

Choose a reason for hiding this comment

jkotas Jul 4, 2023

Choose a reason for hiding this comment

jkotas Jul 4, 2023

Choose a reason for hiding this comment

MichalPetryka Jul 4, 2023 • edited Loading

Choose a reason for hiding this comment

jkotas Jul 4, 2023

Choose a reason for hiding this comment

jkotas Jul 5, 2023

Choose a reason for hiding this comment

MichalPetryka Jul 5, 2023

Choose a reason for hiding this comment

MichalPetryka commented Jul 4, 2023

jkotas commented Jul 5, 2023

jkotas commented Jul 5, 2023

MichalPetryka commented Jul 5, 2023

jkotas commented Jul 5, 2023

stephentoub commented Jul 6, 2023 • edited Loading

EgorBo commented Jul 6, 2023 • edited Loading

MichalPetryka commented Jul 6, 2023

stephentoub commented Jul 6, 2023

jkotas commented Jul 6, 2023 • edited Loading

MichalPetryka commented Jul 6, 2023

stephentoub commented Jul 6, 2023

MichalPetryka commented Jun 24, 2023 •

edited

Loading

jkotas Jul 4, 2023 •

edited

Loading

MichalPetryka Jul 4, 2023 •

edited

Loading

MichalPetryka Jul 4, 2023 •

edited

Loading

stephentoub commented Jul 6, 2023 •

edited

Loading

EgorBo commented Jul 6, 2023 •

edited

Loading

jkotas commented Jul 6, 2023 •

edited

Loading