Skip to content
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

Is there a reason for not using slice(0)? #221

Open
nikoskalogridis opened this issue Apr 10, 2018 · 8 comments
Open

Is there a reason for not using slice(0)? #221

nikoskalogridis opened this issue Apr 10, 2018 · 8 comments

Comments

@nikoskalogridis
Copy link
Contributor

nikoskalogridis commented Apr 10, 2018

export function copy (a) {

ie

export function copy (a) {
  return a.slice(0);
}
@briancavalier
Copy link
Member

Hi @nikoskalogridis. One of the reasons that most/prelude exists is for performance. At the time it was original written, it was more performant (cpu) on most VMs to process arrays "manually" using a for loop, than to use builtins. I haven't reevaluated that recently, so it's possible that builtins have caught up in some VMs (it would be great if that's the case). That's probably something we should do at some point.

@nikoskalogridis
Copy link
Contributor Author

I see, at least in modern vms though there is a clear advantage on using native functions instead of loops https://jsperf.com/array-for-loops-vs-slice/1
I could make a complete test for this prelude lib if you see value in it

@briancavalier
Copy link
Member

Thanks @nikoskalogridis. A jsperf test suite for prelude would be really great! That would help us make decisions about when to deprecate bits of prelude and just use builtins.

I was a little worried that some VMs would discard the calls in that JS Perf, since it'd be easy for an optimizer to prove that the result of copy and/or copyWithSlice isn't used. For example, here's a Firefox run:

screen shot 2018-04-11 at 7 42 53 am

That's an astonishing margin of victory! One technique that's typically used to ensure that perf tests are valid is to use the result in some way. For example, by adding the resulting array's .length to a global len variable. So, I figured I'd try that:

screen shot 2018-04-11 at 8 03 16 am

Once again, astonishing! In fact, Safari, Chrome, and FF all gave the same results after consuming the resulting .length. It's possible that VMs are special casing slice(0) in some way. In any case, this seems to be good news :)

Thanks again for the offer to put together a complete test. We'd very much appreciate it!

@briancavalier
Copy link
Member

Just to be totally clear: I'm absolutely in favor of perf testing prelude and changing/deprecating/dropping functions that no longer provide benefit. I just want to make sure we are basing those decisions on valid data. Perf testing on optimizing VMs is hard since optimization is largely opaque (v8 provides some visibility, but I afaik, other VMs don't 🙁 ).

@davidchase
Copy link
Member

Interesting finds, @nikoskalogridis out of curiosity did you run into some issues with the current implementation of copy and it was resolved by changing to slice(0) ?

@Frikki
Copy link
Member

Frikki commented Apr 11, 2018

Browser support may also be considered. That is, how is the performance on somewhat older browsers?

@nikoskalogridis
Copy link
Contributor Author

@davidchase not at all. I was just browsing through the code and I was impressed that it used for loops for manipulating arrays. I have recently also noticed myself that there is a huge performance difference on using slice(x) over other methods . So that is why I asked.

My opinion regarding custom for loops, implementing native calls, is to avoid them. Browsers get updated and native calls improve all the time in each iteration. Now if there are perf issues in older browsers I think its fair to penalize the older browser instead of the new ones. But that is only my opinion ;-)

@Frikki
Copy link
Member

Frikki commented Oct 22, 2018

I am just bumping this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants