Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upSimpler implementation of Native.List.repeat #87
Conversation
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
jvoigtlaender
Jan 10, 2015
Contributor
Have you actually measured this? The point of the array doubling here is not just about growing the array dynamically in a reasonable way. Note that your code performs a number of iterations linear in n, whereas the code it replaces performs only logarithmically many so.
Maybe read http://stackoverflow.com/a/23252495.
|
Have you actually measured this? The point of the array doubling here is not just about growing the array dynamically in a reasonable way. Note that your code performs a number of iterations linear in Maybe read http://stackoverflow.com/a/23252495. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
jwmerrill
Jan 10, 2015
Contributor
The call to fromArray in the existing code does the same linear work that my loop in the new code does.
I'm deeply skeptical that there is any advantage to building an array that way in JS (by concatenating copies), but in this case, it's beside the point, because it isn't necessary (or helpful) to build an array at all.
In any case, I don't want to waste a lot of anyone's time over this. I doubt this will matter much to anyone who is using the code (rather than reading it). But since I was reading anyway and saw a chance to make something simpler at no cost, I thought I'd take it.
|
The call to fromArray in the existing code does the same linear work that my loop in the new code does. I'm deeply skeptical that there is any advantage to building an array that way in JS (by concatenating copies), but in this case, it's beside the point, because it isn't necessary (or helpful) to build an array at all. In any case, I don't want to waste a lot of anyone's time over this. I doubt this will matter much to anyone who is using the code (rather than reading it). But since I was reading anyway and saw a chance to make something simpler at no cost, I thought I'd take it. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
jvoigtlaender
Jan 10, 2015
Contributor
Yeah, sure. I was just commenting on the array doubling stuff, and that your "probably" about that seems to be contradicted by other people's measurements.
On the whole, due to the fromArray call at the end of the old code, your replacement is indeed a better idea than what there currently is.
|
Yeah, sure. I was just commenting on the array doubling stuff, and that your "probably" about that seems to be contradicted by other people's measurements. On the whole, due to the |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
jwmerrill
Jan 10, 2015
Contributor
FWIW, I don't have a lot of faith that the benchmarks in that SO post actually measure something useful and generalizable. Microbenchmarks are hard to get right, especially when you take a completely black box approach and don't try to figure out what code the js compiler is actually generating.
Here's an amusingly written post on the topic: http://mrale.ph/blog/2012/12/15/microbenchmarks-fairy-tale.html
I actually recommend pretty much anything by this author: http://mrale.ph/
But you could talk in circles about these topics all day. Luckily, as we've agreed, it isn't actually necessary to come to a conclusion about the best way to build an array of repeated elements in JS for the purposes of this PR.
|
FWIW, I don't have a lot of faith that the benchmarks in that SO post actually measure something useful and generalizable. Microbenchmarks are hard to get right, especially when you take a completely black box approach and don't try to figure out what code the js compiler is actually generating. Here's an amusingly written post on the topic: http://mrale.ph/blog/2012/12/15/microbenchmarks-fairy-tale.html I actually recommend pretty much anything by this author: http://mrale.ph/ But you could talk in circles about these topics all day. Luckily, as we've agreed, it isn't actually necessary to come to a conclusion about the best way to build an array of repeated elements in JS for the purposes of this PR. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
evancz
Nov 19, 2015
Member
With TCO in 0.16, I just moved this into Elm. I guess now we need to make sure Elm is fast instead :P Thanks for pushing for making this nicer!
|
With TCO in 0.16, I just moved this into Elm. I guess now we need to make sure Elm is fast instead :P Thanks for pushing for making this nicer! |
jwmerrill commentedJan 10, 2015
The array doubling pattern used by the old implementation probably isn't a good
idea compared to a plain
forloop that pushes onto an array, because all(modern) javascript engines dynamically grow arrays in a reasonble way such
that
pushis amortized constant time. There's no need to roll your ownimplementation of that, and doing so probably hurts performance.
But in the case of
repeat, there is no need to create an array at all.