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

some optimizations? #1

Closed
leeoniya opened this issue Sep 5, 2022 · 2 comments
Closed

some optimizations? #1

leeoniya opened this issue Sep 5, 2022 · 2 comments
Assignees

Comments

@leeoniya
Copy link

leeoniya commented Sep 5, 2022

@anonrig

key = key.replace(/\+/g, " ");
value = value.replace(/\+/g, " ");

.replaceAll('+', ' ') is probably faster than the regex variant.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replaceAll

if (c === 38 || isNaN(c)) {

Number.isNaN() is more robust

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isNaN

@anonrig
Copy link
Owner

anonrig commented Sep 5, 2022

Hey @leeoniya

Thank you for the issue!

  • replaceAll vs replace with regex

You are absolutely right. I did a benchmark and it seems regex is 80% slower. I would appreciate if you can open a pull request to change this!

  • Number.isNaN vs isNaN

I believe Number.isNaN is slower than isNaN, due to the type check. A quick benchmark showed that Number.isNaN is 0.17% slower on Safari, and 0.01% slower on Chrome. I think it's negligible in terms of performance. Therefore, would you open a pull request for this?

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 5, 2022

I proposed #2 which actually avoids the whole isNaN issue.

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

No branches or pull requests

3 participants