Fsharp: Have map/bind be inlined by default on Option/Result etc

[NinoFloris]

We were having "one of those chats" in FSSF slack about map and bind — and possibly others after consideration — not being inlined on Option/ValueOption/Result etc.

We came to the preliminary conclusion that there seem to be no strong reasons to have these not be marked inline.

AFAIK pros for inlining:

• They aren't expected to change ever.
• Helps Release stack traces be more focussed.
• Helps code to be faster and lower GC pressure.
• Reduce code bloat (inlining a match vs adding an FSharpFunc type for the binder/mapping etc).

For debug builds the experience may be confusing when you miss those calls in traces and debugging. Having non essential inlining, anything but SRTP, be a compiler switch (maybe already exists?) that's turned off during debug builds could be a decent solution.

Thoughts?

/cc: @dsyme

[dsyme]

Yes, these can be marked inlined. Debugging and profiling may actually improve though it's worth double checking.

[dsyme]

You could also look at the inliner heuristics to work out why these aren't being inlined by default, maybe there's something to tweak there

[abelbraaksma]

I remember I saw many of these functions being auto-inlined by the JIT. In which case performance benefits may not be as much as anticipated. But it's been a while that I checked this.

[dsyme]

@abelbraaksma Then the key thing is whether additional optimizations and reductions can be performed by the F# compiler. But nothing beats a benchmark for proof :)

[cartermp]

Just a basic benchmark, not sure if this is representative of actual workloads:

// Learn more about F# at http://fsharp.org

open BenchmarkDotNet.Running
open BenchmarkDotNet.Attributes

module InlinedOptions = 
    let inline bind binder option = match option with None -> None | Some x -> binder x
    let inline map mapping option = match option with None -> None | Some x -> Some (mapping x)

let binder (x: string) = Some (x.ToCharArray() |> Array.rev |> string)
let mapper (x: string) = x.ToCharArray() |> Array.rev |> string

let testNormalBind opts =
    opts |> Array.map (fun opt -> opt |> Option.bind binder) |> ignore

let testNormalMap opts =
    opts |> Array.map (fun opt -> opt |> Option.map mapper) |> ignore

let testInlineBind opts =
    opts |> Array.map (fun opt -> opt |> InlinedOptions.bind binder) |> ignore

let testInlineMap opts =
    opts |> Array.map (fun opt -> opt |> InlinedOptions.map mapper) |> ignore

[<MemoryDiagnoser>]
type Bench() =

    let opts = [| for x in 1..100 -> if x % 2 <> 0 then Some (string x) else None |]

    [<Benchmark>]
    member __.NormalBind() = testNormalBind opts

    [<Benchmark>]
    member __.NormalMap() = testNormalMap opts

    [<Benchmark>]
    member __.InlinedBind() = testInlineBind opts

    [<Benchmark>]
    member __.InlinedMap() = testInlineMap opts

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

Results:

BenchmarkDotNet=v0.11.5, OS=macOS Mojave 10.14.4 (18E226) [Darwin 18.5.0]
Intel Core i7-7700HQ CPU 2.80GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.0.100-preview3-010426
  [Host]     : .NET Core 3.0.0-preview3-27428-8 (CoreCLR 4.6.27422.72, CoreFX 4.7.19.12807), 64bit RyuJIT DEBUG
  DefaultJob : .NET Core 3.0.0-preview3-27428-8 (CoreCLR 4.6.27422.72, CoreFX 4.7.19.12807), 64bit RyuJIT

| Method | Mean | Error | StdDev | Median | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------ |---------:|----------:|----------:|---------:|--------:|------:|------:|----------:|
| NormalBind | 5.699 us | 0.1845 us | 0.5083 us | 5.593 us | 46.7224 | - | - | 7.45 KB |
| NormalMap | 5.222 us | 0.1163 us | 0.3223 us | 5.075 us | 46.6995 | - | - | 7.45 KB |
| InlinedBind | 4.336 us | 0.1524 us | 0.4347 us | 4.118 us | 31.9977 | - | - | 5.11 KB |
| InlinedMap | 4.103 us | 0.0901 us | 0.2435 us | 4.005 us | 31.9977 | - | - | 5.11 KB |

[NinoFloris]

Yes the JIT can't erase the FSharpFunc allocation away. It also won't inline that FSharpFunc body because Map/Bind are expecting the base type FSharpFunc.

Now the JIT is getting better and it may actually at some point first inline Map/Bind and after it's done that be like "oh hey this FSharpFunc is actually in a local with type Derived : FSharpFunc<int,int> ah that type is sealed, let's inline the body of Invoke too!" (which I asked for here https://github.com/Microsoft/visualfsharp/issues/3020)

For the allocation however the JIT's not there yet. They are working on escape analysis for stack allocation of objects but the timeline on that is unknown.

It may mean in the future the JIT will be able to erase everything iff inlining heuristics are deeming it favorable. But for now adding inline will get us there directly and without leaving things to chance. As a bonus it also reduces type and therefore assembly bloat because there's no need to actually emit any derived FSharpFunc types.