Skip to content

Conversation

jhrcek
Copy link

@jhrcek jhrcek commented Apr 23, 2019

I did some benchmarks and it turns out that thanks to this improvement the implementation of List.concat about 2x (in Google Chrome) to 3x (in Firefox) faster than the current implementation.

@jhrcek jhrcek changed the title Improve performance of List.concat Improve performance of List.append Apr 24, 2019
@robinheghan
Copy link
Contributor

robinheghan commented Apr 26, 2019

So ++ is faster than List.append because ++ compiles down to a native javascript function which first checks if the arguments are strings (++ works on both strings and lists) and then performs the correct appending operation depending on what is being appended.

In theory, this should mean that ++ is slower than List.append which doesn't need to perform reflection to figure out what types its arguments are. However, ++ uses a low-level JS code to perform the List appending, which is much faster than the written-in-elm implementation of List.append since JS doesn't need to ensure immutability at each step.

This PR proposes to increase performance of List.append by aliasing it to ++. While this does improve performance, it's not the best solution, as the typeof "string" check that ++ performs is unnecessary, and makes batch operations like List.concat slower than it needs to be.

There are several different ways to solve this, all of which requires careful benchmarking and perhaps some design discussion with Evan to make sure the actual fix has long life (it would be bad to implement a fix which is negated by future code generation or language changes).

I'm planning on taking a closer look at the implementation of List functions during the next week or two, and I'll be sure to figure out the best way to improve the performance of List.append in the process of doing that.

If I reach the conclusion that this PR is as good a fix as any other, I'll be sure to say so and recommend the merging of this PR (credit were credit is due). I do, however, believe there is a better fix to this and I'm planning on writing that up soon.

@robinheghan
Copy link
Contributor

Fixed in #1027

@jhrcek
Copy link
Author

jhrcek commented May 9, 2019

Closing in favor of #1027
@Skinney Thanks for working on better fix!

@jhrcek jhrcek closed this May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants