-
Notifications
You must be signed in to change notification settings - Fork 7
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: avoid domainToAscii on pure lowercase ascii cases #67
Conversation
Are we testing this branch in the tests? I'm not sure 🤔 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
I'd say yes. Can regex be replaced by just a cursor on the string? We tried to remove the maximum of regex from the original codebase |
I have no clue what you mean with "just a cursor on the string". Do you want to iterate over the string? |
Yes, sometimes it's faster. Really depends on the case. |
Something like const hexLookUp = Array.from({ length: 256 }, (v, k) => /[^!"$&'()*+,.;=_`a-z{}~-]/.test(String.fromCharCode(k)))
function nonSimpleDomain (value) {
let code = 0
for (var i = 0, len = value.length; i < len; ++i) {
code = value.charCodeAt(i)
if (code > 255 || hexLookUp[code]) {
return true
}
}
return false
} |
Yup! |
@zekth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
(sorry for the late review)
@zekth if Numbers are neglectable, we could add numbers to the lookup |
we might also add capital letters |
Let me investigate |
before:
after:
Only thing i want to note:
nodes own: URL.domainToAscii('1') returns '0.0.0.1', the same for URL.domainToAscii('11') returns '0.0.0.11',
So basically transforms the decimal notation to the dot notation. Is this neglectable?
Checklist
npm run test
andnpm run benchmark
and the Code of conduct