Add special Comparer/EqualityComparer for Nullable<Enum>

[EgorBo]

Fixes #67842

Example:

bool Test(DayOfWeek? d1, DayOfWeek? d2) => EqualityComparer<DayOfWeek?>.Default.Equals(d1, d2);

Codegen diff: https://www.diffchecker.com/7qPt1KtF
It doesn't box (allocate) any more. The final codegen is still a bit suboptimal but it's a different problem (Nullable<> is not promoted)

Same codegen for e.g.:

record struct Test(DayOfWeek? Day);

bool Test(Test t1, Test t2) => t1.Equals(t2);

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[jkotas]

Would it be better to instead figure out how to optimize the boxing from the constrained calls to Enum.Equals?

[jkotas]

This needs tests and extra code to ensure that the binary serialization continues to work.

[jkotas]

This also needs to be handled in src\coreclr\tools\Common\TypeSystem\IL\Stubs\ComparerIntrinsics.cs

[jkotas]

Do we want to add it for Mono too? We generally try to add optimizations like these for all runtimes.

[jkotas]

Let's intrinsify Nullable<>.Equals instead

How is this fixing the problem with boxing in EqualityComparer from the linker issue?

[EgorBo]

Let's intrinsify Nullable<>.Equals instead

How is this fixing the problem with boxing in EqualityComparer from the linker issue?

@jkotas it's called by the ObjectEqualityComparer.

The current problem that

bool Test(DayOfWeek? d1, DayOfWeek? d2) => EqualityComparer<DayOfWeek?>.Default.Equals(d1, d2);

ends up as Nullable<>.Equals(object) call which is then inlined into Enum.Equals(object) where input enum becomes a multi-use boxing we're not able to handle currently #9118

so while this PR indeed works it's quite verbose and doesn't handle CompareTo so I think it's better to rely on a custom equalitycomparer for now

[jkotas]

EqualityComparer<DayOfWeek?>.Default.Equals(d1, d2);
ends up as Nullable<>.Equals(object) call which is then inlined into Enum.Equals(object)

It should end up as NullableEqualityComparer.Equals

NullableEqualityComparer.Equals calls Enum.Equals here:

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

Line 107 in 57bfe47

if (y.HasValue) return x.value.Equals(y.value);

.

I do not see anything calling Nullable.Equals on this path.

[EgorBo]

It should end up as NullableEqualityComparer.Equals

it does not currently because it doesn't pass this check

runtime/src/coreclr/vm/jitinterface.cpp

Lines 8822 to 8827 in 7bfc61b

	TypeHandle iequatable = TypeHandle(CoreLibBinder::GetClass(CLASS__IEQUATABLEGENERIC)).Instantiate(nullableInst);
	if (nullableInst[0].CanCastTo(iequatable))
	{
	TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__NULLABLE_COMPARER)).Instantiate(nullableInst);
	return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
	}

🤔 (what if I enable it for enums...)

[jkotas]

Ok, make sense.

[jkotas]

I am wondering whether we can just fix the existing NullableEqualityComparer instead of introducing a new one. Drop IEquatable<T> constraint from NullableEqualityComparer<T> and change x.value.Equals(y.value); in NullableEqualityComparer implementation to EqualityComparer<T>.Default.Equals(x, y)?

[EgorBo]

I am wondering whether we can just fix the existing NullableEqualityComparer instead of introducing a new one. Drop IEquatable<T> constraint from NullableEqualityComparer<T> and change x.value.Equals(y.value); in NullableEqualityComparer implementation to EqualityComparer<T>.Default.Equals(x, y)?

Great idea! Let's see how it goes...

[ghost]

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

Issue Details

---

Fixes #67842

Example:

bool Test(DayOfWeek? d1, DayOfWeek? d2) => EqualityComparer<DayOfWeek?>.Default.Equals(d1, d2);

Codegen diff: https://www.diffchecker.com/7qPt1KtF
It doesn't box (allocate) any more. The final codegen is still a bit suboptimal but it's a different problem (Nullable<> is not promoted)

Same codegen for e.g.:

record struct Test(DayOfWeek? Day);

bool Test(Test t1, Test t2) => t1.Equals(t2);

Author:	EgorBo
Assignees:	EgorBo
Labels:	area-System.Runtime
Milestone:	-

[jkotas]

Could you please check asm diffs for the existing NullableEqualityComparer uses to make sure there is no significant code quality regression?

Otherwise looks good.

[EgorBo]

@jkotas Thanks! Sorry for the delay between changes - I was on a month-long "vacation".

The codegen diff is more or less the same:

1. Nullable Primitives - no diffs
2. Nullable Struct implementing IEquatable/IComparable - a few redundant movs
3. Nullable structs and enums - much better codegen and no allocations (boxing)

Had to remove a test in Libraries

not relevant anymore

- it used to rely on fact that a round-trip binary serialization should produce the same or assignable object but it doesn't work now.

e.g.

EqualityComparer<T> orig = EqualityComparer<T>.Default;
bf.Serialize(s, orig);
s.Position = 0;
object result = bf.Deserialize(s);
Assert.IsAssignableFrom<EqualityComparer<T>>(result);

For T being Nullable<MyInt16Enum> we used to get ObjectEqualityComparer<Nullable<MyInt16Enum>> object from .Default but now we get NullableEqualityCompdarer<MyInt16Enum>
which after a round-trip serialization becomes ObjectEqualityComparer<MyInt16Enum> due to backward compatibility hack

[EgorBo]

oh, wait a minute that might be a bug actually - let me check

[EgorBo]

Right, it was my bug, fixed with 481e782

[EgorBo]

Failure is unrelated (#68511)