FSharp.Core: Map: optimize tree layout

[buybackoff]

Following discussion and POC code from #5360 (comment)

Changes are very straightforward and do not touch public API:

• Performance improves by a huge margin
• Code size is smaller or same
• Memory is same
• No low level tricks, just simple code (see asNode comments for potential micro-optimizations, which are not visible after all; these comments are to be deleted)

Benchmarks code is here: https://github.com/buybackoff/fsharp-benchmarks

Method	Job	BuildConfiguration	Size	Mean	Error	StdDev	Rank	Gen 0	Gen 1	Gen 2	Allocated	Code Size
getItem	After	LocalBuild	100	36.21 ns	0.199 ns	0.167 ns	1	-	-	-	-	126 B
getItem	Before	Default	100	62.51 ns	0.143 ns	0.127 ns	2	-	-	-	-	126 B
getItem	After	LocalBuild	10000	76.57 ns	0.140 ns	0.124 ns	3	-	-	-	-	126 B
getItem	Before	Default	10000	120.02 ns	0.182 ns	0.170 ns	4	-	-	-	-	126 B
getItem	After	LocalBuild	10000000	129.45 ns	0.126 ns	0.118 ns	5	-	-	-	-	126 B
getItem	Before	Default	10000000	209.35 ns	0.496 ns	0.464 ns	6	-	-	-	-	126 B
containsKey	After	LocalBuild	100	35.63 ns	0.201 ns	0.188 ns	1	-	-	-	-	177 B
containsKey	Before	Default	100	64.01 ns	0.351 ns	0.328 ns	2	-	-	-	-	276 B
containsKey	After	LocalBuild	10000	65.63 ns	0.150 ns	0.125 ns	3	-	-	-	-	177 B
containsKey	Before	Default	10000	123.82 ns	0.149 ns	0.139 ns	5	-	-	-	-	276 B
containsKey	After	LocalBuild	10000000	95.05 ns	0.082 ns	0.072 ns	4	-	-	-	-	177 B
containsKey	Before	Default	10000000	204.39 ns	0.338 ns	0.282 ns	6	-	-	-	-	276 B
itemCount	After	LocalBuild	100	231.39 ns	0.406 ns	0.360 ns	1	-	-	-	-	96 B
itemCount	Before	Default	100	539.74 ns	1.923 ns	1.798 ns	2	-	-	-	-	151 B
itemCount	After	LocalBuild	10000	33,160.50 ns	194.709 ns	182.131 ns	3	-	-	-	-	96 B
itemCount	Before	Default	10000	63,074.34 ns	138.682 ns	129.724 ns	4	-	-	-	-	151 B
itemCount	After	LocalBuild	10000000	62,332,911.90 ns	252,973.481 ns	224,254.402 ns	5	-	-	-	148 B	96 B
itemCount	Before	Default	10000000	94,745,625.56 ns	205,640.690 ns	192,356.429 ns	6	-	-	-	-	151 B
iterForeach	After	LocalBuild	100	3,355.75 ns	9.540 ns	7.448 ns	1	0.9727	-	-	6120 B	291 B
iterForeach	Before	Default	100	3,866.56 ns	10.148 ns	8.996 ns	2	0.9689	-	-	6120 B	291 B
iterForeach	After	LocalBuild	10000	348,359.43 ns	1,148.753 ns	959.260 ns	3	95.2148	-	-	600120 B	291 B
iterForeach	Before	Default	10000	398,419.61 ns	513.959 ns	480.758 ns	4	95.2148	-	-	600120 B	291 B
iterForeach	After	LocalBuild	10000000	391,889,200.00 ns	1,604,306.946 ns	1,500,669.712 ns	5	95000.0000	-	-	600000120 B	321 B
iterForeach	Before	Default	10000000	445,099,028.57 ns	1,380,498.715 ns	1,223,776.153 ns	6	95000.0000	-	-	600000120 B	321 B
addItem	After	LocalBuild	100	181.25 ns	0.961 ns	0.899 ns	1	0.0586	0.0003	-	369 B	621 B
addItem	Before	Default	100	311.85 ns	0.601 ns	0.562 ns	2	0.0586	-	-	369 B	697 B
addItem	After	LocalBuild	10000	40,893.49 ns	174.683 ns	163.398 ns	3	11.0156	3.2813	-	69324 B	621 B
addItem	Before	Default	10000	71,746.33 ns	130.309 ns	121.891 ns	4	11.0156	3.3594	-	69324 B	697 B
addItem	After	LocalBuild	10000000	87,178,251.47 ns	250,148.324 ns	233,988.898 ns	5	18680.0000	960.0000	10.0000	117146915 B	621 B
addItem	Before	Default	10000000	146,799,424.80 ns	286,531.195 ns	268,021.458 ns	6	18680.0000	960.0000	10.0000	117146915 B	697 B
removeItem	After	LocalBuild	100	13.64 ns	0.112 ns	0.105 ns	1	0.0064	-	-	40 B	469 B
removeItem	Before	Default	100	16.38 ns	0.071 ns	0.067 ns	2	0.0064	-	-	40 B	519 B
removeItem	After	LocalBuild	10000	1,329.24 ns	9.087 ns	8.056 ns	3	0.6372	-	-	4000 B	469 B
removeItem	Before	Default	10000	1,607.21 ns	5.566 ns	5.206 ns	4	0.6372	-	-	4000 B	519 B
removeItem	After	LocalBuild	10000000	1,232,230.00 ns	6,303.414 ns	5,896.218 ns	5	630.0000	-	-	4000000 B	469 B
removeItem	Before	Default	10000000	1,801,088.33 ns	8,945.674 ns	8,367.789 ns	6	630.0000	-	-	4000000 B	519 B

[buybackoff]

Not sure why CI tests fail with:

src/fsharp/FSharp.Core/prim-types.fs#L3374
src/fsharp/FSharp.Core/prim-types.fs(3374,5): error FS0073: (NETCORE_ENGINEERING_TELEMETRY=Build) internal error: internal error: Map.spliceOutSuccessor (Failure)

I tested locally the FSharp.Core.UnitTests suite. The changes are below prim-types.fs and are internal to the Map file.

[abelbraaksma]

Wow, this is great! I've been wanting to take a look at this for some time. The numbers speak for themselves!

About the test: running it locally in VS typically runs it against one framework only. You can run them from the commandline to find out if you can get it to fail locally. See TESTGUIDE.md for some gotchas with running and analyzing tests.

[buybackoff]

@abelbraaksma I ran it from Rider from both net472 and netcoreapp3.1, with net5.0-rc1 installed.

Really could not afford running all the tests locally after every change, it's taking too long.

[cartermp]

Just a quick style pass for now - the code looks good, strange that an internal error is getting thrown. Sadly, FSharp.Core isn't the easiest thing in the world to update :(

[buybackoff]

@cartermp ctors are done

I've started the entire test suite locally after the first failure in CI.. and it's still running (35+ minutes), with some failures in random places, seemingly unrelated to my changes. At least directly. Maybe there are some deeper indirect checks that depend on the layout.

Without help I can't understand what's going on with the tests.

Yet all the test failures point to error: Map.spliceOutSuccessor, maybe I made a typo there and it's not covered in FSharp.Core.Test well. Will review tomorrow.

[cartermp]

Yeah, looking at the code this does feel like it is related to the changes. Subtle though. Thanks for looking into it, and thanks for the proposed changes so far! This could be really great. We also use the Map data structure very heavily internally, so this would improve the F# compiler itself, especially for tooling situations.

[buybackoff]

@cartermp if this goes well, I will follow up with 2-3 more things:

• comparer specialization if it's default. Similar to this and what S.C.G.Dictionary<,> does. Should be very visible for primitive keys.
• same changes for Set
• Optional: findLT/LE/GE/GT. Would you accept such public API additions?

Hope to make all tests passing over the weekend.

[buybackoff]

there was a typo indeed, most CI tasks are OK. The one that failed "early" (27 minutes) shows timeouts in logs. The one remaining is running for more than an hour already.

[forki]

IIRC there are two different map implementations in the source. If this one is green we may want to optimize the other as well.

[buybackoff]

@forki where is the second one? why it's not possible to use a single implementation?

[buybackoff]

Who could trigger a re-run of the failed task? (Or how could I do that without re-running the entire suite?)

[forki]

the other one is at: https://github.com/dotnet/fsharp/blob/main/src/utils/TaggedCollections.fs#L702

I don't know all the historical details, but one is used inside the compiler and the other one is in FSharp.Core for user code.

[buybackoff]

@forki

but one is used inside the compiler

So this means that what @cartermp mentioned:

We also use the Map data structure very heavily internally, so this would improve the F# compiler itself, especially for tooling situations.

requires updates to the internal one and FSharp.Core doesn't affect the compiler performance?

[forki]

well IIRC it's not 100% consistent. AFAIK later compiler phases use the one from FSharp.Core as well. But yes, if you want to make the compiler itself faster, then this internal Map needs to be touched as well

[abelbraaksma]

I've started the entire test suite locally after the first failure in CI.. and it's still running (35+ minutes),

I rarely do that. If you have failing tests, you probably didn't pass the -c Release argument. You said the failure was with FSharp.Core, in that case it should be enough to mimic the CI behavior locally by running:

build -testFSharpCore -ci -nobl -c Release

Quote from the test guide (which I've recently updated as I wanted to make it easier to run tests):

Tests are grouped as noted below. Some test groups can only be run in CI configuration, for that, you need to pass the -ci -bl or -ci -nobl arguments. Some test groups can only be run in Release mode, this is indicated below. Some tests can only be run on Windows.

I've worked a lot with the FSharp.Core tests, I can have a look if they continue to fail.

It's also possible to add a filter to locally running tests, but that's not yet in the test guide and non trivial to get right. I agree that it's annoying to wait so long for tests, that's why many people only run them after sending in a PR. Maybe with @vzarytovskii test improvements we can mark long running tests so that they can be skipped unless specified on the commandline.

[buybackoff]

well IIRC it's not 100% consistent. AFAIK later compiler phases use the one from FSharp.Core as well. But yes, if you want to make the compiler itself faster, then this internal Map needs to be touched as well

Could we extract MapTree module to a separate file, add all needed stuff to the common implementation from the two places, and include that module to two .fsprojs?

[buybackoff]

@abelbraaksma I was running local tests from Rider GUI, it works normally, the only thing is build and test times are very slow (not related to Rider).

[forki]

yes that's probably possible, but I remember @dsyme saying that he was actually preferring to keep it as two copies. Mostly because both things may be optimized differently in the future.

[forki]

@buybackoff in order to get the changes in I recommend to put any possible changes to that internal map thing into a separate PR. Also: to make things more interesting we have similar situation for sets ;-)

[buybackoff]

in order to get the changes in I recommend to put any possible changes to that internal map thing into a separate PR. Also: to make things more interesting we have similar situation for sets ;-)

Yes, I've mentioned Set above. Should be as simple as Map.

But before that I really want to see if this self-contained PR will be merged.

And for the next thing I want to play with comparer optimization when a comparer is default. It could then be used in all 4 cases 2xMap/2xSet.

[forki]

Yes. Small steps are important. Otherwise we never get anything merged Victor Baybekov <notifications@github.com> schrieb am Sa., 26. Sept. 2020, 12:17:

…

in order to get the changes in I recommend to put any possible changes to that internal map thing into a separate PR. Also: to make things more interesting we have similar situation for sets ;-) Yes, I've mentioned Set above. Should be as simple as Map. But before that I really want to see if this self-contained PR will be merged. And for the next thing I want to play with comparer optimization when a comparer is default. It could then be used in all 4 cases 2xMap/2xSet. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10188 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAOANEQQLEXML7ZAF3NEXDSHW5VDANCNFSM4R2O5NEQ> .

[buybackoff]

It's green! 🎉 What is needed to merge it?

[cartermp]

What is needed to merge it?

Just a review by two folks :)

[cartermp]

I don't know all the historical details, but one is used inside the compiler and the other one is in FSharp.Core for user code.

The code looks nearly identical and it is likely a relic of the past:

• Needed for compiler internals
• Eventually exposed for consumers but in FSharp.Core
• Reluctance to depend on the public one for fear that FCS consumers would need to have a baseline of the latest F# at the time

This is no longer a problem since we stopped really caring that the latest and greatest FCS can be built on very old FSharp.Core binaries. It was never really that useful of a scenario.

So the internal one can probably just be swapped out with this one once it is merged.

[cartermp]

I think this looks good. Thanks a lot! Will need another signoff on this though.

[vzarytovskii]

LGTM, but probably @TIHan or @KevinRansom should take a glance as well.

[buybackoff]

FYI, I had three more micro changes, but their impact is not very visible buybackoff@b167a47. Won't do anything with them.

Just wanted to use inlined IL. Must admit, for this PR I was finally nerd-sniped by this feature (as a replacement for S.R.CS.Unsafe discussed in the old linked thread, but ended up with simple cast in this PR), wanted to try it for a long time :)

[dsyme]

This is fantastic, thank you.

I presume both the Set in FSharp.Core and the compiler Map implementations could be given a similar treatment in separate PRs?

The compiler has separate implementations because it tracks the comparison function in the type logic, also for flexibility in changing representations for the particular quirks of the compiler loads (e.g. in case we found we couldn't change the FSHarp.Core ones for compat reasons, but could change the internal compiler ones). It's just part of the cost of maintaining at least some degree of agility.

[buybackoff]

I presume both the Set in FSharp.Core and the compiler Map implementations could be given a similar treatment in separate PRs?

Yes. If such PRs are merged easily I could do that. I was afraid there could be a long discussion on style, usage of null, inheritance, and requests for more and more tests, etc. Thank you for approving it quickly without all of that :)

3 more PRs will be with exactly same changes.