paulmillr / es6-shim

ECMAScript 6 compatibility shims for legacy JS engines
http://paulmillr.com
MIT License
3.11k stars 388 forks source link

The shim for the repeat function can be improved #431

Closed AurelioDeRosa closed 7 years ago

AurelioDeRosa commented 7 years ago

Hi.

The current shim for the repeat function can be improved in regard of performance. A couple of faster implementations can be found here and here.

ljharb commented 7 years ago

Although jsperf is no longer up, https://github.com/paulmillr/es6-shim/blob/master/es6-shim.js#L776 links to one that demonstrates that we are already using the fastest possible algorithm across browsers, which is quite important.

If you'd like to provide benchmarks across all ES3, ES5, and noncompliant-ES6 browsers that we support, I'll be happy to tweak the implementation (but given that virtually nobody's doing millions or even thousands of string repeat operations a second, I'm skeptical of the value of investing that time)

AurelioDeRosa commented 7 years ago

I strongly disagree with the approach here. The time complexity of the implementations you have is O(n), while the other two have a time complexity of O(log n). It makes a difference even for small numbers (e.g. n < 100). Also, one of the two implementations linked uses features available since ever, so it'll be faster cross-browser. To me, it seems that you're discarding a faster implementation without a real reason. Of course this isn't my project so you can't do whatever you want.

ljharb commented 7 years ago

@AurelioDeRosa this implementation was chosen because it was proven to be faster, which trumps theoretical big O complexity. I'm more than happy to accept a PR with another implementation than comes with similar proofs via benchmarks on all supported engines.

However, I'm already convinced from previous evidence that this is the fastest implementation - since you're making the claim that other implementations are faster, I'm asking you to bear the burden of demonstrating that before I make a change.

AurelioDeRosa commented 7 years ago

Test here: https://jsfiddle.net/gcn38zj0/

It results faster in Chrome, IE, Edge, and Opera. Firefox shows similar results. IE has an improvement of ~40%. Clearly, you can create other tests with a fixed amount of repetitions to check the results from a different point of view.

On a side note, you claim "it was proven to be faster". Can I ask compared to what? The fact that MDN (among others) doesn't report the O(n) solution as a polyfill must mean something.

ljharb commented 7 years ago

For one, MDN's polyfills are examples, and should never be used in production - nor are they endorsements of a particular method of polyfilling.

Second, you'll note that our algorithm isn't actually O(n), because it doesn't run N times, it runs "the square root of N" times - see https://en.wikipedia.org/wiki/Exponentiation_by_squaring#Computational_complexity.

As to "compared to what" - the jsperf which is, sadly, not still up - contained about 6-10 different methods - and while some of the algorithms were faster on some browsers than "exponentiation by squaring", they were much slower on other browsers. This implementation was the best compromise - it had the most upside and the least downside.