Could Burst have an optimization to prioritize Equals(T) over Equals(object) when possible?

[weary tangle]

Below is just a pseudo code to demonstrate the issue.

public struct ValueHolder<T> : IEquatable<ValueHolder<T>>
{
    public T value;
    
    public bool Equals(ValueHolder<T> other)
        => this.value.Equals(other.value);
}

public struct A : IEquatable<A>
{
    public bool Equals(A other)
    {
        UnityEngine.Debug.Log("Equals(A)");
        return true;
    }
    
    public override bool Equals(object obj)
    {
        UnityEngine.Debug.Log("Equals(object)");
        return false;
    }
}

ValueHolder<A> a = default;
ValueHolder<A> b = default;

a.Equals(b);

• Currently the last line will print "Equals(object)".
• To print "Equals(A)", we have to constraint T to IEquatable<T>.
• However, ValueHolder<T> is a example of a multi-purpose type, T can't be constrained to anything.

Given that equality comparison permeats every codebase and it's not always possible to write explicit code path (e.g. NativeHashMap requires TKey to be IEquatable) or maintain another Burst-compatible version of some types, could Burst automatically prioritize Equals(T) when possible?

[weary echo]

Looking at the code I see no reason why T can't be constrained to unmanaged, IEquatable<T>?

As a rule I avoid implementing equals(object) as it leads to accidental bugs, there's like 1 exception where the compiler throws up a stink if you don't

[real harness]

I think that's more of a C# issue than Burst
Ideally you would be able to define that ValueHolder<T> implements IEquatable<ValueHolder<T>> only when T implements IEquatable<T> but only languages such as Scala or Rust support that kind of stuff (maybe F# too?)

[weary tangle]

Because ValueHolder can be used in other contexts. T has to be general. It's all easy if the code is meant for Burst only. But it's not the case here.

[weary tangle]

C# has runtime optimization (for newer versions) that can make EqualityComparer to use the non-boxing code path.

But EqualityComparer is out of question within Burst.

[weary echo]

And if those types want to use a container that does equity checks, they must implement it. That's kind of the point of restraints

[weary tangle]

I think it might be better if Burst can optimize managed code path out if unmanaged code path is available. It might help in more use cases, for example type checking or pattern matching against interfaces.

[stark patio]

replacing object.Equals calls with ISomeInterface.Equals would be changing language semantics; optimizations should never change defined semantics

what could work is Burst guaranteeing to inline calls to object.Equals, and then if you implement object.Equals by forwarding to IEquatable.Equals you get the desired behavior

[weary tangle]

Yes, that's the optimization I'm talking about, but I intentionally leave it vague, the compiler should choose how to work on it.

[stark patio]

attempting to find those in all managed code would probably be very challenging but having a special case for object.Equals with an obj as T cast would probably be much more feasible

and could be documented as a guaranteed working pattern

(similar to the string stuff in https://docs.unity3d.com/Packages/com.unity.burst@1.8/manual/csharp-string-support.html)

[pale pier]

i'm very confused by this whole thread. i would think no bursted code could ever call equals(object) because object is managed and burst can't get there from here, no?

[stark patio]

the idea is that much like burst has some special cases for string despite it being a managed type, burst could have a special case to make the following code work:

struct A { public int value; public override bool Equals(object other) { if(other is A otherA) { return value == otherA.value; } return false; } }

void DoSomething<T>(T x, T y) where T : unmanaged { if(x.Equals(y)) { ... } }

DoSomething(new A(), new A());

so basically in any context where x is a value of a known value type (possibly a generic type parameter), burst could have special-case support to:

• always attempt to inline calls to object.Equals(object) with x as the this argument (in other words, x.Equals), and
• in such an inlined context, support managed-to-unmanaged cast operations on the object argument

at this point (due to inlining) burst knows the types of the objects at compile time, so it can skip the box-unbox sequence, and thus avoid dealing with any actual managed objects at runtime

[weary tangle]

As anttirt said, the point is Burst should be able to dispatch the call to the Equals(T) if the method presents on T. Even though generic in C# is not template in C++, body of the open generic type won't be copied to the closed generic type, .NET can still optimized out the Equals(object) on any type T that implements IEquatable.

Burst should be able to call A.Equals(A) from ValueHolder<A>.Equals(ValueHolder<A>).

[pale pier]

but no bursted code ever boxes anything anyway?

like are you saying that there's code that isn't burstable today that you think could be burstable with a new scheme?

[weary tangle]

Yes, the Equals method.

For example the NativeHashMap requires TKey to be IEquatable, because Burst can't figure it out if TKey is not.

[pale pier]

but is that a big problem?

[weary echo]

(is this not the point of interfaces?)

[weary tangle]

Constraints cannot be always applicable. That's the point of my OP.

T in ValueHolder<T> should not be constrainted to IEquatable in some case.

[weary echo]

but your op can be solved by putting IEquatable<T> on it and on the rare types you want to store in it, that you don't own yourself and can't implement, you can implement a tiny implicit wrapper

[pale pier]

oh this is probably because you want to be able to define complementary stuff where it's like this when T satisfies one constraint and like that when T satisfies the opposite constraint and c# basically doesn't let you do that

[weary tangle]

Because I need ValueHolder to hold value that never implement IEquatable in the first place, and its code is out of my hand?

[weary echo]

but you can write a tiny struct that wraps a type that doesn't implement it

and the types that you would hold in here that don't have equatable are types you haven't implemented yourself, so it should be minimal

[weary tangle]

There is not single wrapper. I will have to write wrapper for each specific case. But Burst can always help in this by optimizing out the Equals(object).

Mind that that ValueHolder is being used in more contexts than just in Burst.

Currently I have to adapt other contexts just to satisfy Burst.

Since .NET can optimize this out, I think why Burst cannot? So I present the issue here.

[weary echo]

    public struct EquatableWrapper<T> : IEquatable<EquatableWrapper<T>>
    {
        private T value;

        public static implicit operator T(EquatableWrapper<T> wrapper)
        {
            return wrapper.value;
        }

        public static implicit operator EquatableWrapper<T>(T val)
        {
            return new EquatableWrapper<T> { value = val };
        }

        public bool Equals(EquatableWrapper<T> other)
        {
            return value.Equals(other.value);
        }

        public override int GetHashCode()
        {
            return this.value.GetHashCode();
        }
    }```

[weary tangle]

This is doable if other contexts is minority compared to Burst. The current situation is reversed. Burst is minority here.

[weary echo]

but it works in both cases

when you have unmanaged types only it'll be fine in burst

[weary tangle]

Then I have to use wrapper in every other cases.

[weary echo]

you only have to use it for type sthat don't implement IEquatable<T>

[weary tangle]

Yeah doable. Just not what I really want. Though I will take your workaround for the current situation.

I still keep a stand that this optimization specifically for Equals can be justified, when compare it with .NET.

[weary echo]

well it's not an optimization because burst already only allows the faster path?

it'd be purely changing syntax rules?

[weary tangle]

Hmm do you mind expand on this?

[weary echo]

well in burst, you can only use IEquatable<T> the equals(object) is not allowed

so auto converting equals(object) to equals(T) is the same result as what is allowed in burst atm

burst prevents you usnig the slow Equals(object) path

i will say on a side note, one of the things i am looking forward to with the coreclr update is enums finally having iequatable implemented

so they will be usable as keys again in hashmaps (at least that's what i'm hoping)

[weary tangle]

Do you mean that even if Equals(T) exists on type T, Burst should still see Equals(object) because the generic container does not put a constraint on T?

Taking another look at the codebase I have now, I don't think it's possible to enforce IEquatable for T. ValueHolder is being used for too many things that is not IEquatable. And just a small amount of equality checks happen in Burst context.

[stark patio]

this implementation of Equals is still not burst-compatible because value.Equals(other.value) always resolves to object.Equals(object), so it doesn't solve the initial problem

but with the proposed burst "relaxation" feature, it would work

(for any unmanaged type T that implements object.Equals with a standard cast pattern)

[weary echo]

if your using a type that doesn't have Equals(T) then it's never going to be burst compatible

the point of that solution was you wanted to use it outside burst for objects

[stark patio]

not with the current version of burst, but an inlining + box-unbox elision pass would solve that without having to actually touch any managed objects whatsoever at runtime

it's ultimately a fairly simple peephole optimization, for some variable x of an unmanaged type T, object o = x; if(x is T x_) { F(x_); } becomes F(x);, this doesn't change the behavior of the code but it removes any managed objects making it runtime-compatible with burst