Mitigate #2924

[nicolasstucki]

To make the stack shorter we implement foldRightBN with
a foldLeftBN which allocates only half of the closures.
By doing so we need to eagerly reverse the list which is
in most cases small or empty.

[nicolasstucki]

test performance please

[dottybot]

performance test scheduled: 1 job(s) in queue.

[dottybot]

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3076 to see the changes.

Benchmarks is based on merge(s) with master

[allanrenucci]

Should be

final def foldLeftBN[U](acc: => U)(op: (=> U, T) => U): U = {
  ...
  case x :: xs1 => fold(xs1, op(acc, x))

That's what I meant by "accumulator is the first arg of the closure"

[nicolasstucki]

That is right, but if I do that the op must be put in another closure to reverse the parameter order.
I will make fold an inner method of foldRightBN and avoid this altogether.

[nicolasstucki]

test performance please

[dottybot]

performance test scheduled: 1 job(s) in queue.

[dottybot]

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3076 to see the changes.

Benchmarks is based on merge(s) with master

[nicolasstucki]

While executing vulpix run tests, foldRightBN is mostly called on lists of size 0 and 1. This does not count the recursive calls that would have been in the original implementation.

size->invocations
0->1111034
1->1081070
2->142603
3->44560
4->19410
5->8234
6->5164
7->2277
8->1635
9->963
10->789
11->542
12->433
13->292
14->239
15->198
16->214
17->159
18->177
19->153
20->199
21->178
22->145
23->93
24->27
25->35
26->150
27->27
28->27
29->7
30->28
31->24
32->24
33->3
35->9
36->9
37->5
39->9
40->2
43->5
45->7
46->28
49->38
51->3
52->31
53->4
54->5
56->3
57->7
58->7
61->3
62->14
65->7
67->2
69->5
70->7
73->3
79->3
86->7
95->3
109->14
111->7
164->3
453->7
523->3
1424->7

[allanrenucci]

LGTM

[biboudis]

@allanrenucci I was going to write on my review that we may need an extra test case. :-) Yesterday, we discussed with @nicolasstucki, that maybe an extra test would be needed to stress test this, but if it unblocks the CI then it is ok.

[allanrenucci]

@biboudis This does not solve the core issue but mitigates it. @nicolasstucki will try to solve the core issue. We can include the test case in the next PR.

I merged this one to unblock some CI builds.

[biboudis]

👍 👍