From 92c37fe5fde36581532312fd44abea919c645e41 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 10 Aug 2017 00:52:35 +0200 Subject: [PATCH] http2: improve perf of passing headers to C++ By passing a single string rather than many small ones and a single block allocation for passing headers, save expensive interactions with JS values and memory allocations. PR-URL: https://github.com/nodejs/node/pull/14723 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell --- benchmark/http2/headers.js | 47 ++++++++++++++ lib/internal/http2/util.js | 14 ++-- src/node_http2.cc | 65 +++++++++++++++++++ src/node_http2.h | 46 ++----------- test/parallel/test-http2-util-headers-list.js | 56 ++++++---------- 5 files changed, 147 insertions(+), 81 deletions(-) create mode 100644 benchmark/http2/headers.js diff --git a/benchmark/http2/headers.js b/benchmark/http2/headers.js new file mode 100644 index 00000000000000..09449d1e92f208 --- /dev/null +++ b/benchmark/http2/headers.js @@ -0,0 +1,47 @@ +'use strict'; + +const common = require('../common.js'); +const PORT = common.PORT; + +var bench = common.createBenchmark(main, { + n: [1e3], + nheaders: [100, 1000], +}, { flags: ['--expose-http2', '--no-warnings'] }); + +function main(conf) { + const n = +conf.n; + const nheaders = +conf.nheaders; + const http2 = require('http2'); + const server = http2.createServer(); + + const headersObject = { ':path': '/' }; + for (var i = 0; i < nheaders; i++) { + headersObject[`foo${i}`] = `some header value ${i}`; + } + + server.on('stream', (stream) => { + stream.respond(); + stream.end('Hi!'); + }); + server.listen(PORT, () => { + const client = http2.connect(`http://localhost:${PORT}/`); + + function doRequest(remaining) { + const req = client.request(headersObject); + req.end(); + req.on('data', () => {}); + req.on('end', () => { + if (remaining > 0) { + doRequest(remaining - 1); + } else { + bench.end(n); + server.close(); + client.destroy(); + } + }); + } + + bench.start(); + doRequest(n); + }); +} diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js index 33e8ff33e64edb..5054d26e593aa3 100644 --- a/lib/internal/http2/util.js +++ b/lib/internal/http2/util.js @@ -375,7 +375,8 @@ function assertValidPseudoHeaderTrailer(key) { function mapToHeaders(map, assertValuePseudoHeader = assertValidPseudoHeader) { - const ret = []; + let ret = ''; + let count = 0; const keys = Object.keys(map); const singles = new Set(); for (var i = 0; i < keys.length; i++) { @@ -402,7 +403,8 @@ function mapToHeaders(map, const err = assertValuePseudoHeader(key); if (err !== undefined) return err; - ret.unshift([key, String(value)]); + ret = `${key}\0${String(value)}\0${ret}`; + count++; } else { if (kSingleValueHeaders.has(key)) { if (singles.has(key)) @@ -415,16 +417,18 @@ function mapToHeaders(map, if (isArray) { for (var k = 0; k < value.length; k++) { val = String(value[k]); - ret.push([key, val]); + ret += `${key}\0${val}\0`; } + count += value.length; } else { val = String(value); - ret.push([key, val]); + ret += `${key}\0${val}\0`; + count++; } } } - return ret; + return [ret, count]; } class NghttpError extends Error { diff --git a/src/node_http2.cc b/src/node_http2.cc index 2155061ef51feb..28661f9a668ba9 100755 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -9,6 +9,8 @@ using v8::Boolean; using v8::Context; using v8::Function; using v8::Integer; +using v8::String; +using v8::Uint32; using v8::Undefined; namespace http2 { @@ -1075,6 +1077,69 @@ void Http2Session::Unconsume() { } +Headers::Headers(Isolate* isolate, + Local context, + Local headers) { + CHECK_EQ(headers->Length(), 2); + Local header_string = headers->Get(context, 0).ToLocalChecked(); + Local header_count = headers->Get(context, 1).ToLocalChecked(); + CHECK(header_string->IsString()); + CHECK(header_count->IsUint32()); + count_ = header_count.As()->Value(); + int header_string_len = header_string.As()->Length(); + + if (count_ == 0) { + CHECK_EQ(header_string_len, 0); + return; + } + + // Allocate a single buffer with count_ nghttp2_nv structs, followed + // by the raw header data as passed from JS. This looks like: + // | possible padding | nghttp2_nv | nghttp2_nv | ... | header contents | + buf_.AllocateSufficientStorage((alignof(nghttp2_nv) - 1) + + count_ * sizeof(nghttp2_nv) + + header_string_len); + // Make sure the start address is aligned appropriately for an nghttp2_nv*. + char* start = reinterpret_cast( + ROUND_UP(reinterpret_cast(*buf_), alignof(nghttp2_nv))); + char* header_contents = start + (count_ * sizeof(nghttp2_nv)); + nghttp2_nv* const nva = reinterpret_cast(start); + + CHECK_LE(header_contents + header_string_len, *buf_ + buf_.length()); + CHECK_EQ(header_string.As() + ->WriteOneByte(reinterpret_cast(header_contents), + 0, header_string_len, + String::NO_NULL_TERMINATION), + header_string_len); + + size_t n = 0; + char* p; + for (p = header_contents; p < header_contents + header_string_len; n++) { + if (n >= count_) { + // This can happen if a passed header contained a null byte. In that + // case, just provide nghttp2 with an invalid header to make it reject + // the headers list. + static uint8_t zero = '\0'; + nva[0].name = nva[0].value = &zero; + nva[0].namelen = nva[0].valuelen = 1; + count_ = 1; + return; + } + + nva[n].flags = NGHTTP2_NV_FLAG_NONE; + nva[n].name = reinterpret_cast(p); + nva[n].namelen = strlen(p); + p += nva[n].namelen + 1; + nva[n].value = reinterpret_cast(p); + nva[n].valuelen = strlen(p); + p += nva[n].valuelen + 1; + } + + CHECK_EQ(p, header_contents + header_string_len); + CHECK_EQ(n, count_); +} + + void Initialize(Local target, Local unused, Local context, diff --git a/src/node_http2.h b/src/node_http2.h index bf7134b2358435..0290ac43fa1be8 100755 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -515,54 +515,20 @@ class ExternalHeader : class Headers { public: - Headers(Isolate* isolate, Local context, Local headers) { - headers_.AllocateSufficientStorage(headers->Length()); - Local item; - Local header; - - for (size_t n = 0; n < headers->Length(); n++) { - item = headers->Get(context, n).ToLocalChecked(); - CHECK(item->IsArray()); - header = item.As(); - Local key = header->Get(context, 0).ToLocalChecked(); - Local value = header->Get(context, 1).ToLocalChecked(); - CHECK(key->IsString()); - CHECK(value->IsString()); - size_t keylen = StringBytes::StorageSize(isolate, key, ASCII); - size_t valuelen = StringBytes::StorageSize(isolate, value, ASCII); - headers_[n].flags = NGHTTP2_NV_FLAG_NONE; - Local flag = header->Get(context, 2).ToLocalChecked(); - if (flag->BooleanValue(context).ToChecked()) - headers_[n].flags |= NGHTTP2_NV_FLAG_NO_INDEX; - uint8_t* buf = Malloc(keylen + valuelen); - headers_[n].name = buf; - headers_[n].value = buf + keylen; - headers_[n].namelen = - StringBytes::Write(isolate, - reinterpret_cast(headers_[n].name), - keylen, key, ASCII); - headers_[n].valuelen = - StringBytes::Write(isolate, - reinterpret_cast(headers_[n].value), - valuelen, value, ASCII); - } - } - - ~Headers() { - for (size_t n = 0; n < headers_.length(); n++) - free(headers_[n].name); - } + Headers(Isolate* isolate, Local context, Local headers); + ~Headers() {} nghttp2_nv* operator*() { - return *headers_; + return reinterpret_cast(*buf_); } size_t length() const { - return headers_.length(); + return count_; } private: - MaybeStackBuffer headers_; + size_t count_; + MaybeStackBuffer buf_; }; } // namespace http2 diff --git a/test/parallel/test-http2-util-headers-list.js b/test/parallel/test-http2-util-headers-list.js index c895a4f7a027f3..ac86e3473ea347 100644 --- a/test/parallel/test-http2-util-headers-list.js +++ b/test/parallel/test-http2-util-headers-list.js @@ -82,16 +82,11 @@ const { 'BAR': [1] }; - assert.deepStrictEqual(mapToHeaders(headers), [ - [ ':path', 'abc' ], - [ ':status', '200' ], - [ 'abc', '1' ], - [ 'xyz', '1' ], - [ 'xyz', '2' ], - [ 'xyz', '3' ], - [ 'xyz', '4' ], - [ 'bar', '1' ] - ]); + assert.deepStrictEqual( + mapToHeaders(headers), + [ [ ':path', 'abc', ':status', '200', 'abc', '1', 'xyz', '1', 'xyz', '2', + 'xyz', '3', 'xyz', '4', 'bar', '1', '' ].join('\0'), 8 ] + ); } { @@ -103,15 +98,11 @@ const { 'xyz': [1, 2, 3, 4] }; - assert.deepStrictEqual(mapToHeaders(headers), [ - [ ':status', '200' ], - [ ':path', 'abc' ], - [ 'abc', '1' ], - [ 'xyz', '1' ], - [ 'xyz', '2' ], - [ 'xyz', '3' ], - [ 'xyz', '4' ] - ]); + assert.deepStrictEqual( + mapToHeaders(headers), + [ [ ':status', '200', ':path', 'abc', 'abc', '1', 'xyz', '1', 'xyz', '2', + 'xyz', '3', 'xyz', '4', '' ].join('\0'), 7 ] + ); } { @@ -124,15 +115,11 @@ const { [Symbol('test')]: 1 // Symbol keys are ignored }; - assert.deepStrictEqual(mapToHeaders(headers), [ - [ ':status', '200' ], - [ ':path', 'abc' ], - [ 'abc', '1' ], - [ 'xyz', '1' ], - [ 'xyz', '2' ], - [ 'xyz', '3' ], - [ 'xyz', '4' ] - ]); + assert.deepStrictEqual( + mapToHeaders(headers), + [ [ ':status', '200', ':path', 'abc', 'abc', '1', 'xyz', '1', 'xyz', '2', + 'xyz', '3', 'xyz', '4', '' ].join('\0'), 7 ] + ); } { @@ -144,14 +131,11 @@ const { headers.foo = []; headers[':status'] = 200; - assert.deepStrictEqual(mapToHeaders(headers), [ - [ ':status', '200' ], - [ ':path', 'abc' ], - [ 'xyz', '1' ], - [ 'xyz', '2' ], - [ 'xyz', '3' ], - [ 'xyz', '4' ] - ]); + assert.deepStrictEqual( + mapToHeaders(headers), + [ [ ':status', '200', ':path', 'abc', 'xyz', '1', 'xyz', '2', 'xyz', '3', + 'xyz', '4', '' ].join('\0'), 6 ] + ); } // The following are not allowed to have multiple values