-
Notifications
You must be signed in to change notification settings - Fork 49
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.
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
Improve foldr performance ~4x on large lists #180
Conversation
instance foldableList :: Foldable List where | ||
foldr f b = foldl (flip f) b <<< rev | ||
where | ||
rev = foldl (flip Cons) Nil | ||
rev = go Nil | ||
where | ||
go acc Nil = acc | ||
go acc (x : xs) = go (x : acc) xs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the core change. Wondering if the reverse
function should be moved from Data.List
to this file to deduplicate code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! I’m not concerned about the duplication of reverse
, since it’s so little code and it might look odd in the docs to say that reverse
is re-exported from Data.List.Types whereas almost none of the other List functions are.
This slower reverse pattern is duplicated in a few other places (just search for purescript-lists/src/Data/List/Types.purs Line 135 in 62900a5
purescript-lists/src/Data/List/Types.purs Line 141 in 62900a5
purescript-lists/src/Data/List/Types.purs Line 145 in 62900a5
purescript-lists/src/Data/List/Types.purs Line 153 in 62900a5
purescript-lists/src/Data/List/Types.purs Line 260 in 62900a5
So thinking these should all be replaced with a common I'm also wondering if we should do more benchmarking in the browser (blocked by purescript/purescript-minibench#16) with bundling optimizations and see if there's still a performance difference. Ideally, I'd like to keep the original concise code. |
Performance is improved by using an inlined reverse function, rather than leveraging
foldl
.Both versions appear to be identical, so it's a bit of a bummer that the concise version is so much slower.
There's a moderate speedup for all list sizes, but it's most significant on large lists. Here are the benchmarking results:
Before:
After:
Also refactored the benchmarking code.