From 665bac07857e233c82f6d412b5bef94f0597cb30 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Tue, 24 Apr 2018 10:24:51 +0200 Subject: [PATCH 1/2] http: refactor outgoing headers processing Use a shared function, for..in instead of Object.keys, do less work in `setHeader` and instead defer some of it until later, and other minor changes to improve clarity, as well as a slight boost in performance. --- benchmark/http/headers.js | 36 ++++ lib/_http_outgoing.js | 178 ++++++++---------- .../test-http-outgoing-internal-headers.js | 10 +- test/sequential/test-benchmark-http.js | 1 + 4 files changed, 122 insertions(+), 103 deletions(-) create mode 100644 benchmark/http/headers.js diff --git a/benchmark/http/headers.js b/benchmark/http/headers.js new file mode 100644 index 00000000000000..5b2aec554ae9d6 --- /dev/null +++ b/benchmark/http/headers.js @@ -0,0 +1,36 @@ +'use strict'; + +const common = require('../common.js'); +const http = require('http'); + +const bench = common.createBenchmark(main, { + duplicates: [1, 100], + n: [10, 1000], +}); + +function main({ duplicates, n }) { + const headers = { + 'Connection': 'keep-alive', + 'Transfer-Encoding': 'chunked', + }; + + for (var i = 0; i < n; i++) { + headers[`foo${i}`] = []; + for (var j = 0; j < duplicates; j++) { + headers[`foo${i}`].push(`some header value ${i}`); + } + } + + const server = http.createServer(function(req, res) { + res.writeHead(200, headers); + res.end(); + }); + server.listen(common.PORT, function() { + bench.http({ + path: '/', + connections: 1 + }, function() { + server.close(); + }); + }); +} diff --git a/lib/_http_outgoing.js b/lib/_http_outgoing.js index 770e555d1ead8a..f075e0ecad7a2b 100644 --- a/lib/_http_outgoing.js +++ b/lib/_http_outgoing.js @@ -52,6 +52,8 @@ const { utcDate } = internalHttp; const kIsCorked = Symbol('isCorked'); +const hasOwnProperty = Function.call.bind(Object.prototype.hasOwnProperty); + var RE_CONN_CLOSE = /(?:^|\W)close(?:$|\W)/i; var RE_TE_CHUNKED = common.chunkExpression; @@ -116,7 +118,7 @@ Object.defineProperty(OutgoingMessage.prototype, '_headers', { if (val == null) { this[outHeadersKey] = null; } else if (typeof val === 'object') { - const headers = this[outHeadersKey] = {}; + const headers = this[outHeadersKey] = Object.create(null); const keys = Object.keys(val); for (var i = 0; i < keys.length; ++i) { const name = keys[i]; @@ -129,7 +131,7 @@ Object.defineProperty(OutgoingMessage.prototype, '_headers', { Object.defineProperty(OutgoingMessage.prototype, '_headerNames', { get: function() { const headers = this[outHeadersKey]; - if (headers) { + if (headers !== null) { const out = Object.create(null); const keys = Object.keys(headers); for (var i = 0; i < keys.length; ++i) { @@ -138,9 +140,8 @@ Object.defineProperty(OutgoingMessage.prototype, '_headerNames', { out[key] = val; } return out; - } else { - return headers; } + return null; }, set: function(val) { if (typeof val === 'object' && val !== null) { @@ -164,14 +165,14 @@ OutgoingMessage.prototype._renderHeaders = function _renderHeaders() { } var headersMap = this[outHeadersKey]; - if (!headersMap) return {}; - - var headers = {}; - var keys = Object.keys(headersMap); + const headers = {}; - for (var i = 0, l = keys.length; i < l; i++) { - var key = keys[i]; - headers[headersMap[key][0]] = headersMap[key][1]; + if (headersMap !== null) { + const keys = Object.keys(headersMap); + for (var i = 0, l = keys.length; i < l; i++) { + const key = keys[i]; + headers[headersMap[key][0]] = headersMap[key][1]; + } } return headers; }; @@ -285,72 +286,40 @@ OutgoingMessage.prototype._storeHeader = _storeHeader; function _storeHeader(firstLine, headers) { // firstLine in the case of request is: 'GET /index.html HTTP/1.1\r\n' // in the case of response it is: 'HTTP/1.1 200 OK\r\n' - var state = { + const state = { connection: false, contLen: false, te: false, date: false, expect: false, trailer: false, - upgrade: false, header: firstLine }; - var field; var key; - var value; - var i; - var j; if (headers === this[outHeadersKey]) { for (key in headers) { - var entry = headers[key]; - field = entry[0]; - value = entry[1]; - - if (value instanceof Array) { - if (value.length < 2 || !isCookieField(field)) { - for (j = 0; j < value.length; j++) - storeHeader(this, state, field, value[j], false); - continue; - } - value = value.join('; '); - } - storeHeader(this, state, field, value, false); + const entry = headers[key]; + processHeader(this, state, entry[0], entry[1], false); } - } else if (headers instanceof Array) { - for (i = 0; i < headers.length; i++) { - field = headers[i][0]; - value = headers[i][1]; - - if (value instanceof Array) { - for (j = 0; j < value.length; j++) { - storeHeader(this, state, field, value[j], true); - } - } else { - storeHeader(this, state, field, value, true); - } + } else if (Array.isArray(headers)) { + for (var i = 0; i < headers.length; i++) { + const entry = headers[i]; + processHeader(this, state, entry[0], entry[1], true); } } else if (headers) { - var keys = Object.keys(headers); - for (i = 0; i < keys.length; i++) { - field = keys[i]; - value = headers[field]; - - if (value instanceof Array) { - if (value.length < 2 || !isCookieField(field)) { - for (j = 0; j < value.length; j++) - storeHeader(this, state, field, value[j], true); - continue; - } - value = value.join('; '); + for (key in headers) { + if (hasOwnProperty(headers, key)) { + processHeader(this, state, key, headers[key], true); } - storeHeader(this, state, field, value, true); } } + let { header } = state; + // Date header if (this.sendDate && !state.date) { - state.header += 'Date: ' + utcDate() + CRLF; + header += 'Date: ' + utcDate() + CRLF; } // Force the connection to close when the response is a 204 No Content or @@ -364,9 +333,9 @@ function _storeHeader(firstLine, headers) { // It was pointed out that this might confuse reverse proxies to the point // of creating security liabilities, so suppress the zero chunk and force // the connection to close. - var statusCode = this.statusCode; - if ((statusCode === 204 || statusCode === 304) && this.chunkedEncoding) { - debug(statusCode + ' response should not use chunked encoding,' + + if (this.chunkedEncoding && (this.statusCode === 204 || + this.statusCode === 304)) { + debug(this.statusCode + ' response should not use chunked encoding,' + ' closing connection.'); this.chunkedEncoding = false; this.shouldKeepAlive = false; @@ -377,13 +346,13 @@ function _storeHeader(firstLine, headers) { this._last = true; this.shouldKeepAlive = false; } else if (!state.connection) { - var shouldSendKeepAlive = this.shouldKeepAlive && + const shouldSendKeepAlive = this.shouldKeepAlive && (state.contLen || this.useChunkedEncodingByDefault || this.agent); if (shouldSendKeepAlive) { - state.header += 'Connection: keep-alive\r\n'; + header += 'Connection: keep-alive\r\n'; } else { this._last = true; - state.header += 'Connection: close\r\n'; + header += 'Connection: close\r\n'; } } @@ -396,9 +365,9 @@ function _storeHeader(firstLine, headers) { } else if (!state.trailer && !this._removedContLen && typeof this._contentLength === 'number') { - state.header += 'Content-Length: ' + this._contentLength + CRLF; + header += 'Content-Length: ' + this._contentLength + CRLF; } else if (!this._removedTE) { - state.header += 'Transfer-Encoding: chunked\r\n'; + header += 'Transfer-Encoding: chunked\r\n'; this.chunkedEncoding = true; } else { // We should only be able to get here if both Content-Length and @@ -416,7 +385,7 @@ function _storeHeader(firstLine, headers) { throw new ERR_HTTP_TRAILER_INVALID(); } - this._header = state.header + CRLF; + this._header = header + CRLF; this._headerSent = false; // wait until the first body chunk, or close(), is sent to flush, @@ -424,10 +393,23 @@ function _storeHeader(firstLine, headers) { if (state.expect) this._send(''); } -function storeHeader(self, state, key, value, validate) { - if (validate) { - validateHeader(key, value); +function processHeader(self, state, key, value, validate) { + if (validate) + validateHeaderName(key); + if (Array.isArray(value)) { + if (value.length < 2 || !isCookieField(key)) { + for (var i = 0; i < value.length; i++) + storeHeader(self, state, key, value[i], validate); + return; + } + value = value.join('; '); } + storeHeader(self, state, key, value, validate); +} + +function storeHeader(self, state, key, value, validate) { + if (validate) + validateHeaderValue(key, value); state.header += key + ': ' + escapeHeaderValue(value) + CRLF; matchHeader(self, state, key, value); } @@ -439,6 +421,7 @@ function matchHeader(self, state, field, value) { switch (field) { case 'connection': state.connection = true; + self._removedConnection = false; if (RE_CONN_CLOSE.test(value)) self._last = true; else @@ -446,32 +429,39 @@ function matchHeader(self, state, field, value) { break; case 'transfer-encoding': state.te = true; + self._removedTE = false; if (RE_TE_CHUNKED.test(value)) self.chunkedEncoding = true; break; case 'content-length': state.contLen = true; + self._removedContLen = false; break; case 'date': case 'expect': case 'trailer': - case 'upgrade': state[field] = true; break; } } -function validateHeader(name, value) { - let err; +function validateHeaderName(name) { if (typeof name !== 'string' || !name || !checkIsHttpToken(name)) { - err = new ERR_INVALID_HTTP_TOKEN('Header name', name); - } else if (value === undefined) { + const err = new ERR_INVALID_HTTP_TOKEN('Header name', name); + Error.captureStackTrace(err, validateHeaderName); + throw err; + } +} + +function validateHeaderValue(name, value) { + let err; + if (value === undefined) { err = new ERR_HTTP_INVALID_HEADER_VALUE(value, name); } else if (checkInvalidHeaderChar(value)) { debug('Header "%s" contains invalid characters', name); err = new ERR_INVALID_CHAR('header content', name); } if (err !== undefined) { - Error.captureStackTrace(err, validateHeader); + Error.captureStackTrace(err, validateHeaderValue); throw err; } } @@ -480,25 +470,14 @@ OutgoingMessage.prototype.setHeader = function setHeader(name, value) { if (this._header) { throw new ERR_HTTP_HEADERS_SENT('set'); } - validateHeader(name, value); + validateHeaderName(name); + validateHeaderValue(name, value); - if (!this[outHeadersKey]) - this[outHeadersKey] = {}; + let headers = this[outHeadersKey]; + if (headers === null) + this[outHeadersKey] = headers = Object.create(null); - const key = name.toLowerCase(); - this[outHeadersKey][key] = [name, value]; - - switch (key) { - case 'connection': - this._removedConnection = false; - break; - case 'content-length': - this._removedContLen = false; - break; - case 'transfer-encoding': - this._removedTE = false; - break; - } + headers[name.toLowerCase()] = [name, value]; }; @@ -507,18 +486,18 @@ OutgoingMessage.prototype.getHeader = function getHeader(name) { throw new ERR_INVALID_ARG_TYPE('name', 'string', name); } - if (!this[outHeadersKey]) return; - - var entry = this[outHeadersKey][name.toLowerCase()]; - if (!entry) + const headers = this[outHeadersKey]; + if (headers === null) return; - return entry[1]; + + const entry = headers[name.toLowerCase()]; + return entry && entry[1]; }; // Returns an array of the names of the current outgoing headers. OutgoingMessage.prototype.getHeaderNames = function getHeaderNames() { - return (this[outHeadersKey] ? Object.keys(this[outHeadersKey]) : []); + return this[outHeadersKey] !== null ? Object.keys(this[outHeadersKey]) : []; }; @@ -543,7 +522,8 @@ OutgoingMessage.prototype.hasHeader = function hasHeader(name) { throw new ERR_INVALID_ARG_TYPE('name', 'string', name); } - return !!(this[outHeadersKey] && this[outHeadersKey][name.toLowerCase()]); + return this[outHeadersKey] !== null && + !!this[outHeadersKey][name.toLowerCase()]; }; @@ -573,7 +553,7 @@ OutgoingMessage.prototype.removeHeader = function removeHeader(name) { break; } - if (this[outHeadersKey]) { + if (this[outHeadersKey] !== null) { delete this[outHeadersKey][key]; } }; diff --git a/test/parallel/test-http-outgoing-internal-headers.js b/test/parallel/test-http-outgoing-internal-headers.js index e36917a970d987..de75a44e8a05d7 100644 --- a/test/parallel/test-http-outgoing-internal-headers.js +++ b/test/parallel/test-http-outgoing-internal-headers.js @@ -21,8 +21,10 @@ const { OutgoingMessage } = require('http'); Origin: 'localhost' }; - assert.deepStrictEqual(outgoingMessage[outHeadersKey], { - host: ['host', 'risingstack.com'], - origin: ['Origin', 'localhost'] - }); + assert.deepStrictEqual( + Object.entries(outgoingMessage[outHeadersKey]), + Object.entries({ + host: ['host', 'risingstack.com'], + origin: ['Origin', 'localhost'] + })); } diff --git a/test/sequential/test-benchmark-http.js b/test/sequential/test-benchmark-http.js index 13d0e4d16aaf8f..22efce51909481 100644 --- a/test/sequential/test-benchmark-http.js +++ b/test/sequential/test-benchmark-http.js @@ -18,6 +18,7 @@ runBenchmark('http', 'chunkedEnc=true', 'chunks=0', 'dur=0.1', + 'duplicates=1', 'input=keep-alive', 'key=""', 'len=1', From e21e7ff444bb5a4d3050693a0a664ecfc53d69a8 Mon Sep 17 00:00:00 2001 From: Anatoli Papirovski Date: Tue, 24 Apr 2018 14:21:24 +0200 Subject: [PATCH 2/2] fixup: fix benchmark --- benchmark/http/headers.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/benchmark/http/headers.js b/benchmark/http/headers.js index 5b2aec554ae9d6..748865afbf3a04 100644 --- a/benchmark/http/headers.js +++ b/benchmark/http/headers.js @@ -14,7 +14,7 @@ function main({ duplicates, n }) { 'Transfer-Encoding': 'chunked', }; - for (var i = 0; i < n; i++) { + for (var i = 0; i < n / duplicates; i++) { headers[`foo${i}`] = []; for (var j = 0; j < duplicates; j++) { headers[`foo${i}`].push(`some header value ${i}`); @@ -28,7 +28,7 @@ function main({ duplicates, n }) { server.listen(common.PORT, function() { bench.http({ path: '/', - connections: 1 + connections: 10 }, function() { server.close(); });