[CompilerPerf] Faster equality in generic contexts

[manofstick]

Rising from the ashes of #513 (3 years later!) comes a new implementation. The new implementation is much simpler, albeit with a smaller scope (I'm no longer attempting to handle Tuples, no longer wandering into forbidden code generation - even if it was only MakeGenericType)

So what does this do? It increases performance of Equals, CompareTo, GetHashCode when used in a generic context. In a non-generic context the fsharp compiler inserts efficient code through a combination of inline IL, statically resolved type parameters and compiler optimizations (lots of magic).

So what is a generic context?

A type which has generated equality/comparison such as a record type:

type Thing<'T> = {
    Field : 'T
}

or

let f<'a when 'a : equality> =
    let comp = HashIdentity.Structural<'a>
    ...

So any of the containers, such as Map, Set, dict as well as other places in the standard library isn't handled by inline functions also has potential for improvement.

[abelbraaksma]

Very, very interesting. I rely heavily on CustomComparisonAttribute and CustomEqualityAttribute, these require me to implement the non-generic IComparable interface and an Equals (plus GetHashCode()) overload, which requires (expensive?) matching on the type to get right.

Would this improve such situations as well, or would existing code benefit if it also implemented the generic IEqualityComparer<'T> (or IEquatable<'T>) and/or IComparer<'T> interfaces? Or are custom comparers/equals out of scope?

[dsyme]

This is getting close to the territory we would accept, if backed up by perf results

Here are some current failures:

****failure on [nan; 1.0] [nan; 1.0]


................TEST FAILED...............


****failure on [nan; 1.0] [nan; 1.0]


................TEST FAILED...............


****failure on ("Foo", {h = 5.9;
         w = nan;}) ("Foo", {h = 5.9;
         w = nan;})


................TEST FAILED...............


****failure on ("Foo", {h = 5.9;
         w = nan;}) ("Foo", {h = 5.9;
         w = nan;})


................TEST FAILED...............


****failure on [nanf; 1.0f] [nanf; 1.0f]


................TEST FAILED...............


****failure on [nanf; 1.0f] [nanf; 1.0f]


................TEST FAILED...............


****failure on ("Foo", {h = 5.9000001f;
         w = nanf;}) ("Foo", {h = 5.9000001f;
         w = nanf;})


................TEST FAILED...............


****failure on ("Foo", {h = 5.9000001f;
         w = nanf;}) ("Foo", {h = 5.9000001f;
         w = nanf;})


................TEST FAILED...............

[manofstick]

@abelbraaksma

If you are create a sealed type with [<CustomEquality>] and provide IEquatable<'T> but don't provide IStructuralEquality then after this PR will use the Equality System.Collections.Generic.EqualityComparer<'T>.Default. This can make a real difference (faster) for value types. Same rules will apply for comparison, but not implemented yet. I think it would be helpful to add a new attribute which removes the implementation of IStructuralEquatable and IStructuralComparable so that it could benefit (and really it's a bit of a misnomer, as you still get structural equality through the standard Equals - what if different is how they handle NaNs) (this is a little bit more to this story, but basically that you be how the 99% would be affected - and even then there are, from memory, inconsistencies with generic types, etc. I posted some of there ages ago, I can probably dig them up)

[manofstick]

@dsyme

Sorry, WIP, so I will bash my way through failures as I get time...

Oh, currently this is just equality (GetHashCode/Equals + associated operators) but I plan on getting to comparison (CompareTo + associated operators). Should that be done as a seperate PR? I think there is value in that, shrinking the surface area of an individual change?

[dsyme]

Oh, currently this is just equality (GetHashCode/Equals + associated operators) but I plan on getting to comparison (CompareTo + associated operators). Should that be done as a seperate PR? I think there is value in that, shrinking the surface area of an individual change?

If the technique is the same then do it in the same PR

[manofstick]

@dsyme

This is getting close to the territory we would accept

Hahahah! I guess I have been a little... ummm... adventurous (?) with my PRs. Anyway, I always kind of intended them to be group efforts and really I see myself throwing out ideas as much as anything... But hey, after 3 years I guess this just isn't they way that this open source stuff works. Get an idea and polish it off yourself or be damned :-) (obviously a bit tongue in cheek - I didn't have a number of helpers on the Seq work...)

if backed up by perf results

Let's drag up some of the old test from the previous PRs... (micro benchmarks, blah, blah, blah - I haven't run these extensively, numbers jump around, looking for overall trend... at the end I realized I shouldn't have had as many significant figures in the %s as really everything is +/- a few %... But anyway...)

In #574 we are linking to this gist:

Test	Old 64-bit	New 64-bit	New/Old 64-bit	Old 32-bit	New 32-bit	New/Old 32-bit
custom dynamic	188.67	113.67	60.25%	452.67	106.17	23.45%
custom structural	190	136.17	71.67%	457	221.83	48.54%
custom default	119.83	117.83	98.33%	110.83	109	98.35%
value dynamic	864.17	917.67	106.19%	2487.83	2403	96.59%
value structural	483.33	481.83	99.69%	548.17	542.83	99.03%
value default	451	445.83	98.85%	447.67	445.33	99.48%
gen value dynamic	1846.33	1052.67	57.01%	5303	2639.33	49.77%
gen value structural	1397	629.5	45.06%	3262.5	920.67	28.22%
gen value default	1150.67	447.17	38.86%	2018	477	23.64%
ref dynamic	959.17	1242.5	129.54%	2469.33	2344.33	94.94%
ref structural	706.83	699.33	98.94%	697.33	698.17	100.12%
ref default	751.5	742	98.74%	773	751.5	97.22%
gen ref dynamic	1996.83	1396.17	69.92%	5352.5	2550	47.64%
gen ref structural	1726.5	921.67	53.38%	3907	1412.17	36.14%
gen ref default	1732	815.83	47.10%	2597.17	878.67	33.83%
tuple dynamic	565	594.83	105.28%	1157	1163.33	100.55%
tuple structural	245.67	242.5	98.71%	253.83	251.67	99.15%
tuple default	682.5	676.83	99.17%	761.67	753	98.86%
value tuple dynamic	391	147.67	37.77%	988	144.83	14.66%
value tuple structural	386.17	147.33	38.15%	981.83	143.67	14.63%
value tuple default	146.83	147.17	100.23%	151	145	96.03%

In #549 where I've copied the code to this gist:

Test	Old 64-bit	New 64-bit	New/Old 64-bit	Old 32-bit	New 32-bit	New/Old 32-bit
seqGroupBy	1710	674	39.42%	1011	986	97.53%
seqCountBy	2793	676	24.20%	1391	1296	93.17%
listGroupBy	1701	716	42.09%	857	758	88.45%
listCountBy	2490	457	18.35%	1056	949	89.87%
arrayCountBy	2340	304	12.99%	892	818	91.70%
arrayGroupBy	1241	217	17.49%	470	432	91.91%
arrayCountBy	870	384	44.14%	544	525	96.51%

In #930 where I've copied the code to this gist:

Test	Old 64-bit	New 64-bit	New/Old 64-bit	Old 32-bit	New 32-bit	New/Old 32-bit
Perf	11149	4822	43.25%	20301	5512	27.15%

In #513 we're I've copied the code to this gist:

Test	Old 64-bit	New 64-bit	New/Old 64-bit	Old 32-bit	New 32-bit	New/Old 32-bit
StructureInt	2578	2639	102.37%	2785	2664	95.66 %
StructureGeneric	5635	2891	51.30	8990	3083	34.29 %

More in #513 where I've copied the code to this gist:

Test	Old 64-bit	New 64-bit	New/Old 64-bit	Old 32-bit	New 32-bit	New/Old 32-bit
Perf	503	301	59.84%	847	288	34.00%

More in #513 where I've copied the code to this gist:

Test	Old 64-bit	New 64-bit	New/Old 64-bit	Old 32-bit	New 32-bit	New/Old 32-bit
Perf	3557	710	19.96%	7510	915	12.18 %

[manofstick]

@dsyme

(seems to be a build server issue, and builds/tests on my machine, and the build before the failure had a 120 minute timeout and thus was incomplete... that wasn't this PR, but something to do with Span...)

Anyway, I'd be happy if I could leave this here for the moment. i.e. I have intentions of doing the comparison side of things, but this stands on its own, and I don't think I currently have the time to do the work (from my, admittedly old, memory I did an initial phase of refactoring out the exception throwing throwing behaviour and I think a bit more refactoring before being able to effectively do the same process as here)

So I'm changing the title of this PR to reflect that it is just equality... and also that it isn't a WIP anymore...

[dsyme]

I'm not totally comfortable with these tailcalls disappearing - have we discussed this? Thanks

[dsyme]

@manofstick I see we talked about the tailcall removal in the context of #513: I added this issue here to track it #946.

My specific reply was here: #946 (comment)

Actually maybe this isn't too bad after all. I it seems that the particular form listed above is possibly the only case that currently works and after #513 doesn't.

.. .If a generic argument is instantiated to a record or union in a way that makes the type recursive, then it current works, but now doesn't..... I would be willing to accept that limitation (he says, with his fingers crossed behind his back)......

I am also willing to accept this limitation. I think the best thing is to submit testing that pins down this behaviour in positive cases systematically.

Still, I wonder if we should make the tailcall removal a separate PR? In case we needed to revert it at a later point?

Thanks!
Don

[dsyme]

@dotnet-bot test this please

[manofstick]

@dsyme

Hmm. Removing the tail call avoiding code does smash performance, in many cases making things worse than they were.

The 64-bit JIT now appears to do tail calls internally as what would of causes a stack overflow under 32-bit, doesn't under 64-bit.

I create some example types here.

Anyway, I then modified all calls in FSharp.Core to be of the form:

        let GenericHashIntrinsic input =
            if Environment.Is64BitProcess
            then avoid_tail_call_int (fun () -> GenericHashT<'T>.GetHashCode input)
            else GenericHashT<'T>.GetHashCode input

Which, albeit a bit ugly, does work.

Thoughts?

[zpodlovics]

@manofstick Yes, tail calls prevent inlining - hopefully will be revisited soon:

https://github.com/dotnet/coreclr/issues/18361#issuecomment-396060473
https://github.com/dotnet/coreclr/issues/18406

[mrange]

@manofstick I applaud your efforts in trying to optimize the big and the small! It's a dangerous game you play (might break things) but someone has to do it :)

Btw in the referenced issue I actually saw performance drop when surpressing .tail. It's not easy to predict performance.

[AndyAyersMS]

I would suggest leaving the tail prefix alone as not all jits are going to be able to recognize tail call opportunities without this prefix.

For cases where performance might improve without tail I suspect the real issue is that tail blocks the jit (at least in RyuJit, likely also jit32) from inlining:

• the jit will not inline at call sites with a tail prefix
• the jit will not inline methods that contain call sites with tail prefixes.

These are things we can revisit on .Net Core (see dotnet/coreclr#18406).

Since we generally only see tail prefixes from F# code it would be great to have more examples like dotnet/coreclr#18361 to look at.

[manofstick]

OK; had some good progress. Basically scrapped everything (well except the core idea) and re-implemented.

So, to make everyone happy I have restored tail calls which now doesn't have as much of an impact on performance as I have managed to drop one level of call indirection in some cases - but looking with great anticipation for when @AndyAyersMS can implement some optimizations in the JIT!

@mrange - yes, yes I am a sucker for punishment :-) But there is a fairly significant set of tests I put in when I working on #513, so hopefully have a reasonable degree of confidence...

[dsyme]

This looks very promising, will review more completely. Can you report latest performance results? thanks

[dsyme]

Nit: This is odd formatting for an object expression, we would always use

{ new X with 
    member ...
}

or

{ new X with 
    member ...     }

If piping is defined at this point then the box can be like this:

{ new X with 
    member ...     }
|> box

[manofstick]

I don't think piping is available in prim_types but I'll see... I'll move the brace down anyway...

[dsyme]

I'm aware that this is now more expensive initially: we have the table lookup followed by an indirect call, before getting to GenericEqualityObj.

Am I correct that that's the fundamental tradeoff inherent to the PR? i.e. those costs are small compared to subsequent savings of not boxing (for primitive value types) and the advantages of using the default equality comparer (for other types)?

[manofstick]

There is a slight cost, but the cost of boxing (for value types) and type-based pattern matching (in GenericEqualityObj) are an order of magnitude higher (he says without actually running the numbers, but from general feel.)

Also remember that these functions are only kicking in when equality doesn't know the type at compile time, so it's not a hit all over the place.

[dsyme]

Please use Type[]

[manofstick]

Really? I haven't been following any F# formatting recommendation discussion but find that a bit surprising. I really like the consistency of array with list and just generics in general. Anyway, if that's the way that it goes then happy enough to change it.

[dsyme]

wonky formatting here

[dsyme]

Using true && ... and false || ... is a bit odd, just remove

[manofstick]

The wonky formatting is what is aligned with - i.e what the not, && or || corresponds to. I find it much easier to read than any other formatting - but happy to change it whatever you want.

The true at the start of && and the false at the start of || allow you to comment out any of the corresponding lines without changing any other lines - the compiler is smart enough to remove them - but if you don't like 'em I'll get rid of them.

[dsyme]

So I assume this means non-type-specialized hash/eq/compare on keys that are instances of F# structural record and union types (e.g. option values) are a little bit slower overall? But the F# compiler de-virtualizes these normally? thanks

[manofstick]

Not too sure what your saying, I can't use EqualityComparer.Default on records and union types because they have IStruturalEquality which has different ER/PER symantics:

type Boo = Boo of float
let nan = Boo System.Double.NaN
printfn "%b" (HashIdentity.Structural.Equals (nan, nan))
printfn "%b" (System.Collections.Generic.EqualityComparer.Default.Equals (nan, nan))

// false
// true

They are de-virtualized by the compiler under normal conditions - none of the things I'm changing should change generated code (which is borne out by no changed to the IL as part of the test suite)

[dsyme]

Why do you need to special case ValueTuple? There is no corresponding ValueTuple case in GenericEqualityObj?

Have you put this in because ValueTuple is known to respect structural checking, i.e. if T1 and T2 support default comparison then ValueTuple<T1,T2>? Would it make sense to add Result and ValueOption to this table too?

[dsyme]

I was confused about whether this code applies to enum and value types. I see // covers enum and value types but it seems it applies to reference types as well? Could you add more comments in this please?

Also the double negative not (..... isValidGenericType false "System.ValueTuple1")` is really confusing and something feels wrong here, How can we have both

 not (isValidGenericType false "System.ValueTuple`1" || .... )

and

isValidGenericType true "System.Nullable`1"

The recursive call to isValidGenericType seems to be used in both a positive and negative sense in these different cases. Simplify/clarify the code please so it's really easy to think about, since this routine is really important to have correct.

[manofstick]

ValueTuples have nice implementation of IEquatable<>.Equals which utilizes EqualityComparer<'Tn>.Default so if all the 'Tns are safe (which we check recursively) then we get a really quick comparison. Unfortuantely standard tuple in its IEquatable<>.Equals method uses EqualityComparer.Default on all Tns, so really doesn't help. I haven't looked at Result and ValueOption.

The second parameter in isValidGenericType is if the root type doesn't match (i.e. is the type Nullable? no, then return true so check the next && condition - or is the type a ValueTuple? no, then return false to check the next || condition...), otherwise the result of the function is true if all it's generic arguments are good or false if one of them fails.

The && not (false followed by the ||s can be read as unless ...

I did play around a bit with this whole logic. I'm not sure if there is a way to have it so that on first glance it reads well, but I think the combination of the wonky formatting :-) and maybe a couple of comments might help? I don't know, if you think you can make it much easier then feel free to mess around with it. I just think the first way that it clicks in your head is the easiest - which might be different for different people so not sure of the gain of changing it? But happy to do so.

But I will fix up some comments.

[mrange]

@manofstick Perhaps we should start looking at dotnet core jitter? Improve how it handle .tail calls to improve for F#? I am interested in doing something but is looking for an idea of limited scope to get started with.

[manofstick]

Updated all the perf tests. Lots of transcribing by hand as I haven't automated this so might be so copy and paste errors, etc. But think it should be pretty good.

So worse, some better. The final test was a lot better. Should get even better if there is some work done to improve tail calls in the JIT!

In #574 we are linking to this gist:

Test	Old 64-bit	New 64-bit	New/Old 64-bit	Old 32-bit	New 32-bit	New/Old 32-bit
custom dynamic	199.83	120.33	60%	474.33	113.17	24%
custom structural	197.50	136.83	69%	478.17	277.17	58%
custom default	123.17	123.67	100%	114.83	112.67	98%
value dynamic	917.67	933.83	102%	2692.67	2365.50	88%
value structural	497.83	501.00	100%	566.00	577.67	102%
value default	467.17	465.67	99%	459.33	481.67	105%
gen value dynamic	1906.33	1273.67	67%	5626.83	3186.83	57%
gen value structural	1435.50	838.00	58%	3329.50	1702.00	51%
gen value default	1328.17	572.00	43%	2231.50	603.33	27%
ref dynamic	1046.00	1002.67	96%	2632.83	2289.00	40%
ref structural	724.83	740.00	102%	722.50	722.00	18%
ref default	768.33	783.50	101%	780.50	774.50	99%
gen ref dynamic	2167.33	1469.83	68%	5697.17	3055.83	54%
gen ref structural	1809.33	1135.83	63%	4123.67	2231.50	54%
gen ref default	1867.33	1011.67	54%	2761.67	1079.33	39%
tuple dynamic	604.00	593.17	98%	1252.33	1208.67	96%
tuple structural	254.67	251.33	99%	261.50	261.00	100%
tuple default	720.17	706.83	98%	792.33	792.33	100%
value tuple dynamic	436.67	149.50	34%	1052.50	152.67	15%
value tuple structural	423.50	150.67	36%	1046.83	153.17	15%
value tuple default	151.00	149.33	99%	153.17	154.33	100%

In #549 where I've copied the code to this gist:

Test	Old 64-bit	New 64-bit	New/Old 64-bit	Old 32-bit	New 32-bit	New/Old 32-bit
seqGroupBy	1796	646	36%	1023	947	93%
seqCountBy	2813	675	24%	1400	1251	89%
listGroupBy	1739	705	41%	851	793	93%
listCountBy	2527	458	18%	1040	936	90%
arrayCountBy	2375	286	12%	900	796	88%
arrayGroupBy	1230	214	17%	471	429	91%
arrayCountBy	890	378	42%	537	522	97%

In #930 where I've copied the code to this gist:

Test	Old 64-bit	New 64-bit	New/Old 64-bit	Old 32-bit	New 32-bit	New/Old 32-bit
Perf	11142	4904	44%	20995	5470	26%

In #513 we're I've copied the code to this gist:

Test	Old 64-bit	New 64-bit	New/Old 64-bit	Old 32-bit	New 32-bit	New/Old 32-bit
StructureInt	2611	2634	100%	3085	2831	92%
StructureGeneric	5749	3536	62%	8585	4366	51%

More in #513 where I've copied the code to this gist:

Test	Old 64-bit	New 64-bit	New/Old 64-bit	Old 32-bit	New 32-bit	New/Old 32-bit
Perf	509	329	65%	862	404	47%

More in #513 where I've copied the code to this gist:

Test	Old 64-bit	New 64-bit	New/Old 64-bit	Old 32-bit	New 32-bit	New/Old 32-bit
Perf	3684	287	8%	7652	456	6%

[manofstick]

@mrange Yeah I think you might be right, but besides allowing inlining on methods with tail instructions I'm not sure what the path would be (and hey, if @AndyAyersMS is going to look after those then woo hoo!)

The problem, as I understand it (which, believe me, is rather limited!), is that the calling convention under the 64-bit JIT is that anything wider than 64-bits is passed on the stack (where-as otherwise the first 4 (?) 64-bit or less arguments are just passed on registers), and its this spilling onto the stack which causes grief. There is some __vectorcall calling convention which allows wider things, but hey, I know as much about that as my insights into the meaning of life...

So yeah, as always, willing to give anything a crack, but I'm (currently!) not standing on the shoulders of any giants... Giants. Damn Giants. Come over here...

[manofstick]

@dsyme

OK; few more tweeks, but think now I'm done. I made changes based on your comments as well as some additional code. The changes have resulted in more improvements across the board, so I'll redo the results soon - I have also added an extra test.

Oh, and I think there was a problem with the build server, which is why some of the builds failed, so maybe after the weekend they could be trigger to start again...

[manofstick]

Added new test that checks comparison of list<option<T>> for various Ts from simple types to tuples, struct tuples and custom types. The test is available here. Not that where the times are > 100% this is due to tail calls on 64-bit where the underlying data-type is a value type where its size > 64-bits.

Test	Old 64-bit	New 64-bit	New/Old 64-bit	Old 32-bit	New 32-bit	New/Old 32-bit
int	1831	671	37%	5435	2439	45%
float	1837	926	50%	5472	3795	69%
struct int*int	3479	1230	35%	8911	3079	35%
struct int*int64	13528	18486	137%	9138	3200	35%
struct int*float	13682	21521	157%	8800	8216	93%
struct int64*int64*int64*int64*int64*int64*int64*int64	20237	19626	97%	24513	5498	22%
struct float*float*float*float*float*float*float*float	20285	28992	143%	22526	22137	98%
int*int	3343	3021	90%	8900	8348	94%
int*float	3368	3008	89%	8595	8042	94%
int64*int64*int64*int64*int64*int64*int64*int64	8972	8928	100%	22091	21023	95%
float*float*float*float*float*float*float*float	9207	8600	93%	19977	18985	95%
TestRecordData	1384	1317	95%	5729	4542	79%
TestGenericRecordData	3366	1495	44%	11161	6482	58%
TestUnionData	1900	1907	100%	5802	5094	88%
TestGenericUnionData	2605	1991	76	7503	5739	76%

[KevinRansom]

@manofstick , @dsyme , @cartermp - what do we want to do with this PR? It looks like we got quite close, then it just lost momentum? Do we want to resurrect it?

Thanks

KEvin