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

replace forEach() with native for-loops for performance improvement #287

Closed
coffeeandwork opened this issue Apr 11, 2024 · 5 comments
Closed
Assignees
Labels
enhancement New feature or request next release Available in the next release

Comments

@coffeeandwork
Copy link
Collaborator

In this benchmark I have tested various looping constructs: https://jsbench.me/dulu7atajk/4. forEach() appears slower by an order of magnitude compared to a native for-loop. This makes sense because for each iteration inside forEach() we are doing a callback invocation that comes with its extra costs that we don't incur with a native for loop.

@coffeeandwork coffeeandwork added the enhancement New feature or request label Apr 11, 2024
@coffeeandwork coffeeandwork self-assigned this Apr 11, 2024
coffeeandwork pushed a commit to coffeeandwork/beercss that referenced this issue Apr 11, 2024
@leonardorafael
Copy link
Collaborator

leonardorafael commented Apr 12, 2024

Hi @coffeeandwork , there are something wrong here, I published a js perf here (https://jsperf.app/tecele) and got some results. I always understood that the native for loop is the fastest but, why these results? For me it's ok to change to native for loop too (but this is strange).

Chrome 123 (3 runs)

go001
go002
go003

Firefox 124 (3 runs)

ff001
ff002
ff003

@leonardorafael
Copy link
Collaborator

I believe that modern browsers can do some performance trick here. 🪄

@coffeeandwork
Copy link
Collaborator Author

I think it's more likely that the console.log() is computationally expensive and is overshadowing the relatively smaller difference in the performance of forEach vs native for-loop. I have modified your jsperf slight and I was able to get a consistent result of the native for-loop being way faster than forEach: https://jsperf.app/tecele/2 Let me know if you are seeing something different.

leonardorafael added a commit that referenced this issue Apr 15, 2024
#287 replace forEach() with native for-loops
@leonardorafael
Copy link
Collaborator

Nice!

@leonardorafael
Copy link
Collaborator

leonardorafael commented Apr 15, 2024

To be fair I removed all content inside loop. The forEach is 15x slower than others. Seems good to change it. The best perfomance isn't the most readable, but it's ok.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request next release Available in the next release
Projects
None yet
Development

No branches or pull requests

2 participants