net: make isIPv4 and isIPv6 more efficient#5478
net: make isIPv4 and isIPv6 more efficient#5478vkurchatkin wants to merge 1 commit intonodejs:masterfrom
isIPv4 and isIPv6 more efficient#5478Conversation
src/cares_wrap.cc
Outdated
There was a problem hiding this comment.
Do we need a check that args[0] is a string here?
There was a problem hiding this comment.
I don't think so. Utf8Value calls ToString under the hood, should be fine.
There was a problem hiding this comment.
maybe include a test passing null, an object, and a number? Just a suggestion
|
LGTM with a suggestion |
lib/net.js
Outdated
There was a problem hiding this comment.
Is there a reason why you don't just assign it as exports.isIPv4 = cares.isIPv4; ?
There was a problem hiding this comment.
No particular reason, but I've just added check for symbol in js, so wrapper is necessary
|
Added some tests and also support for symbols. PTAL |
|
Still LGTM. I think the symbol changes should make it semver-major though. |
|
@evanlucas well, I don't think so. It seems like an oversight, so just a bug. |
|
Do we really expect people to pass symbols to APIs that are expecting a string? I think this check there is penaltizing the common case. Otherwise LGTM. |
|
I think the additional checks for symbols should be in a separate pr/commit since those should be semver-major. We are going from throwing an error, to not throwing an error. The performance changes should be in it's own commit, so it can be backported. Thoughts? |
Definitely, it's not hard.
It's exactly what bug fixing usually does :-) |
|
LGTM |
`isIPv4` and `isIPv6` are implemented on top of `isIP`, which in turn checks the sting for being both IPv4 and IPv6, which can be inefficient in some scenarios. This commit makes them use `uv_inet_pton` directly instead.
|
Removed controversial commit, squashed and rebased. PTAL |
|
Still LGTM |
|
LGTM |
1 similar comment
|
LGTM |
|
Marking this as a watch for v4 but want to hold off a bit before landing, just to be safe. /cc @thealphanerd |
|
LGTM |
`isIPv4` and `isIPv6` are implemented on top of `isIP`, which in turn checks the sting for being both IPv4 and IPv6, which can be inefficient in some scenarios. This commit makes them use `uv_inet_pton` directly instead. PR-URL: #5478 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>
|
Landed in 3b20941 |
`isIPv4` and `isIPv6` are implemented on top of `isIP`, which in turn checks the sting for being both IPv4 and IPv6, which can be inefficient in some scenarios. This commit makes them use `uv_inet_pton` directly instead. PR-URL: #5478 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>
`isIPv4` and `isIPv6` are implemented on top of `isIP`, which in turn checks the sting for being both IPv4 and IPv6, which can be inefficient in some scenarios. This commit makes them use `uv_inet_pton` directly instead. PR-URL: #5478 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>
`isIPv4` and `isIPv6` are implemented on top of `isIP`, which in turn checks the sting for being both IPv4 and IPv6, which can be inefficient in some scenarios. This commit makes them use `uv_inet_pton` directly instead. PR-URL: #5478 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>
`isIPv4` and `isIPv6` are implemented on top of `isIP`, which in turn checks the sting for being both IPv4 and IPv6, which can be inefficient in some scenarios. This commit makes them use `uv_inet_pton` directly instead. PR-URL: #5478 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>
Pull Request check-list
make -j8 test(UNIX) orvcbuild test nosign(Windows) pass withthis change (including linting)?
test (or a benchmark) included?
existing APIs, or introduces new ones)?
Affected core subsystem(s)
net
Description of change
isIPv4andisIPv6are implemented on top ofisIP, which in turnchecks the sting for being both IPv4 and IPv6, which can be inefficient
in some scenarios. This commit makes them use
uv_inet_ptondirectlyinstead.