dns: improve performance #13261
Merged
dns: improve performance #13261
Conversation
This file contains 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
mscdex
added
dns
mscdex
force-pushed
the
dns-remove-callback-overhead
branch
from
b34df4d
to
33759f2
Compare
Trott
reviewed
May 28, 2017
| const lookup = require('dns').lookup; | ||
|
|
||
| const bench = common.createBenchmark(main, { | ||
| name: ['', '127.0.0.1', '::1'], |
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?
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
| 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // 96-111 | ||
| 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 // 112-127 | ||
| ]; | ||
| function isIPv4(str) { |
There was a problem hiding this comment.
any reason to not use net.isIPv4 here?
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.
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;
}There was a problem hiding this comment.
@lpinca ~19% faster.
|
/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
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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)