Making Map.unorderedTraverse stack safe

[TonioGela]

Fixes #4461.

The implementation is copied from List.traverse_.

I added just simple tests for the Map implementation, that is the only one that changed.
I'm not sure where to add tests for other unorderedTraverse implementations, they're not worth a TestLaw class IMHO.

[TonioGela]

The implementation calls the Map constructor and uses ++ to concat every couple of maps, so I expect it to produce a lot of GC pressure.
@johnynek should we write a chain-based implementation instead? WDYT?

[johnynek]

You can benchmark if you have time, but I bet the chain based will be faster.

In the case of List for a small to medium list it was faster IIRC to not convert back and fourth from Vector and Chain.

[TonioGela]

I noticed that the CI fails for native, I presume it's due to some stack size issue, not stack depth.
In any case, @johnynek I added a couple of benchmarks to the PR, that should test the 2 implementation, via chain and via tree, using 1_000 and 1_000_000 items maps.
WDYT? Are they worth running? I'll leave the laptop running them this night if you think they are :D

[johnynek]

I think they are worth running.

Thanks!

[johnynek]

I think chain.iterator.toMap would be more efficient potentially since it can use builder internally.

[TonioGela]

Seems reasonable, I'll use it for the bench!

[TonioGela]

I think they are worth running.

Thanks!

I'll update you tomorrow, thanks 👋

[TonioGela]

There was no need to wait a night, the bench took ~45 minutes.

These tests were done on a Apple M2 Pro
Scala Version: 2.13.11
JDK:

openjdk 17 2021-09-14
OpenJDK Runtime Environment Temurin-17+35 (build 17+35)
OpenJDK 64-Bit Server VM Temurin-17+35 (build 17+35, mixed mode)

These are the results:

[info] Benchmark                                                  Mode  Cnt        Score       Error  Units
[info] UnorderedTraverseMapBench.unorderedTraverseTupleViaChain1  avgt   25      144.320 ±     0.530  us/op
[info] UnorderedTraverseMapBench.unorderedTraverseTupleViaChain2  avgt   25   192889.802 ±  2467.065  us/op
[info] UnorderedTraverseMapBench.unorderedTraverseTupleViaTree1   avgt   25     1182.669 ±    14.740  us/op
[info] UnorderedTraverseMapBench.unorderedTraverseTupleViaTree2   avgt   25  2687201.422 ± 31730.496  us/op

They show that the unorderedTraverse that uses internally traverseViaChain is ~8 time faster on a 1000 element map and ~14 times faster for a 1_000_000 element map.

I'll use that implementation to fix the stack overflowing bug, as it could possibly solve the Scala Native issue in CI as I expect the Chain and .iterator.toMap to cause less GC pressure.

[johnynek]

thanks for doing this!

[TonioGela]

@armanbilge made me just notice that according to @djspiewak the M series is not great when it comes to bench marks: https://fosstodon.org/@SethTisue/110500223425754529
Apparently they're too optimized (it's like having a huge L3 cache) and as such they'll profoundly differ from the machines the code will tipically run on.
Atm I have no other machine to run the benchmarks on, so it's time for me to ask for some help :(
Can someone run these benchmarks on a non m1/2 machine?

(if we don't find out noone we can try to run them in CI, but the machines are pretty crappy)

[TonioGela]

I've managed to run these benchs on an Intel machine using java 11.0.11 2021-04-20 LTS

These are the results:

[info] Benchmark                                                  Mode  Cnt        Score       Error  Units
[info] UnorderedTraverseMapBench.unorderedTraverseTupleViaChain1  avgt   25      235.831 ±     6.017  us/op
[info] UnorderedTraverseMapBench.unorderedTraverseTupleViaChain2  avgt   25   274436.964 ±  5842.414  us/op
[info] UnorderedTraverseMapBench.unorderedTraverseTupleViaTree1   avgt   25     1883.380 ±    27.511  us/op
[info] UnorderedTraverseMapBench.unorderedTraverseTupleViaTree2   avgt   25  3733516.461 ± 23333.342  us/op

The ratios are more or less the same, 8 times faster in one case, 13 in the other

cc @armanbilge

[TonioGela]

any news on this?

[johnynek]

I don't know the rules these days but went ahead and merged. It's been a while, I reviewed, there were great benchmarks, and in the worst case we can amend.

Thank you!