List module performance improvements

[robinheghan]

This PR improves performance of the following functions:

• append
• filterMap
• indexedMap
• concatMap
• concat
• foldr (for large lists)

The improvements range from 20% to 100% depending on the browser and function.

More specifically, this is what has been done:

• indexedMapped, concatMap and filterMap has been re-implemented.
• append is now implemented directly in javascript, and the (++) operator now yields to this function. It's now expected that List.append is faster than (++), while appending via the operator is as fast as before.
• foldrs threshold for reverting to a slower, but stack safe, implementation is now doubled to match the threshold of take (which has a similar implementation).

[robinheghan]

Thank you, @harrysarson.

[turboMaCk]

thanks for looking into this

[turboMaCk]

nitpick - missing spaces around ->

[turboMaCk]

I wonder why did you change this magic number but I assume some measurements suggested to do so.

[robinheghan]

It’s to match the similar number for take. The number should be as big as possible, without risking that people run into stack overflows. Since take has had the number at 1000 for a long while now, and people don’t complain about stack overflows, it seemed sensible to increase it for foldr as well.

[turboMaCk]

nitpick - I wonder about naming here. For instance cons uses this convention var _List_cons = F2(_List_Cons); (note same naming but different cases)

core/src/Elm/Kernel/List.js

Line 16 in 4fbcb15

var _List_cons = F2(_List_Cons);

[robinheghan]

_List_Cons is a constructor (the function returns a type). _List_cons is a function, which calls the constructor. You’ll see it defined in the List module as a function.

[harrysarson]

I tried running the test suite (using @jerith666's #1017) for this pull and got:

elm: Bad kernel symbol: List_ap
CallStack (from HasCallStack):
  error, called at compiler/src/Elm/Kernel.hs:188:19 in main:Elm.Kernel
elm: thread blocked indefinitely in an MVar operation
Compilation failed while attempting to build /home/travis/build/harrysarson/core/tests/tests/Main.elm

This may be a quirk of the hacks used to run the test suite but I thought it worth flagging in case there is an actual problem here.

[turboMaCk]

@harrysarson try to rm --rf elm-stuff and start over. I just tried it as well and all tests passed for me without any error. I think this error is unrelated to the change (MVar is a Haskell concurrency primitive used in the compiler and the error indicates dead lock in the compiler). I'm using NixOS on unstable-branch and manual compilation of elmi-to-json and elm-test (both from master) and plain 0.19.0 compiler

[turboMaCk]

@harrysarson sorry for the miss information! when I use run-tests.sh I also get the same error and even after cleaning elm-stuff (which script does internally anyway).

[harrysarson]

@turboMaCk I have no confidence that run-tests.sh is not causing this error by the weird things it has to do. How did you run the tests?

[turboMaCk]

@harrysarson My first attempt was just to run elm-test but later I realized this might not be running expected thing for elm-core as there might be some hacks needed to really run tests against the code in the repository. I haven't investigated deeper but my feeling now is that it's a legit issue.

My guess is that it's this call https://github.com/elm/core/pull/1027/files#diff-99e42729736fe9132da4bba9d117b0e0R182 to __List_ap defined in List.js from Utils.js which is problematic and that probably this function needs to be imported somehow here

core/src/Elm/Kernel/Utils.js

Line 7 in c9975bf

import Elm.Kernel.List exposing (Cons, Nil)

but I think @Skinney would know those things better.

[robinheghan]

I actually know very little about how these things work. I doubt anyone but Evan knows.

[harrysarson]

Looks good: https://travis-ci.org/harrysarson/core/builds/528445404

[harrysarson]

Now #1017 has landed, if you re run the CI it should go green. (Best way to re run CI is to open and close this pull request)

[Janiczek]

@sparksp figured out a faster concatMap:

fasterConcatMap : (a -> List b) -> List a -> List b
fasterConcatMap f =
    List.foldr (f >> (++)) []

Granted, this doesn't take the JS changes into account, so maybe overall your version is faster, but in current elm/core the above snippet wins the benchmark.

[sparksp]

Luckily this is the same as the proposed concatMap 😄 (f >> (++)) is functionally the same as helper just less expressive.

[Janiczek]

beware: append vs (++) does introduce a difference, IIRC.

[robinheghan]

The JS changes in this PR makes ++ and List.append equally fast. So if this PR was to be merged, I don't think there would be a difference between these two implementations.

[turboMaCk]

exactly. The point of this PR is to make the non-obvious performance difference between List.append and (++) to go away which will implicitly improve all the other functions defined in terms of append.

I wonder if you have any idea how @evancz feels about the change @Skinney. It's been a while since this was opened.

[robinheghan]

I remember that me and Evan looked through this, but I forget what the conclusion of that meeting was. I don't think he was opposed to any of the changes, though.