[CompilerPerf] Removed TaggedCollections from Compiler

[manofstick]

Last in line: #5360 -> #5307 -> #5278 -> #5112. Unfortunately during rebasing to master the linage isn't preserved. But should merge fine. Hopefuly.

TaggedCollections is a copy of FSharp.Core's Map and Set modules that was tuned for performance in FSharp.Compiler.Private (possibily/probably pre-dating FSharp.Core.) The functionality was exposed via Zmap and Zset, where some additionality functionality resided.

TaggedCollections also allowed customization via providing a IComparer<'a>, rather than relying on LanguagePrimitives.FastGenericComparer<'a>. This was exposed during the creation of the collection - either getting an empty collectiton or creation (such as ofList). As the type of the comparer was lost it meant that some runtime checking would be required in the case where you had different comparers for underlying collections on set operations (i.e. as an example if you'd had a forward and reverse value comparer and you did a union of the sets then you'd be in bad territory)

This PR replaces this functionality with FSharp.Core's version of Map and Set, with the aid of a helper key class, which wraps the key. This wrapped key contains the type of the key and the comparer, making all operations type safe at compiler time. The comparer is defined in a ValueType that implement a IComparer<'key>. The comparer implementation much be stateless. By following these rules, it can be very cheaply be created and the underlying comparer calls.

Let's take a look:

[<Struct; CustomComparison; CustomEquality>]
type SortKey<'Key, 'Comparer when 'Comparer :> IComparer<'Key> and 'Comparer : struct> = {
    CompareObj : 'Key
}
with
    interface IComparable<SortKey<'Key, 'Comparer>> with
        member lhs.CompareTo (rhs:SortKey<'Key, 'Comparer>): int = 
            Unchecked.defaultof<'Comparer>.Compare(lhs.CompareObj, rhs.CompareObj)

And this is used with a compare implemented like:

[<Struct;NoComparison;NoEquality>]
type TyparOrder =
    interface IComparer<Typar> with
        member __.Compare(v1: Typar, v2: Typar): int =
            v1.Stamp.CompareTo v2.Stamp

And this is used like:

inplaceConstraints : Map<SortKey<Typar,TyparOrder>,TType>

And then used like

map |> Map.add {CompareObj=key} value

In order to make this a bit more palatable there is a type alias

type zmap<'Key,'Comparer,'Value when 'Comparer :> IComparer<'Key> and 'Comparer : struct> = Map<SortKey<'Key,'Comparer>,'Value>

Which makes the definition:

inplaceConstraints : zmap<Typar,TyparOrder,TType>

And a helper class:

[<Sealed; AbstractClass>]
type Zmap<'Key,'Value>() =
    static member inline add<'Comparer when 'Comparer :> IComparer<'Key> and 'Comparer : struct> (k:'Key) (v:'Value) (m:zmap<'Key,'Comparer,'Value>) =
        Map.add {CompareObj=k} v m

    // ... and many others ...

Which means it's usage almost exactly the same as previously.

Assuming that this doesn't actually degrade performance (which on small tests that I have tried this on there doesn't appear to be anything significant - in fact due to other changes made due to prior PRs it is slightly better) I think this would be a good public relations move, as it shows that the libraries that are used in FSharp.Core are the same as those used by the compiler.

NB. This uses the modification to Map/Set where the rebalance is weight based rather than height based. This is somewhat controversial, but could easily be reverted.

[TIHan]

@manofstick to let you know we definitely value your work that you are putting into these perf PRs.

This might be something that we can review for dev16.

[manofstick]

@TIHan

Hopefully you can kick the build server- doesn't appear to be doing much... just license check, which hey, I think I can ignore! (but hey, I passed, hooray!)

[cartermp]

cc @brettfo - any idea why CI is down for this PR? It kicked off for other PRs that came in over the weekend.

[brettfo]

I'm not sure why the CI wasn't started on this; I don't see anything relevant in the job history.

@manofstick Can you resolve the merge conflicts? I don't think that's related, but it might help us track down what's happening if we can get a clean merge.

Edit: I think I know what's happening. Short version, you'll need to rebase onto a current master (or merge in master.) There was a rename of the file that controls the CI from .vsts-ci.yaml to .vsts-pr.yaml and your branch still shows the old file name so my guess is that the merge resulted in the new file not existing so the CI didn't respond to the PR creation.

[manofstick]

@brettfo / @cartemp

Argh! Hmm. This is a long chain of PRs that build on each other: #5112 -> #5278 -> #5307 -> #5360 -> #5463... Any thoughts on how best to proceed?

Maybe rebase the first of master and then ripple the rebases through the chain - although I expect that might get messy here due to the conflicts?

Is current master a good stable place?

[manofstick]

OK, well think I made a bit of a hash of that :-|

Hm. ended up rebasing #5112 off master and #5278 off the updated #5112. But then I got into rebase hell, and so just rebased #5307 & #5360 directly off master so they are now disconnected from their parents.

And, because this one had more merge conflicts I haven't attempted to attack it yet. Hmmm. That'll learn me.

[manofstick]

Anyway, we're back up and building... Hopefully all this doesn't cause too much grief when this is eventually merged. If this is eventually merged :-)

[manofstick]

@dsyme / @TIHan / whoever else is listening !

I've now gutted (assuming that this build succeeds...) the TaggedCollections version of Map from the compiler, so I would be interested if you did have any performance figures to see if this is better/worse (I mean I did this more from a maintenance perspective as I didn't want to have to copy the changes I'd done in Map around the place). Assuming all the dependent PRs pass then I think this would be a worthy step just to slightly lesson total lines of code.

So yeah anyway I am interested to see if this affects performance at all. So please update me if you have any figures....

Oh, and if you're wondering how I did this...

I created a SortKey<'Key,'Comparer> class I can then use as the key in the normal Map. By using my old trick of ValueType interfacing with a suitable comparer class and some minor helper class for a bit of sugar I can then do types like Map<SortKey<Val, ValByStamp>, IncrClassValRepr>

Anyway still got Set to barge through, and then I think I'll be back to having a rest for a while before that only phoenix (seq!) needs to rise from it's ashes with another think/try to make it palatable...

But that might be a while away. Hopefully a while away. My children need me :-)

[TIHan]

@manofstick again, thanks for your work.

It will be a little bit, but we should be able to get perf numbers.

[manofstick]

@TIHan - no worries - when you guys are sorted; good to keep up communication.

Anyway, I guess I'm not expecting this really to make much of a difference, as the main improvement was the removal of MapOne/SetOne that was already removed in the TaggedCollections version. So this is really just a code consolidation (deleting code, such joy!) which hopefully doesn't make things worse.

[manofstick]

@TIHan

OK - this is looking pretty good I think. I have updated the description to give a bit of a summary of the whole thing. (@dsyme this might be of interest to you too).

Anyway, that's me done. Basically. Hopegfully now I can move onto some of the other projects of mine that have been piling up! (I might eyeball and tweak a couple of things, but I mean basically done... from equality comparison to ordering to optimizing collections to deprecating classes in the compiler it's been a bit of a journey but a logical progression and I feel relieved I'm at the end. Bit obsessive really. Hmm!!)

[forki]

Related elm work: elm/core#959

[KevinRansom]

@manofstick could you take care of the merge issue.

Thanks

Kevin

[manofstick]

@KevinRansom

Off in other lands so getting back to this to fix merge issues a bit of a pain. I'll try to make some time at some stage. If urgent then give me a prod...

[KevinRansom]

@manofstick no urgency

[KevinRansom]

@dsyme , @manofstick -- I am going to close this PR, it is very old untouched for 2 years. If you get the time to work on it again please feel free to reopen it, or submit a PR.

Thanks

Kevin