Optimize DateTime comparison/equality

[cartermp]

In a trace from here: https://developercommunity.visualstudio.com/content/problem/1035124/trace-of-editing-experience-with-in-memory-cross-p-1.html

I found that ComputeMaxTimeStamp was allocating a lot of DateTimes, stemming from the call to max:

This replaced it with a call to DateTime.Compare, which won't allocate.

After talking with @TIHan and @KevinRansom, this added some DateTime specializations to FSharp.Core so that equality, comparison, min, max, etc. all specialize to the right method rather than box the DateTime structs.

[cartermp]

This is a small benchmark program I used to compare the two:

open System
open BenchmarkDotNet.Attributes
open BenchmarkDotNet.Running

[<MemoryDiagnoser>]
type DateTimeBench() =
    let maxTime = DateTime.MaxValue

    let data =
        [
            for x in 0..100 ->
                DateTime (int64 x)
        ]

    [<Benchmark(Baseline=true)>]
    member _.UsingMax() =
        let rec f xs acc =
            match xs with
            | [] -> acc
            | h::t -> f t (max acc h)
        f data maxTime

    [<Benchmark>]
    member _.UsingDateTimeCompare() =
        let rec f xs acc =
            match xs with
            | [] -> acc
            | h::t ->
                let res = DateTime.Compare(acc, h)
                if res >= 0 then
                    f t acc
                else
                    f t h
        f data maxTime

[<EntryPoint>]
let main argv =
    let summary = BenchmarkRunner.Run<DateTimeBench>()
    printfn "%A" summary
    0 // return an integer exit code

It yielded these results:

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.18363.836 (1909/November2018Update/19H2)
Intel Core i7-6700K CPU 4.00GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=5.0.100-preview.4.20258.7
  [Host]     : .NET Core 5.0.0 (CoreCLR 5.0.20.25106, CoreFX 5.0.20.25106), X64 RyuJIT DEBUG
  DefaultJob : .NET Core 5.0.0 (CoreCLR 5.0.20.25106, CoreFX 5.0.20.25106), X64 RyuJIT

Method	Mean	Error	StdDev	Ratio	Gen 0	Gen 1	Gen 2	Allocated
UsingMax	2,738.80 ns	21.445 ns	20.059 ns	1.00	1.1559	-	-	4848 B
UsingDateTimeCompare	98.42 ns	0.234 ns	0.207 ns	0.04	-	-	-	-

[forki]

wow - rewrite rule for Optimizer? ;-)

[cartermp]

I think the more appropriate general purpose fix is to get these generic functions to stop allocating on structs that aren't specialized in FSharp.Core

[KevinRansom]

@cartermp did you run the perftest with max using this fix?

[cartermp]

The test failures are from baseline comparisons against the emitted IL in the fsharpqa suite. How does babby update?

[smoothdeveloper]

@cartermp you should be able to use https://github.com/dotnet/fsharp/blob/master/TESTGUIDE.md#baselines after seeing the tests fail locally.

[KevinRansom]

Updated failing baselines

[TIHan]

I think the test changes are good, but I think we don't need to update FSharp.Core. We simply need to do devirtualization on equality and comparison and just emit the equals. Here was the PR when devirting equality, #6175. We could expand it to comparison.

This sort of thing needs to be expanded to all struct types...

[TIHan]

Seems I was a little too late as this was merged 2 minutes before submitting my review.

[cartermp]

Now I'm confused - @TIHan @KevinRansom what we discussed was that we would place these in FSharp.Core, which is why I made this adjustment rather than keeping them in the compiler. This was because it's an easy improvement with no apparent problems, whereas #6175 and extensions to it would be too tricky and time-consuming to land right now. Or am I mistaken in my recollection of that discussion?

[KevinRansom]

@cartermp , your recollection matches mine. I think we need a backlog item to make the change that will is thinking about. However, that requires time to design, bake and think through.
So your change is good, and I propose to leave it in.