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.

Sign up for GitHub

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

Jump to bottom

Do not parse numbers on url parsers - ipv4 #36

Closed
anonrig opened this issue Jan 9, 2023 · 5 comments · Fixed by nodejs/node#46227
Closed

Do not parse numbers on url parsers - ipv4 #36

anonrig opened this issue Jan 9, 2023 · 5 comments · Fixed by nodejs/node#46227
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@anonrig
Copy link
Member

anonrig commented Jan 9, 2023

EndsInNumber parses the number, but does not use the result of it. Just using the validations, without the parsing, would make ipv4 parsing 28% faster.

Similar comparison is: ada-url/ada#36
@anonrig anonrig added good first issue Good for newcomers help wanted Extra attention is needed labels Jan 16, 2023
@debadree25
Copy link
Member

So basically instead of int64_t num = ParseIPv4Number(last_str, last_str + last.size()); we would just use a validator here?

@anonrig
Copy link
Member Author

anonrig commented Jan 16, 2023

There are 2 optimizations can be done here.

  1. Removing vector initialization on line 412
std::vector<std::string> parts = SplitString(input, '.', false);
  1. Removing ParseIPv4Number and replacing it with a validator that returns boolean
int64_t num = ParseIPv4Number(last_str, last_str + last.size());

@debadree25
Copy link
Member

Understood! Thanks! will attempt this!

miguelteixeiraa added a commit to miguelteixeiraa/node that referenced this issue Jan 16, 2023
@miguelteixeiraa
src: refactor EndsInANumber in node_url.cc and adds IsIPv4NumberValid
ff9c478 
Removes the use of vector in EndsInANumber and uses
IsIPv4NumberValid instead of parsing the number to check
if it is valid.

Fixes: nodejs/performance#36
Refs: ada-url/ada#36
@miguelteixeiraa miguelteixeiraa mentioned this issue Jan 16, 2023
Merged
@miguelteixeiraa
Copy link

I'm really sorry @debadree25 ! : ((
I was already working on it and should have commented something here.
I apologize for that

@debadree25
Copy link
Member

I'm really sorry @debadree25 ! : (( I was already working on it and should have commented something here. I apologize for that

No issue at all checking out your PR to learn!!

nodejs-github-bot pushed a commit to nodejs/node that referenced this issue Jan 23, 2023
@miguelteixeiraa
src: refactor EndsInANumber in node_url.cc and adds IsIPv4NumberValid
762a3a8 
Removes the use of vector in EndsInANumber and uses
IsIPv4NumberValid instead of parsing the number to check
if it is valid.

Fixes: nodejs/performance#36
Refs: ada-url/ada#36
PR-URL: #46227
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@anonrig anonrig closed this as completed Jan 23, 2023
ruyadorno pushed a commit to nodejs/node that referenced this issue Feb 1, 2023
@miguelteixeiraa @ruyadorno
src: refactor EndsInANumber in node_url.cc and adds IsIPv4NumberValid
640d111 
Removes the use of vector in EndsInANumber and uses
IsIPv4NumberValid instead of parsing the number to check
if it is valid.

Fixes: nodejs/performance#36
Refs: ada-url/ada#36
PR-URL: #46227
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
juanarbol pushed a commit to nodejs/node that referenced this issue Mar 3, 2023
@miguelteixeiraa @juanarbol
src: refactor EndsInANumber in node_url.cc and adds IsIPv4NumberValid
8d90f5c 
Removes the use of vector in EndsInANumber and uses
IsIPv4NumberValid instead of parsing the number to check
if it is valid.

Fixes: nodejs/performance#36
Refs: ada-url/ada#36
PR-URL: #46227
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
juanarbol pushed a commit to nodejs/node that referenced this issue Mar 5, 2023
@miguelteixeiraa @juanarbol
src: refactor EndsInANumber in node_url.cc and adds IsIPv4NumberValid
b06298c 
Removes the use of vector in EndsInANumber and uses
IsIPv4NumberValid instead of parsing the number to check
if it is valid.

Fixes: nodejs/performance#36
Refs: ada-url/ada#36
PR-URL: #46227
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

src: refactor EndsInANumber in node_url.cc and adds IsIPv4NumberValid miguelteixeiraa/node
3 participants
@anonrig @debadree25 @miguelteixeiraa