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

perf: use native Array methods instead of for loop #3519

Closed
wants to merge 6 commits into from

Conversation

edison1105
Copy link
Member

@edison1105 edison1105 commented Mar 31, 2021

@edison1105 edison1105 marked this pull request as ready for review April 1, 2021 03:32
@edison1105 edison1105 changed the title perf: use native Array methods instead of for perf: use native Array methods instead of for loop Apr 1, 2021
@HcySunYang HcySunYang added the 🧹 p1-chore Priority 1: this doesn't change code behavior. label Apr 1, 2021
@CarterLi
Copy link
Contributor

CarterLi commented Apr 6, 2021

It was unfair.

At least for for loop vs Array.some test case, Array#some only tests element present in the array. In your benchmark:

var array = new Array(1000);

which generates an array with only holes will never call the callback function function(i) { return array[i] === 877; }.

array.some(x => !x) // => false

Besides, the two tests generates different results because they do different things.


In my benchmark, Array#some is about 94.6% slower than plain for loop. That is because Array#some is a native function. JIT compiler can't inline callback for native functions in most cases. Even for of loop which uses iterator is much faster than Array#some

image


We use Array methods not because they are fast, but because they are easier to write and result in cleaner code. For library uses, I do think plain for loop are better for performance reasons.

@edison1105
Copy link
Member Author

thats weird.
https://www.measurethat.net/Benchmarks/Show/12451/0/for-loop-vs-arraysome-vs-for-of

We use Array methods not because they are fast, but because they are easier to write and result in cleaner code. For library uses, I do think plain for loop are better for performance reasons.

I dont think so.
see
#3206 (comment)
https://gomakethings.com/how-performant-are-modern-array-methods-vs-old-school-for-loops-in-vanilla-js/

@syuilo
Copy link
Contributor

syuilo commented Apr 6, 2021

Basically, simpler code tends to benefit more from optimization by the engines.
So (even if the performance is temporary worse now), this PR should be worth it.
JFYI: #402 (comment)

@CarterLi
Copy link
Contributor

CarterLi commented Apr 6, 2021

Basically, simpler code tends to benefit more from optimization by the engines

Maybe yes for other cases. But for for loop no.

Because forEach, some and other Array methods are basically implemented ( and can only be ) as plain for loop, with other additional (and usually unnecessary) checks. A standard conformant implementation of forEach can be unexpectedly complex:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach#polyfill

I don't think browsers can do lots of things for it. There is no silver bullet.

Note forEach are ES5 things, and being supported since IE 9, Chrome 1. It's been more than 10 years and still no optimization. So no.

@CarterLi
Copy link
Contributor

CarterLi commented Apr 6, 2021

thats weird.
measurethat.net/Benchmarks/Show/12451/0/for-loop-vs-arraysome-vs-for-of

We use Array methods not because they are fast, but because they are easier to write and result in cleaner code. For library uses, I do think plain for loop are better for performance reasons.

I dont think so.
see
#3206 (comment)
gomakethings.com/how-performant-are-modern-array-methods-vs-old-school-for-loops-in-vanilla-js

I got similar results with console.time

image

https://runkit.com/carterli/for-loop-vs-array-some

@edison1105
Copy link
Member Author

@CarterLi Indeed, you are right that this change lowers performance.
Close this one.

@edison1105 edison1105 closed this Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧹 p1-chore Priority 1: this doesn't change code behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants