Skip to content

List module performance improvements #1027

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robinheghan wants to merge 5 commits into from
+49 −33
Closed

List module performance improvements #1027

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a017feb
Improve performance of several functions.
robinheghan May 3, 2019
199e7d9
increase threshold of when foldr reverts to stack-safe implementation…
robinheghan May 3, 2019
3e877b5
Use native impl for List.append.
robinheghan May 3, 2019
c9975bf
Add missing return
harrysarson May 4, 2019
b97247c
Fixed compile errors.
robinheghan May 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/Elm/Kernel/List.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,23 @@ var _List_map5 = F6(function(f, vs, ws, xs, ys, zs)
return _List_fromArray(arr);
});

var _List_append = F2(_List_ap);

function _List_ap(xs, ys)
Copy link
Copy Markdown
Contributor

@turboMaCk turboMaCk May 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Contributor Author

@robinheghan robinheghan May 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_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.

{
if (!xs.b)
{
return ys;
}
var root = _List_Cons(xs.a, ys);
xs = xs.b
for (var curr = root; xs.b; xs = xs.b) // WHILE_CONS
{
curr = curr.b = _List_Cons(xs.a, ys);
}
return root;
}

var _List_sortBy = F2(function(f, xs)
{
return _List_fromArray(_List_toArray(xs).sort(function(a, b) {
Expand Down
16 changes: 3 additions & 13 deletions src/Elm/Kernel/Utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import Array exposing (toList)
import Basics exposing (LT, EQ, GT)
import Dict exposing (toList)
import Elm.Kernel.Debug exposing (crash)
import Elm.Kernel.List exposing (Cons, Nil)
import Elm.Kernel.List exposing (Cons, Nil, ap)
import Set exposing (toList)

*/
Expand Down Expand Up @@ -178,16 +178,6 @@ function _Utils_ap(xs, ys)
return xs + ys;
}

// append Lists
if (!xs.b)
{
return ys;
}
var root = __List_Cons(xs.a, ys);
xs = xs.b
for (var curr = root; xs.b; xs = xs.b) // WHILE_CONS
{
curr = curr.b = __List_Cons(xs.a, ys);
}
return root;
// append Lists
return __List_ap(xs, ys);
}
49 changes: 29 additions & 20 deletions src/List.elm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,17 @@ element (starting at zero).
-}
indexedMap : (Int -> a -> b) -> List a -> List b
indexedMap f xs =
map2 f (range 0 (length xs - 1)) xs
reverse (indexedMapHelper f 0 xs [])


indexedMapHelper : (Int -> a -> b) -> Int -> List a -> List b -> List b
indexedMapHelper fn index list result =
case list of
[] ->
result

x :: xs ->
indexedMapHelper fn (index + 1) xs (cons (fn index x) result)


{-| Reduce a list from the left.
Expand Down Expand Up @@ -198,7 +208,7 @@ foldrHelper fn acc ctr ls =
d :: r4 ->
let
res =
if ctr > 500 then
if ctr > 1000 then
Copy link
Copy Markdown
Contributor

@turboMaCk turboMaCk May 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@robinheghan robinheghan May 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

foldl fn acc (reverse r4)
else
foldrHelper fn acc (ctr + 1) r4
Expand Down Expand Up @@ -227,17 +237,16 @@ from an untrusted source and you want to turn them into numbers:
-}
filterMap : (a -> Maybe b) -> List a -> List b
filterMap f xs =
foldr (maybeCons f) [] xs


maybeCons : (a -> Maybe b) -> a -> List b -> List b
maybeCons f mx xs =
case f mx of
Just x ->
cons x xs
let
helper mx acc =
case f mx of
Just x ->
cons x acc

Nothing ->
xs
Nothing ->
acc
in
foldr helper [] xs


-- UTILITIES
Expand Down Expand Up @@ -363,13 +372,8 @@ product numbers =
You can also use [the `(++)` operator](Basics#++) to append lists.
-}
append : List a -> List a -> List a
append xs ys =
case ys of
[] ->
xs

_ ->
foldr cons ys xs
append =
Elm.Kernel.List.append


{-| Concatenate a bunch of lists into a single list:
Expand All @@ -387,7 +391,12 @@ concat lists =
-}
concatMap : (a -> List b) -> List a -> List b
concatMap f list =
concat (map f list)
let
helper val acc =
append (f val) acc
in
foldr helper [] list
Copy link
Copy Markdown
Contributor

@Janiczek Janiczek Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown

@sparksp sparksp Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@Janiczek Janiczek Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@robinheghan robinheghan Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@turboMaCk turboMaCk Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@robinheghan robinheghan Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.




{-| Places the given value between all members of the given list.
Expand Down