[SPARK-44631][CONNECT][CORE] Remove session-based directory when the isolated session cache is evicted

What changes were proposed in this pull request?
This PR proposes to remove session-based directories when the isolated session is evicted from the cache.
Why are the changes needed?
SPARK-44078 added the cache for isolated sessions, and SPARK-44348 added the session-based directory for isolation.
When the isolated session cache is evicted, we should remove the session-based directory so it doesn't fail when the same session is used, see also https://github.com/apache/spark/pull/41625#discussion_r1251427466
Does this PR introduce any user-facing change?
No to end users since the feature has not been released yet.
How was this patch tested?
I manually tested as described in https://github.com/apache/spark/pull/41292. Especially, I reduced the TTL to few minutes, and tested as below at the last step:
spark.range(10).select(plug_one("id")).show()
spark.range(10).select(plug_one("id")).show()
# Wait few minutes
spark.range(10).select(plug_one("id")).show()

I verified that the same session can be added back to the cache, and creates the directory with the same name by reading executor's stderr at Spark UI.

cc @vicennial and @hvanhovell mind taking a look please?

Merged to master and branch-3.5.


Optimization: Generic specialization for equality

NB: This is not ready for merging yet (I need to find some more time; two cheeky monkeys consume more of my free time!) but it is intended to start a conversation. I will add more to this over time, assuming you are happy with the direction.
NB 2: There are some massive performance differences between .net 4.6 and lesser versions. I have been using 4.6 (and results specified here are from that version) but the performance optimizations are still applicable to older versions (I haven't gone to the assembly code level; but it appears to my ignorant eyes that older version were not inlining code that originated in fsharp.core.... or something :-) )
So what is this optimization?
As you are aware, F# automatically creates GetHashCode/Equals/Compare for Records and structs. In the case properties of concrete types, these are optimized to IL, but where you have generic parameters the functions are delegated to helper functions in LanguagePrimitives. For complex types, the performance of this is probably not a concern, but where you have like a simple struct 2 value key it can be a bit slow.
The following is an example (OK; it's a micro-benchmark; take with a grain of salt; blah-blah-blah; but I have come across things like this multiple times in the real world where you find an inner loop that is not too dissimilar):
open System
open System.Diagnostics
open System.Collections.Generic

type StructureInt =
    struct
        val A : int
        val B : int
        new(a, b) = {
            A = a
            B = b
        }
    end

type StructureGeneric<'a,'b> =
    struct
        val A : 'a
        val B : 'b
        new(a, b) = {
            A = a
            B = b
        }
    end

let mutable r = Random ()

let a createRandomObj =
    let set = HashSet ()
    let mutable count = 0

    let sw = Stopwatch.StartNew()

    for i = 0 to 200000 do
        set.Add (createRandomObj ()) |> ignore

        for i = 0 to 10 do
            count <- count + if set.Contains (createRandomObj ()) then 1 else 0

    printfn "%A (%A)" sw.ElapsedMilliseconds count

[<EntryPoint>]
let main argv =
    r <- Random 314159265
    for i = 0 to 10 do
        a (fun () -> StructureInt (r.Next 250, r.Next 250))

    printfn "-------"

    r <- Random 314159265
    for i = 0 to 10 do
        a (fun () -> StructureGeneric (r.Next 250, r.Next 250))

    0

The unmodified results for this are (first section is concrete type, the second part after the "------" is the generic type; the first number is the time in ms, the second number is just a checksum; first batch and second batch should match, but really it's just junk in these tests....) :
32-bit:
429L (1542136)
405L (1540851)
368L (1538895)
381L (1538510)
335L (1541558)
338L (1540543)
334L (1539466)
335L (1543310)
333L (1540047)
337L (1541890)
338L (1538828)
-------
1182L (1542136)
1160L (1540851)
1166L (1538895)
1177L (1538510)
1163L (1541558)
1167L (1540543)
1176L (1539466)
1162L (1543310)
1166L (1540047)
1182L (1541890)
1163L (1538828)

64-bit:
468L (1542136)
430L (1540851)
385L (1538895)
381L (1538510)
321L (1541558)
320L (1540543)
326L (1539466)
326L (1543310)
324L (1540047)
325L (1541890)
323L (1538828)
-------
800L (1542136)
786L (1540851)
780L (1538895)
792L (1538510)
788L (1541558)
773L (1540543)
780L (1539466)
784L (1543310)
776L (1540047)
784L (1541890)
788L (1538828)

Including the optimization I have trhe following results:
32-bit:
437L (1542136)
354L (1540851)
340L (1538895)
333L (1538510)
334L (1541558)
337L (1540543)
332L (1539466)
340L (1543310)
338L (1540047)
338L (1541890)
336L (1538828)
-------
615L (1542136)
603L (1540851)
602L (1538895)
618L (1538510)
613L (1541558)
609L (1540543)
618L (1539466)
612L (1543310)
610L (1540047)
619L (1541890)
611L (1538828)

64-bit:
589L (1542136)
411L (1540851)
324L (1538895)
323L (1538510)
321L (1541558)
325L (1540543)
321L (1539466)
321L (1543310)
320L (1540047)
323L (1541890)
321L (1538828)
-------
417L (1542136)
402L (1540851)
396L (1538895)
408L (1538510)
405L (1541558)
401L (1540543)
404L (1539466)
403L (1543310)
404L (1540047)
403L (1541890)
406L (1538828)

So to summarise; in the example of smashing a HashSet, times for both 32-bit and 64-bit have been halved (obviously it's even greater speed up than that, as that is including all the HashSet functionality).
Anyway; let me know if this is the kind of thing you are interested in. I have been running the smoketest and everything seems fine (but not really 100% happy as I haven't had time to explore the code around there). Anyway I need to head off to bed now, so hopefully I have left enough information here for you to evaluate!

Hi @manofstick, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@manofstick, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.
Thanks, MSBOT;

This looks pretty cool!  We are definitely interested in perf optimizations in the runtime.
I tried syncing this locally and running the tests, and everything seems to be working fine. GenericXIntrinsic, does not get inlined, so this should not affect user codegen?

@latkin ok, well let me finish this off; I need to also do some work for the Compare functions, as well as a bit more magic around IStructure* interfaces. I'll see how my time goes; but hopefully I'll get some peace over the weekend.
Oh, and a bit more exploring I see that the the equality stuff is all around the shop; so get benefits in things like list<int> even. An example of this is:
// create lots of lists to try not to get cache benefits
let lists =
    Array.init 1000000 (fun i ->
        List.init 10 (fun j ->
            if i % 2 = 0 then -j else j))

let sw = Stopwatch.StartNew ()
let r = Random 42
let mutable count = 0
for i=0 to 1000000 do
    let a = lists.[r.Next lists.Length]
    let b = lists.[r.Next lists.Length]
    count <- count + if a = b then 1 else 0

printfn "%A (%A)" sw.ElapsedMilliseconds count

64-bit original
810L (500464)
1015L (500464)
913L (500464)
902L (500464)
996L (500464)
753L (500464)
974L (500464)
762L (500464)
892L (500464)
1002L (500464)
760L (500464)

32-bit-original
1500L (500464)
1434L (500464)
1497L (500464)
1485L (500464)
1485L (500464)
1500L (500464)
1522L (500464)
1542L (500464)
1504L (500464)
1522L (500464)
1517L (500464)

64-bit modified
685L (500464)
449L (500464)
522L (500464)
431L (500464)
508L (500464)
441L (500464)
524L (500464)
420L (500464)
502L (500464)
440L (500464)
452L (500464)

32-bit-modified
743L (500464)
655L (500464)
615L (500464)
531L (500464)
485L (500464)
523L (500464)
506L (500464)
579L (500464)
530L (500464)
550L (500464)
549L (500464)

So pretty happy with that.

@dsyme sure no problem. I had only avoided it as I'm not particularly comfortable with the IL voodoo syntax; but obviously a copy and paste will do the trick...

Bit of wasted time over the weekend maybe trying to be a little bit too smart. I was trying to stop boxing of IStructuralEquality types, by using some classes dynamically created using the Reflection API, but it just didn't work. I think it is a reasonable idea, and do wonder what I did wrong, but I just don't have the time to investigate. If someone else does, then I would be appreciative. (I'm not checking it in, but the code is here; I will move on to other things that need to be done)
// Runtime template type specialization code
let getGenericSpecialializationTypeStaticFuncProperty<'typedef, 't, 'func> () =
    let t = typedefof<'typedef>.MakeGenericType [|typeof<'t>|]
    let func = t.GetProperty ("Func", Reflection.BindingFlags.Static ||| Reflection.BindingFlags.Public)
    let getter = func.GetGetMethod ()
    match getter.Invoke (null, [||]) with
    | :? 'func as f -> f
    | _ -> raise (Exception "Invalid logic")

type GenericSpecializeGetHashCode<'a when 'a : equality>() =
    static let _func = Func<_,_,_>(fun (_:IEqualityComparer) (a:'a) ->
        match box a with
        | null -> 0
        | _ -> a.GetHashCode ())
    static member Func = _func

let StructuralEquatableGetHashCode (iec:IEqualityComparer) (o:#IStructuralEquatable) =
    o.GetHashCode iec

type GenericSpecializeStructuralEquatableStructGetHashCode<'a when 'a : struct and 'a :> IStructuralEquatable>() =
    static let _func =
        Func<_,_,_>(fun (iec:IEqualityComparer) (a:'a) ->
            StructuralEquatableGetHashCode iec a)
    static member Func = _func

[<CustomEquality; NoComparison>]
type GenericSpecializeStructuralEquatableStructGetHashCodeDummy =
    struct
        interface IStructuralEquatable with
            member __.GetHashCode _ = raise (Exception "dummy class for typedefof")
            member __.Equals (_,_) = raise (Exception "dummy class for typedefof")
    end

type GenericSpecializeStructuralEquatableReferenceGetHashCode<'a when 'a :> IStructuralEquatable>() =
    static let _func =
        Func<_,_,_>(fun (iec:IEqualityComparer) (a:'a) ->
            match box a with
            | null -> 0
            | _ -> StructuralEquatableGetHashCode iec a)
    static member Func = _func

type GenericSpecializeHash<'a>() =
    static let generalize (func:Func<IEqualityComparer,'aa,int>) =
        match box func with
        | :? Func<IEqualityComparer,'a,int> as f -> f
        | _ -> raise (Exception "invalid logic")
    // this function must replicate GenericHashParamObj
    static let _func : Func<IEqualityComparer, 'a, int> =
        match typeof<'a> with
        | t when t.IsArray ->
            // I should optimize these as well, but probably not a particularly great gain.
            null
        | t when typeof<IStructuralEquatable>.IsAssignableFrom t ->
            if not t.IsValueType
                then getGenericSpecialializationTypeStaticFuncProperty<GenericSpecializeStructuralEquatableReferenceGetHashCode<_>, 'a, Func<IEqualityComparer, 'a, int>> ()
                else
                    // This throws a "System.NotSupportedException" from the GetProperty method of the generated type
                    // from getGenericSpecialializationTypeStaticFuncProperty. i.e. it appears that the MakeGenericType
                    // function works; because it doesn't throw an exception, which as I understand the documentation
                    // it should if the type doesn't meet the generic type constraints. So I return null, and to get
                    // fallback functionality, but would like someone else to investigate.
                    //
                    // getGenericSpecialializationTypeStaticFuncProperty<GenericSpecializeStructuralEquatableStructGetHashCode<GenericSpecializeStructuralEquatableStructGetHashCodeDummy>, 'a, Func<IEqualityComparer, 'a, int>> ()
                    //
                    null

        | t when t.IsSealed && not t.IsValueType ->
            getGenericSpecialializationTypeStaticFuncProperty<GenericSpecializeGetHashCode<_>, 'a, Func<IEqualityComparer, 'a, int>> ()

        // A general t.IsSealed for value types "should" work I think, but suffers from the same exception as above
        | t when t.Equals typeof<bool>       -> generalize (Func<_,_,_>(fun (_:IEqualityComparer) (a:bool)       -> a.GetHashCode()))
        | t when t.Equals typeof<sbyte>      -> generalize (Func<_,_,_>(fun (_:IEqualityComparer) (a:sbyte)      -> a.GetHashCode()))
        | t when t.Equals typeof<int16>      -> generalize (Func<_,_,_>(fun (_:IEqualityComparer) (a:int16)      -> a.GetHashCode()))
        | t when t.Equals typeof<int32>      -> generalize (Func<_,_,_>(fun (_:IEqualityComparer) (a:int32)      -> a.GetHashCode()))
        | t when t.Equals typeof<int64>      -> generalize (Func<_,_,_>(fun (_:IEqualityComparer) (a:int64)      -> a.GetHashCode()))
        | t when t.Equals typeof<byte>       -> generalize (Func<_,_,_>(fun (_:IEqualityComparer) (a:byte)       -> a.GetHashCode()))
        | t when t.Equals typeof<uint16>     -> generalize (Func<_,_,_>(fun (_:IEqualityComparer) (a:uint16)     -> a.GetHashCode()))
        | t when t.Equals typeof<uint32>     -> generalize (Func<_,_,_>(fun (_:IEqualityComparer) (a:uint32)     -> a.GetHashCode()))
        | t when t.Equals typeof<uint64>     -> generalize (Func<_,_,_>(fun (_:IEqualityComparer) (a:uint64)     -> a.GetHashCode()))
        | t when t.Equals typeof<nativeint>  -> generalize (Func<_,_,_>(fun (_:IEqualityComparer) (a:nativeint)  -> a.GetHashCode()))
        | t when t.Equals typeof<unativeint> -> generalize (Func<_,_,_>(fun (_:IEqualityComparer) (a:unativeint) -> a.GetHashCode()))
        | t when t.Equals typeof<char>       -> generalize (Func<_,_,_>(fun (_:IEqualityComparer) (a:char)       -> a.GetHashCode()))
        | t when t.Equals typeof<string>     -> generalize (Func<_,_,_>(fun (_:IEqualityComparer) (a:string)     -> a.GetHashCode()))
        | t when t.Equals typeof<decimal>    -> generalize (Func<_,_,_>(fun (_:IEqualityComparer) (a:decimal)    -> a.GetHashCode()))
        | t when t.Equals typeof<float>      -> generalize (Func<_,_,_>(fun (_:IEqualityComparer) (a:float)      -> a.GetHashCode()))
        | t when t.Equals typeof<float32>    -> generalize (Func<_,_,_>(fun (_:IEqualityComparer) (a:float32)    -> a.GetHashCode()))

        | _ -> null

    static member Func = _func

/// Intrinsic for calls to depth-unlimited structural hashing that were not optimized by static conditionals.
//
// NOTE: The compiler optimizer is aware of this function (see uses of generic_hash_inner_vref in opt.fs)
// and devirtualizes calls to it based on type "T".
let GenericHashIntrinsic x =
    match GenericSpecializeHash.Func with
    | null -> GenericHashParamObj fsEqualityComparerUnlimitedHashingPER (box x)
    | func -> func.Invoke (fsEqualityComparerUnlimitedHashingPER, x)

/// Intrinsic for calls to depth-limited structural hashing that were not optimized by static conditionals.
let LimitedGenericHashIntrinsic limit x =
    match GenericSpecializeHash.Func with
    | null -> GenericHashParamObj (CountLimitedHasherPER limit) (box x)
    | func -> func.Invoke ((CountLimitedHasherPER limit), x)

/// Intrinsic for a recursive call to structural hashing that was not optimized by static conditionals.
//
// "iec" is assumed to be either fsEqualityComparerUnlimitedHashingER, fsEqualityComparerUnlimitedHashingPER or
// a CountLimitedHasherPER.
//
// NOTE: The compiler optimizer is aware of this function (see uses of generic_hash_withc_inner_vref in opt.fs)
// and devirtualizes calls to it based on type "T".
let GenericHashWithComparerIntrinsic<'T> (iec : System.Collections.IEqualityComparer) (x : 'T) : int =
    match GenericSpecializeHash.Func with
    | null -> GenericHashParamObj iec (box x)
    | func -> func.Invoke (iec, x)

Oh, and if anyone does (did?) get around to having a look at the code that I couldn't get working (as per last comment in this thread), I should have also added thatthe Type created worked fine in an isolated test project - it was only when it was going into FSharp.Core that the problem with it surfaced. Anyways; not the end of the world; but might be some fun for someone to look at if they are bored...

@latkin, @dsyme, I assume I'll not allowed to break existing functionality (damn!) But wondering if they following is OK for the genetic comparer:-
first of all I want in preference to use the generic IComparable<> interface, but that is unmentioned in existing code as far as I can see.
Now this is where I would like to make an assumption which is trying to check if the object had an f# compiler generated struct or record thus with a compiler provided implementation of istructualcomparable, icomparable and genetic version. If this is true, then I have a sealed type which have equivalent versions of comparison so it shouldn't matter which version I take (this I'd only for top level, so no comparer is being passed through the istructualcomparable call)
A second case would be where only the genetic version of the interface exists on an object (this was true for the nodatime objects I believe - although I think this meant that the f# compiler didn't believe they were comparable from memory - but aiming this is not the case)
(Obviously I would preferable just like to take the generic icomparable<> always, but I assume this is unacceptable due to it possible changing someone's probably buggy runtime code...)

Yes, for now I think that's a great approach.
It's possible that we would ultimately be willing to take a change along the lines of "As of F# V.v and FSharp.Core X.X.X.X, the generic comparison logic will compare types defined in freshly compiled code which implement IComparable<T> or IStructuralComparable<T> via the generic interface rather then the non-generic interface IComparable".
If we backed that up by performance figures showing the reduction in boxing then it could well seem very acceptable.  However that should definitely be a separate PR to the core work you're doing to improve performance without changing the preference order for comparison.

Some more performance details...
In Rounding out Visual F# 4.0 in VS 2015 RC there is a Tortoise (misspelled!) example under the "Optimized non-structural comparison operators" section. Ignoring the Hare, and just running the Tortoise on FSharp.Core.dll changes:
Code
module Tortoise =
    let test () =
        let today = DateTime.Now
        let tomorrow = today.AddDays 1.0
        let mutable result = 0
        for i = 1 to 10000000 do
            result <- result + if today = tomorrow then 1 else 0

Results
32 bit - original
Real: 00:00:01.143, CPU: 00:00:01.138, GC gen0: 200, gen1: 2, gen2: 0

32 bit - modified
Real: 00:00:00.403, CPU: 00:00:00.405, GC gen0: 0, gen1: 0, gen2: 0

64 bit - original
Real: 00:00:00.690, CPU: 00:00:00.686, GC gen0: 305, gen1: 0, gen2: 0

64 bit - modified
Real: 00:00:00.193, CPU: 00:00:00.202, GC gen0: 0, gen1: 0, gen2: 0

@manofstick Those performance results are fantastic - specially the complete elimination of GC.  Still not quite the Hare (so I'm glad we added NonStructuralCommparison), but the changes definitely make the default cases much faster.

@dsyme ,
Here is some badness for you (in current FSharp.Core), my implementation currently doesn't match this, but I will "fix" it so that it does (I'm currently matching the non-generic version in the generic version).
Badness
type A = { A : string }
type B<'a> = { B : 'a }

[<EntryPoint>]
let main argv =
    let a1, a2 = { A = "Hello"}, { A = "HELLO" }
    let b1, b2 = { B = "Hello"}, { B = "HELLO" }

    let compareStringWithUpper=
        let toupper (s:string) = s.ToUpper ()
        { new IEqualityComparer with
            member this.GetHashCode item =
                match item with
                | :? string as s -> (toupper s).GetHashCode ()
                | _ -> failwith "Not in this example..."
            member this.Equals (lhs, rhs) =
                match lhs, rhs with
                | (:? string as s1), (:? string as s2) -> (toupper s1).Equals(toupper s2)
                | _ -> failwith "Not in this example..." }

    let a_is_good = (a1 :> IStructuralEquatable).Equals(a2, compareStringWithUpper)
    let b_is_good = (b1 :> IStructuralEquatable).Equals(b2, compareStringWithUpper)

    printfn "A is %s" <| if a_is_good then "Good" else "Bad"
    printfn "B is %s" <| if b_is_good then "Good" else "Bad"

    if a_is_good <> b_is_good then
        printfn "But worse than those results are that they are inconsistent!"

With the output
A is Bad
B is Good
But worse than those results are that they are inconsistent!

So "at some stage"; I think the implementation of IStructuralEquatable.Equals shouldn't used inlined IL, but rather always defer back to supplied IEqualityComparer. Obviously this will make performance much worse, but after this PR is complete, hopefully it won't be too onerous.

These results are really impressive, unlike my ability to spell tortoise. :blush: (now fixed)

@latkin ; @dsyme
A few of questions:

Where should items like the existing FSharp.Core weirdness go? It does't really affect this case; I'll just try and duplicate the weirdness; I'm happy for you to store it on your own internal backlog; I obviously not in a position to determine a change in existing functionality...
I have some other ideas of some optimizations around the Seq module (and who knows where my path takes me...). Do you want you all optimizations just lumped into this pull request; or shall I isolate them? How small/large should they be? (i.e. I probably could have just split this task into 3 - hash, equals & compare - would that have been better? - still possible I guess...)
Because I couldn't get the generic based static class code method to work (for unknown reason), I'm using some dynamic code generation techniques (emit via linq expressions). Are you happy (well...) with that? If not, I'm not sure what else can be done...
 I'm just liberally throwing #if FX_ATLEAST_40 around the shop.

...that'll do for now...

I have tried to follow the "first do no harm" principle as much as possible (although there will be a slight cost of construction; but minimal) but where I have failed is actually where I wanted to succeed the most; with value types. Sigh.
Now this is not a new problem (I have an email conversation dated 15/6/2013 to Don about it) but I have made it a bit worse. The fact that it was already a problem though may mean that it can be ignored?
So what is this problem? It's value types that are > 64 bits, tailcalls and the 64-bit JIT.
So some code:
open System
open System.Collections.Generic
open System.Diagnostics

type TheStruct =
    struct
        val A : int64
#if BIG
        val B : int64
        new (a, b) = { A=a; B=b }
#else
        new (a) = { A=a }
#endif
    end

type Container<'a> =
    struct
        val Item : 'a
        new (i) = { Item = i }
    end

let runTest createRandomObj =
    let set = HashSet ()
    let mutable count = 0
    let sw = Stopwatch.StartNew()
    for i = 0 to 200000 do
        set.Add (createRandomObj ()) |> ignore
        for i = 0 to 10 do
            count <- count + if set.Contains (createRandomObj ()) then 1 else 0

    printfn "    %A (%A)" sw.ElapsedMilliseconds count

[<EntryPoint>]
let main argv =
    let create =
        let r = Random 314159265
        let n x = int64 (r.Next x)
        fun () ->
#if BIG
            Container (TheStruct (n 16, n 16))
#else
            Container (TheStruct (n 256))
#endif

    for i = 1 to 10 do
        runTest create
    0

Now if this runs without BIG set, then the following times are obtained (results are in the format "time (hit count - which should be ~ equal for similar timing)":
513L (2197320)
235L (2197168)
241L (2197198)
231L (2197114)
166L (2197153)
168L (2197158)
166L (2197361)
168L (2197090)
167L (2197199)
167L (2197400)

But with BIG set, I get the following ~20 times slower (it should be a little slower, as we have two fields now rather than 1, but nowhere near this...):
4232L (2197116)
4058L (2197293)
4075L (2197057)
4055L (2197259)
4057L (2197364)
4067L (2197354)
4060L (2197468)
4067L (2197161)
4066L (2196999)
4058L (2197368)

Now if I turn "generate tail calls" off in this app I get:
3363L (2197116)
2854L (2197293)
2870L (2197057)
2891L (2197259)
2863L (2197364)
2858L (2197354)
2880L (2197468)
2912L (2197161)
2862L (2196999)
2934L (2197368)

BUT, if I also do a build of FSharp.Core with "generate tail calls" off then I get:
430L (2197116)
243L (2197293)
258L (2197057)
241L (2197259)
237L (2197364)
237L (2197354)
237L (2197468)
238L (2197161)
237L (2196999)
237L (2197368)

Which is actually the result that I desire.
Now I can force this by adding a "not(not(equals))" and a "((hashcode)* -1)* -1), which is what I might do following this comment, but it is hardly desirable. So what do you think of maybe adding an attribute such as [] or something like that so methods can selectively be optimised?

@manofstick Re your questions here

the inconsistency in the IStructuralEquatable.Equals implementation is not, AFAIK, intentional. It will be tricky to fix without amore direct repro - but I think you may have provided one of this in a separate issue report (assuming it's the same underlying issue).  Even then it may be hard for us to fix the issue for stability reasons but I still appreciate your reporting it.

You should definitely isolate optimizations into separate PRs where possible

3+4. Using some code generation here is OK for the .NET 4.x profiles.  Very few other platforms or PCLs will have codegen abilities I suppose. Using #if FX_ATLEAST_40  is OK for now.  We can isolate a specific new define later.

@manofstick - avoiding the tailcall looks reasonable if that's what the old code did and it affects perf. I don't mind using a hack for the purposes of preparing this PR (which feels still a bit of distance from finalized) but we should find a better solution.  If you can abstract it into a (private) avoidTailcall function then that would be helpful as we can test that separately.

@dsyme
RE: IStructuralEquatable.Equals, not sure if I had created a proper issue, so it is now created as #527.
RE: About this PR being finalised; yeah I still have to attack Compare properly. But I'm starting to feel quite burnt out between work, kids and trying to get this over the line, so I might need a little break for a while. But, I think that it should be pretty good for GetHashCode and Equals in its current state.
(And when I finally do finish this PR, I have 5 more ideas that I want to try out - I think I need a clone :-)

Just a data point to consider - the increase in generic specialization added by this change leads to fairly significant growth in the size of FSharp.Core native images (and working set of client apps after JIT).

FSharp.Core.ni.dll
Before
After

32 bit
6.64MB
7.32 MB (+10.2%)

64 bit
8.77 MB
9.72 MB (+10.7%)

@latkin
That's great! I'll be able to claim that I wrote 10% of FSharp.Core :-)
Hmmm... But anyway!
What tools are there to analyse the native images? 1Mb of GetHashCodes/Equals sounds like it's gone a bit haywire?
I can try and play around a bit with how the code is laid out; I have more static classes than I need; possibly clumping them might be better?
But, besides running "ngen" once or twice, I'm not familiar at all with native images. I'm not even sure where they live. So any assistance in analysis would be helpful.

@manofstick just run src\update.cmd <release|debug> -ngen from an admin prompt to refresh the GAC/NGEN with your latest-built stuff.  Native images are stored as FSharp.Core.ni.dll under C:\windows\assembly\..., your private-built guys will be the copies with the newest timestamps.
There is nothing haywire, the increase in native code size is expected, which is why I wanted to see the numbers.  The managed assembly has just one generic copy of the code, but at the native code level, JIT or AOT compilers need to produce separate native copies for every generic instantiation (ref types share 1 copy, at least). http://joeduffyblog.com/2011/10/23/on-generics-and-some-of-the-associated-overheads/ is a pretty good description.

@latkin
OK; I've clawed back about a quarter of the space used, when I get the some more time I'll claw back some more.
My previous issue in regards to using reflection on types created in FSharp.Core appears worse that I originally thought, as it also appears to be an issue with non-generic types with generic methods. I didn't actually try just straight non-generic types with non-generic methods; but I probably should - hopefully time will allow at some stage. Weird anyway.

@latkin
OK, some good news; although I can't get GetMethod to work on classes as per my previous whingeing, I found that Delegate.CreateDelegate has an overload which takes a string method name (and somehow that manages to work), and so I have been able to leverage off that to achieve what I wanted to do from the start. This means that I can deprecate the Linq Expression compiled code.
BUT! The portable47 profile doesn't have this overload, which is a bit of a bummer. I have just used the  FX_ATLEAST_40 compilation symbol to avoid it. What I'm possibly thinking is that maybe I just wrap the whole changes in a conditional compile, and for the portable profiles we just return the slow, original, methods. Not fantastic; but maintaining the linq expression version as well as this version just seems to be a lot of pain for not much gain.
Thoughts?

@latkin
Getting closer, I now just need to focus on Compare and then adding a good test suite. I haven't looked at how your tests are structured yet. Any hints as to how to approach?
(Oh, and the "new" methodology using the Delegate.CreateDelegate has had vast improvements with the 32 bit code. In some of my little test runs in has halved the time, up to and beating the 64 bit impl in some cases.)

@manofstick thanks for your continued hard work on this, it will definitely have a big impact on F# perf.
Here are my thoughts at this point:

This is becoming a fairly large, deep change to a critical part of the runtime. It's about to the level of a "feature" as opposed to small perf optimization.  I would love to see a short speclet that summarizes the goals of the new design, how it was implemented, pros/cons of this design, and any notable gotchas/pitfalls to be aware of. That will help clarify how best to review and test this, and will serve as a reference for future fixes/changes in this code. A basic markdown doc on gist would be great.

We should have a set of tests that demonstrate the perf benefit and prove the correctness/compat of the changes.  Today, I see tests covering this basic area in a few places:

Various tests for structural comparison and comparison codegen scattered through the fsharpqa suite
Decent set of comparison tests covering various classes of types (primitives, generics, DUs/Records, structs, tuples, etc) at tests/fsharp/core/libtest
Some targeted (but limited) testing of primtypes.fs comparison operations at src\fsharp\fsharp.core.unittests\fsharp.core\PrimTypes.fs

Altogether I feel like we have "acceptable" but not "comprehensive" functional validation here. None of the existing tests is suitable for perf verification, though.
I would propose filling in the unit tests with more comprehensive coverage wrt all classes of types, and designing these tests so that they can demonstrate perf impact, not just correctness. e.g. off the top of my head perhaps running a single iteration by default, but many thousands (with a timing report) if some environment variable is set.  The runtime unit tests seem like the appropriate suite, given that these are runtime APIs. This is the easiest way to get coverage on the portable profiles, too, which is clearly important to cover per your description of various workarounds needed.

I know nothing other than scanning through blog posts, but fscheck could possibly prove quite useful in the testing of this functionality? I see that @forki in #502 has started this effort for library functions. Thoughts?

@latkin
OK, well I think I'm feature complete now. Need to do a few passes of cleanup, and obviously the tests and the docs, but I think I'm covering all the cases that I want, in the way I want (any potential bugs aside...)
I've have tried to get my footprint down, so I think it is about ~4% now - lot better than the ~10% it was. I did a quick and dirty hack through the other existing "Tables" types in prim-types.fs using the same technique (i.e. moving the generation code out into helper functions) and I think that yielded a saving of ~ 1%, so that's a pretty simple option if you do want to shrink the existing footprint (but more care than I took with my dirty run probably... Still... should only take a couple of hours...)

Another test, from some code in old posting of mine from StackOverflow ... I've just done before and after runs and then got the % of time. Only from a single run for each; the numbers should be taken with a grain of salt, but the general trend should be fine.
| 64 bit create | read | 32 bit create | read
------------ | ------------- | ------------- | ------------- | -------------
Dictionary KeyRecord | 108.9% | 101.2% | 97.2% | 101.8%
Dictionary KeyGenericRecord | 87.0% | 73.3% | 72.5% | 59.2%
Dictionary KeyStruct | 86.0% | 99.2% | 107.4% | 99.8%
Dictionary KeyGenericStruct | 84.9% | 85.6% | 66.0% | 47.8%
Dictionary Tuple | 99.2% | 98.1% | 97.9% | 98.2%
dict KeyRecord | 99.5% | 96.4% | 81.2% | 67.1%
dict KeyGenericRecord | 79.4% | 80.6% | 63.8% | 41.5%
dict KeyStruct | 70.0% | 70.8% | 79.8% | 71.5%
dict KeyGenericStruct | 69.3% | 69.8% | 60.2% | 48.2%
dict Tuple | 98.5% | 97.7% | 86.0% | 80.1%
Map KeyRecord | 92.9% | 82.2% | 67.6% | 39.2%
Map KeyGenericRecord | 77.3% | 57.1% | 45.8% | 25.5%
Map KeyStruct | 30.6% | 11.1% | 56.9% | 28.3%
Map KeyGenericStruct | 28.6% | 13.2% | 39.8% | 20.5%
Map Tuple | 302.3% | 601.2% | 100.0% | 90.8%
So I've obviously done something bad for using a Tuple as a key for a map (maybe there is a specific optimization in existing code?) but some reasonable results, but I still have some plans for dict...

In this last set of checkins, the specialty equality/comparers for Tuples, I would say was moderately successful. With the comparer, it halved the time of the previous test for use in Map<> , but with equality the results were much more modest - in the order of 10% ish (well it did halve it when I changed the GetHashCode to match the F# generated on for records/structs but that has 2 problems, first it is only really applicable in specific data cases and secondly it cause existing test cases to break)
And as well as doing that it cause the ngen'd files to blow out a bit. Admittedly I was doing a bit of overkill here, supporting all tuple types for 1 to 8 args, so a more modest approach may still be suitable.
So being somewhat realistic, I think I should remove the equality ones, and cut the comparer ones down to ones that take between 2 and (4 or 5) args.

@latkin,
I've had some new thoughts as hope I'm attacking this problem, so will attempt to get a start on those shortly, but in the meantime I have put in a separate #549

@dsyme, @latkin,
OK; My madness is slowly becoming reality so that's good (maybe!). I would suggest that you don't look at the code at the moment, as it isn't the cleanest that it could be!
Anyway, I'm must say I'm very happy with the way things are shaping up.
First some results, and then I'll give a brief overview of what I'm doing. I'm unsure if you'll label it brilliant or horrible!
So the follow results are from almost the same test suite as was included in 549 except that I have substituted Tuples for the Value Type (so as not to have the issue that was associated with that pull request.) I have included the modified code at the end of this message.

Test
x86_old
x86_new
x64_old
x64_new

x86_new
x64_new

seqGroupBy
2969
1088
1788
1062

36.65%
59.40%

seqCountBy
4950
1031
2595
974

20.83%
37.53%

listGroupBy
2808
565
1615
585

20.12%
36.22%

listCountBy
4665
561
2368
619

12.03%
26.14%

arrayCountBy
4411
484
2077
482

10.97%
23.21%

arrayGroupBy
2357
337
1227
327

14.30%
26.65%

dict
1737
630
1314
778

36.27%
59.21%

Now the test suite isn't particularly realistic, as it is creating tuples with 4 and 5 arguments (as well as 2 and 3), but it does show dramatic performance improvements are achieved for Tuples in containers with the generic HashIdentity.Structural that are unknown at compile time.
So this achieved through Clayton's Code Gen - the code gen you do when you're not doing code gen (a reference that probably only Don will get). So I'll give a full write up on this in a design doc (misnomer really, maybe "code map doc" is a better term) at some stage, but the key to it is that I'm using the fact that Value Types when you as arguments to generic classes they are expanded and compiled.
With this I have templates for Tuples, and for IEqualityComparer that are populated with the underlying components, in the same way that the F# compiler does for Tuples that are known at compile time.
The code
module Program

open System.Diagnostics

let seqTest f n createKey =
    let sw = Stopwatch.StartNew ()
    let count =
        Seq.init n id
        |> f createKey
        |> Seq.length
    count, sw.ElapsedMilliseconds

let listTest f n createKey =
    let sw = Stopwatch.StartNew ()
    let count =
        List.init n id
        |> f createKey
        |> List.length
    count, sw.ElapsedMilliseconds

let arrayTest f n createKey =
    let sw = Stopwatch.StartNew ()
    let count =
        Array.init n id
        |> f createKey
        |> Array.length
    count, sw.ElapsedMilliseconds

let dictTest n createKey =
    let sw = Stopwatch.StartNew ()
    let count =
        Array.init n (fun n -> createKey n, n)
        |> dict
        |> fun d -> d.Count
    count, sw.ElapsedMilliseconds

let createKey2 n = (n%11,n%7)
let createKey3 n = (n%11,n%7,n%5)
let createKey4 n = (n%11,n%7,n%5,n%3)
let createKey5 n = (n%11,n%7,n%5,n%3,n%2)
let createDictKey2 n = (n,n)
let createDictKey3 n = (n,n,n)
let createDictKey4 n = (n,n,n,n)
let createDictKey5 n = (n,n,n,n,n)

let runSeqGroupByTest () =
    let size = 100000
    let mutable totalTime = 0L
    for i = 1 to 5 do
        let c2, t2 = seqTest Seq.groupBy size createKey2
        let c3, t3 = seqTest Seq.groupBy size createKey3
        let c4, t4 = seqTest Seq.groupBy size createKey4
        let c5, t5 = seqTest Seq.groupBy size createKey5
        totalTime <- totalTime + t2 + t3 + t4 + t5
        printfn "seqGroupBy %d,%d,%d,%d (%d,%d,%d,%d)" t2 t3 t4 t5 c2 c3 c4 c5
    printfn "seqGroupBy totalTime=%d" totalTime

let runSeqCountByTest () =
    let size = 100000
    let mutable totalTime = 0L
    for i = 1 to 5 do
        let c2, t2 = seqTest Seq.countBy size createKey2
        let c3, t3 = seqTest Seq.countBy size createKey3
        let c4, t4 = seqTest Seq.countBy size createKey4
        let c5, t5 = seqTest Seq.countBy size createKey5
        totalTime <- totalTime + t2 + t3 + t4 + t5
        printfn "seqCountBy %d,%d,%d,%d (%d,%d,%d,%d)" t2 t3 t4 t5 c2 c3 c4 c5
    printfn "seqCountBy totalTime=%d" totalTime

let runListGroupByTest () =
    let size = 100000
    let mutable totalTime = 0L
    for i = 1 to 5 do
        let c2, t2 = listTest List.groupBy size createKey2
        let c3, t3 = listTest List.groupBy size createKey3
        let c4, t4 = listTest List.groupBy size createKey4
        let c5, t5 = listTest List.groupBy size createKey5
        totalTime <- totalTime + t2 + t3 + t4 + t5
        printfn "listGroupBy %d,%d,%d,%d (%d,%d,%d,%d)" t2 t3 t4 t5 c2 c3 c4 c5
    printfn "listGroupBy totalTime=%d" totalTime

let runListCountByTest () =
    let size = 100000
    let mutable totalTime = 0L
    for i = 1 to 5 do
        let c2, t2 = listTest List.countBy size createKey2
        let c3, t3 = listTest List.countBy size createKey3
        let c4, t4 = listTest List.countBy size createKey4
        let c5, t5 = listTest List.countBy size createKey5
        totalTime <- totalTime + t2 + t3 + t4 + t5
        printfn "listCountBy %d,%d,%d,%d (%d,%d,%d,%d)" t2 t3 t4 t5 c2 c3 c4 c5
    printfn "listCountBy totalTime=%d" totalTime

let runArrayGroupByTest () =
    let size = 100000
    let mutable totalTime = 0L
    for i = 1 to 5 do
        let c2, t2 = arrayTest Array.groupBy size createKey2
        let c3, t3 = arrayTest Array.groupBy size createKey3
        let c4, t4 = arrayTest Array.groupBy size createKey4
        let c5, t5 = arrayTest Array.groupBy size createKey5
        totalTime <- totalTime + t2 + t3 + t4 + t5
        printfn "arrayGroupBy %d,%d,%d,%d (%d,%d,%d,%d)" t2 t3 t4 t5 c2 c3 c4 c5
    printfn "arrayGroupBy totalTime=%d" totalTime

let runArrayCountByTest () =
    let size = 100000
    let mutable totalTime = 0L
    for i = 1 to 5 do
        let c2, t2 = arrayTest Array.countBy size createKey2
        let c3, t3 = arrayTest Array.countBy size createKey3
        let c4, t4 = arrayTest Array.countBy size createKey4
        let c5, t5 = arrayTest Array.countBy size createKey5
        totalTime <- totalTime + t2 + t3 + t4 + t5
        printfn "arrayCountBy %d,%d,%d,%d (%d,%d,%d,%d)" t2 t3 t4 t5 c2 c3 c4 c5
    printfn "arrayCountBy totalTime=%d" totalTime

let runDictTest () =
    let size = 100000
    let mutable totalTime = 0L
    for i = 1 to 5 do
        let c2, t2 = dictTest size createDictKey2
        let c3, t3 = dictTest size createDictKey3
        let c4, t4 = dictTest size createDictKey4
        let c5, t5 = dictTest size createDictKey5
        totalTime <- totalTime + t2 + t3 + t4 + t5
        printfn "arrayCountBy %d,%d,%d,%d (%d,%d,%d,%d)" t2 t3 t4 t5 c2 c3 c4 c5
    printfn "arrayCountBy totalTime=%d" totalTime

[<EntryPoint>]
let main _ =
    runSeqGroupByTest()
    runSeqCountByTest()
    runListGroupByTest()
    runListCountByTest()
    runArrayCountByTest()
    runArrayGroupByTest()
    runDictTest ()
    0

@latkin, @dsyme
Rerunning the performance tests and some are slightly worse, but nothing particularly bad. But on the positive side, all the 32bit times are vastly better now (basically the same as previous 64bit numbers, sometimes better), and the Tortoise.test() times have better than halved!
Well I'm excited anyway :-)

@latkin, @dsyme
Oh I'm bombarding you with messages this weekend aren't I? As I said, it's because I'm excited :-)
Anyway, this one is to say that we now have a perverse situation where the compiler generated code is slower than the dynamically generated code :-) [not by much, but still...]
So given the test, we get the results

check
x32_old
x32_new
x64_old
x64_new

NonStructural
153
153
110
102

Structural static
690
193
408
166

Structural dynamic
812
180
399
160

So in the 32-bit world, the % diff between dynamically generated Structural comparers and the NonStructural is quite small. The 64-bit world they still have a bit of an edge, although for any algorithm more complex than a simple count then this is really going to just get drowned out...
The comparers were created as follows:
let dynamicallyGenerated<'key when 'key : comparison> () =
    match HashIdentity.Structural<'key> with
    | :? IEqualityComparer<DateTime> as ec -> ec
    | _ -> failwith "unexpected"

let staticallyGeneratedNonStructural = HashIdentity.NonStructural<DateTime>
let staticallyGeneratedStructural    = HashIdentity.Structural<DateTime>
let dynamicallyGeneratedStructural   = dynamicallyGenerated<DateTime> ()

@latkin,
Now that I'm just using interfaces, I don't need my Delegate.CreateDelegate, so I can reintroduce the code to other profiles. I have done this, but I still have stubbed out hasFSharpCompilerGeneratedEquality and hasFSharpCompilerGeneratedComparison which used reflection stuff that I couldn't easily find out how to used in profile47.
Are you an expert on such stuff? or know who would be? I don't even know what profile47 means!

@manofstick What APIs in particular on profile 47 are missing? reflect.fs might be useful to study wrt reflection workarounds.
The delta is now almost 2 KLOC, I am wondering if you will tell us that it's done or if you'd prefer we tell you when it's done :laughing:
Any progress on putting together a set of tests to validate this authoritatively?

@latkin
Well I had though a week or so back that I was feature complete, but then I realised I needed a tuples solution. Had to head back to my quiver to slay that dragon. Unfortunately the arrow required didn't fit the bow, so I had to do a bit of whittling to get that right.
The bad news is that I'm back to the code bloat that I had when you first looked.
The good news is that I'm now dealing with tuples in a way that makes me happy.
So hopefully I am now feature complete, and after a couple of passes of clean up I'll get onto some docs. And after the docs I'll get onto the tests.

@latkin, @dsyme
Well things are looking up. It's pretty much complete from a code perspective, although now we have a few contentious items to deal with.

Delete the statically created compares that area made directly with  MakeGenericComparer/MakeGenericEqualityComparer. I think they should go as the new dynamic ones have basically the same performance profile (a little testing required)
Add a new HashIdentity type. The problem with Structural is that is PER, which is just a poor choice  for a number of reasons. First is that it provides different semantics from System.Collections.Generic.EqualityComparer.Default. Second is that using it as a key then you can put nan in a collection which can never be retrieved. And third, the most important to me, is that it stops an optimization I could make with ER semantics (i.e. for compiler generated types, the IEquatable<>.Equals is implemented with ER semantics so it can be called in that case, otherwise I need to drop back to the possibly boxing IStructuralEquality - with this optimization in place, the generated IEqualityComparers have basically the same performance profile as the EqualityComparer.Default ones, meaning that they become the no brainer choice...)

Thoughts?

Oh this does feel a bit like writing a diary; mostly it is just for myself :-)
Anyway, I reran this with my recent optimizations, and it's good.
The difference between the NonStructural and the dynamic are now almost non-existant. < 5% longer for 32-bit, and < 10% longer for 64-bit.
Hooray!

And continuing on from the last comment, I reran the tortoise test:
64-bit
Real: 00:00:00.054, CPU: 00:00:00.046, GC gen0: 0, gen1: 0, gen2: 0

32-bit
Real: 00:00:00.185, CPU: 00:00:00.187, GC gen0: 0, gen1: 0, gen2: 0

So for 32-bit, that's less than half of originally reported time, and about a quarter of the time for 64-bit.

@latkin, @dsyme
OK; well I think we're basically code complete.
Probably a few tweaks here and there. Also still have to think about those questions previously posed
And then some info for you; any idea how the wiki works here on github? i.e. looks like I can clone it, but then what about pull requests etc? Is that the right forum? how much info do you want?
A brief summary:

It might be a lot of code, but more of it is just "templates"
The templates have weird names to remind you are that they are templates. Their Invoke method is called Ensorcel (it's to bewitch - I hit the thesaurus for works like Invoke!). There is use of a phantom<>. But ultimately the template are just plain old classes, generic and otherwise, that implement one or more of the IEssenceOf(Compare|Hash|Equals) interfaces.
Code generation is achieved by enclosing one (or more in the case of tuples) of the templates inside an enclosing class which implements either one of the desired output interfaces (IComparer<>, IEqualityComparer<>) or an internal base class with an Invoke method. By "enclosing" I mean that these wrapper classes are generics, and they take as generic arguments the templates.
The templates are all defined as structs, which is why this whole thing works (not completely true as I originally did allow for reference types via a helper object [with another weird name, a flacon - it was a little container], but dropped it for performance reasons.) i.e. the JIT takes the wrappers, filling in the templates, inlining code, etc, and out pops a fully functional class with great performance characteristics. All without one line of Emit!
The actually parts that choose the templates for equals, hash & compare are all very similar. They just run through the checklist of traits on the input type and choose the first one that matches.
Oh, and a slight change to how the comparer in PER mode deals with NaN. It used to throw an exception which was caught internally, but now I just return a value that is always false.

So that's it.
So it achieves my original goal of improving performance of generic types and value types, which includes great handling for Tuples. And along the way it has also sped up some handling of floating point numbers, and it also deals with Nullable<> types (not that F# does; but now maybe they could. i.e. give them equality/comparable traits of the underlying type), as well as all other types. And it's almost as fast as the standard EqualityComparer.Default/Comparer.Default.
Hope you like it :-)
Now just some tests... ug.  I'm almost an unknown person in my household. :-)

Some other info; I haven't explorer why (probably delay loading of uncommon parts of the JIT?), but the creation of the first comparer takes in the order of ~100ms. Further comparers take only ~1ms.

Hereis a gist showing an example of the initial priming time (and some tuples for good luck)
| x86_old | x86_new | x64_old | x64_new
------------- | ------------- | ------------- | ------------- | -------------
Initial Priming (ms) | 3 | 106 | 3 | 94
Further Priming (ms) | 0 | 1 | 0 | 1
Tuple 2 (ms) | 10870 | 2441 | 5189 | 2336
Tuple 3 (ms) | 11156 | 2706 | 5110 | 2502
Tuple 4 (ms) | 10782 | 2801 | 5128 | 3006
So one again, seeing quite massive performance improvement in the 32-bit world. Modest in 64-bit world.

...and what a difference a JIT makes. Because of all the current bugs flying around with RyuJIT I disabled it and just reran that last test with the following results (these are all 64-bit, 32 bit was unchanged, which makes sense considering ryujit was for 64-bit!)
(old/new refer to before this PR and after this PR)
| old_ryujit | old_original_jit | new_ryujit | new_original_jit
------------- | ------------- | ------------- | ------------- | -------------
Initial Priming (ms) | 3 | 15 | 94 | 177
Further Priming (ms) | 0 | 0.5 |  1 |2
Tuple 2 (ms) | 5189 | 5337 | 2336 | 2027
Tuple 3 (ms) | 5110 | 5286 | 2502 | 2006
Tuple 4 (ms) | 5128 | 5295 | 3006 | 1999
So although "new" (i.e. post pull request) are always better, but with RyuJIT the wider the Tuple, the much greater the performance hit. The original jit obviously inlined everything in the way that nature intended!
A bit disappointing I guess (although all the benchmarks I have been outputting here have been with RyuJIT, so I guess this should be seen as a positive - i.e. there is obviously an avenue for improving the results with some work in the JIT...)

@manofstick  Great detective work as always! We should give these specific performance tests to the RyuJIT team. BTW I would be interested in the timings with self-defined tuples-as-structs as well!

@dsyme
Funny thing you mention the struct tuples are it's on my roadmap =) (well names again, maybe my goat's trail...)
My plan is for "internal" dictionaries (i.e. groupBy/countBy/dict/whatever) to turn a normal tuple into an internal struct tuple (and then back again, if need be, such as when Keys would be requested on a dict). Now that conversion might chew up whatever performance is gained, I'm not sure, but even if the time is the same, then it would be "better" because the keys wouldn't be on the heap; thus if those dictionaries are long lived you are not paying the cost each GC cycle.
If this is successful, a possible extension would be to use this same structure, but extend the API with a "mutable_dict" to create a mutable dict (obviously!). So possibly you could get the benefits of larger structs without all the pain of damage with tails...
Anyway, all sitting in my brain at the moment... Time is the only problem :-)

@latkin,
I did some further investigation into my "Initial Priming" time and found out that it wasn't my code at all! Rather the bulk of it is from the ErrorStrings module where the calls to GetString are rather expensive. I think it would be prudent to fix that - do you need to cache strings for errors that mainly don't occur?
Anyway, that makes me pretty happy that it's relatively cheap to generate my dynamic comparison operators.

@dsyme, @latkin
RE: your message Don, I assume you mean you guys (MS) contacting the RyuJIT team? (I would recommend a couple of weeks wait while the 4.6 bugs are sorted out!)
It would obviously be great if they could get the performance back to previous JIT level, but I can understand that the path I have travelled is... ahem... a bit... unique? Not to say that it couldn't be done, in fact it is probably very simple, as it probably means just always inlining generic classes that have been created with Value Type interfaces, (or even honouring the MethodImplOptions.AggressiveInlining Attribute [and yes, I had to modify the compiler to even let me create methods with carried that attribute, but it didn't work anyway, the inlining, not the attribute, that worked, I'll make that another Pull Request at some stage...]
(But, as an aside, I think the whole solution is quite elegant really, and it actually could be generalized as a standard mechanism for run-time inlining, i.e. creating a mechanism where you can inject code into a module without exposing any internals - think of a version of the "Seq" module with traits like the Nessos Streams module - this was actually what I thinking about when originally thought of the idea, and then it just turned out to also be a solution to the dynamic creation of Tuples, so I brought it into this project. I whipped up a prototype in c# a little while back and it was possible to compose the inline functions and not loose too much performance, but if the back-end was actually aware of this, then probably it could be done even better...)
...so now you might be thinking, "you did what?!?!"
:-)

@manofstick re: ErrorStrings - See https://github.com/Microsoft/visualfsharp/issues/159, it is actually a functional bug that the error string do static initialization, they should be lazily loaded only when required. A fix was attempted for 4.0 but we realized very late that it introduced a binary compat issue, so we just rolled it all back.  For vNext we will make a proper fix, and that should correct the init times you are seeing, if I understand correctly.

@manofstick since this should not affect binary surface area, we could in principle include it in an update release.  Do you have a rough ETA of when you feel this will be complete, with reasonable test coverage? If it's fairly soon then we should consider this for Update 1.
@dsyme have you had a chance to do a proper review of this all-up? I admit I have not, yet.

It's a big change! "1,783 additions, 172 deletions not shown" - how do I see the diff in github?

Not sure how to review it online, I've resorted to applying it locally and using local diff tools.

@latkin
RE: timeframe, I have noticed a couple of areas I would like to polish around comparison of floats (it's doing the nan check up front, which the inlined IL wasn't doing) and inequality operators (just pushing call up one level, as currently doing a type cast on comparer and then branching when it already knew what it was). Total time of these < 2 hours.
Non performance based test suite; well if I know I have a deadline, then probably about a week or two of my time (i.e. probably a couple of days of full time work).
Performance based test suite; I've got a variety of little tests like the ones posted in this pull request, but would be unsure how to make them nice from an automated perspective.
Now are you happy working from this pull request? I could just close this pull request and then make a new one from a better root, in a single check-in if that was easier?
Oh, and I guess a big question mark in my mind is Mono. I "tried" (like eating brussel spouts "tried") to get it to run but to no luck, and that was from the branch before any of my changes went in. My attempt was only on Windows. So I think some testing should obviously be done there.
Give me a timeframe anyway, and I can make sure things are done.

FWIW one criteria we may need to consider is whether the implementation of this kind of specialize-on-the-fly feature might make it harder to run vanilla F# code on .NET Native - depending partly on what FSharp.Core would be used in that scenario anyway. @latkin Any thoughts on that?

@dsyme
Argh, damn .net native :-)... But looks like my usage is not the only usage of MakeGenericType in FSharp.Core (Query.fs uses it a bit). Hmmm... funny thing is that the compiler could know the types and thus generate the browse metadata required for .net native.
i.e. you could modify the compiler to work its way backwards from any MakeGenericType callsite, recording some function metadata which denotes the types when they become concrete, and once they are all concrete save that off.
Unfortunately in my case though there is an extra level of indirection though, as I use a type generated by a generated type. (Although if the first bit of logic to do this had been implement, then it  wouldn't require too much extra to get to that state I guess! i.e. if you have known inputs signature of (Type)+ -> Type then once again all that knowledge is known at compile time, so it could work...)
Hmmm... still all a fair bit of work I'm guessing.
Probably just easier to have a conditional compile target! Although a lot less fun.

@dsyme, @latkin
I've had some further thoughts about .net native, and I now think the best approach would be an instrumentation step collecting data for the browse metadata.
This would probably be significantly simpler that modifying the compiler to trace back all calls to MakeGenericType (with all the extra metadata that would need to be stored with all assemblies, etc.)
So I'm imagining some profiling like step which records the calls to MakeGenericType and then creates the required data when stopped. This also has the benefit of any type that is not know at compile time (i.e. maybe created with a string) also being picked up. But on the down side it means that you have to make sure all required code paths are executed.
Anyway, just some thoughts.

@latkin
Unbelievable I know, but I think I'm good to go :-)

@latkin
....hardly the right place for this message; but anyway...
I see from twitter that you are leaving microsoft - best of luck in your future adventures! I also see how small a world it is, as someone commented on you sharing an office with David Kean. Although he probably wouldn't remember me, I was at his wedding (!) as his wife was a friend of my wife's from high school!
Anyway, I hope my pull requests still end up being sucked through the machine without your guidance. Things have been pretty quiet, so I'm not sure of status...
Best Wishes,
manofstick.

@KevinRansom, @dsyme
All quiet on the Western Front...
With the departure of @latkin, is this still on the agenda for Update 1?
I could, if it would be easier, close this pull request and create a new one branched from the current head with just a single checkin (thus removing a number of missteps that were made along the way, and possibly making it easier to review?)
Cheers,
manofstick.

@manofstick no need to create another pull request, if you want to squash all your commit in a single commit for review you can do that, and push --force to this branch

@enricosada
Thanks for the advice, my knowledge of git amounts to this!
(Although really I was just throwing up an option for those inside the gates of MS for what they wanted in order to progress this PR. Maybe they are already happy. Maybe they have decided this is too big a change.  Really I just want some feedback to know how to progress this (or if it is still on the conveyor belt for release), or if it's just going to get dropped, no matter what I can offer. I just haven't heard anything since the departure of @latkin, so some small assessment would be nice...)

I think it's great. I'll review it again and see if there are any remaining grounds for concern.

@dsyme
Cheers Don.
Sorry about the whining, just glad to know it's still in the picture, that's all I was after.
I'll zip my lips now :-)

This is a candidate for the Dev 15 branch, I think we probably want to see enough bake time on this, rather than putting it straight in VS2015.

@KevinRansom
Ahh, but grasshopper, for something to bake it must be in the oven.
Anyway, as per my comments over the last month or two this has been my concern. Disappointed, but not surprised.

@manofstick Actually this is definitely in my get to sooner pile.  My plan was to pull it today, until I ran into an entirely unrelated build issue.

How about that ... whilst I was typing the test pass completed.

@KevinRansom No worries! Sorry if I have potentially caused any grief (not my intention). Definitely understand that this has a large potential for impact, thus warranted care. (and I wouldn't be surprised for it to be tweaked to suit coding style/naming/etc. I think even splitting prim_types.fs is almost called for; damn interface files make that a not so fun experience though, unless happy with name leakage...)
(...anyway, the bad thing for me in it getting it over the line is that I now have no reason stopping me from working on my next change!! Argh!!)

Merged : https://github.com/Microsoft/visualfsharp/commit/378d267c1fe870756030de741ead9ad4bbe61327
Thank you for your contribution.

I'd really like to take a good long look at a clean final diff of this.  However it's too big for GitHub.  Anyone know the best way to look at this??
Also is that the right commit?  Perhaps it is this one as well? https://github.com/Microsoft/visualfsharp/commit/0735d37c2a5ca5fcfdde6405f6a4a94700404c3d

Per this comment I'm requesting that this PR reverted until the code in prim-types.fs has been properly reviewed.  Thanks

Hi @manofstick, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
The agreement was validated by Microsoft and real humans are currently evaluating your PR.
TTYL, MSBOT;

Not longer makes sense after #966


Add alternative way to get processor frequency (/proc/cpuinfo).

Signed-off-by: Taras Drozdovskyi<EMAIL_ADDRESS>Description
An alternative method was added to obtain the processor frequency for calculating performance score (the value is read from the  /proc/cpuinfo file).
By default, the frequency value is read from /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_max_freq, but in case of a failure it reads from the /proc/cpuinfo file (this happens when Linux starts on a Virtual Machine), the default /proc/cpuinfo cannot be used, since the current frequency displayed in the /proc/cpuinfo may differ from the maximum (the processor may be in low power mode).
Fixes #81
Type of change

[x] Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?
The Edge Orchestration must run successfully in a Virtual Machine and calculate own performance score.
Test Configuration:

Firmware version: Host: Windows 10 - VirtualBox Guest: Ubuntu 16.04
Hardware: x86-64
Toolchain: recommended by default Docker and Go versions
Edge Orchestration Release: Baobab

Checklist:

[x] My code follows the style guidelines of this project
[x] I have performed a self-review of my own code
[x] My changes generate no new warnings
[x] I have added tests that prove my fix is effective or that my feature works
[x] New and existing unit tests pass locally with my changes

The value changes in cpuinfo. Hence this approach is not perfect and also it is not maximum cpu frequency

Yes you are right the meaning is changing. I wrote about it. There is no other way in virtual machines. Default is used '/sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_max_freq' and if it is absent (when used in virtual machines), we can take inaccurate processor parameters. I think this is better than a system that does not work.

@suresh-lc Yes you are right the meaning is changing. I wrote about it. There is no other way in virtual machines. Default is used '/sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_max_freq' and if it is absent (when used in virtual machines), we can take inaccurate processor parameters. I think this is better than a system that does not work.
Home-Edge don't support kvm itself and the scenario was created since initial reporter didn't have the device. hence supporting with inaccurate by this approach would not be good one, as this is not normal flow(use case)

Yes, we never mentioned that we support Virtual Machines, but is it really bad if our system supports this? I think we should be more open to external developers.

I feel then we could reply on following param in the cpuinfo file
'model name	: Intel(R) Core(TM) i3-2120 CPU @ 3.30GHz'
Parse the above parameter and take the 3.30GHz and convert and use it. This is more in lines of "cpuinfo_max_freq" file

@suresh-lc  I considered this idea also, but it doesn't reflect the maximum value, it is the optimal processor frequency. See real example below:
processor	: 0
vendor_id	: GenuineIntel
cpu family	: 6
model		: 58
model name	: Intel(R) Core(TM) i7-3635QM CPU @ 2.40GHz
stepping	: 9
microcode	: 0x21
cpu MHz		: 3273.342
cache size	: 6144 KB
physical id	: 0

So I didn't complicate parsing of /proc/cpuinfo.
If you have any idea please share it.
I believe together we can make the Edge Orchestration better!

@suresh-lc I considered this idea also, but it doesn't reflect the maximum value, it is the optimal processor frequency. See real example below:
processor	: 0
vendor_id	: GenuineIntel
cpu family	: 6
model		: 58
model name	: Intel(R) Core(TM) i7-3635QM CPU @ 2.40GHz
stepping	: 9
microcode	: 0x21
cpu MHz		: 3273.342
cache size	: 6144 KB
physical id	: 0

So I didn't complicate parsing of /proc/cpuinfo.
If you have any idea please share it.
I believe together we can make the Edge Orchestration better!

Following is the real example which we had crossed verified in many systems
processor	: 0
vendor_id	: GenuineIntel
cpu family	: 6
model		: 42
model name	: Intel(R) Core(TM) i3-2120 CPU @ 3.30GHz
stepping	: 7
microcode	: 0x2f
cpu MHz		: 1926.375
cache size	: 3072 KB
physical id	: 0
siblings	: 4
core id		: 0
cpu cores	: 2
apicid		: 0
initial apicid	: 0
fpu		: yes
fpu_exception	: yes
cpuid level	: 13
wp		: yes

@suresh-lc I agree with you, the value of CPU freq can be different (it depends from power mode cpu), especially in Virtual Machines. and now I am implementing the parser and tests of model name : Intel(R) Core(TM) i3-2120 CPU @ 3.30GHz. When everything is ready I will update PR.
P.S. I would like to note that by default the value of the maximum processor frequency will be read from the /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_max_freq and only in cases of failure from the mechanism proposed by me. Therefore, it will not affect PCs where the Edge Orchestration will run in the host Linux OS
Thank you

@suresh-lc @MoonkiHong I've updated PR, please review.


Side nav collapses top-level items when clicked

Explore a cluster.
(You'll see that the 'Cluster' section is not expanded on entry)
In the side nav, click on one of the other top-level groups, e.g. 'Storage'.
Now, click on 'Cluster' in the side nav - the section will expand and then immediately collapse.
This is a regression.
I think also related - install Monitoring - click on 'Monitoring' and then click on 'Monitors' - the side nav collapses and Monitoring is not expanded, which it should be.


Fix typo

Spotted a small typo in the codec readme file.

I signed it!

thanks for your contribution :)


Deprecate shrinkWrap and replace with API that is clearer about performance trade-offs

Problem
When shrinkWrap is used for scrolling widgets, developers are often unaware of what it actually means, a loss of performance due to the entire contents of the scrollable being evaluated.
We have been working on bettering the experience here through the Scroll Performance Pitfalls Project for example:

https://github.com/flutter/flutter/issues/116237
https://github.com/flutter/flutter/pull/129377
https://github.com/flutter/news_toolkit/pull/746
https://github.com/flutter/news_toolkit/pull/739
https://github.com/flutter/samples/pull/1530
https://github.com/flutter/flutter/pull/116236
https://github.com/flutter/flutter/pull/104008

There are a handful of times when shrinkWrap is appropriate, but most of the time users do not need to use it and cost themselves performance issues. It is very easy to just toggle the shrinkWrap flag, and either forget to come back and toggle it off (often the case when fleshing out the UI and the user gets the unbounded error) or revisit it to refactor for efficient scrolling. In some cases, it can be very difficult to refactor if additional UI has been built around the shrinkWrapped viewport (see the news toolkit PRs above). The internet is also working against us here, as there are multiple instances of shrinkWrap on StackOverflow and elsewhere as a recommended solution.

Proposal
For these reasons, we believe shrinkWrap should be deprecated and replaced with an API that is much less obscure as far as what decision the user is making.
For example:
// Old API
ListView(
  shrinkWrap: true, // This would be deprecated and ultimately removed
  children: myChildrenList,
);

// New API
NonLazyListView(
  children: myChildrenList,
);

// * Name subject to better minds' ideas :)

This code could be relatively simple, since ListView, GridView and CustomScrollView are already subclasses of ScrollView, the new classes for these could just explicitly set the shrinkWrap property of the super class rather than exposing it as a constructor parameter.

Communication & Migration
The biggest effort here will be communication and migration. There should be 4 main cases:

In about half of the cases I have found, shrinkWrap can actually be removed. This often happens when an unbounded error is thrown, the user sets the flag, but then later the scroll view ends up being constrained and the flag is no longer necessary.

The developer should refactor just wrapping an Expanded widget around the ScrollView widget. This is commonly seen when a ListView (or other) is placed inside of a Column.

The developer should refactor to use a CustomScrollView & slivers instead of ListView (or other).
a. https://github.com/flutter/website/issues/8435 - Refactoring away from shrinkWrap via the CustomScrollView option will be easier if we have more examples of CustomScrollView for our developers
b. https://github.com/flutter/website/issues/8433

The developer should refactor to use a SingleChildScrollView if there is only one or very few children, it (basically) operates the same way as shrinkWrap.

If it is a safe and relevant use of shrinkWrap, the developer can just switch to the new shrinkWrapping class by renaming the scrolling widget that they used to set shrinkWrap: true on.
a. example: ListView -> NonLazyListView

We should tackle this as a whole covering all of these cases:

https://github.com/flutter/website/issues/8434

Affected Classes

ScrollView

unsure if this one as the super class should remain, based on comments above

ListView
GridView
CustomScrollView
ReorderableListView
AnimatedList
ReorderableList
BoxScrollView

Primary Tasks

[ ] Identify remaining shrinkWrap cases in the framework - refactor if possible
[ ] Create design doc that goes into more detail on the new classes, migration plan, and everything covered here
[ ] Add new classes (whatever they may be called)
[ ] Migrate any remaining shrinkWrap in framework, or in use by customers to the new classes
[ ]  Deprecate shrinkWrap in all scrolling widgets - referring to new classes
Document:

[ ] best practices on website
[ ] samples for migration via each path above
[ ] migration guide on website : https://docs.flutter.dev/release/breaking-changes
[ ] flutter-announce

FYI: I added a fifth scenario above for developers to refactor away from shrinkwrap.

@Piinks Do we have an estimation of when you think this change will be effective? Thanks!

Any other widget properties that performance trade-off  other than shrinkWrap ?

I was just having a problem with scrolling performance today and it took me like 2 hours to figure out that the shrinkwrap flag was the cause by trial and error. So good work.

Our team use custom scroll view and sliver widget to archive similar behavior with shrink wrap. But it too verbose to use it. Curious how other framework handle this behavior (ex, compose, react, etc)


[ENH] Support JSON queries

Is your feature request related to a problem? Please describe.
Some SQL engines, most notably Postgres, allow for JSON based queries. It would be great for Dask-SQL to support these as well.
Describe the solution you'd like
Extend SQL support to JSON-based queries, postgresql style.
Describe alternatives you've considered
Alternative SQL engines, like DuckDB, already support these, but not in distributed manner.
Additional context
sqlparser-rs seems to support JSON already, but I'm not sure if DataFusion does.

Thanks for raising this issue.
@andygrove Do you happen to know offhand if datafusion supports the json syntax and generates a logic plan that we can use?

Thanks for raising this issue. @andygrove Do you happen to know offhand if datafusion supports the json syntax and generates a logic plan that we can use?

There is no SQL JSON support as far as I know (I did just search the repo and did not see anything).


Strange token limit exceeded error with gpt-4

Describe the bug
A very short gpt-4 conversation exceeded the token limit easily when the Max Response Token Length  is high
To Reproduce
Set Max Response Token Length to a large number, start a new chat, keep talking a few times, boom!
Expected behavior
A conversation not so long shouldn't exceeded the token limit so easily
Screenshots

Max Response Token Length
Chat Screenshot

Environment:

Browser: Firefox v112
Extension Version: 2.2.6

Max Response Token Length is sent as max_tokens parameter in requests. When your conversation's total token length plus Max Response Token Length  exceeds the limit set by OpenAI (4097 for gpt-3.5-turbo, 8192 for gpt-4-8k ), you will not be able to continue the conversation. When this problem occurs, you should reduce the Max Response Token Length setting

The interesting thing is sometimes we can't estimate the response length properly, for the example provided, the input and the output are short enough, but the limit exceeded, wouldn't that be strange?

In your screenshots, the minimum Max Response Token Length you set is 8000, while the default token limit for gpt-4 is 8192. This means you can only provide a context of 192 tokens in length. In my opinion, this matches the screenshot content you provided

Open about:debugging#/runtime/this-firefox and inspect this extension
Create a chat and ask something
You can see the actual response and error in console and network
Sorry, I can't reproduce this issue here, so you need to follow the above content to operate by yourself
By the way, I didn't do any special treatment regarding this error prompt. I just returned the error from the server as is, and replaced it with a more user-friendly text explanation

You're right, I should dig it first.
Turned out it's not this extension's issue, and I might recalled it wrongly, about the max token been set and the model being used. Still, even though the input and output is very short, a max_tokens paramater in the input really impact the situation.
Thanks anyway.

you are right, that's ok

v2.2.8 now it always shows complete error details


Add casovni bloki

Thanks for your commit! Please be patient as i'm on vacation and cannot confirm and test changes.

Kar polahko.

Tnx for your contribution.

@frlequ as you changed the string from casovniBlok to casovni_blok in moj_elektro_api.py you need to do the same in sensor.py.

Uff that's right. Thank you!

I've created a fix in the new PR so you can just merge that one and it will be solved.


[SPARK-12213] [SQL] use multiple partitions for single distinct query

Currently, we could generate different plans for query with single distinct (depends on spark.sql.specializeSingleDistinctAggPlanning), one works better on low cardinality columns, the other
works better for high cardinality column (default one).
This PR change to generate a single plan (three aggregations and two exchanges), which work better in both cases, then we could safely remove the flag spark.sql.specializeSingleDistinctAggPlanning (introduced in 1.6).
cc @yhuai @nongli @marmbrus

Test build #47442 has started for PR 10228 at commit e3f2e79.

Test build #47443 has started for PR 10228 at commit 5e42c76.

oh, just realized that the plan for a query like SELECT COUNT(DISTINCT a) FROM table will be
AGG-2 (count distinct)
  Shuffle to a single reducer
    AGG-1 (grouping on a)
      Shuffle by a
        Partial-AGG-1 (grouping on a)

Ideally, we should still use four aggregate operators like the one shown below but without the overhead of using Expand.
AGG-2 (count distinct)
  Shuffle to a single reducer
    Partial-AGG-2 (count distinct)
      AGG-1 (grouping on a)
        Shuffle by a
          Partial-AGG-1 (grouping on a)

We could move the planning of a distinct queries entirely to the DistinctAggregateRewriter. This would require us to merge the non-distinct aggregate paths and the first distinct group aggregate path, so we could avoid the expand in case of a single disinct column group.
This is quite a bit of work; I don't know if this is worth the effort.

Test build #47533 has started for PR 10228 at commit 3f60962.

Test build #47536 has started for PR 10228 at commit a9eae30.

Test build #47533 has finished for PR 10228 at commit 3f60962.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/47533/
Test FAILed.

Test build #47536 has finished for PR 10228 at commit a9eae30.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/47536/
Test PASSed.

Merged build finished. Test PASSed.

@hvanhovell The difficulty of doing this in DistinctAggregateRewriter is that DistinctAggregateRewriter will generate two logical plan, but some aggregation functions have different updateExpression and mergeExpression, so will could not work as update-merge-update-final, they should work as update-merge-merge-final.

Test build #47544 has started for PR 10228 at commit 3f1ea7f.

@davies don't get me wrong. I think this PR is an improvement of the current situation (it never crossed my mind to change partitioning when I was working on that part of the code), and should be added.
I am also not to keen on changing the MultipleDistinctRewriter; given the time it'll take and the objections you've raised. The only thing that bugs me is, is that we currently rewrite distinct aggregates in two places.

@hvanhovell If we could figure out a better solution, it's definitely welcomed.

Test build #47544 has finished for PR 10228 at commit 3f1ea7f.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/47544/
Test FAILed.

Test build #47559 has started for PR 10228 at commit 8262ad8.

Test build #47559 has finished for PR 10228 at commit 8262ad8.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/47559/
Test FAILed.

Test build #47570 has started for PR 10228 at commit 740e725.

Test build #47570 has finished for PR 10228 at commit 740e725.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/47570/
Test FAILed.

Merged build finished. Test FAILed.

Test build #2211 has started for PR 10228 at commit 740e725.

Test build #47601 has started for PR 10228 at commit 71e0b1c.

Test build #2212 has started for PR 10228 at commit 71e0b1c.

Test build #2211 has finished for PR 10228 at commit 740e725.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Test build #47601 has finished for PR 10228 at commit 71e0b1c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/47601/
Test PASSed.

Test build #2212 has finished for PR 10228 at commit 71e0b1c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

@hvanhovell With this change, we will use the planner rule to handle single distinct aggregation and use the rewriter to handle multiple distinct aggregations, which is the same as when you originally introduced the rewriter. I guess this workflow is better than having two logics that handle the same case. What do you think?

@davies I only left a few minor comments. Overall, it is very cool!

@yhuai I think having the two clearly separated paths (this PR) is an improvement of the current situation. I also admit that I am responsible for introducing the second path. Your comment on having four aggregate steps without the exhange triggered me, and I was thinking out loud on how we could do this using the rewriting rule (the removal of one of the paths would have been a bonus).

Test build #47622 has started for PR 10228 at commit 51ca055.

Test build #47622 has finished for PR 10228 at commit 51ca055.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/47622/
Test PASSed.

@hvanhovell Yea, it's great to think about how to use a single rule to handle aggregation queries with distinct after we have this improvement. The logical rewriter rules probably is a good place because rewriting logical plans is easier. If it is the right approach, we can make some changes to our physical planner to make it respect the aggregation mode of an agg expression in a logical agg operator (right now, our physical planner always ignore the mode). So, when we create physical plan, we can understand that, for example, a logical agg operator is used to merge aggregation buffers.

@hvanhovell How about we merge this first and we take a look at how to use a single rule to handle aggregation queries with distinct?

@yhuai LGTM. Yea, lets merge this one, I'll create a ticket for the distinct rules

Cool. I am merging this one to master.


cache_limit decreases utilization

Hi!
I have a problem with cache_limit. I have a large dataset (more than 1 TB) that is located on a remote s3. If I set a limit, productivity and learning speed drops. Moreover, the lower the limit, the greater the drawdown. In this case, the limit works as it should: the node does not download more data than the limit at a given time. If I don’t set a limit, I’m satisfied with the performance, but this is how all the datasets are downloaded, and in multi-nodier mode it can take about an hour, which is a very long time. What could be the problem? Thank you.

@germanjke Did you get a chance to try out Simulator (link)?

Simulator is good, works fine and gives u min cache limit


Don't use conflicting module names - messes up 'included' blocks, etc

This isn't evident with ClassMethods, but only because ActiveRecord::Concern does some magic...

+1 for pulling this in. Fixes a problem I ran into when upgrading to Rails 4.


Fix exclusive child focus grab, when there are more than two child windows.

Follow exclusive child chain to the end and activate all widows, instead of only next one.
e.g. :

Before: Clicking on Main window activates Create New Project leaving Open a directory at the bottom and inactive.
After: Clicking on Main window brings Create New Project on top of Main window, and Open a directory on top of all and active.

Thanks!


Local benchmark, voxblox loco planner, mav local planner, gazebo sim

In order of decreasing relevance/importance:

Wrote mav_local_planner

This node takes in the current odometry of the MAV and intelligently plans toward waypoints or series of waypoints.
If the waypoint or waypoints are in known free space, and a valid path exists, uses a path smoother as specified by the "smoother_name" ros param.
If not, then (if "avoid_collisions" param is on), tries to navigate using voxblox_loco_planner toward the waypoint until it is reached or a max number of failures is encountered.
Inputs: single waypoint (geometry_msgs/PoseStamped), multiple waypoints (geometry_msgs/PoseArray) (and connections to ESDF maps from a separate voxblox node).
Outputs: 100 Hz sampled trajectory to controller.

Wrote voxblox_loco_planner

Wrapper for the underlying loco optimization to allow random restarts, exploration of unknown space, etc.
Still to do: finding intermediate goals intelligently (ICRA 2018 paper).

Set up gazebo simulation using rotors_simulator to test out local planner

Runs mapping and planning at 4 Hz in simulation based on VI sensor depth. :) (screenshots below)
Introduces a hand-made gazebo world called maze_house, which is a house that is a maze.

Wrote scaffolding for local benchmarking

Will be used to re-create or repeat experiments in ICRA 2018 paper, currently just scaffolding.

Hi Helen,
Thank you so much for sharing this great work, it's really helpful.
I wonder can I ask a question about using "Local Planning"? I followed your instructions and launched the gazebo window normally. However, I don't have topic /firefly/voxblox_node/mesh in rviz, and there are also no point clouds shown in rivz. There are no response by using Send Waypoint, but the drone in Gazebo have responses by using Send to Controller. Thank you very much.
Regards,
Jie

@jwangjie Hi, I get the same problems, did you finally work it out? I am looking forward to your response. Thank you very much.


Generalize integrand f to any callable

Changes

Removes requirement that integrand f be of type Function

Closes #132

Updates relevant docs
Updates tests to include coverage of callable struct as integrand.

The codecov/patch test fails because these changes touch the Tetrahedron methods, whose tests are currently tagged under extended/local-only testing.


all: make NetState struct more rusty

Use a new() function with an impl block
with is more rust idiomatic
Signed-off-by: Antonio Cardace<EMAIL_ADDRESS>

I must read the rust document wrong. From my view, the NetState::new() is creating a object in memory not retrieving network state from kernel.
And NetState::new() does not handle this future design:
state = NetState::new(); // Empty state
state.ifaces["eth1"] = EthernetIface::new("eth1");
state.ifaces["eth1"].mtu = 1280
state.apply().

If you think your proposal is rusty way, please provide rust document indication so.

According to [Rust API Guideline] 1, the Class::new():
The name new should generally be used for the primary method of instantiating a type

For this PR, I would suggest NetState::retrieve() -> Result<NetState, NisporError>.

I must read the rust document wrong. From my view, the NetState::new() is creating a object in memory not retrieving network state from kernel.
And NetState::new() does not handle this future design:
state = NetState::new(); // Empty state
state.ifaces["eth1"] = EthernetIface::new("eth1");
state.ifaces["eth1"].mtu = 1280
state.apply()

If you think your proposal is rusty way, please provide rust document indication so.

Well it was just an indication of how I usually see things done in rust, I was not aware of this future design, but anyway I have to say it looks good to me.
Maybe better with
state.add_link(EthernetIface::new("eth1"));

But I am not that experienced with rust.

According to Rust API Guideline, the Class::new():
The name new should generally be used for the primary method of instantiating a type

For this PR, I would suggest NetState::retrieve() -> Result<NetState, NisporError>.

retrieve() looks good as well but new() in this PR fits exactly the case of instantiating a new type, doesn't it?

According to Rust API Guideline, the Class::new():
The name new should generally be used for the primary method of instantiating a type

For this PR, I would suggest NetState::retrieve() -> Result<NetState, NisporError>.

retrieve() looks good as well but new() in this PR fits exactly the case of instantiating a new type, doesn't it?

Normally, the new() function does not generate failures but a new instance which does not fit into this use case as we
are expecting NisporError to be raised when terrible failure found.

According to Rust API Guideline, the Class::new():
The name new should generally be used for the primary method of instantiating a type

For this PR, I would suggest NetState::retrieve() -> Result<NetState, NisporError>.

retrieve() looks good as well but new() in this PR fits exactly the case of instantiating a new type, doesn't it?

Normally, the new() function does not generate failures but a new instance which does not fit into this use case as we
are expecting NisporError to be raised when terrible failure found.

Mhmm, so maybe Netstate should have a new() function to create the object and a retrieve() to be filled with infos from netlink, what do you think? Otherwise you can just close this.

According to Rust API Guideline, the Class::new():
The name new should generally be used for the primary method of instantiating a type

For this PR, I would suggest NetState::retrieve() -> Result<NetState, NisporError>.

retrieve() looks good as well but new() in this PR fits exactly the case of instantiating a new type, doesn't it?

Normally, the new() function does not generate failures but a new instance which does not fit into this use case as we
are expecting NisporError to be raised when terrible failure found.

Mhmm, so maybe Netstate should have a new() function to create the object and a retrieve() to be filled with infos from netlink, what do you think? Otherwise you can just close this.

We don't allow user to create NetState yet. So there is no need of NetState::new(). But I am OK to use NetState::retrieve()
for state retrieving.
You may continue work on this PR or create a new one(please also fix test and python binding).

According to Rust API Guideline, the Class::new():
The name new should generally be used for the primary method of instantiating a type

For this PR, I would suggest NetState::retrieve() -> Result<NetState, NisporError>.

retrieve() looks good as well but new() in this PR fits exactly the case of instantiating a new type, doesn't it?

Normally, the new() function does not generate failures but a new instance which does not fit into this use case as we
are expecting NisporError to be raised when terrible failure found.

Mhmm, so maybe Netstate should have a new() function to create the object and a retrieve() to be filled with infos from netlink, what do you think? Otherwise you can just close this.

We don't allow user to create NetState yet. So there is no need of NetState::new(). But I am OK to use NetState::retrieve()
for state retrieving.
You may continue work on this PR or create a new one(please also fix test and python binding).

Ok I updated the PR, I should have addressed everything!

Merged. Thanks.


brain-score.org (user:484) | Add new plugin(s): models: [regnet]

Plugin-only changes from brain-score.org submission. Generated via Jenkins job create_github_pr, build 46. Plugins added: models: [regnet]

Reopening to retrigger checks (Jenkins build failed on pytorch download timeout)


Make Hologram Tasks Async (Improves Performance)

Checks

[x] /moducoredump command works without error

If you push a change that breaks the dump command, it is hard to debug issues if people have them.

Description

Resolves #102


Sync pending VisualServer commands after ScriptServer finalization

This is needed as C# may free resources from the finalizer thread during
CSharpLanguage::finish(). Previously this would result in RIDs not being freed.
This fixes the Image leak mentioned in #34954.

@reduz said that it makes sense, so let's merge :)

Thanks!


feat: add superchain erc20 baseline

Closes OPT-199

External review at https://github.com/ethereum-optimism/optimism/pull/11675

This branch will be closed since its not needed. Dev will be synced once merged to Op repo. PR: https://github.com/ethereum-optimism/optimism/pull/11675


Issue 1266 nested support

I have made things!

Checklist

[x] I have double checked that there are no unrelated changes in this pull request (old patches, accidental config files, etc)
[x] I have created at least one test case for the changes I have made
[x] I have updated the documentation for the changes I have made
[x] I have added my changes to the CHANGELOG.md

Related issues

🙏 Please, if you or your company is finding wemake-python-styleguide valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/wemake-python-styleguide. As a thank you, your profile/company logo will be added to our main README which receives hundreds of unique visitors per day.

Current commit passes make test


First run produces an temporary error

Information card

[x] I am on the latest Hyper.app version
[x] I have searched the issues of this repo and believe that this is not a duplicate

OS version and name: darwin x64 17.4.0
Hyper.app version: Hyper 1.4.8 (Electron 1.7.8)
Contents of the .hyper.js file: 1258180b739509563cbbf3687f47e3bd
The issue is reproducible in vanilla Hyper.app: yes
Relevant information from devtools:

Issue
When downloaded and installed Hyper.app from the Hyper website and opened as the first time, I'm getting the following notification:

After quitting Hyper.app and re-opening the application seems to fix the notification. The warning in the dev tools console: Unimplemented DEC Private Mode: 1034 remains.

Hey there, please try again with the new Hyper v2 release and open a new issue if the problem stills exists. Thank you!


Integration specs cannot run in Windows

I want to write some integration specs, but I run the integration test.
First, I type ATOM_INTEGRATION_TESTS_ENABLED=1 atom in terminal to launch atom, then I input run all spec in atom command palette to run all specs that contains integration test. Unfortunately, it cannot run and throw some errors.

I want to fix this bug. I find the atom-launcher.sh file in spec/integration/helpers can't run in Windows, so I write the similar bat file for Windows, but it also can't launch it. I don't known webdriverio and chromedriver well, so I can't find the problem. Who can help me?


Compile error with gcc 4.8.2

Using gcc 4.8.2 on Ubuntu 14.04.
src/wren_vm.c: In function ‘runInterpreter’:
src/wren_vm.c:384:41: error: operation on ‘fiber->stackTop’ may be undefined [-Werror=sequence-point]
   #define PUSH(value)  (*fiber->stackTop++ = value)
                                         ^
src/wren_vm.c:562:23: note: in expansion of macro ‘PUSH’
     CASE_CODE(DUP):   PUSH(PEEK()); DISPATCH();
                       ^

Did notice it too, in the Travis CI output. Quite odd.

It appears to be caused by e740a4c95 adding line 563 to wren_vm.c.

Here's those two macros expanded:
(*fiber->stackTop++ = (*(fiber->stackTop - 1)))

Might it be undefined due to the ++? Perhaps these should be multi-line macros to make the intent clearer / more defined.

Yeah, my mistake. It's fixed now. :)
For some reason, clang doesn't complain about it, so I didn't catch this locally.

@zeckalpha I think it might be the =? Either way, the result of the expansion is an expression where there isn't a defined sequence point between accessing and setting stackTop.

Neither MSC with the default settings complains about it. Nor Cppcheck.
It is due to the post increment operator, @zeckalpha is right.
The evaluation order of the subexpressions is not defined (and mostly compiler dependant), and the post increment side effect could affect the outcome.
(l-value subexpressions are not necessarily evaluated after the r-value ones)


Assert rather than throw for unhandled platforms

Failure to select a default platform is an error on the part of the
framework, not something that a developer should reasonably expect to
recover from at runtime.

Sending out as a what-do-you-think?
Can also see the argument for it being an exception to ensure we never miss it even in release mode, though at that point if it's unhandled, it's also arguably too late.

All this will do is delay the exception to a slightly later harder to understand exception in release mode, and make the exception uglier in checked mode.

sgtm - killing it!


Expose 'unSatInt' and 'fromSatInt' and add instances for 'CostModelApplyError'

As requested in #5613.

Sure, I'll add one, I just wasn't sure if I should change something here.