Faster parallel array partition and choose

[jackmott]

Runtime is about 1.3x faster on my machine (4 core i7 mobile), with no memory use penalty. Would love input on this one, there may be some ways to improve on the approach. I am deep in uncharted Parallel.For territory.

BenchmarkDotNet results, partitioning a random int array with (fun x -> x % 2 = 0)

Note that the benchmark results are somewhat stochastic, they will bounce around from 35% to 10% better each time I run the suite, there doesn't appear to be a general trend where it gets better or worse moving from 1 million to 10 million. I've also used more complex predicates and the relative differences are similar.

Host Process Environment Information:
BenchmarkDotNet=v0.9.8.0
OS=Microsoft Windows NT 6.2.9200.0
Processor=Intel(R) Core(TM) i7-4712HQ CPU 2.30GHz, ProcessorCount=8
Frequency=2240907 ticks, Resolution=446.2479 ns, Timer=TSC
CLR=MS.NET 4.0.30319.42000, Arch=64-bit RELEASE [RyuJIT]
GC=Concurrent Workstation
JitModules=clrjit-v4.6.1590.0
Type=SIMDBenchmark Mode=Throughput Platform=X64 Jit=RyuJit GarbageCollection=Concurrent Workstation 

Partition

Method	Length	Median	StdDev	Scaled	Gen 0	Gen 1	Gen 2	Bytes Allocated/Op
Original	1000	21.8514 us	0.5300 us	1.00	0.16	-	-	3,471.77
New	1000	20.5297 us	0.8840 us	0.94	0.17	-	-	3,489.75
Original	10000	160.0466 us	3.1249 us	1.00	1.21	-	-	28,955.03
New	10000	118.1885 us	2.8572 us	0.74	1.20	-	-	28,666.02
Original	100000	1,282.9827 us	7.3705 us	1.00	-	-	10.17	211,334.08
New	100000	917.0063 us	17.4501 us	0.71	-	-	7.27	151,441.53
Original	1000000	12,467.9427 us	728.8799 us	1.00	-	-	65.99	2,353,833.73
New	1000000	9,700.7108 us	990.4339 us	0.78	-	-	65.24	2,309,151.64
Scalar	1000000	13,680.7468 us	212.0151 us	1.10	3.25	-	37.57	4,361,586.45
Original	10000000	125,043.1745 us	1,753.3497 us	1.00	-	-	35.28	29,670,713.02
New	10000000	86,908.7271 us	1,472.4448 us	0.70	-	-	35.53	29,909,345.33

Choose

Method	Length	Median	StdDev	Scaled	Gen 0	Gen 1	Gen 2	Bytes Allocated/Op
Original	1000	24.1173 us	1.8355 us	1.00	0.52	-	-	13,721.83
New	1000	25.8602 us	0.3443 us	1.07	0.47	-	-	12,292.06
Original	10000	235.2287 us	4.9615 us	1.00	4.55	-	-	157,065.29
New	10000	190.6397 us	5.1752 us	0.81	4.72	-	-	167,680.01
Original	100000	2,352.3452 us	58.5964 us	1.00	18.99	-	10.23	1,206,016.17
New	100000	1,696.8924 us	38.6374 us	0.72	18.30	-	9.54	1,093,127.52
Original	1000000	23,316.6749 us	899.0359 us	1.00	269.10	1.62	41.29	14,659,078.49
New	1000000	15,062.9299 us	1,155.5808 us	0.65	294.00	1.39	46.05	15,348,631.44
Original	10000000	178,700.9122 us	24,916.3155 us	1.00	2,524.00	-	38.00	142,952,816.14
New	10000000	163,852.8507 us	17,740.5511 us	0.92	2,634.24	-	35.28	141,815,184.40

[msftclas]

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

[jackmott]

Whats does this mean, from the QA tests. Something about assembly version being wrong, what would cause that?
"Conformance\DeclarationElements\CustomAttributes\AttributeUsage (AssemblyVersion03.fs) -- failed"

[forki]

Probably unrelated

[jackmott]

Going to probably pull in the same trick for Parallel.choose in a minute.

[ReedCopsey]

Note that using Parallel.For can have bad repercussions in a situation where the algorithm is being used in a threaded context. If this is called on a thread while the other threads are already saturated via CPU usage, best case, it'd be the same as doing it in serial, and worst (and more likely) case, significantly slower.

I'd be very careful about using Parallel loops within core library functions, since you can't control where they're going to be used, and this could be disastrous when used in highly threaded code. It's also unexpected - you'd expect to be safe using core library functions when threading at a "higher level" in your application, and this would make that assumption invalid.

[jackmott]

It was a Parallel.For originally, I just pulled the accumulation into it, rather than iterating over the array again to count it up afterwards.

Also this is in a separate sub module - Array.Parallel, which was already in place, so chance of accidental improper usage is low.

I agree with your points completely though, and was thinking of blogging about that very issue!

[cartermp]

@jackmott This is fantastic, as with your other PRs thus far. 👍

Could you also add your test script to this PR and further PRs? @smoothdeveloper and I were chatting, and he had mentioned that it would be great to have the perf test scripts for these changes in the repo so we can use them as a baseline for further improvements.

The location for this would be in tests/scripts/{some-folder-name-which-makes-sense}.

Thanks!

[jackmott]

@cartermp do you mean a script to test that performance has not regressed? Or for correctness? The former, I'm not sure how to do, since the old function wouldn't exist any more. The latter I assume already exists, since I haven't added a new function. Happy to help, need guidance. Thanks!

[KevinRansom]

Not a test case, more an indication of how the numbers were gathered.

It gives other developers an idea about how to go about testing their performance improvements.

Kevin

From: Jack Mott [mailto:notifications@github.com]
Sent: Friday, August 12, 2016 1:54 PM
To: Microsoft/visualfsharp visualfsharp@noreply.github.com
Subject: Re: [Microsoft/visualfsharp] Faster parallel array partition and choose (#1423)

@cartermphttps://github.com/cartermp do you mean a script to test that performance has not regressed? Or for correctness? The former, I'm not sure how to do, since the old function wouldn't exist any more. The latter I assume already exists, since I haven't added a new function. Happy to help, need guidance. Thanks!

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHubhttps://github.com//pull/1423#issuecomment-239557184, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AE76FqMB2ilp5Rnv74NQ5qGXi2MFhG0Rks5qfN19gaJpZM4Jisxr.

[jackmott]

Gotcha. Would it be ok to pull BenchmarkDotNet into the solution? Are there other perf tests scripts I can use as a guide?

[ReedCopsey]

@jackmott Oh, great - sorry - just saw "for Array.choose" and wasn't thinking this was for Array.Parallel. Sorry about that! This is a great improvement - using the accumulators is definitely better.

[cartermp]

@jackmott I'd be fine with a package reference. I think it would be best if the .dll for it wasn't checked in, though. As for examples, it looks like all we have is Compiler Perf Scripts. Not too much to go off of, so I think it's safe to say that you could do whatever you feel is reasonable and it'll be fine.

Biggest thing is just to ensure that anyone can clone the repo and begin running the scripts to see performance results themselves. I'm also happy to iterate here and just get some scripts checked in while we figure out a good solution.

[jackmott]

Ok I will give it a go

[KevinRansom]

Given that the tests will run on different hardware, and that there is a before and after component. Seeing the results is less interesting than seeing the methodology. And the ability to examine the test code to look for where it may impzact the result demonstrated.

Kevin

From: Phillip Carter [mailto:notifications@github.com]
Sent: Friday, August 12, 2016 2:44 PM
To: Microsoft/visualfsharp visualfsharp@noreply.github.com
Cc: Kevin Ransom Kevin.Ransom@microsoft.com; Comment comment@noreply.github.com
Subject: Re: [Microsoft/visualfsharp] Faster parallel array partition and choose (#1423)

@jackmotthttps://github.com/jackmott I'd be fine with a package reference. I think it would be best if the .dll for it wasn't checked in, though. As for examples, it looks like all we have is Compiler Perf Scriptshttps://github.com/Microsoft/visualfsharp/tree/master/tests/scripts. Not too much to go off of, so I think it's safe to say that you could do whatever you feel is reasonable and it'll be fine.

Biggest thing is just to ensure that anyone can clone the repo and begin running the scripts to see performance results themselves. I'm also happy to iterate here and just get some scripts checked in while we figure out a good solution.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHubhttps://github.com//pull/1423#issuecomment-239567197, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AE76Fogf49IRykHqGp_rD9nUCCznM-nWks5qfOklgaJpZM4Jisxr.

[jackmott]

Sample perf test script added. Let me know if I should rename/relocate/revise.
It is not formatted as an fsx, because I never use the REPL so am not familiar with it, but I could learn!

[jackmott]

It would be neat if you had coverage of any perf relevant functions, and maybe weekly, check today's master against 1 week ago, report any that have regressed by some % for review. Massively time consuming test, couldn't do it on every PR/commit I don't think.

[KevinRansom]

@dotnet-bot test this please

[KevinRansom]

Appveyor Failure is due to appveyor timeout, it will be fixed by: #1425

[KevinRansom]

@dotnet-bot test this please

[veikkoeeva]

It would be neat if you had coverage of any perf relevant functions, and maybe weekly, check today's master against 1 week ago, report any that have regressed by some % for review.

@jackmott In that case dotnet/BenchmarkDotNet#155 looks like very promising.

[KevinRansom]

@jackmott If benchmark.net is a nuget package, then adding it will be great.

[jackmott]

@KevinRansom it is, but it pulls down a large set of dependencies I believe. Would you want it added to one of the existing projects? Create a new project with it inside the scripts folder?

[rojepp]

I might be mistaken, but shouldn't this be partitionNew ?

[jackmott]

yep!

[KevinRansom]

Just add it to packages.config. It’s a one time cost and I’m guessing it’s less than the cost of pulling down the enormous set of dependencies we currently pull. I appreciate that optimizers may grumble … but the payback of a standardized way of reproducing and reporting performance issues is going to be tremendous. Your performance PR’s are already much easier to approve than others because of it. So I’m all for adding it.

Kevin
From: Jack Mott [mailto:notifications@github.com]
Sent: Monday, August 15, 2016 10:55 AM
To: Microsoft/visualfsharp visualfsharp@noreply.github.com
Cc: Kevin Ransom Kevin.Ransom@microsoft.com; Mention mention@noreply.github.com
Subject: Re: [Microsoft/visualfsharp] Faster parallel array partition and choose (#1423)

@KevinRansomhttps://github.com/KevinRansom it is, but it pulls down a large set of dependencies I believe. Would you want it added to one of the existing projects? Create a new project with it inside the scripts folder?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com//pull/1423#issuecomment-239875660, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AE76FolXzl296zz8xS1DizSD_zOV8jmSks5qgKfkgaJpZM4Jisxr.

[jackmott]

All set here, benchmarkdotnet added to packges.config

[KevinRansom]

Looks great, thanks for this work

Kevin

[jackmott]

Anything else needed here? I think it is all ready.

[KevinRansom]

@jackmott
Thank you for this work

Kevin