Support faster null checks

[benaadams]

To improve the common use of right-sided null x == null upstream.

If we trim most of the class operator== methods to only null check (some also ReferenceEquals and check either arg for null) then switch the common order of the params used for .Equals (which is normally left.Equals(right); so:

public static bool operator== (MyClass left, MyClass right)
    => (right is null) ? (left is null) : right.Equals(left);

Then it drops under the [below ALWAYS_INLINE size] inlining threshold; after inlining the Jit can remove the branch (as right is constant null) so it becomes just left is null

A fast-path .Equals that does ReferenceEquals is then added so it doesn't inline for the is null check; but does inline for a vs variable check (as base.Equals(object) method is virtual and can't inline).

Current

Previous PR #21736 (faster null checks)

This PR with both commits (operator== change + ReferenceEquals fast path in strongly typed Equals)

/cc @jkotas @stephentoub better?

Contributes to https://github.com/dotnet/corefx/issues/34283#issuecomment-451015598

[benaadams]

@dotnet-bot test Ubuntu16.04 arm64 Cross Checked Innerloop Build and Test
@dotnet-bot test Windows_NT arm Cross Debug Innerloop Build

[benaadams]

Also added a ReferenceEquals inlinable fast-path for operator==(string, string) which helps interned string compares (as string.Equals is too big to inline)

[jkotas]

I do not think it is worth it to inline the reference check for strings. It is pretty rare to compare interned strings in the real world code. String comparisons are very common. Adding code bloat to every place that compares strings did not seem to be a good idea when I have looked into it some time ago.

[benaadams]

Dropped the string change

[benaadams]

Infra error

error MSB3027: Could not copy 
"D:\j\w\perf_perflab_---89ec9f56\bin\int\Managed\JIT\jit64\gc\regress\vswhidbey\143837\143837.exe" to 
"D:\j\w\perf_perflab_---89ec9f56\bin\tests\Windows_NT.x64.Release\JIT\jit64\gc\regress\vswhidbey\143837\143837.exe". 
Exceeded retry count of 10. Failed.  
[D:\j\w\perf_perflab_---89ec9f56\tests\src\JIT\jit64\gc\regress\vswhidbey\143837.csproj

@dotnet-bot test Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness
@dotnet-bot test Windows_NT x86 min_opt ryujit CoreCLR Perf Tests Correctness

[benaadams]

Null check goes from

G_M6073_IG02:
       488BCF               mov      rcx, rdi
       33D2                 xor      rdx, rdx
       E800000000           call     MethodInfo:op_Equality(ref,ref):bool
       85C0                 test     eax, eax
       0F8500000000         jne      G_M6073_IG16

To

G_M6076_IG02:
       4885FF               test     rdi, rdi
       0F94C1               sete     cl
       0FB6C9               movzx    rcx, cl
       85C9                 test     ecx, ecx
       0F8500000000         jne      G_M6076_IG16

Looking to see if can skip the extra sete,movzx,test; looks like https://github.com/dotnet/coreclr/issues/914? /cc @mikedn

[redknightlois]

Shouldn't this have to be tackled entirely at JIT level? Sounds to me that these mutations could be done for all code as the pattern is pretty straightforward. Cc @AndyAyersMS

[benaadams]

Shouldn't this have to be tackled entirely at JIT level?

There's a bug for it #914

With the addition of a16d5dd an example asm change is:

G_M6073_IG02:
       488BCF               mov      rcx, rdi
       33D2                 xor      rdx, rdx
       E800000000           call     MethodInfo:op_Equality(ref,ref):bool
       85C0                 test     eax, eax
       0F8500000000         jne      G_M6073_IG16

G_M6073_IG03:
       33DB                 xor      ebx, ebx
       488BEF               mov      rbp, rdi
       4885ED               test     rbp, rbp
       740F                 je       SHORT G_M6073_IG04
       488D0D00000000       lea      rcx, [(reloc)]
       48394D00             cmp      qword ptr [rbp], rcx
       7402                 je       SHORT G_M6073_IG04
       33ED                 xor      rbp, rbp

G_M6073_IG04:
       488BCD               mov      rcx, rbp
       33D2                 xor      rdx, rdx
       E800000000           call     MethodInfo:op_Equality(ref,ref):bool
       85C0                 test     eax, eax
       0F8487000000         je       G_M6073_IG09
       488BEF               mov      rbp, rdi
       4885ED               test     rbp, rbp
       740F                 je       SHORT G_M6073_IG05
       488D0D00000000       lea      rcx, [(reloc)]
       48394D00             cmp      qword ptr [rbp], rcx
       7402                 je       SHORT G_M6073_IG05
       33ED                 xor      rbp, rbp

G_M6073_IG05:
       488BCD               mov      rcx, rbp
       33D2                 xor      rdx, rdx
       E800000000           call     MethodInfo:op_Equality(ref,ref):bool
       85C0                 test     eax, eax
       0F852C000000         jne      G_M6073_IG17

G_M6073_IG06:
       4C8B7538             mov      r14, gword ptr [rbp+56]
       4D85F6               test     r14, r14
       743C                 je       SHORT G_M6073_IG08
       498BCE               mov      rcx, r14
       E800000000           call     RuntimeTypeHandle:HasInstantiation(ref)

To

G_M6076_IG02:
       4885FF               test     rdi, rdi
       0F8400000000         je       G_M6076_IG15
       33DB                 xor      ebx, ebx
       488BEF               mov      rbp, rdi
       488D0D00000000       lea      rcx, [(reloc)]
       48394D00             cmp      qword ptr [rbp], rcx
       7402                 je       SHORT G_M6076_IG03
       33ED                 xor      rbp, rbp

G_M6076_IG03:
       4885ED               test     rbp, rbp
       0F8589010000         jne      G_M6076_IG13

G_M6076_IG04:
       488BEF               mov      rbp, rdi
       488D0D00000000       lea      rcx, [(reloc)]
       48394D00             cmp      qword ptr [rbp], rcx
       7402                 je       SHORT G_M6076_IG05
       33ED                 xor      rbp, rbp

G_M6076_IG05:
       4885ED               test     rbp, rbp
       0F842C000000         je       G_M6076_IG16
       4C8B7538             mov      r14, gword ptr [rbp+56]
       4D85F6               test     r14, r14
       0F8448010000         je       G_M6076_IG12
       498BCE               mov      rcx, r14
       E800000000           call     RuntimeTypeHandle:HasInstantiation(ref):bool

[mikedn]

Shouldn't this have to be tackled entirely at JIT level? Sounds to me that these mutations could be done for all code as the pattern is pretty straightforward.

I don't see how the JIT could now that left.Equals(right) and right.Equals(left) are the same thing. Come to think of it, we don't know that either. We just assume that Equals doesn't do any crazy things and thus the 2 variants are equivalent.

[mikedn]

There's a bug for it #914

Ah, that thing. Maybe I should take a look at that again, perhaps there's some easy way to handle at least some of these cases. Though I'd guess that there is not...

[gfoidl]

I don't see how the JIT could now that left.Equals(right) and right.Equals(left) are the same thing.

The special case where right = null could be handled by the JIT?

[benaadams]

The special case where right = null could be handled by the JIT?

Not if you've overriden operator==; depends what the code decides to do with a null value in that method

[mikedn]

The special case where right = null could be handled by the JIT?

How? If you have something like

if (left is null)
    return right is null; 
else
    return left.Equals(right);

Pretty much the only thing that the JIT can reasonably do is

if (left is null)
    return true;
else
    return left.Equals(right);

It obviously cannot do

if (true)
    return left is null;
else
    return left.Equals(right);

That would result in left.Equals(null) no longer being called when the intention of the original code was to call that.
Nor it can do

if (true)
    return left is null;
else
    return right.Equals(left);

It still won't call Equals and even if it calls it, it does so with swapped arguments.

The only way the JIT could do this is if it does interprocedural analysis to prove that Equals behaves in a certain way. And the JIT does not do such analysis and it likely won't do that anytime soon, if ever.

It may also do this if Equals gets inlined. But that's not very interesting for a lot of these cases.

[mikedn]

The more I look at this, the more weird it seems. Because the natural implementation of == is

if (left is null)
    return right is null; 
else
    return left.Equals(right);

and what it being done here is some rather special casing. And only because people can't just use x is null and call it a day, either because it's new and they're not used to it or because its negation sucks in C#.

And contorting the code (especially when done on such a large scale) or asking the JIT to do the impossible aren't exactly solutions.

[benaadams]

because people can't just use x is null and call it a day, either because it's new and they're not used to it or because its negation sucks in C#.

Changing to is null at the callsites was a bit bigger #21736

Though is null's negation does suck #21736 (comment)

[mikedn]

Changing to is null at the callsites was a bit bigger

I know. But that's for ALL callsites. Are all relevant to performance? I'd bet no.

[benaadams]

rebased

[benaadams]

Total bytes of diff: -1547 (-0.04% of base)
    diff is an improvement.

Total byte diff includes 0 bytes from reconciling methods
        Base had    2 unique methods,       54 unique bytes
        Diff had    2 unique methods,       54 unique bytes

Top file improvements by size (bytes):
       -1547 : System.Private.CoreLib.dasm (-0.04% of base)

1 total files with size differences (1 improved, 0 regressed), 0 unchanged.

Top method regessions by size (bytes):
          68 : System.Private.CoreLib.dasm - ManifestBasedResourceGroveler:GetManifestResourceStream(ref,ref,byref):ref:this
          56 : System.Private.CoreLib.dasm - ResourceManager:.ctor(ref,ref,ref):this (2 methods)
          56 : System.Private.CoreLib.dasm - ResourceManager:.ctor(ref,ref):this
          49 : System.Private.CoreLib.dasm - ResourceManager:GetResourceSet(ref,bool,bool):ref:this
          49 : System.Private.CoreLib.dasm - Assembly:op_Inequality(ref,ref):bool
          49 : System.Private.CoreLib.dasm - ConstructorInfo:op_Inequality(ref,ref):bool
          49 : System.Private.CoreLib.dasm - FieldInfo:op_Inequality(ref,ref):bool
          49 : System.Private.CoreLib.dasm - MemberInfo:op_Inequality(ref,ref):bool
          49 : System.Private.CoreLib.dasm - MethodBase:op_Inequality(ref,ref):bool
          49 : System.Private.CoreLib.dasm - EventInfo:op_Inequality(ref,ref):bool
          49 : System.Private.CoreLib.dasm - MethodInfo:op_Inequality(ref,ref):bool
          49 : System.Private.CoreLib.dasm - Module:op_Inequality(ref,ref):bool
          49 : System.Private.CoreLib.dasm - PropertyInfo:op_Inequality(ref,ref):bool
          49 : System.Private.CoreLib.dasm - ModuleBuilder:GetMethodTokenNoLock(ref,bool):struct:this
          48 : System.Private.CoreLib.dasm - ResourceManager:.ctor(ref):this
          47 : System.Private.CoreLib.dasm - ManifestBasedResourceGroveler:CaseInsensitiveManifestResourceStreamLookup(ref,ref):ref:this
          43 : System.Private.CoreLib.dasm - ResourceManager:SetAppXConfiguration():this
          39 : System.Private.CoreLib.dasm - AssemblyBuilder:GetModuleBuilder(ref):ref:this
          39 : System.Private.CoreLib.dasm - SortVersion:op_Inequality(ref,ref):bool
          38 : System.Private.CoreLib.dasm - ModuleHandle:Equals(ref):bool:this
          38 : System.Private.CoreLib.dasm - ManifestBasedResourceGroveler:GetNeutralResourcesLanguage(ref,byref):ref
          38 : System.Private.CoreLib.dasm - MdFieldInfo:CacheEquals(ref):bool:this
          37 : System.Private.CoreLib.dasm - RuntimeType:InvokeMember(ref,int,ref,ref,ref,ref,ref,ref):ref:this
          33 : System.Private.CoreLib.dasm - ResourceManager:ShouldUseSatelliteAssemblyResourceLookupUnderAppX(ref):bool:this
          30 : System.Private.CoreLib.dasm - Version:op_Inequality(ref,ref):bool

Top method improvements by size (bytes):
        -427 : System.Private.CoreLib.dasm - MemberInfo:op_Equality(ref,ref):bool
        -210 : System.Private.CoreLib.dasm - SortVersion:Equals(ref):bool:this (2 methods)
        -192 : System.Private.CoreLib.dasm - MethodBase:op_Equality(ref,ref):bool
        -154 : System.Private.CoreLib.dasm - CustomAttributeBuilder:InitCustomAttributeBuilder(ref,ref,ref,ref,ref,ref):this
         -86 : System.Private.CoreLib.dasm - CompareInfo:get_Version():ref:this
         -75 : System.Private.CoreLib.dasm - DynamicILGenerator:Emit(struct,ref):this (6 methods)
         -66 : System.Private.CoreLib.dasm - RuntimeParameterInfo:GetRuntimeModule():ref:this
         -56 : System.Private.CoreLib.dasm - ModuleBuilder:InternalGetConstructorToken(ref,bool):struct:this
         -55 : System.Private.CoreLib.dasm - Type:FindMembers(int,int,ref,ref):ref:this
         -53 : System.Private.CoreLib.dasm - DynamicMethod:.ctor(ref,int,int,ref,ref,ref,bool):this (2 methods)
         -52 : System.Private.CoreLib.dasm - Attribute:GetParentDefinition(ref):ref (2 methods)
         -51 : System.Private.CoreLib.dasm - DefaultBinder:BindToMethod(int,ref,byref,ref,ref,ref,byref):ref:this
         -45 : System.Private.CoreLib.dasm - CustomAttributeData:GetCustomAttributes(ref):ref (4 methods)
         -44 : System.Private.CoreLib.dasm - Delegate:RemoveAll(ref,ref):ref
         -42 : System.Private.CoreLib.dasm - Attribute:InternalGetCustomAttributes(ref,ref,bool):ref (2 methods)
         -41 : System.Private.CoreLib.dasm - ModuleBuilder:GetFieldTokenNoLock(ref):struct:this
         -40 : System.Private.CoreLib.dasm - Attribute:GetParentDefinition(ref,ref):ref
         -38 : System.Private.CoreLib.dasm - Attribute:InternalIsDefined(ref,ref,bool):bool (2 methods)
         -38 : System.Private.CoreLib.dasm - Version:CompareTo(ref):int:this (2 methods)
         -38 : System.Private.CoreLib.dasm - DynamicMethod:.ctor(ref,ref,ref,ref):this (2 methods)
         -36 : System.Private.CoreLib.dasm - Attribute:GetCustomAttributes(ref,ref,bool):ref (4 methods)
         -36 : System.Private.CoreLib.dasm - Attribute:GetCustomAttributes(ref,bool):ref (4 methods)
         -36 : System.Private.CoreLib.dasm - DynamicMethod:.ctor(ref,ref,ref,ref,bool):this (2 methods)
         -34 : System.Private.CoreLib.dasm - MemberInfoCache`1:PopulateProperties(struct,ref,ref,ref,byref):this
         -34 : System.Private.CoreLib.dasm - CustomAttributeNamedArgument:.ctor(ref,ref):this

215 total methods with size differences (137 improved, 78 regressed), 17339 unchanged.

[benaadams]

37 bytes on RuntimeType:InvokeMember seemed like it might be more of a common call; but as a percentage of the method doesn't look important

-; Total bytes of code 5026, prolog size 64 for method RuntimeType:InvokeMember(ref,int,ref,ref,ref,ref,ref,ref):ref:this
+; Total bytes of code 5063, prolog size 64 for method RuntimeType:InvokeMember(ref,int,ref,ref,ref,ref,ref,ref):ref:this

Changes are similar to

G_M605_IG88:
       mov      rcx, gword ptr [rbp-80H]
       xor      rdx, rdx
       call     MethodBase:op_Equality(ref,ref):bool
       test     eax, eax
       jne      G_M605_IG117

To

G_M612_IG99:
       cmp      gword ptr [rbp-80H], 0
       je       SHORT G_M612_IG100
       xor      ecx, ecx
       jmp      SHORT G_M612_IG101
G_M612_IG100:
       mov      ecx, 1
G_M612_IG101:
       test     ecx, ecx
       jne      G_M612_IG130
G_M612_IG102:
       mov      rcx, gword ptr [rbp-80H]

The je, mov, test, jne sequence seems a bit funny...

Should be something like?

 G_M612_IG99:
        cmp      gword ptr [rbp-80H], 0
+       je       G_M612_IG130
-       je       SHORT G_M612_IG100
-       xor      ecx, ecx
-       jmp      SHORT G_M612_IG101
-G_M612_IG100:
-       mov      ecx, 1
-G_M612_IG101:
-       test     ecx, ecx
-       jne      G_M612_IG130
G_M612_IG102:
        mov      rcx, gword ptr [rbp-80H]

[mikedn]

The je, mov, test, jne sequence seems a bit funny...

I suppose you've reached the limits of what you can do with the b ? true : false hack.

[benaadams]

Tried a few permutations, looks like best its going to go.

Its mostly produces clean null checks; other than a couple odd places where it adds unnecessary extra compares and jmps #21765 (comment) which feels similar to https://github.com/dotnet/coreclr/issues/914

Further improvements could be:

Avoid need for ternary true/false hack https://github.com/dotnet/coreclr/issues/914 (and weirdness around tests on inlined booleans above)

Change call sites to use is null though that was a large change #21736 and C# needs a better inverse for it rather than !(x is null) or x is object as those make the code more unreadable.

[mikedn]

Avoid need for ternary true/false hack #914 (and weirdness around tests on inlined booleans above)

I'm looking into #914. Hopefully we can avoid this circus with b ? true : false. It has lasted long enough.

[benaadams]

@dotnet-bot test Windows_NT arm64 Cross Debug Innerloop Build

[jkotas]

@benaadams Thanks