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

isISIN optimization #1633

Merged
merged 3 commits into from
Apr 17, 2021
Merged

Conversation

bmacnaughton
Copy link
Contributor

i was playing around and challenged myself to optimize isISIN. i'm not sure it's 100% there, but it's significantly more performant that it was.

$ node ./util.bench.js old-version
group times: [1390.93, 1344.84, 1396.37, 1426.44, 1408.29, 1373.00, 1517.00, 1381.53, 1372.97]
average per 100000: 1401.263
standard deviation of 9 intervals: 46.406
total: gc count: 1568, gc time: 137.500
done

$ node ./util.bench.js v3
group times: [211.69, 207.08, 213.71, 213.63, 214.08, 214.64, 213.91, 212.19, 213.39]
average per 100000: 212.703
standard deviation of 9 intervals: 2.168
total: gc count: 17, gc time: 3.633
done


## Checklist

- [x] PR contains only changes related; no stray files, etc.
- [x] README updated (where applicable) (NA)
- [x] Tests written (where applicable) (NA but one item added)

@codecov
Copy link

codecov bot commented Mar 20, 2021

Codecov Report

Merging #1633 (c31da98) into master (deb1d1e) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1633   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          100       100           
  Lines         1807      1852   +45     
=========================================
+ Hits          1807      1852   +45     
Impacted Files Coverage Δ
src/lib/isISIN.js 100.00% <100.00%> (ø)
src/lib/isTaxID.js 100.00% <0.00%> (ø)
src/lib/isLicensePlate.js 100.00% <0.00%> (ø)
src/lib/isPassportNumber.js 100.00% <0.00%> (ø)
src/lib/isStrongPassword.js 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update deb1d1e...c31da98. Read the comment docs.

Copy link
Member

@tux-tn tux-tn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @bmacnaughton and sorry for the delay. A little question, isn't isISIN using the luhn algorithm?

@bmacnaughton
Copy link
Contributor Author

bmacnaughton commented Apr 16, 2021

@tux-tn yes, it is. and it still is using the algorithm (it has to in order to work correctly). it's just executing it in one pass over the string now. but the performance benefits come mostly from not creating many substrings and running parseInt on each of them. it just makes one pass over the characters and converts them to the integer they represent directly.

i didn't really have any need for this, but whenever i see that kind of pattern in code i look at it as a puzzle to be solved. here's an example, completely unrelated to this, that shows another case where the performance of a slightly larger solution is dramatically better: https://stackoverflow.com/questions/34309988/byte-array-to-hex-string-conversion-in-javascript/55426656#55426656

Copy link
Member

@tux-tn tux-tn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tux-tn yes, it is. and it still is using the algorithm (it has to in order to work correctly). it's just executing it in one pass over the string now. but the performance benefits come mostly from not creating many substrings and running parseInt on each of them. it just makes one pass over the characters and converts them to the integer they represent directly.

i didn't really have any need for this, but whenever i see that kind of pattern in code i look at it as a puzzle to be solved. here's an example, completely unrelated to this, that shows another case where the performance of a slightly larger solution is dramatically better: https://stackoverflow.com/questions/34309988/byte-array-to-hex-string-conversion-in-javascript/55426656#55426656

Actually i was asking this to know if your optimization is related to luhn algorithm (other validators such as isCreditCard would profit from it) but after checking and running your code i understand that the optimization is related to string manipulation and type casting.
Thank you for taking the time to optimize the code and to do some benchmarking 👍

@tux-tn tux-tn added the ready-to-land For PRs that are reviewed and ready to be landed label Apr 16, 2021
@profnandaa profnandaa merged commit c33fca6 into validatorjs:master Apr 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-land For PRs that are reviewed and ready to be landed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants