From 1c086e43d98400c80de2ee869c9cdbf8f78b4818 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Fri, 5 Jan 2018 09:03:10 -0500 Subject: [PATCH 1/3] async_hooks: update defaultTriggerAsyncIdScope for perf The existing version of defaultTriggerAsyncIdScope creates an Array for the callback's arguments which is highly inefficient. Instead, use rest syntax and allow V8 to do that work for us. This yields roughly 2x performance for this particular function. PR-URL: https://github.com/nodejs/node/pull/18004 Reviewed-By: Andreas Madsen Reviewed-By: Luigi Pinca Reviewed-By: James M Snell Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig --- lib/dgram.js | 4 ++-- lib/internal/async_hooks.js | 4 ++-- lib/net.js | 14 +++++++------- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/dgram.js b/lib/dgram.js index 59534b6c269d3d..cd70dc5be91f0d 100644 --- a/lib/dgram.js +++ b/lib/dgram.js @@ -450,8 +450,8 @@ Socket.prototype.send = function(buffer, const afterDns = (ex, ip) => { defaultTriggerAsyncIdScope( this[async_id_symbol], - [ex, this, ip, list, address, port, callback], - doSend + doSend, + ex, this, ip, list, address, port, callback ); }; diff --git a/lib/internal/async_hooks.js b/lib/internal/async_hooks.js index 461dad84ebedf4..f411f64928cc09 100644 --- a/lib/internal/async_hooks.js +++ b/lib/internal/async_hooks.js @@ -260,7 +260,7 @@ function getDefaultTriggerAsyncId() { } -function defaultTriggerAsyncIdScope(triggerAsyncId, opaque, block) { +function defaultTriggerAsyncIdScope(triggerAsyncId, block, ...args) { // CHECK(Number.isSafeInteger(triggerAsyncId)) // CHECK(triggerAsyncId > 0) const oldDefaultTriggerAsyncId = async_id_fields[kDefaultTriggerAsyncId]; @@ -268,7 +268,7 @@ function defaultTriggerAsyncIdScope(triggerAsyncId, opaque, block) { var ret; try { - ret = Reflect.apply(block, null, opaque); + ret = Reflect.apply(block, null, args); } finally { async_id_fields[kDefaultTriggerAsyncId] = oldDefaultTriggerAsyncId; } diff --git a/lib/net.js b/lib/net.js index 6e21e9d6c83ee6..886365092c05a6 100644 --- a/lib/net.js +++ b/lib/net.js @@ -304,7 +304,7 @@ function onSocketFinish() { return this.destroy(); var err = defaultTriggerAsyncIdScope( - this[async_id_symbol], [this, afterShutdown], shutdownSocket + this[async_id_symbol], shutdownSocket, this, afterShutdown ); if (err) @@ -1017,7 +1017,7 @@ Socket.prototype.connect = function(...args) { path); } defaultTriggerAsyncIdScope( - this[async_id_symbol], [this, path], internalConnect + this[async_id_symbol], internalConnect, this, path ); } else { lookupAndConnect(this, options); @@ -1063,8 +1063,8 @@ function lookupAndConnect(self, options) { if (self.connecting) defaultTriggerAsyncIdScope( self[async_id_symbol], - [self, host, port, addressType, localAddress, localPort], - internalConnect + internalConnect, + self, host, port, addressType, localAddress, localPort ); }); return; @@ -1092,7 +1092,7 @@ function lookupAndConnect(self, options) { debug('connect: dns options', dnsopts); self._host = host; var lookup = options.lookup || dns.lookup; - defaultTriggerAsyncIdScope(self[async_id_symbol], [], function() { + defaultTriggerAsyncIdScope(self[async_id_symbol], function() { lookup(host, dnsopts, function emitLookup(err, ip, addressType) { self.emit('lookup', err, ip, addressType, host); @@ -1114,8 +1114,8 @@ function lookupAndConnect(self, options) { self._unrefTimer(); defaultTriggerAsyncIdScope( self[async_id_symbol], - [self, ip, port, addressType, localAddress, localPort], - internalConnect + internalConnect, + self, ip, port, addressType, localAddress, localPort ); } }); From 731c0d9c3b84bb16b46b7082f51067ee785a56b6 Mon Sep 17 00:00:00 2001 From: Andreas Madsen Date: Fri, 5 Jan 2018 15:40:47 +0100 Subject: [PATCH 2/3] async_hooks,http: set HTTPParser trigger to socket This allows more easy tracking of where HTTP requests come from. Before this change the HTTPParser would have the HTTPServer as the triggerAsyncId. The HTTPParser will still have the executionAsyncId set to the HTTP Server so that information is still directly available. Indirectly, the TCP socket itself also has its triggerAsyncId set to the TCP Server. PR-URL: https://github.com/nodejs/node/pull/18003 Reviewed-By: Anna Henningsen Reviewed-By: Anatoli Papirovski Reviewed-By: James M Snell Reviewed-By: Colin Ihrig Reviewed-By: Jon Moss Reviewed-By: Daijiro Wachi --- lib/_http_server.js | 28 ++++++++++----- lib/internal/async_hooks.js | 12 ++++++- test/async-hooks/test-graph.http.js | 56 +++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 10 deletions(-) create mode 100644 test/async-hooks/test-graph.http.js diff --git a/lib/_http_server.js b/lib/_http_server.js index 5857e43d79c787..dee0de74bc75de 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -37,6 +37,10 @@ const { } = require('_http_common'); const { OutgoingMessage } = require('_http_outgoing'); const { outHeadersKey, ondrain } = require('internal/http'); +const { + defaultTriggerAsyncIdScope, + getOrSetAsyncId +} = require('internal/async_hooks'); const errors = require('internal/errors'); const Buffer = require('buffer').Buffer; @@ -292,6 +296,12 @@ Server.prototype.setTimeout = function setTimeout(msecs, callback) { function connectionListener(socket) { + defaultTriggerAsyncIdScope( + getOrSetAsyncId(socket), connectionListenerInternal, this, socket + ); +} + +function connectionListenerInternal(server, socket) { debug('SERVER new http connection'); httpSocketSetup(socket); @@ -299,13 +309,13 @@ function connectionListener(socket) { // Ensure that the server property of the socket is correctly set. // See https://github.com/nodejs/node/issues/13435 if (socket.server === null) - socket.server = this; + socket.server = server; // If the user has added a listener to the server, // request, or response, then it's their responsibility. // otherwise, destroy on timeout by default - if (this.timeout && typeof socket.setTimeout === 'function') - socket.setTimeout(this.timeout); + if (server.timeout && typeof socket.setTimeout === 'function') + socket.setTimeout(server.timeout); socket.on('timeout', socketOnTimeout); var parser = parsers.alloc(); @@ -315,8 +325,8 @@ function connectionListener(socket) { parser.incoming = null; // Propagate headers limit from server instance to parser - if (typeof this.maxHeadersCount === 'number') { - parser.maxHeaderPairs = this.maxHeadersCount << 1; + if (typeof server.maxHeadersCount === 'number') { + parser.maxHeaderPairs = server.maxHeadersCount << 1; } else { // Set default value because parser may be reused from FreeList parser.maxHeaderPairs = 2000; @@ -336,8 +346,8 @@ function connectionListener(socket) { outgoingData: 0, keepAliveTimeoutSet: false }; - state.onData = socketOnData.bind(undefined, this, socket, parser, state); - state.onEnd = socketOnEnd.bind(undefined, this, socket, parser, state); + state.onData = socketOnData.bind(undefined, server, socket, parser, state); + state.onEnd = socketOnEnd.bind(undefined, server, socket, parser, state); state.onClose = socketOnClose.bind(undefined, socket, state); state.onDrain = socketOnDrain.bind(undefined, socket, state); socket.on('data', state.onData); @@ -345,7 +355,7 @@ function connectionListener(socket) { socket.on('end', state.onEnd); socket.on('close', state.onClose); socket.on('drain', state.onDrain); - parser.onIncoming = parserOnIncoming.bind(undefined, this, socket, state); + parser.onIncoming = parserOnIncoming.bind(undefined, server, socket, state); // We are consuming socket, so it won't get any actual data socket.on('resume', onSocketResume); @@ -364,7 +374,7 @@ function connectionListener(socket) { } } parser[kOnExecute] = - onParserExecute.bind(undefined, this, socket, parser, state); + onParserExecute.bind(undefined, server, socket, parser, state); socket._paused = false; } diff --git a/lib/internal/async_hooks.js b/lib/internal/async_hooks.js index f411f64928cc09..598021efe34f9e 100644 --- a/lib/internal/async_hooks.js +++ b/lib/internal/async_hooks.js @@ -26,7 +26,7 @@ const async_wrap = process.binding('async_wrap'); * It has a fixed size, so if that is exceeded, calls to the native * side are used instead in pushAsyncIds() and popAsyncIds(). */ -const { async_hook_fields, async_id_fields } = async_wrap; +const { async_id_symbol, async_hook_fields, async_id_fields } = async_wrap; // Store the pair executionAsyncId and triggerAsyncId in a std::stack on // Environment::AsyncHooks::ids_stack_ tracks the resource responsible for the // current execution stack. This is unwound as each resource exits. In the case @@ -248,6 +248,15 @@ function newUid() { return ++async_id_fields[kAsyncIdCounter]; } +function getOrSetAsyncId(object) { + if (object.hasOwnProperty(async_id_symbol)) { + return object[async_id_symbol]; + } + + return object[async_id_symbol] = newUid(); +} + + // Return the triggerAsyncId meant for the constructor calling it. It's up to // the user to safeguard this call and make sure it's zero'd out when the // constructor is complete. @@ -378,6 +387,7 @@ module.exports = { disableHooks, // Sensitive Embedder API newUid, + getOrSetAsyncId, getDefaultTriggerAsyncId, defaultTriggerAsyncIdScope, emitInit: emitInitScript, diff --git a/test/async-hooks/test-graph.http.js b/test/async-hooks/test-graph.http.js new file mode 100644 index 00000000000000..5c0c99f408c824 --- /dev/null +++ b/test/async-hooks/test-graph.http.js @@ -0,0 +1,56 @@ +'use strict'; + +const common = require('../common'); +const initHooks = require('./init-hooks'); +const verifyGraph = require('./verify-graph'); +const http = require('http'); + +const hooks = initHooks(); +hooks.enable(); + +const server = http.createServer(common.mustCall(function(req, res) { + res.end(); + this.close(common.mustCall()); +})); +server.listen(0, common.mustCall(function() { + http.get(`http://127.0.0.1:${server.address().port}`, common.mustCall()); +})); + +process.on('exit', function() { + hooks.disable(); + + verifyGraph( + hooks, + [ { type: 'TCPSERVERWRAP', + id: 'tcpserver:1', + triggerAsyncId: null }, + { type: 'TCPWRAP', id: 'tcp:1', triggerAsyncId: 'tcpserver:1' }, + { type: 'TCPCONNECTWRAP', + id: 'tcpconnect:1', + triggerAsyncId: 'tcp:1' }, + { type: 'HTTPPARSER', + id: 'httpparser:1', + triggerAsyncId: 'tcpserver:1' }, + { type: 'HTTPPARSER', + id: 'httpparser:2', + triggerAsyncId: 'tcpserver:1' }, + { type: 'TCPWRAP', id: 'tcp:2', triggerAsyncId: 'tcpserver:1' }, + { type: 'Timeout', id: 'timeout:1', triggerAsyncId: 'tcp:2' }, + { type: 'TIMERWRAP', id: 'timer:1', triggerAsyncId: 'tcp:2' }, + { type: 'HTTPPARSER', + id: 'httpparser:3', + triggerAsyncId: 'tcp:2' }, + { type: 'HTTPPARSER', + id: 'httpparser:4', + triggerAsyncId: 'tcp:2' }, + { type: 'Timeout', + id: 'timeout:2', + triggerAsyncId: 'httpparser:4' }, + { type: 'TIMERWRAP', + id: 'timer:2', + triggerAsyncId: 'httpparser:4' }, + { type: 'SHUTDOWNWRAP', + id: 'shutdown:1', + triggerAsyncId: 'tcp:2' } ] + ); +}); From e6794d9185bbdef9520251d97001d054f286a920 Mon Sep 17 00:00:00 2001 From: Andreas Madsen Date: Sun, 14 Jan 2018 11:09:24 +0100 Subject: [PATCH 3/3] async_hooks,test: only use IPv6 in http test If IPv6 is not supported on a machine, the IPv6 handle will first be created, this will then fail and default to an IPv4 handle. This causes the graph to change, as there now is an extra handle. PR-URL: https://github.com/nodejs/node/pull/18143 Fixes: https://github.com/nodejs/node/issues/18003 Reviewed-By: Anna Henningsen Reviewed-By: Rich Trott Reviewed-By: Anatoli Papirovski Reviewed-By: James M Snell --- test/async-hooks/test-graph.http.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/async-hooks/test-graph.http.js b/test/async-hooks/test-graph.http.js index 5c0c99f408c824..eea72ca3bac72c 100644 --- a/test/async-hooks/test-graph.http.js +++ b/test/async-hooks/test-graph.http.js @@ -1,6 +1,9 @@ 'use strict'; const common = require('../common'); +if (!common.hasIPv6) + common.skip('IPv6 support required'); + const initHooks = require('./init-hooks'); const verifyGraph = require('./verify-graph'); const http = require('http'); @@ -13,7 +16,11 @@ const server = http.createServer(common.mustCall(function(req, res) { this.close(common.mustCall()); })); server.listen(0, common.mustCall(function() { - http.get(`http://127.0.0.1:${server.address().port}`, common.mustCall()); + http.get({ + host: '::1', + family: 6, + port: server.address().port + }, common.mustCall()); })); process.on('exit', function() {