More Efficient algorithm for List.groupBy

[liboz]

Also fixes a related unit test.

Here's a gist showing the difference: https://gist.github.com/liboz/d1f2ef35c9c46a5019127bd3b4f99d17
Note that I just made a clone of the original List.groupBy called List.groupBy2 for the purposes of the test

There seems to be inconsistent inlining in the List module, but I inlined the stuff I did here because the original was inlined.

Will edit in a benchmark with BenchmarkDotNet in a couple hours.

EDIT:

Here's the source code for the BenchmarkDotNet: https://gist.github.com/liboz/d1d513c378aefbc78889f2db27f2800f
and I pasted the results:. It's mostly the same test suite as the fsi gist I have above.

CLR=MS.NET 4.0.30319.42000, Arch=64-bit RELEASE [RyuJIT]
GC=Concurrent Workstation
JitModules=clrjit-v4.6.1080.0

Type=groupBy Mode=Throughput GarbageCollection=Concurrent Workstation

Method	count	Median	StdDev	Gen 0	Gen 1	Gen 2	Bytes Allocated/Op
groupByModulus	10	705.8428 ns	42.6993 ns	0.03	-	-	469.55
groupByOriginalModulus	10	1,839.0410 ns	93.7306 ns	0.04	-	-	763.10
groupBy	10	1,255.7102 ns	61.1925 ns	0.07	-	-	1,232.88
groupByOriginal	10	3,987.1858 ns	160.5903 ns	0.09	-	-	1,610.93
groupByMixed	10	582,407.5966 ns	30,728.3895 ns	6.92	4.30	-	337,104.46
groupByOriginalMixed	10	736,841.4709 ns	37,799.7629 ns	8.59	5.55	-	379,064.74
groupByModulus	100	4,406.7663 ns	169.0106 ns	0.15	-	-	2,729.34
groupByOriginalModulus	100	7,344.0738 ns	255.2806 ns	0.20	-	-	3,557.86
groupBy	100	10,367.8809 ns	381.3533 ns	0.62	-	-	11,856.14
groupByOriginal	100	32,412.6751 ns	1,597.1644 ns	0.85	-	-	16,286.61
groupByMixed	100	572,375.6532 ns	22,456.2116 ns	6.59	4.49	-	332,997.24
groupByOriginalMixed	100	741,901.9452 ns	35,264.1825 ns	5.69	4.86	-	288,494.12
groupByModulus	10000	525,283.8138 ns	28,838.6492 ns	5.29	3.39	-	254,351.92
groupByOriginalModulus	10000	664,096.0494 ns	26,908.3865 ns	7.44	4.89	-	328,690.80
groupBy	10000	2,761,663.8746 ns	64,557.9006 ns	17.59	-	17.50	1,213,041.70
groupByOriginal	10000	5,317,136.8731 ns	254,277.2331 ns	20.54	4.88	16.69	1,516,082.90
groupByMixed	10000	1,300,531.4866 ns	61,808.7686 ns	11.74	11.40	-	794,595.49
groupByOriginalMixed	10000	1,630,609.7672 ns	73,949.6140 ns	16.05	11.84	-	896,730.53
groupByModulus	1000000	231,907,137.1163 ns	4,046,452.1418 ns	301.91	225.42	108.98	19,284,699.38
groupByOriginalModulus	1000000	231,722,288.2738 ns	2,443,656.7017 ns	274.10	188.98	119.02	19,670,409.44
groupBy	1000000	804,278,639.9725 ns	22,017,307.4191 ns	656.45	578.39	118.52	64,999,778.71
groupByOriginal	1000000	1,191,626,049.8725 ns	13,775,845.9996 ns	1,125.25	736.46	136.33	87,812,227.41
groupByMixed	1000000	688,763,766.6713 ns	21,067,158.4101 ns	781.33	661.33	146.00	52,232,920.00
groupByOriginalMixed	1000000	635,669,734.7538 ns	22,632,039.2767 ns	964.44	775.56	209.78	69,421,290.06
groupByModulus	10000000	2,536,577,746.9513 ns	62,383,748.7772 ns	1,919.75	3,452.31	148.25	192,795,744.98
groupByOriginalModulus	10000000	2,696,990,104.5825 ns	58,256,910.0715 ns	3,744.00	3,749.00	358.00	326,685,879.41
groupBy	10000000	7,250,552,647.7425 ns	245,053,956.2612 ns	7,797.71	8,358.86	212.00	808,063,523.82
groupByOriginal	10000000	11,373,338,310.5325 ns	255,350,492.3006 ns	11,926.00	8,566.67	192.00	931,257,868.36
groupByMixed	10000000	6,623,608,707.1300 ns	212,181,053.7540 ns	6,827.45	7,176.06	180.74	503,127,305.38
groupByOriginalMixed	10000000	6,849,243,043.4850 ns	134,781,808.1311 ns	7,022.13	7,195.38	198.00	588,532,280.50

[msftclas]

Hi @liboz, 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;

[forki]

ok the appveyor fail is probably unrelated, but still interesting. It seems property-based testing revealed an issue with Async.Choice.

 1) Error : FSharp.Core.Unittests.FSharp_Core.Microsoft_FSharp_Control.AsyncModule.Async.Choice specification test
System.Exception : Falsifiable, after 45 tests (0 shrinks) (StdGen (1588791491,296183942)):
Original:
ChoiceWorkflow
([NoneResultAfter 0; NoneResultAfter 7; SomeResultAfter -8; SomeResultAfter -1;
ExceptionAfter 9; ExceptionAfter 7; NoneResultAfter -11; NoneResultAfter -1;
SomeResultAfter 6; SomeResultAfter -9; ExceptionAfter -2; ExceptionAfter 8;
NoneResultAfter -10; NoneResultAfter 0],Some 10)
with exception:
NUnit.Framework.AssertionException: Expected: less than or equal to 4
But was: 5

@eiriktsarpalis any ideas what this might be?

[forki]

@eiriktsarpalis I tried to reproduce the test in #1379 - all green. meh.

[eiriktsarpalis]

@forki I'm pretty certain that those values should not be negative. They represent timeouts.

[forki]

then the pbt should be narrowed?

[forki]

mhm the normalize function is already using abs to make that positive

[eiriktsarpalis]

yeah, just remembered that now. No problem then.

[forki]

but it still crashed!? do we ignore that?

[smoothdeveloper]

Haven't look at the test but if we are chasing a race condition, we should run the code many many times for more chances to reproduce it isn't it?

[eiriktsarpalis]

@forki sorry, I meant no problem w.r.t. the tested inputs obviously.

[eiriktsarpalis]

It is possible for the test to fail, provided that the thread pool behaves pathologically enough. I have made the durations be multiples of 2 seconds, assuming that it's way larger than any possible delay introduced by the thread pool. Might be related to this issue http://stackoverflow.com/questions/17429246/preventing-thread-pool-starvation-with-a-timer-running-at-same-time-as-parallel

[forki]

let's continue discussion on #1379 - it's unrelated to this PR

[liboz]

Well that's unexpected to reveal a random other bug as well with this.

[dsyme]

You evaluate ie.Current three times on each iteration - please evaluate it only once.

[KevinRansom]

This is a huge method to inline is it really advantageous to inline it?

[liboz]

@KevinRansom So, the original function was inlined, which is why I have this one inlined. If this function isn't inlined, then the inlining on the function in list.fs doesn't do anything and there is probably an over 15% performance hit in a simple benchmark I tried.

[liboz]

Actually I reran some tests and inline seems to not make any difference. I've removed it.

[liboz]

It appears Async.Choice failed on AppVeyor.

[KevinRansom]

@dotnet-bot test this please

[KevinRansom]

This looks good to me.