[WIP] Non-boxing equality for enums (in generic contexts)

[manofstick]

Building on the equality framework, this adds non-boxing equality for System.Enum derived objects (i.e. .net enums) via the "trick" of using System.Collections.Generic.EqualityComparer.Default.Equals. The same trick was attempted for the comparison functionality, but the underlying System object was still boxing.

[msftclas]

Hi @manofstick, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

[mexx]

@manofstick I can't judge whether this will be a breaking change. My hope is that it's only a performance optimization, right?

[manofstick]

@mexx

Sorry, I just assumed that anyone reading these would have been familiar with #513 that just recently got merged, so was maybe more brief that I should of been in the description.

So yes, it is a performance optimization that sits within the #513 framework.

System.Enum derived objects (i.e. enumerations) only derive from ValueType, IComparable, IFormattable & IConvertible. i.e. specifically they don't derive from IEquatable<'a>, which means that the have no type based overload of the Equals method. Used in a generic context then, there are no generic constraints that allow you to test for equality in a non-boxing fashion (unless I am wrong...)

#513 was all about building up nice equality operators (and comparison operators) in this generic context for HashIdentity. (When not used in a generic context (including inlining obviously), the fsharp compiler sneakily creates IEqualityComparer<'a> derived objects. And when in generic contexts, FSharp.Core has hard-coded common System types (i.e. int, char, string...))

Anyway, the long and short of it is in code that is compiled using HashIdentity.Structural on a type that isn't fully known at compile time, think (groupBy, dict, ...) then you fall down this path.

So yes, some tests were in the scope of the the "non breaking change" that you were after (possibly not exhaustive, I didn't feel the change was particularly meaty), and the bit where I said "non-boxing" was meant to imply that this would put less pressure on the garbage collector, and thus is a performance improvement. When I get home, I'll run some numbers for you.

[mexx]

@manofstick No need to be sorry, thank you for the great explanation. Looking towards the numbers 👍

[manofstick]

OK @mexx I have tried to make a semi-realistic (squint, and twist your head a little...) example where we are doing a groupBy on an enum field.

The code was compiled from the command line with

fsc -O --platform:xxx

where xxx was x32 or x64 as per listed times below

type EnumA = A0 = 0 | A1 = 1 | A2 = 2 | A3 = 3 | A4 = 4 | A5 = 5 | A6 = 0x7
let getAnEnum i = match i &&& 7 with 0 -> EnumA.A0 | 1 -> EnumA.A1 | 2 -> EnumA.A2 | 3 -> EnumA.A3 | 4 -> EnumA.A4 | 5 -> EnumA.A5 | _ -> EnumA.A6

type R = {
    GroupByKey : EnumA
    OtherData : float
}

let data = [|
    let r = System.Random 42
    for i = 1 to 10000000 do
        yield { GroupByKey = getAnEnum (r.Next()); OtherData = 42. }
|]

let total_time = System.Diagnostics.Stopwatch.StartNew ()
for i = 1 to 10 do
    let sw = System.Diagnostics.Stopwatch.StartNew ()
    let grouped =
        data
        |> Array.groupBy (fun d -> d.GroupByKey)
    printf " iteration=%dms [" sw.ElapsedMilliseconds
    grouped |> Array.iter (fun (key,items) -> printf "%A=%d;" key items.Length)
    printfn "]"
printfn "total_time=%dms" total_time.ElapsedMilliseconds

The results:

State	x32	% of release	x64	% of release
release	21730	100%	13311	100%
master	9482	44%	10091	76%
branch	6989	32%	6716	50%

Where:

• release is current f# dropped with Visual Studio 2015
• master is the branch from which this changed request was forked from
• branch is this PR branch
• In the x32/x64 column, they are the total_times of the test in milliseconds

NB. Times in the master branch are already significantly better than release due to both #549 and #513

[manofstick]

Not longer makes sense after #966

[dsyme]

I'll reopen and mark WIP, we don't want to lose this

[msftclas]

Hi @manofstick, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

[KevinRansom]

@dsyme Did you change your mind about WIP? I note that it isn't marked WIP.

[manofstick]

@dsyme & @KevinRansom

...and I don't think it was this one that was meant to be reopened - I think it was supposed to be #961...

[KevinRansom]

@manofstick

closing and reopening #961