mostjs / core

Most.js core event stream
http://mostcore.rtfd.io
MIT License
403 stars 36 forks source link

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

Open nikoskalogridis opened 6 years ago

nikoskalogridis commented 6 years ago

https://github.com/mostjs/core/blob/82b380deea1f80f685efb532bdea20ca8f07087a/packages/prelude/src/array.js#L67

ie

export function copy (a) {
  return a.slice(0);
}
briancavalier commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

nikoskalogridis commented 6 years ago

@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 commented 6 years ago

I am just bumping this.