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

Minor perf #22

Closed
wants to merge 2 commits into from
Closed

Minor perf #22

wants to merge 2 commits into from

Conversation

billouboq
Copy link
Contributor

Minor perf improvement

@dougwilson dougwilson self-assigned this Mar 15, 2017
@dougwilson dougwilson added the pr label Mar 15, 2017
@dougwilson
Copy link
Contributor

Nice @billouboq ! I'm wondering about the following, if you can help me out:

  1. Would you mind sharing the benchmark code you used? Sometimes these types of changes can speed up one version of Node.js but hurt others, so we want to make sure the change is neutral to positive on all versions of Node.js this module supports.

@billouboq
Copy link
Contributor Author

billouboq commented Mar 19, 2017

Would you mind sharing the benchmark code you used? Sometimes these types of changes can speed up one version of Node.js but hurt others, so we want to make sure the change is neutral to positive on all versions of Node.js this module supports.

The regex part was tested on chrome 56 in jsperf.

And for the every replacement I think for loop is faster than any array functions for now in current v8 versions. Also every check for all value in the array while the for loop stops when the condition is true so less work in some cases.

Do you want me to benchmark all this ?

@dougwilson
Copy link
Contributor

Oops, I merged & released this, but the for loop behaves differently and caused a regression :S I'm fixing it plus adding a test for the future :)

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

Successfully merging this pull request may close these issues.

2 participants