Make groupsOf... family of functions fully tail recursive. #47
Conversation
This commit makes the `groupsOf...` family of functions fully tail recursive by forcing them to use the tail recursive version of List.take (normally List.take is only tail recursive for lists larger than 1000, but since the `groupsOf...` functions are themselves recursive this can result in potential call stack overflow from the successive accumulation of (up to) 1000-long non-recursive List.take calls during the recursion). This is an alternative to PR elmcraft#46 which would instead just add a note to the documentation warning users about the potential overflow.
|
Also (if you can be bothered or if anyone else wants to pitch in), I'd love to see a benchmark (see our benchmarks directory for samples) comparing the two versions on performance. |
|
There's never enough procrastination at work...
Source: https://gist.github.com/Janiczek/b8e3f66cba281aac1dba4ea7dd80b709 |
|
I'm also gonna run a benchmark on scaling the |
|
Code is in the same gist as |
|
From my point of view, let's ship it! |
|
@Janiczek I added your code to the PR (we tend to keep benchmarking code around to have a better record of performance work). I have slightly different looking results on Safari: 
Eye-balling it, looks like about 30% performance drop. |
|
Thanks @jmbromley and @Janiczek for taking the time on this one! |
This commit makes the
groupsOf...family of functions fully tail recursive by forcing them to use the tail recursive version of List.take (normally List.take is only tail recursive for lists larger than 1000, but since thegroupsOf...functions are themselves recursive this can result in potential call stack overflow from the successive accumulation of (up to) 1000-long non-recursive List.take calls during the recursion).This is an alternative to PR #46 which would instead just add a note to the documentation warning users about the potential overflow.