FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

/ node Public

net: make isIPv4 and isIPv6 more efficient - #5478

Closed
vkurchatkin wants to merge 1 commit into
nodejs:masterfrom
vkurchatkin:isipv-4-6
ConversationCommits (1)ChecksFiles changed
Closed

net: make isIPv4 and isIPv6 more efficient#5478
vkurchatkin wants to merge 1 commit into
nodejs:masterfrom
vkurchatkin:isipv-4-6

Conversation

vkurchatkin commented Feb 28, 2016

Copy link
Copy Markdown
Contributor

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 reviewed Feb 28, 2016
View reviewed changes
Comment thread src/cares_wrap.cc Outdated

evanlucas Feb 28, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

vkurchatkin Feb 28, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

evanlucas Feb 28, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

evanlucas commented Feb 28, 2016

Copy link
Copy Markdown
Contributor

LGTM with a suggestion

evanlucas commented Feb 28, 2016

Copy link
Copy Markdown
Contributor

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

brendanashworth reviewed Feb 28, 2016
View reviewed changes
Comment thread lib/net.js Outdated

brendanashworth Feb 28, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

vkurchatkin Feb 29, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

brendanashworth Mar 15, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What about now?

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

vkurchatkin commented Feb 29, 2016

Copy link
Copy Markdown
Contributor Author

Added some tests and also support for symbols. PTAL

evanlucas commented Feb 29, 2016

Copy link
Copy Markdown
Contributor

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

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

vkurchatkin commented Feb 29, 2016

Copy link
Copy Markdown
Contributor Author

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

silverwind commented Feb 29, 2016

Copy link
Copy Markdown
Contributor

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 commented Mar 1, 2016

Copy link
Copy Markdown
Contributor

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 commented Mar 1, 2016

Copy link
Copy Markdown
Contributor Author

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 commented Mar 2, 2016

Copy link
Copy Markdown
Member

LGTM

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 removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 13, 2016

vkurchatkin commented Mar 13, 2016

Copy link
Copy Markdown
Contributor Author

Removed controversial commit, squashed and rebased. PTAL

jasnell commented Mar 14, 2016

Copy link
Copy Markdown
Member

Still LGTM

evanlucas commented Mar 14, 2016

Copy link
Copy Markdown
Contributor

LGTM

1 similar comment

cjihrig commented Mar 14, 2016

Copy link
Copy Markdown
Contributor

LGTM

jasnell commented Mar 14, 2016

Copy link
Copy Markdown
Member

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

jasnell commented Mar 14, 2016

Copy link
Copy Markdown
Member

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

silverwind commented Mar 14, 2016

Copy link
Copy Markdown
Contributor

LGTM

jasnell pushed a commit that referenced this pull request Mar 15, 2016
net: make isIPv4 and isIPv6 more efficient
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>

jasnell commented Mar 15, 2016

Copy link
Copy Markdown
Member

Landed in 3b20941

jasnell closed this Mar 15, 2016
evanlucas pushed a commit that referenced this pull request Mar 15, 2016
net: make isIPv4 and isIPv6 more efficient
ae25084 
`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 mentioned this pull request Mar 15, 2016
Merged
rvagg pushed a commit that referenced this pull request Mar 16, 2016
net: make isIPv4 and isIPv6 more efficient
b16f67a 
`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>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
net: make isIPv4 and isIPv6 more efficient
d8594a6 
`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>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
net: make isIPv4 and isIPv6 more efficient
8648420 
`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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

net Issues and PRs related to the net subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL