Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC FS-1006 Struct Tuples and Interop with C# 7.0 Tuples #1043

Merged
merged 45 commits into from Jul 27, 2016

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Apr 7, 2016

This is the WIP PR for F# RFC FS-1006 Struct Tuples and Interop with C# 7.0 Tuples

Please discuss the implementation on this thread, and the RFC and design on the RFC discussion thread.

@dsyme dsyme changed the title [WIP] Struct tuple spike [WIP] RFC FS-1006 Struct Tuples and Interop with C# 7.0 Tuples Apr 28, 2016
@dsyme
Copy link
Contributor Author

dsyme commented Apr 29, 2016

Current failures are all to do with the added FSHarp.Core surface area. This will disappear once we can use a System.ValueTuples.dll from .NET

[00:21:11] 1) Failed : FSharp.Core.Unittests.SurfaceArea.SurfaceAreaTest.VerifyArea
[00:21:11] 2) Failed : FSharp.Core.Unittests.Portable.SurfaceArea.SurfaceAreaTest.VerifyArea
[00:21:11] 3) Failed : FSharp.Core.Unittests.Portable.SurfaceArea.SurfaceAreaTest.VerifyArea
[00:21:11] 4) Failed : FSharp.Core.Unittests.Portable.SurfaceArea.SurfaceAreaTest.VerifyArea
[00:21:11] 5) Failed : FSharp.Core.Unittests.Portable.SurfaceArea.SurfaceAreaTest.VerifyArea

and remapTupInfoAux _tyenv unt =
match unt with
| TupInfo.Const _ -> unt
//| TupInfo.Var tp as unt ->
Copy link
Member

@TIHan TIHan May 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is all this commented out? Just curious! That's all :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove it. It was related to this possible extension

@TIHan
Copy link
Member

TIHan commented May 26, 2016

I just did a brief scan of everything. I do have a pet peeve when I see commented code without an explanation for why something was commented out. I put in some comments, I don't know if they are helpful. I'm no expert here, so there may be good reasons for everything and I am only trying to help when I can! Otherwise, everything seems to look fine on my end; I haven't tested anything though.

@vasily-kirichenko
Copy link
Contributor

Just a side note, I've tested struct tuples in a scenario where they were supposed to speedup returning multiple values from a function http://vaskir.blogspot.ru/2016/05/upcoming-f-struct-tuples-are-they.html
It turned out they makes no difference.

@TIHan
Copy link
Member

TIHan commented May 27, 2016

@vasily-kirichenko The current example isn't a good estimate, as if you compile the first non-struct tuple example in Release, it compiles to this:

[CompilationMapping(SourceConstructFlags.Module)]
public static class Program
{
  [EntryPoint]
  public static int main(string[] _arg1)
  {
    Stopwatch stopwatch = Stopwatch.StartNew();
    for (int index = 1; index < 100000001; ++index)
      Math.Sin((double) index / 1000.0);
    stopwatch.Stop();
    PrintfModule.PrintFormatLineToTextWriter<FSharpFunc<TimeSpan, Unit>>(Console.Out, (PrintfFormat<FSharpFunc<TimeSpan, Unit>, TextWriter, Unit, Unit>) new PrintfFormat<FSharpFunc<TimeSpan, Unit>, TextWriter, Unit, Unit, TimeSpan>("Run: %O")).Invoke(stopwatch.Elapsed);
    stopwatch.Restart();
    GC.Collect(2);
    stopwatch.Stop();
    PrintfModule.PrintFormatLineToTextWriter<FSharpFunc<TimeSpan, Unit>>(Console.Out, (PrintfFormat<FSharpFunc<TimeSpan, Unit>, TextWriter, Unit, Unit>) new PrintfFormat<FSharpFunc<TimeSpan, Unit>, TextWriter, Unit, Unit, TimeSpan>("GC.Collect: %O")).Invoke(stopwatch.Elapsed);
    Console.ReadKey();
    return 0;
  }
}

It doesn't even construct the tuples in the first place.

Try this:

open System
open System.Diagnostics

[<EntryPoint>]
let main _ = 
    let foo x = (x, Math.Sin x)

    let mutable v = (0., 0.)

    let sw = Stopwatch.StartNew()
    for n in 1..100000000 do
        v <- foo (float n / 1000.)
        ()
    sw.Stop()
    printfn "Run: %O" sw.Elapsed
    sw.Restart()
    GC.Collect 2
    sw.Stop()
    printfn "GC.Collect: %O" sw.Elapsed
    Console.ReadKey() |> ignore
    0

@vasily-kirichenko
Copy link
Contributor

@TIHan same time for your last snippet.

@TIHan
Copy link
Member

TIHan commented May 27, 2016

When I use the first example @vasily-kirichenko posted, the compiler optimizes out the tuples. I didn't use this branch though. If I use the snippet I posted, I get similar timings.

@TIHan
Copy link
Member

TIHan commented May 27, 2016

I also did this as a test:

open System
open System.Diagnostics

[<Struct>]
type StructTuple<'T1, 'T2> =

    val X : 'T1

    val Y : 'T2

    new (x, y) = { X = x; Y = y }

[<EntryPoint>]
let main _ = 
    let foo x = StructTuple (x, Math.Sin x)

    let mutable v = StructTuple (0., 0.)

    let sw = Stopwatch.StartNew()
    for n in 1..100000000 do
        v <- foo (float n / 1000.)
        ()
    sw.Stop()
    printfn "Run: %O" sw.Elapsed
    sw.Restart()
    GC.Collect 2
    sw.Stop()
    printfn "GC.Collect: %O" sw.Elapsed
    Console.ReadKey() |> ignore
    0

My timings were:

Run: 00:00:00.2693048
GC.Collect: 00:00:00.0001226

An order of magnitude faster.

I should just use this branch.

@vasily-kirichenko
Copy link
Contributor

I added results in release mode and added decompiled code as well (in the blog post).

@dsyme StructTuples are not erased in Release mode, but old Tuples are. Is it intentionally?

@dsyme
Copy link
Contributor Author

dsyme commented Jul 6, 2016

Awesome to see this passing green

KevinRansom and others added 2 commits July 6, 2016 16:05
…e FX_NOSTRUCTTUPLE now that dependency is reflection based.
Remove fsharp.core.dll references to System.ValueTuple.dll,
@dsyme dsyme changed the title [WIP] RFC FS-1006 Struct Tuples and Interop with C# 7.0 Tuples RFC FS-1006 Struct Tuples and Interop with C# 7.0 Tuples Jul 7, 2016
@dsyme
Copy link
Contributor Author

dsyme commented Jul 7, 2016

This looks very close to being ready. However I would expect we should wait until we are 100% certain of the final shape and form of the C# feature, and it has been committed and finalized. I'd hate to have F# commit and then C# change some design detail :)

@dsyme
Copy link
Contributor Author

dsyme commented Jul 18, 2016

This code is raising caught exceptions on every FSharp.Core load

#if FX_ASSEMBLYLOADBYSTRING
            let a = Assembly.Load("System.ValueTuple")
#else
            let a = Assembly.Load(new AssemblyName("System.ValueTuple"))
#endif 

at least if System.ValueTuple is not in the compiler directory by some chance. But in any case it seems a bit inefficient on every library startup

The root cause is that in reflect.fs, these should be computed on-demand rather than be top-level constants, so you only pay the cost once you start to use reflection

    let stuple1 = reflectedValueTupleType 1 [| typedefof<obj> |]
    let stuple2 = reflectedValueTupleType 2 [| typedefof<obj>; typedefof<obj> |]
    let stuple3 = reflectedValueTupleType 3 [| typedefof<obj>; typedefof<obj>; typedefof<obj> |]
    let stuple4 = reflectedValueTupleType 4 [| typedefof<obj>; typedefof<obj>; typedefof<obj>; typedefof<obj> |]
    let stuple5 = reflectedValueTupleType 5 [| typedefof<obj>; typedefof<obj>; typedefof<obj>; typedefof<obj>; typedefof<obj> |]
    let stuple6 = reflectedValueTupleType 6 [| typedefof<obj>; typedefof<obj>; typedefof<obj>; typedefof<obj>; typedefof<obj>; typedefof<obj> |]
    let stuple7 = reflectedValueTupleType 7 [| typedefof<obj>; typedefof<obj>; typedefof<obj>; typedefof<obj>; typedefof<obj>; typedefof<obj>; typedefof<obj> |]
    let stuple8 = reflectedValueTupleType 8 [| typedefof<obj>; typedefof<obj>; typedefof<obj>; typedefof<obj>; typedefof<obj>; typedefof<obj>; typedefof<obj>; deOption(reflectedValueTuple 0) |]

@KevinRansom KevinRansom merged commit fae4db4 into dotnet:master Jul 27, 2016
@KevinRansom
Copy link
Member

@dsyme Thanks for this important feature.

@cartermp
Copy link
Contributor

👍 👍 👍

🎉 🎉 🎉

😸 😸 😸

@KevinRansom
Copy link
Member

@dotnet-bot test this please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants