Skip to content

Optimize equality operators #98196

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

Closed
wants to merge 2 commits into from
Closed

Optimize equality operators #98196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
177ad74
Optimize equality operators
MichalPetryka Feb 8, 2024
7098969
Swap elements back
MichalPetryka Feb 10, 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
14 changes: 6 additions & 8 deletions src/libraries/System.Private.CoreLib/src/System/Delegate.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,27 +164,25 @@ public bool MoveNext()
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool operator ==(Delegate? d1, Delegate? d2)
{
// Test d2 first to allow branch elimination when inlined for null checks (== null)
// so it can become a simple test
if (d2 is null)
if (ReferenceEquals(d1, d2))
{
return d1 is null;
return true;
}

return ReferenceEquals(d2, d1) ? true : d2.Equals((object?)d1);
return d2 is not null && d2.Equals(d1);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool operator !=(Delegate? d1, Delegate? d2)
{
// Test d2 first to allow branch elimination when inlined for not null checks (!= null)
// so it can become a simple test
Comment on lines 178 to 179
Copy link
Member

@tannergooding tannergooding Feb 10, 2024

Choose a reason for hiding this comment

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

These comments aren't really "correct" anymore

We should likely ensure that the same general codegen and dead code elimination still happens when inlined (for RyuJIT and Mono).

Copy link
Member

@jkotas jkotas Feb 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We should likely ensure that the same general codegen and dead code elimination still happens

+1. The MihuBot report has number of regressions. The regressions seem to be caused by dead code elimination no longer happening - a lot of new System.Object:Equals calls that did not exist before.

Copy link
Member

@jkotas jkotas Feb 11, 2024

Choose a reason for hiding this comment

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

The list of regressions seems to be a good data point about methods that use == null / != null, but that should better be using is null / is not null instead.

Copy link
Member

@jkotas jkotas Feb 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The list of regressions seems to be a good data point about methods that use == null / != null, but that should better be using is null / is not null instead.

Are you interested in a submitting PR that fixes these? (Just in the shipping libraries, tests can and should stay as is.)

Copy link
Contributor Author

@MichalPetryka MichalPetryka Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

use == null / != null, but that should better be using is null / is not null instead.

Are custom .Equals implementations that might return true with a single null (like for example with Unity) not a concern here?

Copy link
Member

@jkotas jkotas Feb 13, 2024

Choose a reason for hiding this comment

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

We can look at it case-by-case. Most of the affected operator implementations check for the null upfront and won't call Equals with null.

Do you have an example of where it may be a problem?

if (d2 is null)
if (ReferenceEquals(d1, d2))
{
return d1 is not null;
return false;
}

return ReferenceEquals(d2, d1) ? false : !d2.Equals(d1);
return d2 is not null && !d2.Equals(d1);
}
}
}
6 changes: 3 additions & 3 deletions src/libraries/System.Private.CoreLib/src/System/Globalization/SortVersion.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,12 @@ public override int GetHashCode()
{
// Test "right" first to allow branch elimination when inlined for null checks (== null)
// so it can become a simple test
if (right is null)
if (ReferenceEquals(left, right))
{
return left is null;
return true;
}

return right.Equals(left);
return right is not null && right.Equals(left);
}

public static bool operator !=(SortVersion? left, SortVersion? right) =>
Expand Down
14 changes: 6 additions & 8 deletions src/libraries/System.Private.CoreLib/src/System/MulticastDelegate.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,12 @@ protected MulticastDelegate([DynamicallyAccessedMembers(DynamicallyAccessedMembe
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool operator ==(MulticastDelegate? d1, MulticastDelegate? d2)
{
// Test d2 first to allow branch elimination when inlined for null checks (== null)
// so it can become a simple test
if (d2 is null)
if (ReferenceEquals(d1, d2))
{
return d1 is null;
return true;
}

return ReferenceEquals(d2, d1) ? true : d2.Equals((object?)d1);
return d2 is not null && d2.Equals(d1);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
Expand All @@ -45,12 +43,12 @@ protected MulticastDelegate([DynamicallyAccessedMembers(DynamicallyAccessedMembe

// Test d2 first to allow branch elimination when inlined for not null checks (!= null)
// so it can become a simple test
if (d2 is null)
if (ReferenceEquals(d1, d2))
{
return d1 is not null;
return false;
}

return ReferenceEquals(d2, d1) ? false : !d2.Equals(d1);
return d2 is not null && !d2.Equals(d1);
}
}
#pragma warning restore CS0660, CS0661
Expand Down
10 changes: 1 addition & 9 deletions src/libraries/System.Private.CoreLib/src/System/Reflection/Assembly.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,20 +187,12 @@ public override string ToString()
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool operator ==(Assembly? left, Assembly? right)
{
// Test "right" first to allow branch elimination when inlined for null checks (== null)
// so it can become a simple test
if (right is null)
{
return left is null;
}

// Try fast reference equality and opposite null check prior to calling the slower virtual Equals
if (ReferenceEquals(left, right))
{
return true;
}

return (left is null) ? false : left.Equals(right);
return left is not null && left.Equals(right);
}

public static bool operator !=(Assembly? left, Assembly? right) => !(left == right);
Expand Down
10 changes: 1 addition & 9 deletions src/libraries/System.Private.CoreLib/src/System/Reflection/ConstructorInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,20 +24,12 @@ protected ConstructorInfo() { }
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool operator ==(ConstructorInfo? left, ConstructorInfo? right)
{
// Test "right" first to allow branch elimination when inlined for null checks (== null)
// so it can become a simple test
if (right is null)
{
return left is null;
}

// Try fast reference equality and opposite null check prior to calling the slower virtual Equals
if (ReferenceEquals(left, right))
{
return true;
}

return (left is null) ? false : left.Equals(right);
return left is not null && left.Equals(right);
}

public static bool operator !=(ConstructorInfo? left, ConstructorInfo? right) => !(left == right);
Expand Down
10 changes: 1 addition & 9 deletions src/libraries/System.Private.CoreLib/src/System/Reflection/EventInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,20 +87,12 @@ public virtual void RemoveEventHandler(object? target, Delegate? handler)
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool operator ==(EventInfo? left, EventInfo? right)
{
// Test "right" first to allow branch elimination when inlined for null checks (== null)
// so it can become a simple test
if (right is null)
{
return left is null;
}

// Try fast reference equality and opposite null check prior to calling the slower virtual Equals
if (ReferenceEquals(left, right))
{
return true;
}

return (left is null) ? false : left.Equals(right);
return left is not null && left.Equals(right);
}

public static bool operator !=(EventInfo? left, EventInfo? right) => !(left == right);
Expand Down
10 changes: 1 addition & 9 deletions src/libraries/System.Private.CoreLib/src/System/Reflection/FieldInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,20 +43,12 @@ protected FieldInfo() { }
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool operator ==(FieldInfo? left, FieldInfo? right)
{
// Test "right" first to allow branch elimination when inlined for null checks (== null)
// so it can become a simple test
if (right is null)
{
return left is null;
}

// Try fast reference equality and opposite null check prior to calling the slower virtual Equals
if (ReferenceEquals(left, right))
{
return true;
}

return (left is null) ? false : left.Equals(right);
return left is not null && left.Equals(right);
}

public static bool operator !=(FieldInfo? left, FieldInfo? right) => !(left == right);
Expand Down
10 changes: 1 addition & 9 deletions src/libraries/System.Private.CoreLib/src/System/Reflection/MemberInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,20 +46,12 @@ public virtual Module Module
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool operator ==(MemberInfo? left, MemberInfo? right)
{
// Test "right" first to allow branch elimination when inlined for null checks (== null)
// so it can become a simple test
if (right is null)
{
return left is null;
}

// Try fast reference equality and opposite null check prior to calling the slower virtual Equals
if (ReferenceEquals(left, right))
{
return true;
}

return (left is null) ? false : left.Equals(right);
return left is not null && left.Equals(right);
}

public static bool operator !=(MemberInfo? left, MemberInfo? right) => !(left == right);
Expand Down
10 changes: 1 addition & 9 deletions src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,20 +68,12 @@ this is ConstructorInfo &&
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool operator ==(MethodBase? left, MethodBase? right)
{
// Test "right" first to allow branch elimination when inlined for null checks (== null)
// so it can become a simple test
if (right is null)
{
return left is null;
}

// Try fast reference equality and opposite null check prior to calling the slower virtual Equals
if (ReferenceEquals(left, right))
{
return true;
}

return (left is null) ? false : left.Equals(right);
return left is not null && left.Equals(right);
}

public static bool operator !=(MethodBase? left, MethodBase? right) => !(left == right);
Expand Down
10 changes: 1 addition & 9 deletions src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,20 +41,12 @@ protected MethodInfo() { }
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool operator ==(MethodInfo? left, MethodInfo? right)
{
// Test "right" first to allow branch elimination when inlined for null checks (== null)
// so it can become a simple test
if (right is null)
{
return left is null;
}

// Try fast reference equality and opposite null check prior to calling the slower virtual Equals
if (ReferenceEquals(left, right))
{
return true;
}

return (left is null) ? false : left.Equals(right);
return left is not null && left.Equals(right);
}

public static bool operator !=(MethodInfo? left, MethodInfo? right) => !(left == right);
Expand Down
10 changes: 1 addition & 9 deletions src/libraries/System.Private.CoreLib/src/System/Reflection/Module.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,20 +148,12 @@ public virtual Type[] FindTypes(TypeFilter? filter, object? filterCriteria)
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool operator ==(Module? left, Module? right)
{
// Test "right" first to allow branch elimination when inlined for null checks (== null)
// so it can become a simple test
if (right is null)
{
return left is null;
}

// Try fast reference equality and opposite null check prior to calling the slower virtual Equals
if (ReferenceEquals(left, right))
{
return true;
}

return (left is null) ? false : left.Equals(right);
return left is not null && left.Equals(right);
}

public static bool operator !=(Module? left, Module? right) => !(left == right);
Expand Down
10 changes: 1 addition & 9 deletions src/libraries/System.Private.CoreLib/src/System/Reflection/PropertyInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,20 +62,12 @@ protected PropertyInfo() { }
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool operator ==(PropertyInfo? left, PropertyInfo? right)
{
// Test "right" first to allow branch elimination when inlined for null checks (== null)
// so it can become a simple test
if (right is null)
{
return left is null;
}

// Try fast reference equality and opposite null check prior to calling the slower virtual Equals
if (ReferenceEquals(left, right))
{
return true;
}

return (left is null) ? false : left.Equals(right);
return left is not null && left.Equals(right);
}

public static bool operator !=(PropertyInfo? left, PropertyInfo? right) => !(left == right);
Expand Down
6 changes: 3 additions & 3 deletions src/libraries/System.Private.CoreLib/src/System/Runtime/Versioning/FrameworkName.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,12 +197,12 @@ public FrameworkName(string frameworkName)

public static bool operator ==(FrameworkName? left, FrameworkName? right)
{
if (left is null)
if (ReferenceEquals(left, right))
{
return right is null;
return true;
}

return left.Equals(right);
return left is not null && left.Equals(right);
}

public static bool operator !=(FrameworkName? left, FrameworkName? right)
Expand Down
5 changes: 3 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/Type.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,15 +663,16 @@ internal string FormatTypeName()

public override string ToString() => "Type: " + Name; // Why do we add the "Type: " prefix?

public override bool Equals(object? o) => o == null ? false : Equals(o as Type);
public override int GetHashCode()
{
Type systemType = UnderlyingSystemType;
if (!ReferenceEquals(systemType, this))
return systemType.GetHashCode();
return base.GetHashCode();
}
public virtual bool Equals(Type? o) => o == null ? false : ReferenceEquals(this.UnderlyingSystemType, o.UnderlyingSystemType);

public override bool Equals(object? o) => o is Type type && Equals(type);
public virtual bool Equals(Type? o) => o is not null && ReferenceEquals(this.UnderlyingSystemType, o.UnderlyingSystemType);

[Intrinsic]
public static bool operator ==(Type? left, Type? right)
Expand Down
9 changes: 3 additions & 6 deletions src/libraries/System.Private.CoreLib/src/System/Version.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,15 +390,12 @@ private static bool TryParseComponent(ReadOnlySpan<char> component, string compo
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool operator ==(Version? v1, Version? v2)
{
// Test "right" first to allow branch elimination when inlined for null checks (== null)
// so it can become a simple test
if (v2 is null)
if (ReferenceEquals(v1, v2))
{
return v1 is null;
return true;
}

// Quick reference equality test prior to calling the virtual Equality
return ReferenceEquals(v2, v1) ? true : v2.Equals(v1);
return v2 is not null && v2.Equals(v1);
}

public static bool operator !=(Version? v1, Version? v2) => !(v1 == v2);
Expand Down