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 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
dsyme added 3 commits May 3, 2016
and remapTupInfoAux _tyenv unt =
match unt with
| TupInfo.Const _ -> unt
//| TupInfo.Var tp as unt ->

This comment has been minimized.

Copy link
@TIHan

TIHan May 26, 2016

Contributor

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

This comment has been minimized.

Copy link
@dsyme

dsyme May 28, 2016

Author Contributor

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

@@ -635,6 +674,20 @@ let rec stripTyEqnsA g canShortcut ty =

let stripTyEqns g ty = stripTyEqnsA g false ty

let rec stripTupInfoEqns aexpr =
match aexpr with
//| TupInfo.Var v ->

This comment has been minimized.

Copy link
@TIHan

TIHan May 26, 2016

Contributor

Similar to previous comment elsewhere. Just wondering why some things are commented out?

@@ -5089,8 +5169,13 @@ let rec tyOfExpr g e =
| TOp.ExnConstr _ -> g.exn_ty
| TOp.Bytes _ -> mkByteArrayTy g
| TOp.UInt16s _ -> mkArrayType g g.uint16_ty
<<<<<<< HEAD

This comment has been minimized.

Copy link
@TIHan

TIHan May 26, 2016

Contributor

I think a merge got screwed up here.

/// Some constant, e.g. true or false for tupInfo
| Const of bool

and [<RequireQualifiedAccess>] Measure =

This comment has been minimized.

Copy link
@TIHan

TIHan May 26, 2016

Contributor

This is much nicer than before! :)

@@ -296,45 +296,6 @@ namespace Microsoft.FSharp.Text.StructuredFormat
(let cases = FSharpType.GetUnionCases typ
cases.Length > 0 && equivHeadTypes (typedefof<list<_>>) cases.[0].DeclaringType)

module Type =

This comment has been minimized.

Copy link
@TIHan

TIHan May 26, 2016

Contributor

Why was all of this deleted? Only curious! I'm just ignorant.

This comment has been minimized.

Copy link
@dsyme

dsyme May 28, 2016

Author Contributor

Dead code

@TIHan
Copy link
Contributor

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

vasily-kirichenko commented May 27, 2016

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
Contributor

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

vasily-kirichenko commented May 27, 2016

@TIHan same time for your last snippet.

@TIHan
Copy link
Contributor

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
Contributor

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

vasily-kirichenko commented May 27, 2016

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 Jul 6, 2016
…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
2 of 4 checks passed
2 of 4 checks passed
Windows_NT Release_ci_part2 Build Build finished.
Details
continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
Windows_NT Debug Build Build finished.
Details
Windows_NT Release_ci_part1 Build Build finished.
Details
@KevinRansom
Copy link
Member

KevinRansom commented Jul 27, 2016

@dsyme Thanks for this important feature.

@cartermp
Copy link
Contributor

cartermp commented Jul 27, 2016

👍 👍 👍

🎉 🎉 🎉

😸 😸 😸

@KevinRansom
Copy link
Member

KevinRansom commented Jul 27, 2016

@dotnet-bot test this please

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

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.