Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dns: improve performance #13261

Merged
merged 3 commits into from
Jun 2, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions benchmark/dns/lookup.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
'use strict';

const common = require('../common.js');
const lookup = require('dns').lookup;

const bench = common.createBenchmark(main, {
name: ['', '127.0.0.1', '::1'],
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@mscdex mscdex May 28, 2017

Choose a reason for hiding this comment

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

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.

all: [true, false],
n: [5e6]
});

function main(conf) {
const name = conf.name;
const n = +conf.n;
const all = !!conf.all;
var i = 0;

if (all) {
const opts = { all: true };
bench.start();
(function cb(err, results) {
if (i++ === n) {
bench.end(n);
return;
}
lookup(name, opts, cb);
})();
} else {
bench.start();
(function cb(err, result) {
if (i++ === n) {
bench.end(n);
return;
}
lookup(name, cb);
})();
}
}
85 changes: 37 additions & 48 deletions lib/dns.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,33 +60,29 @@ function errnoException(err, syscall, hostname) {
return ex;
}


// c-ares invokes a callback either synchronously or asynchronously,
// but the dns API should always invoke a callback asynchronously.
//
// This function makes sure that the callback is invoked asynchronously.
// It returns a function that invokes the callback within nextTick().
//
// To avoid invoking unnecessary nextTick(), `immediately` property of
// returned function should be set to true after c-ares returned.
//
// Usage:
//
// function someAPI(callback) {
// callback = makeAsync(callback);
// channel.someAPI(..., callback);
// callback.immediately = true;
// }
function makeAsync(callback) {
return function asyncCallback(...args) {
if (asyncCallback.immediately) {
// The API already returned, we can invoke the callback immediately.
callback.apply(null, args);
} else {
args.unshift(callback);
process.nextTick.apply(null, args);
}
};
const digits = [
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // 0-15
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // 16-31
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // 32-47
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, // 48-63
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // 64-79
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // 80-95
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to not use net.isIPv4 here?

Copy link
Contributor Author

@mscdex mscdex May 30, 2017

Choose a reason for hiding this comment

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

Faster to not have to call into C++ land IIRC, especially since we only need to check at most a few characters anyway.

Copy link
Member

Choose a reason for hiding this comment

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

@mscdex how much faster is this compared to something simpler like

function digits(code) {
  return code >= 48 && code <= 57;
}

Copy link
Contributor Author

@mscdex mscdex May 30, 2017

Choose a reason for hiding this comment

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

@lpinca ~19% faster.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks.

if (!digits[str.charCodeAt(0)]) return false;
if (str.length === 1) return false;
if (str.charCodeAt(1) === 46/*'.'*/)
return true;
else if (!digits[str.charCodeAt(1)])
return false;
if (str.length === 2) return false;
if (str.charCodeAt(2) === 46/*'.'*/)
return true;
else if (!digits[str.charCodeAt(2)])
return false;
return (str.length > 3 && str.charCodeAt(3) === 46/*'.'*/);
}


Expand All @@ -97,25 +93,26 @@ function onlookup(err, addresses) {
if (this.family) {
this.callback(null, addresses[0], this.family);
} else {
this.callback(null, addresses[0], addresses[0].indexOf(':') >= 0 ? 6 : 4);
this.callback(null, addresses[0], isIPv4(addresses[0]) ? 4 : 6);
}
}


function onlookupall(err, addresses) {
var results = [];
if (err) {
return this.callback(errnoException(err, 'getaddrinfo', this.hostname));
}

var family = this.family;
for (var i = 0; i < addresses.length; i++) {
results.push({
address: addresses[i],
family: this.family || (addresses[i].indexOf(':') >= 0 ? 6 : 4)
});
const addr = addresses[i];
addresses[i] = {
address: addr,
family: family || (isIPv4(addr) ? 4 : 6)
};
}

this.callback(null, results);
this.callback(null, addresses);
}


Expand Down Expand Up @@ -153,23 +150,22 @@ function lookup(hostname, options, callback) {
if (family !== 0 && family !== 4 && family !== 6)
throw new TypeError('Invalid argument: family must be 4 or 6');

callback = makeAsync(callback);

if (!hostname) {
if (all) {
callback(null, []);
process.nextTick(callback, null, []);
} else {
callback(null, null, family === 6 ? 6 : 4);
process.nextTick(callback, null, null, family === 6 ? 6 : 4);
}
return {};
}

var matchedFamily = isIP(hostname);
if (matchedFamily) {
if (all) {
callback(null, [{address: hostname, family: matchedFamily}]);
process.nextTick(
callback, null, [{address: hostname, family: matchedFamily}]);
} else {
callback(null, hostname, matchedFamily);
process.nextTick(callback, null, hostname, matchedFamily);
}
return {};
}
Expand All @@ -182,11 +178,9 @@ function lookup(hostname, options, callback) {

var err = cares.getaddrinfo(req, hostname, family, hints);
if (err) {
callback(errnoException(err, 'getaddrinfo', hostname));
process.nextTick(callback, errnoException(err, 'getaddrinfo', hostname));
return {};
}

callback.immediately = true;
return req;
}

Expand Down Expand Up @@ -217,7 +211,6 @@ function lookupService(host, port, callback) {
throw new TypeError('"callback" argument must be a function');

port = +port;
callback = makeAsync(callback);

var req = new GetNameInfoReqWrap();
req.callback = callback;
Expand All @@ -227,8 +220,6 @@ function lookupService(host, port, callback) {

var err = cares.getnameinfo(req, host, port);
if (err) throw errnoException(err, 'getnameinfo', host);

callback.immediately = true;
return req;
}

Expand Down Expand Up @@ -263,7 +254,6 @@ function resolver(bindingName) {
throw new Error('"callback" argument must be a function');
}

callback = makeAsync(callback);
var req = new QueryReqWrap();
req.bindingName = bindingName;
req.callback = callback;
Expand All @@ -272,7 +262,6 @@ function resolver(bindingName) {
req.ttl = !!(options && options.ttl);
var err = binding(req, name);
if (err) throw errnoException(err, bindingName);
callback.immediately = true;
return req;
};
}
Expand Down
7 changes: 4 additions & 3 deletions test/internet/test-dns.js
Original file line number Diff line number Diff line change
Expand Up @@ -492,11 +492,12 @@ TEST(function test_lookupservice_invalid(done) {


TEST(function test_reverse_failure(done) {
const req = dns.reverse('0.0.0.0', function(err) {
// 203.0.113.0/24 are addresses reserved for (RFC) documentation use only
const req = dns.reverse('203.0.113.0', function(err) {
assert(err instanceof Error);
assert.strictEqual(err.code, 'ENOTFOUND'); // Silly error code...
assert.strictEqual(err.hostname, '0.0.0.0');
assert.ok(/0\.0\.0\.0/.test(err.message));
assert.strictEqual(err.hostname, '203.0.113.0');
assert.ok(/203\.0\.113\.0/.test(err.message));

done();
});
Expand Down