Skip to content
New issue

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

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

Already on GitHub? # to your account

Make combine faster #29

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jfmengels
Copy link

@jfmengels jfmengels commented Oct 20, 2021

This improves the performance of the combine function.

There are two main parts to this.

The first part is that this version avoids using Result.map2, which removes a lot of unwrapping of the intermediate accumulator. We also don't wrap the accumulator in an Ok until the very end.

The second part is that whenever we encounter an Err, we stop and return an Err, without continuing to iterate over the rest of the list, which removes a lot of processing time whose result is thrown away.
List.fold* functions are great, but if you want to control how you iterate over the list, recursion is the way to go.

Here is a benchmark you can run: https://ellie-app.com/fCBmw4NHHBHa1

which should give you results like the following:

Screenshot 2021-10-20 at 11 08 54

The performance increase of the second benchmark can be raised even further if you change the size of the length of the list and move the position of the error (which is in 2nd position out of 10 items in these benchmarks).

With the error in first position in a list of 1000 items, the performance changes to something like this:
Screenshot 2021-10-20 at 11 33 30

Since combineMap depends on combine, this will also improve its performance, which I didn't spend time benchmarking.

List.foldr (Result.map2 (::)) (Ok [])
combine list =
combineHelp list []
|> Result.map List.reverse

Choose a reason for hiding this comment

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

If you move the List.reverse into the [] -> branch of combineHelp then you don't need to Result.map over it or pipe. Will probably only add 0.000001% better performance 😅

Copy link
Author

@jfmengels jfmengels Oct 21, 2021

Choose a reason for hiding this comment

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

Oh, you'd be surprised... It's not the same kind of improvement, but for small lists, it's a noticeable improvement (compared to the version I already proposed).

https://ellie-app.com/fDcGZNzBFzGa1

Screenshot from 2021-10-21 20-30-32
Screenshot from 2021-10-21 20-31-15
Screenshot from 2021-10-21 20-32-08

You're absolutely right, and I don't know why I didn't do this in the first place (well, probably because adding List.reverse was something I realized I needed to do a lot later 🤦).

I updated the PR 👍

I didn't benchmark on the Err case but it should also improve performance 👍

@jfmengels
Copy link
Author

Any chance of looking into merging this? 🙂

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants