-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
src: refactor EndsInANumber in node_url.cc and adds IsIPv4NumberValid #46227
src: refactor EndsInANumber in node_url.cc and adds IsIPv4NumberValid #46227
Conversation
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
hello everyone! |
@miguelteixeiraa please use the following command: |
Done! It hasn't changed that much : / |
Yeah, it seems |
Of course! |
Done! |
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1293/ Results
|
Okay, windows problems.. not sure what it is but I'm checking |
src/node_url.cc
Outdated
} | ||
|
||
char delimiter = '.'; | ||
auto pointer_start = input.begin(); |
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.
If you use std::string_view::iterator
it might solve the issues related to std::string_view construct on windows.
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.
done!
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.
Windows build is still failing : /
D:\a\node\node\src\node_url.cc(465,66): error C2440: '<function-style-cast>': cannot convert from 'initializer list' to 'std::string_view' [D:\a\node\node\libnode.vcxproj]
D:\a\node\node\src\node_url.cc(465,66): message : No constructor could take the source type, or constructor overload resolution was ambiguous [D:\a\node\node\libnode.vcxproj]
node_util.cc
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.
I would like to know if I can go with &(*it); again
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.
@addaleax any recommendations?
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.
it'd look like this:
std::string_view(&(*pointer_start), pointer_end - pointer_start));
just to make the build pass on windows
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.
What i understand is that iterators don't need to be pointers although they could be implemented as one. And some implementations of the STL (MSVC++, Mingw) implement interators with special classes (with no pre-difined casts to pointer), so that we cannot use them directly in the string_view constructor that we are using, which needs a charT*.
then we could use * which is overloaded to do what logically mean: convert the iterator type to its pointed-to object, then pass the address of this &(*it).
https://en.cppreference.com/w/cpp/named_req/RandomAccessIterator
https://stackoverflow.com/questions/32654108/c-stdvectoriterator-is-not-a-pointer-why/64910815#64910815
src/node_url.cc
Outdated
|
||
return false; | ||
return IsIPv4NumberValid( | ||
std::string_view(&*pointer_start, pointer_end - pointer_start)); |
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.
This is too hacky (even if it's guaranteed safe).
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.
removed!
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.
I'll try a static_cast<const char*>
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.
@nodejs/cpp-reviewers any recommendations?
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.
Yeah: I would definitely recommeng to go back to the original "hacky" implementation. It works and looks correct, even if it is not visually pretty. (I'd also not call this pointer
if it's an iterator.) const_cast<char*>(pointer_start)
doesn't even really make sense since the string_view constructor takes a const char*
argument.
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.
Yes, I think it only works on the other compilers because their implementations for the iterator are:
std::string_view::iterator' (aka 'const char *')
(I purposely caused an error to catch this 'aka' haha)
microbench: cpu: Apple M1 Pro runtime: node v20.0.0-pre (arm64-darwin) -- with this PR -- |
Are you measuring production build against development build? |
Thinking a little more, the microbench doesnt seem a good idea anymore. Sorry for that. Please just ignore (but yes, I guess I am) |
Landed in 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 <yagiz@nizipli.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
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 <yagiz@nizipli.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
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 <yagiz@nizipli.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
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