Merged
Conversation
mscdex
added
dns
mscdex
force-pushed
the
dns-remove-callback-overhead
branch
from
b34df4d to
33759f2
Compare
Trott
reviewed
May 28, 2017
benchmark/dns/lookup.js
Outdated
Member
There was a problem hiding this comment.
Does it make sense to throw in localhost so that there's a hostname lookup since that probably takes a different path through the code?
Contributor
Author
There was a problem hiding this comment.
I chose this list because they don't actually hit libuv and so (in this particular case) it is a better measure of the forced async callback change. Also when you start passing in non-empty hostnames, the n value will need to be lowered quite a bit.
evanlucas
reviewed
May 30, 2017
lib/dns.js
Outdated
Contributor
There was a problem hiding this comment.
any reason to not use net.isIPv4 here?
Contributor
Author
There was a problem hiding this comment.
Faster to not have to call into C++ land IIRC, especially since we only need to check at most a few characters anyway.
Member
There was a problem hiding this comment.
@mscdex how much faster is this compared to something simpler like
function digits(code) {
return code >= 48 && code <= 57;
}
Contributor
Author
|
/cc @nodejs/collaborators |
silverwind
approved these changes
Jun 1, 2017
jasnell
approved these changes
Jun 1, 2017
0.0.0.0 is more common than other special ipv4 addresses, so it is possible that we may not get ENOTFOUND for such addresses. Instead, this commit uses a less common address that is reserved for documentation (RFC) use only. PR-URL: nodejs#13261 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: James M Snell <jasnell@gmail.com>
It appears that either c-ares no longer calls callbacks synchronously or we have since explicitly taken care of the scenarios in which c-ares would call callbacks synchronously (e.g. resolving an IP address or an empty hostname). Therefore we no longer need to have machinery in place to handle possible synchronous callback invocation. This improves performance significantly. PR-URL: nodejs#13261 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#13261 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: James M Snell <jasnell@gmail.com>
mscdex
force-pushed
the
dns-remove-callback-overhead
branch
from
33759f2 to
3c989de
Compare
jasnell
pushed a commit
that referenced
this pull request
Jun 5, 2017
0.0.0.0 is more common than other special ipv4 addresses, so it is possible that we may not get ENOTFOUND for such addresses. Instead, this commit uses a less common address that is reserved for documentation (RFC) use only. PR-URL: #13261 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell
pushed a commit
that referenced
this pull request
Jun 5, 2017
It appears that either c-ares no longer calls callbacks synchronously or we have since explicitly taken care of the scenarios in which c-ares would call callbacks synchronously (e.g. resolving an IP address or an empty hostname). Therefore we no longer need to have machinery in place to handle possible synchronous callback invocation. This improves performance significantly. PR-URL: #13261 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell
pushed a commit
that referenced
this pull request
Jun 5, 2017
PR-URL: #13261 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: James M Snell <jasnell@gmail.com>
Closed
3 tasks
MylesBorins
added
baking-for-lts
Contributor
|
should this land in LTS? If so it will need to bake a bit longer. Please change labels as appropriate |
MylesBorins
removed
the
baking-for-lts
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Example results with the included benchmark file:
I looked at the original commit where the "forced" async callback mechanism was added and I was not able to reproduce the sync callback issue with the tests included with that commit. My guess is that either c-ares changed or we have since manually taken care of such possible sync situations (e.g. empty hostnames or looking up an IP address).
I've also ran the dns tests in test/internet and there are no problems there either.
CI: https://ci.nodejs.org/job/node-test-pull-request/8343/
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)