net: make `isIPv4` and `isIPv6` more efficient by vkurchatkin · Pull Request #5478 · nodejs/node

vkurchatkin · 2016-02-28T16:54:40Z

Pull Request check-list

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to [CONTRIBUTING.md][0]?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

net

Description of change

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.

evanlucas · 2016-02-28T17:09:07Z

Do we need a check that args[0] is a string here?

I don't think so. Utf8Value calls ToString under the hood, should be fine.

maybe include a test passing null, an object, and a number? Just a suggestion

evanlucas · 2016-02-28T17:49:47Z

LGTM with a suggestion

evanlucas · 2016-02-28T18:02:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1772/

brendanashworth · 2016-02-28T18:12:13Z

Is there a reason why you don't just assign it as exports.isIPv4 = cares.isIPv4; ?

No particular reason, but I've just added check for symbol in js, so wrapper is necessary

What about now?

vkurchatkin · 2016-02-29T11:16:01Z

Added some tests and also support for symbols. PTAL

evanlucas · 2016-02-29T11:40:57Z

Still LGTM. I think the symbol changes should make it semver-major though.

vkurchatkin · 2016-02-29T13:04:20Z

@evanlucas well, I don't think so. It seems like an oversight, so just a bug.

silverwind · 2016-02-29T19:59:41Z

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.

evanlucas · 2016-03-01T17:11:27Z

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?

vkurchatkin · 2016-03-01T18:17:23Z

I think the additional checks for symbols should be in a separate pr/commit

Definitely, it's not hard.

We are going from throwing an error, to not throwing an error

It's exactly what bug fixing usually does :-)

jasnell · 2016-03-02T17:50:07Z

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.

vkurchatkin · 2016-03-13T19:20:06Z

Removed controversial commit, squashed and rebased. PTAL

jasnell · 2016-03-14T15:30:28Z

Still LGTM

evanlucas · 2016-03-14T15:31:16Z

LGTM

cjihrig · 2016-03-14T15:45:01Z

LGTM

jasnell · 2016-03-14T15:50:08Z

Marking this as a watch for v4 but want to hold off a bit before landing, just to be safe. /cc @thealphanerd

jasnell · 2016-03-14T15:58:18Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1921/

silverwind · 2016-03-14T16:47:36Z

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>

jasnell · 2016-03-15T15:33:33Z

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>

evanlucas reviewed Feb 28, 2016
View reviewed changes

brendanashworth reviewed Feb 28, 2016
View reviewed changes

mscdex added the net Issues and PRs related to the net subsystem. label Feb 28, 2016

evanlucas added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 29, 2016

net: make isIPv4 and isIPv6 more efficient
81f269a
`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.

vkurchatkin force-pushed the isipv-4-6 branch from 7f3d5bf to 81f269aCompare March 11, 2016 23:07

vkurchatkin removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 13, 2016

jasnell added the lts-watch-v4.x label Mar 14, 2016

jasnell closed this Mar 15, 2016

evanlucas mentioned this pull request Mar 15, 2016
v5.9.0 proposal #5702
Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 30, 2016

Uh oh!

Uh oh!

Conversation

vkurchatkin commented Feb 28, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

evanlucasFeb 28, 2016

Choose a reason for hiding this comment

Uh oh!

vkurchatkinFeb 28, 2016

Choose a reason for hiding this comment

Uh oh!

evanlucasFeb 28, 2016

Choose a reason for hiding this comment

Uh oh!

evanlucas commented Feb 28, 2016

Uh oh!

evanlucas commented Feb 28, 2016

Uh oh!

brendanashworthFeb 28, 2016

Choose a reason for hiding this comment

Uh oh!

vkurchatkinFeb 29, 2016

Choose a reason for hiding this comment

Uh oh!

brendanashworthMar 15, 2016

Choose a reason for hiding this comment

Uh oh!

vkurchatkin commented Feb 29, 2016

Uh oh!

evanlucas commented Feb 29, 2016

Uh oh!

vkurchatkin commented Feb 29, 2016

Uh oh!

silverwind commented Feb 29, 2016

Uh oh!

evanlucas commented Mar 1, 2016

Uh oh!

vkurchatkin commented Mar 1, 2016

Uh oh!

jasnell commented Mar 2, 2016

Uh oh!

vkurchatkin commented Mar 13, 2016

Uh oh!

jasnell commented Mar 14, 2016

Uh oh!

evanlucas commented Mar 14, 2016

Uh oh!

cjihrig commented Mar 14, 2016

Uh oh!

jasnell commented Mar 14, 2016

Uh oh!

jasnell commented Mar 14, 2016

Uh oh!

silverwind commented Mar 14, 2016

Uh oh!

jasnell commented Mar 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants