Avoid boxing on equality and comparisons

[manofstick]

As per #9348, Updated the old pull request to be (effectively) rebased off current master.

Why does this code change the test results for tests/FSharp.Core.UnitTests/FSharp.Core/ComparersRegression.fs?

This code includes a "breaking change" which fixes a long outstanding bug...

let a, b = -1y, 1y

assert (   a   <   b   )
assert ( [ a ] < [ b ] )
assert ( [|a|] < [|b|] ) // boom, this fails!

Why is the test for IL output changed?

Changes to Optimizer.fs are done in order to bubble up access to the comparers. If this isn't done we get particularly nasty performance in some cases due to > 64 bit value types due to the tail call "optimization" (i.e. you have basically a forwarding call which takes 2 Ts and passes them on to lower layer, which should be able to be inlined by the JIT, but the JIT doesn't do it because of tail, and then if they are > 64 bit they JIT spends time setting up stackframe for recursive calls which is expensive)

This can still be a problem in client code. Using HashIdentity.Structural creates an IEqualityComparer which is just a stub code which calls the underlying LanguagePrimitives.HashCompare.FSharpEqualityComparer_PER<'a>.EqualityComparer. This is not ideal, and an additional change to the optimizer should just return that object (a current work around is just to compile the client with no tail call optimization, which if you're working with > 64bit value types you should probably be doing anyway because of all the places where this same issue raises its ugly head).

Why is reflect.fs changed?

The code was just moved up to prim_types so it could be used from there. As prim_types doesn't know about the option type, the functions were slightly modified to by byrefs with bools instead.

What are the full rules of where this is used.

Here's the code which determines if we can use the default .net comparer (which checks to see if I(Equatable|Comparable) are available, otherwise falling back to obj.(GetHashCode|Equals).

let canUseDotnetDefaultComparisonOrEquality isCustom hasStructuralInterface stringsRequireHandling er (rootType:Type) =
    let processed = System.Collections.Generic.HashSet ()

    let bindingPublicOrNonPublic =
        Reflection.flagsOr BindingFlags.Public BindingFlags.NonPublic

    let rec isSuitableNullableTypeOrNotNullable (ty:Type) =
        // although nullables not explicitly handled previously, they need special handling
        // due to the implicit casting to their underlying generic type (i.e. could be a float)
        let isNullableType = 
            ty.IsGenericType
            && ty.GetGenericTypeDefinition().Equals typedefof<Nullable<_>>
        if isNullableType then
            checkType 0 (ty.GetGenericArguments ())
        else 
            true

    and isSuitableTupleType (ty:Type) =
        ty.IsValueType && // Tuple<...> don't have implementation, but ValueTuple<...> does
        Reflection.isTupleType ty && 
        checkType 0 (ty.GetGenericArguments ())

    and isSuitableStructType (ty:Type) =
        ty.IsValueType &&
        Reflection.isObjectType (ty, bindingPublicOrNonPublic) &&
        (not (isCustom ty)) &&
        checkType 0 (Reflection.getAllInstanceFields ty)

    and isSuitableRecordType (ty:Type) =
        Reflection.isRecordType (ty, bindingPublicOrNonPublic) &&
        (not (isCustom ty)) &&
        ( let fields = Reflection.fieldPropsOfRecordType (ty, bindingPublicOrNonPublic)
            let fieldTypes = Array.ConvertAll (fields, Converter (fun f -> f.PropertyType))
            checkType 0 fieldTypes)

    and isSuitableUnionType (ty:Type) =
        Reflection.isUnionType (ty, bindingPublicOrNonPublic) &&
        (not (isCustom ty)) &&
        ( let cases = Reflection.getUnionTypeTagNameMap (ty, bindingPublicOrNonPublic)
            let rec checkCases idx =
                if idx = cases.Length then true
                else
                    let tag,_ = get cases idx
                    let fields = Reflection.fieldsPropsOfUnionCase (ty, tag, bindingPublicOrNonPublic)
                    let fieldTypes = Array.ConvertAll (fields, Converter (fun f -> f.PropertyType))
                    if checkType 0 fieldTypes then
                        checkCases (idx+1)
                    else
                        false
            checkCases 0)

    and checkType idx (types:Type[]) =
        if idx = types.Length then true
        else
            let ty = get types idx
            if not (processed.Add ty) then
                checkType (idx+1) types
            else
                ty.IsSealed // covers enum and value types; derived ref types might implement from hasStructuralInterface
                && not (isArray ty)
                && (not stringsRequireHandling || (not (ty.Equals typeof<string>)))
                && (er || (not (ty.Equals typeof<float>)))
                && (er || (not (ty.Equals typeof<float32>)))
                && isSuitableNullableTypeOrNotNullable ty
                && ((not (hasStructuralInterface ty))
                    || isSuitableTupleType ty
                    || isSuitableStructType ty
                    || isSuitableRecordType ty
                    || isSuitableUnionType ty)
                && checkType (idx+1) types

    checkType 0 [|rootType|]

[KevinRansom]

@dsyme , @TIHan , Paul has updated his long standing effort to improve equality performance. Can you take a look and offer guidance about whether we wish to pursue this approach. If we do .. how do we wish to progress this PR ...

Thank you

Kevin

[TIHan]

These are now public APIs. We need to be sure the names are what we would want.

[manofstick]

Way past my pay-grade! @cartermp ?

[manofstick]

Hmm.. Some thoughts here though; maybe, to mimic EqualityComparer.Default, FSharpEqualityComparer_PER.EqualityComparer should become FSharpEqualityCompare.Default (and just leave FSharpEqualityCompare_ER... As I think that one is the lesser used...) But then is it right to leave it in it's current namespace? EqualityComparer lives in System.Collections.Generic so should it be in FSharp.Collections? Or should it remain 'public'-lite, i.e. not really for use by anyone except the compiler...

[TIHan]

This code includes a "breaking change" which fixes a long outstanding bug...

Didn't know we had this bug. [|-1y|] < [|1y|] returnin false doesn't seem right, should be true. We should probably discuss how we will deal with this breaking change. @cartermp

Is it possible to make a separate PR that just fixes this bug?

[TIHan]

One thing to clarify in this PR, is how this avoids boxing as it's not entirely clear.

Also, are there any behavior changes that change the choice of what equality/comparison interface methods on a type get called?

For example: F# struct equality will always choose IStructuralEquatable.Equals(obj, IEqualityComparer) if the F# struct implements it (which they do by default). So naturally, this boxes by default. Changing this behavior to ultimately call IEquatable<'T>.Equals(T) is possible without a "technical" breaking change. It does need special care though as an explicit implementation of IStructuralEquatable on a struct will always have to call IStructuralEquatable.Equals(obj, IEqualityComparer) on equality. If .NET structs implement IStructuralEquatable, then IStructuralEquatable.Equals(obj, IEqualityComparer will always have to be called on equality no matter what - any change to that decision is considered "technically" breaking.

[manofstick]

Didn't know we had this bug. [|-1y|] < [|1y|] returnin false doesn't seem right, should be true. We should probably discuss how we will deal with this breaking change. Is it possible to make a separate PR that just fixes this bug?

Due to covariance weirdness. Of course it could be a separate PR... Just would obviously cause merge issues with this PR, because the code changed is deleted with this...

[manofstick]

@TIHan - as I thought I'd put here, but it appears I wrote in a comment in the issue, this is only a partial solution to the problem, and your optimizer changes (which were never finished/merged/whatever) really need to be part of this as well (once they are complete? accepted? etc...).

So this change is really for when equality/comparison is used in a generic context rather than attempting to solve the issue for types known at compile time (which requires more fiddling with the optimizer).

Now your concern about when IStructuralEquality is used is decided upon in the code snippet at the top of this PR - probably hard to spot given that the logic is more complex than I would like, but:

                && ((not (hasStructuralInterface ty))
                    || isSuitableTupleType ty
                    || isSuitableStructType ty
                    || isSuitableRecordType ty
                    || isSuitableUnionType ty)

So it's not only checking for hasStructuralInterface, but that it's only used under very specific conditions, which is a known FSharp type and that the type arguments for FSharp type comply with the set of rules.

I think this whole functions (canUseDotnetDefaultComparisonOrEquality ) logic should also be implemented in the optimizer, so that where the type is known at compile time then we can just directly call the non-boxing functions...

Does that make things clearer? Or need more details?

[TIHan]

this change is really for when equality/comparison is used in a generic context rather than attempting to solve the issue for types known at compile time

We should definitely put that in your main description of the PR and/or title.

So it's not only checking for hasStructuralInterface, but that it's only used under very specific conditions, which is a known FSharp type and that the type arguments for FSharp type comply with the set of rules.

Ok, so this is checking if it has that interface in general. I need to look more carefully at this.

your optimizer changes (which were never finished/merged/whatever) really need to be part of this as well (once they are complete? accepted? etc...).

I should have finished this last year but did not get around to it. I still have some worries with it, such a new --optimize:equality flag. Perhaps, I should just finish the non-breaking optimizations as it's pretty well defined.

Does that make things clearer? Or need more details?

That clears it up :) - thank you.

[manofstick]

@TIHan just to make it clear that my previous comment doesn't preclude any improvements in non-generic code! I.e. it would have sped up the DateTime access that you manually added (but seriously who would use DateTime when you have NodaTime! I only partially talk in jest!) ...

So using a non-IStructural* type, like most non-fsharp defined types, you will be a performance boost.

As an example...

// needs to reference NodaTime - https://nodatime.org/

let data =
    let now = SystemClock.Instance.GetCurrentInstant ()
    let rng = Random ()
    Array.init 2500 (fun _ -> now.Plus (Duration.FromSeconds (int64 (rng.Next (System.Int32.MinValue, System.Int32.MaxValue)))))


for i = 1 to 5 do
    let sw = Stopwatch.StartNew ()

    let mutable count = 0
    for i = 0 to data.Length-1 do
        for j = 0 to data.Length-1 do
            if data.[i] < data.[j] then
                count <- count + 1

    printfn "%d (%d)" sw.ElapsedMilliseconds count

So here, the (<) is converted to LanguagePrimitives.HashCompare.GenericLessThanIntrinsic<Instant>(data[j], data[k]), which is now opimized to not box (it now internall uses LanguagePrimitives.HashCompare.FSharpComparer_ForLessThanComparison<T>.comparer.Compare(x, y) < 0)

(On my machine, this loop takes ~400ms on current build, and ~65ms on build with this PR)

[TIHan]

I am beginning to gasp this and need bake time in my head.

[manofstick]

Oh, and another terrible, terrible performance thing that is fixed by this is when used in a generic context, comparisons of floats where NaNs is (currently) a disaster... As it relies on throwing exceptions. This caught me once (in testing luckily!)...

So if I slighty modify the example previously to...

let runStuff<'a when 'a : comparison> (data:array<'a>) =
    for i = 1 to 5 do
        let sw = Stopwatch.StartNew ()

        let mutable count = 0
        for i = 0 to data.Length-1 do
            for j = 0 to data.Length-1 do
                if data.[i] <= data.[j] then
                    count <- count + 1

        printfn "%d (%d)" sw.ElapsedMilliseconds count

   
let data =
    Array.init 1000 (fun i -> if i = 0 then System.Double.NaN else float i)

runStuff data

then this takes ~140ms currently, and ~5ms after this PR. if you bump up the number of NaNs from 1 in 1000 to the whole array being NaNs... then that takes over 30 second! (It still takes ~5ms after this PR)...

[manofstick]

@TIHan - How's the head baking going? Do you need any further details?

[abelbraaksma]

I really like a lot of what's in this PR, your latest comment on NaN is an eye opener, can't believe it was based on exceptions!

I wonder if a recent proposed optimization, where a boxed known primitive type next calls an interface member would be optimized to a constrained callvirt, would help here, or with @TIHan's optimizations (discussed in my string pr).

I might have some other insights to share, I was planning on playing around a bit with your pr. Again, I really like it!

[TIHan]

@manofstick I have a couple of more details.

There is really some complicated logic that is added to prim-types.fs, namely the mechanism for checkType. We really need to see if we can make this simpler and/or do some cleanup. At the moment, I'm not sure what that would look like as I would need to spend a lot more time on this.

Your idea by making the intrinsic generic functions not do boxing is good; everything can benefit from it without the compiler having to re-write/re-do everything.

However, the reason your approach is working is because of the use of the .NET comparer, where it internally looks for IEquatable<'T> and calls IEquatable<'T>.Equals, if I'm not mistaken? This is technically a breaking change. Today, our generic functions will never look for or choose IEquatable<'T>.Equals, and it will always choose the obj.Equals if IStructuralEquatable doesn't exist. We had this same problem when doing the equality devirtualization and had to have specific rules so that it wouldn't break existing and predictable behavior.

Not saying this puts it off the table, but we have always chosen obj.Equals over IEquatable<'T>.Equals. Changing it to favor IEquatable<'T>.Equals over obj.Equals makes it a breaking change. We need to decide if we can actually accept this kind of breaking change. If we can, then we should make a change in FSharp.Core to do basically what you did. If not, then we have to exclusively make changes in the compiler to choose IEquatable<'T>.Equals in cases where we know it's 100% safe.

In my personal opinion, I think we should consider a breaking change. The fact that will never choose IEquatable<'T>.Equals in all cases that don't implement IStructuralEquatable just feels wrong. Consider the .NET guidance when implementing IEquatable<'T>.Equals:

If you implement Equals(T), you should also override the base class implementations of Equals(Object) and GetHashCode() so that their behavior is consistent with that of the Equals(T) method.

So, in theory, if everyone followed this guidance, then we could make the change and it wouldn't break anyone. But, users can technically make obj.Equals and IEquatable<'T>Equals have different behavior. Therefore, we must discuss the risk.

[TIHan]

@manofstick Looking a bit more closely, I see you are checking for the CustomEquality attribute to determine if you can use the .NET comparer:

                and isSuitableStructType (ty:Type) =
                    ty.IsValueType &&
                    Reflection.isObjectType (ty, bindingPublicOrNonPublic) &&
                    (not (isCustom ty)) &&
                    checkType 0 (Reflection.getAllInstanceFields ty)

While this can cover F# types to ensure its safe to choose IEquatable<'T>.Equals when there is no custom equality, the same is not true for .NET struct types.

[TIHan]

I am incorrect:
Reflection.isObjectType should never return true for .NET struct types. If that's the case, then we might actually be safe with this change since there would be no technical breaking change.

This is some really important nuance here and it should be documented.

[TIHan]

To make this clearer, this should be isFSharpObjectType. Same with the others: isFSharpRecordType, isFSharpUnionType.

[TIHan]

isFSharpType would be clearer

[manofstick]

@TIHan

where it internally looks for IEquatable<'T> and calls IEquatable<'T>.Equals, if I'm not mistaken? This is technically a breaking change.

Yes this is true; I tend to think of them as the same, but obviously that is not true. Hmmm. The whole this is crippled without this (well could still be used for fsharp types but...)

So how would we proceed with either getting that ratified (or not)?

[manofstick]

I am incorrect:
Reflection.isObjectType should never return true for .NET struct types. If that's the case, then we might actually be safe with this change since there would be no technical breaking change.

Buuuuttt... Normal .net struct types don't implement IStructuralXXX so they are not checking that logic. As said it could be harsh only only work for fsharp types, but I want it to work with things like NodaTime... (and Guid and DateTime and ...)

So yes, the code is a breaking change unless I really cripple it... (Would have to put back the special handling for all the intrinsic types as well)

[KevinRansom]

@manofstick ,
Thank you for this contribution, unfortunately we are not at a point where we can accommodate breaking changes. It doesn't look like we will be able to get this in at this time.

Please feel free to reactivate it when you are in a position to work on it some more.

Thanks
Kevin