Discussion: Opportunities for cleaner IL generation

[0x53A]

I have played around a bit with F# and decompilers, and in many cases F# generates complicated IL.

e.g.

this

let sample1() =
    use fs = File.Create("x.txt")
    fs.WriteByte(byte 1)

compiles to

.method public static 
    void sample1 () cil managed 
{
    // Method begins at RVA 0x2050
    // Code size 48 (0x30)
    .maxstack 4
    .locals init (
        [0] class [mscorlib]System.IO.FileStream fs,
        [1] class [FSharp.Core]Microsoft.FSharp.Core.Unit,
        [2] class [mscorlib]System.IDisposable
    )

    IL_0000: nop
    IL_0001: ldstr "x.txt"
    IL_0006: call class [mscorlib]System.IO.FileStream [mscorlib]System.IO.File::Create(string)
    IL_000b: stloc.0
    .try
    {
        IL_000c: ldloc.0
        IL_000d: ldc.i4.1
        IL_000e: callvirt instance void [mscorlib]System.IO.Stream::WriteByte(uint8)
        IL_0013: ldnull
        IL_0014: stloc.1
        IL_0015: leave.s IL_002d
    } // end .try
    finally
    {
        IL_0017: ldloc.0
        IL_0018: isinst [mscorlib]System.IDisposable
        IL_001d: stloc.2
        IL_001e: ldloc.2
        IL_001f: brfalse.s IL_002a

        IL_0021: ldloc.2
        IL_0022: callvirt instance void [mscorlib]System.IDisposable::Dispose()
        IL_0027: ldnull
        IL_0028: pop
        IL_0029: endfinally

        IL_002a: ldnull
        IL_002b: pop
        IL_002c: endfinally
    } // end handler

    IL_002d: ldloc.1
    IL_002e: pop
    IL_002f: ret
} // end of method Program::sample1

Edit:

Opportunities in this this simple program

• ldnull/pop pairs
• typecheck for IDisposable
• second endfinally in try-finally
• ldloc/pop @IL_002d

All these would reduce the size of the method from 0x2f (47) to less than 32 instructions.
32 instructions is the magic upper limit for inlining.

[asik]

Some ideas, speaking as an observer:

All of these could definitely be optimized away:

ldnull
pop

That's what the jit does so it has no performance impact, still looks strange in the IL though.

I don't understand the purpose of:

isinst [mscorlib]System.IDisposable

Since it's a compilation error to have a "use" binding with something that's not IDisposable. Candidate for removal too?

It's also unfortunate that 2 endfinally instructions are generated (along with their useless ldnull pop preludes), one extra for the false branch of the null check on fs, instead of converging both branches on the same instruction.

See also #635

[dsyme]

@0x53A @asik Yes, in short we would accept high quality, well-tested PRs to improve the IL generated if there's some evidence it improves perf.

That said, things like removing the extra "endfinally" is unlikely to improve perf in any mainline case we care about, and it's likely we would judge that the churn-risk associated with making the change may be too great. For example, a bug was injected into F# 4.0 because we accepted an optimization PR that looked ok at code review but actually applied the optimization in invalid situations. It's very expensive to hot-fix such bugs after they creep into shipping versions of F#.

If you do delve into this, I expect most changes would be in IlxGen.fs. There is a fair bit of apparatus there already to simplify generated IL.

I'll close this because we don't track IL improvement suggestions individually via github issues unless matched by a PR. Though we certainly welcome PRs! :)

Best
don

[dsyme]

Closing - see above comment

[0x53A]

@dsyme Ok thanks for the feedback.

I do think there would be some gain by clarified IL, but as you mentioned, that is more than offset by the risk.

If I find ( and am able to fix ) anything that affects performance in a noticeable way, I will send a pr.

[dsyme]

Great!

I'm converting this thread to what can be a long-running discussion about IL cleanliness (or we could use #635)

The tradeoff is also with the complexity of the change. Clean IL is definitely a virtue :) If you find simple ways to remove ldnull/pop pairs, for example, that would be good: Even if you isolate the paths in IlxGen.fs where the ldnull is being generated but not eliminated, that would be useful.

Also, ways to avoid the use of locals is very welcome since I believe some of the JIT optimizers use the number of locals in a target method as a decision about whether to inline or not. But locals in exception handling are less important - F# try/catch and try/finally have result values, unlike C#, so it's not to osurprising we use some extra locals for those.

[manofstick]

@dsyme

How about removing some of the "unbox.any" instructions? These appear quite costly if you are using interfaces a bit (In production code, when I have been optimizing inner loops, I have manually coded around it.)

I would submit a PR for this, but there is a comment in the GenCoerce function that unbox.any is important in "stack merge points" (which I must confess my ignorance about).

Anyway, here is an example of performance issue:

let empty (a:seq<'a>) =
    not ((a.GetEnumerator ()).MoveNext ())

[<EntryPoint>]
let main _ =
    let r = ResizeArray ()

    let sw = System.Diagnostics.Stopwatch.StartNew ()
    let s : seq<_> = upcast r
    let mutable counter = 0
    for i = 0 to 10000000 do
        if empty s then
            counter <- counter + 1
    System.Console.WriteLine ("cast to seq: {0} ({1})", sw.ElapsedMilliseconds, counter)

    let sw = System.Diagnostics.Stopwatch.StartNew ()
    let mutable counter = 0
    for i = 0 to 10000000 do
        if empty r then
            counter <- counter + 1
    System.Console.WriteLine ("as ResizeArray: {0} ({1})", sw.ElapsedMilliseconds, counter)

    0

With output of

cast to seq: 263 (10000001)
as ResizeArray: 1421 (10000001)

If I modify the fsharp compiler to remove the unbox.any then I get

cast to seq: 210 (10000001)
as ResizeArray: 209 (10000001)

This modified compiler passes all the standard .\appveyor-build.cmd tests, so I'm not sure what cases the comment is referring to, so some guidance would be appreciated.

[enricosada]

@manofstick that's a nice cleanup, with perf too, good work!.
Maybe it's better to open a pr to discuss and validate the changes? pr are free 😄 , just add ref to this issue in the pr, it's not more work than discuss there

from @dsyme comment:

I'll close this because we don't track IL improvement suggestions individually via github issues unless matched by a PR. Though we certainly welcome PRs! :)

i think that's a good requirement because a single issue (like this one) can become noise with multiple parallel discussion. Also the pr can show the code diff, can be commented and the discussion is separated from others, you can add your per test inside a the commit. and it's easy to clone a pr ( hub checkout https://github.com/Microsoft/visualfsharp/pull/872 )

[dsyme]

Closing old discussion, and the work to remove unbox.any was already merged