Fix failing tests (including changes codegen for failing IL generation tests for generic comparison)

[dsyme]

These are the changes needed to fix the last failing tests mentioned here: #915

@manofstick The changes are the removal of tailcalls in some generated IL code due to the recent changes in IL comparison in FSharp.Core.

I've looked at these cases carefully and they all look OK to me: tailcalls are not taken further down the chain in any case, or the structures involved are not recursive. The tailcalls are not an essential part off the specification of the generated code in these particular cases.

[manofstick]

@dsyme

Hmmm... This spans both the philosophical and practical.

I'll lay out the thought processes, and maybe this can lead us to the "best" solution.

Why remove tail?

• On 64-bit JIT for value types > 64-bit there can be a significant performance cost when included. This cost can be greater than an order of magnitude over the underlying equality (compare/gethashcode) operation.

Why insert tail?

• For recursive functions we can end up blowing the stack otherwise
• Potentially we can have performance improvement due to less cache bleeding out of cache.

What about the real world?

• Equals/CompareTo/GetHashCode are unlikely to be written using recursive calls that require the tail call instruction. i.e. list<> Equals gets rewritten by the compilers as an imperative loop.
• But; personally I have found value types to mostly be more trouble than they are worth (i.e. unmeasurable performance improvements).
  • Except when <= 64 bit
  • Except when used in big chunks in arrays
  • Except when passed around by reference (which means marking as mutable)

My thoughts from typing this?

Argh, don't ask me for my opinion! OK, well if I have to. Given that making values types useful (i.e. performance improvement) requires lots of bending over backwards currently anyway, I think that removing tail from the standard equality operations is probably more risky that it's worth, even though I think it unlikely that any "real" type would require tails functionality. This does perpetuate the bad name that values types have though, condemning them basically to always remain a second class citizen. (i.e. you should never basically use them in standard contexts)

(A possibly way out of this malaise might be a parallel interfaces along the lines of:

type IEquatableValueType<'T> =
    abstract Equals : byref<'T> -> bool

type IComparableValueType<'T> =
    abstract CompareTo : byref<'T> -> int

which could be created at some stage would would bypass the problem by only passing reference cells which would be "tail" safe, as well as fast. (Purely theoretical, I haven't persued this line in reality; probably would have to be an alternative operator, as existing operators will all act on the actual type...))

[dsyme]

@manofstick My concern is that we just keep things exactly as they were before (no regressions) for the cases we care about.

First, was the removal of tailcall deliberate here, or just an outcome of the changes in FSharp.Core? Thx

These are mostly about generic hash, compare and equality on structures such as

type List<'T> = Nil | Cons of 'T * List<'T>

and similar recursive types. As you mention these tailcall via direct jumps within the critical routine, hence coping with very long lists. Please check there are no changes in tailcalling here - I don't think there are.

The problem with removing tailcall would with mutually recursive types of a similar "right-heavy" shape where the tailcall is not direct (or is indirect through a type variable), e.g.

type List<'T> = Nil | Cons of ListData<'T>
and ListData<'T> = ListData of 'T * List<'T>

The type List<'T> still effectively appears in "tail position" within the recursive type. I believe we are still tailcalling here or at least there is no change in codegen. But please check. I poked around a bit myself looking at the diff between old and new in these cases and I think they are fine. If those two cases (and any variations are covered then I think I'm comfortable, unless someone points out a specific test case where behaviour has changed

When I say "indirect through a type variable" I man like this:

type Pair<'T,'U> = Pair of 'T * 'U

type List<'T> = Nil | Cons of Pair<'T,List<'T>>

So, could you poke around with testing and determine if there has been actual behaviour change here in cases like the above then I'd be grateful.

thanks
don

[manofstick]

...this message is just part of some notes as I explore the space, [1 of n]...

Functionality in code below is unchanged from current release to master.

Note is: If you don't "tail" tuples on recursive union types then you will blow up with stack overflow. Side note: possibly worth of a warning (to blow up code below #define WANT_PROGRAM_TO_STACKOVERFLOW)

type List<'T> = Nil | Cons of ListData<'T>
#if WANT_PROGRAM_TO_STACKOVERFLOW
and ListData<'T> = ListData of List<'T> * 'T
let createListData a b =
    ListData (a, b)
#else
and ListData<'T> = ListData of 'T * List<'T>
let createListData a b =
    ListData (b, a)
#endif

let rec createList make n l  = 
    if n = 0 then l
    else createList make (n-1) (createListData (Cons l) (make n))

let lots = 1000000

let make n =
    n, float n, float32 n, int64 n, decimal n

let a0 = createList make lots (createListData Nil (make 0))
let a1 = createList make lots (createListData Nil (make 0))
let b = createList make lots (createListData Nil (make -1))

let sw = System.Diagnostics.Stopwatch.StartNew ()

let good = a0 = a1 && a0 <> b && a1 <> b

System.Console.WriteLine ("{0} ({1})", good, sw.ElapsedMilliseconds)

Side, side note: When not blowing the stack, this is another good example of performance improvement of #513 ! :-)

[manofstick]

@dsyme

First, was the removal of tailcall deliberate here, or just an outcome of the changes in FSharp.Core? Thx

It is more an outcome of changes of FSharp.Core. That was deliberate, because testing with > 64 bit value types had proven a performance bottleneck that I was trying to eradicate. But I didn't think though the ramifications that all generated equality/comparison operators through all bits of code would then be affected.

[manofstick]

@dsyme

BADNESS! ...modifying the example in note 1 with the Pair structure causes StackOverflow... I think I need to remove those eliminate_tail_calls....

type Pair<'T,'U> = Pair of 'T * 'U
type List<'T> = Nil | Cons of Pair<'T, List<'T>>

let rec createList n l  = 
    if n = 0 then l
    else createList (n-1) (Cons (Pair(n, l)))

let lots = 1000000

let a0 = createList lots (Cons (Pair(0, Nil)))
let a1 = createList lots (Cons (Pair(0, Nil)))

let good = a0 = a1

System.Console.WriteLine ("{0}", good)

[manofstick]

@dsyme

Actually there is current release badness too! If I change those equals to getting the hash, then current release blows up as well! (simplified)

type Pair<'T,'U> = Pair of 'T * 'U
type List<'T> = Nil | Cons of Pair<'T, List<'T>>

let rec createList n l  = 
    if n = 0 then l
    else createList (n-1) (Cons (Pair(n, l)))

let lots = 1000000

let a0 = createList lots (Cons (Pair(0, Nil)))

let value = hash a0

System.Console.WriteLine ("{0}", value)

(Although on contemplation for structures so large one is unlikely to actually want to calculate the hash by a complete descent...)

[manofstick]

Very bad badness... I removed all my eliminate tail call calls and was still getting stack overflow errors. I've had a small investigation into what is the cause of the problem and it appears that in my building block types the compiler isn't adding tail.

Now I'm currently at my daughter's swimming lessons and not sure if I'll get another chance to get to code today, but I hope that my initial investigation is wrong or it's a compiler bug, as opposed to some fundamental reason. I.e. being able to tail call on a constrained type call. Otherwise I might be in trouble here...

[manofstick]

Here is a version of the issue (not a particularly great example of programming; but should recurse until it gets a null reference exception; but doesn't. Gets a stack overflow)

type SomeConstrainedTypeCall<'a when 'a : null and 'a :> System.IEquatable<'a>>() =
    member __.ShouldBeTailable (x:'a, y:'a) =
        x.Equals y

[<AllowNullLiteral>]
type ListOfSorts(parent:ListOfSorts) =
    member __.Next = parent
    interface System.IEquatable<ListOfSorts> with
        member this.Equals other =
            let e = SomeConstrainedTypeCall<ListOfSorts> ()
            e.ShouldBeTailable (this.Next, other.Next)

let rec createList n l  = 
    if n = 0 then l
    else createList (n-1) (ListOfSorts l)

let a0 = createList 100000 (ListOfSorts null)
let e = a0 :> System.IEquatable<ListOfSorts>
let value = e.Equals a0

So the problem is that ShouldBeTailable does not get a tail. The IL is as follows:

IL_0000: nop
IL_0001: ldarg.1
IL_0002: stloc.0
IL_0003: ldloca.s 0
IL_0005: ldarg.2
IL_0006: constrained. !a
IL_000c: callvirt instance bool class [mscorlib]System.IEquatable`1<!a>::Equals(!0)
IL_0011: ret

This is a REALLY BIG PROBLEM. Hmmm... I'm thinking I might have backout #513 if can't mix constrained with tail...

[dsyme]

@manofstick I'm closing this in favour of #941. We will likely commit the updates to the tests, and then open a separate issue about whether the tailcall issue means we need to backout #513 for now