From f6d3a00a3cccd1ec9df06a16f74dc498866dd45f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 10 Aug 2017 00:52:35 +0200 Subject: [PATCH 1/7] 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. --- benchmark/http2/headers.js | 47 +++++++++++++++ lib/internal/http2/util.js | 14 +++-- src/node_http2.cc | 57 ++++++++++++++++++ src/node_http2.h | 46 ++------------- test/parallel/test-http2-util-headers-list.js | 58 +++++++------------ 5 files changed, 141 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..64e8fb573bc41f 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..f244e23a402ad1 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,61 @@ 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(); + size_t 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. + buf_.AllocateSufficientStorage(count_ * sizeof(nghttp2_nv) + + header_string_len); + char* header_contents = *buf_ + (count_ * sizeof(nghttp2_nv)); + nghttp2_nv* const nva = reinterpret_cast(*buf_); + + CHECK_EQ(StringBytes::Write(isolate, + header_contents, header_string_len, + header_string, ASCII), 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..9969a4552432b5 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..9aba909bea5fd3 100644 --- a/test/parallel/test-http2-util-headers-list.js +++ b/test/parallel/test-http2-util-headers-list.js @@ -82,16 +82,12 @@ 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_' + .replace(/_/g, '\0'), + 8 ] + ); } { @@ -103,15 +99,12 @@ 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_' + .replace(/_/g, '\0'), + 7 ] + ); } { @@ -124,15 +117,12 @@ 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_' + .replace(/_/g, '\0'), + 7 ] + ); } { @@ -144,14 +134,10 @@ 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_'.replace(/_/g, '\0'), 6 ] + ); } // The following are not allowed to have multiple values From 327da3894ea61caf8cd48be20b093b4bfe43c636 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 10 Aug 2017 02:32:53 +0200 Subject: [PATCH 2/7] [squash] use WriteOneByte --- src/node_http2.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index f244e23a402ad1..d930ac89e3a4f6 100755 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1086,7 +1086,7 @@ Headers::Headers(Isolate* isolate, CHECK(header_string->IsString()); CHECK(header_count->IsUint32()); count_ = header_count.As()->Value(); - size_t header_string_len = header_string.As()->Length(); + int header_string_len = header_string.As()->Length(); if (count_ == 0) { CHECK_EQ(header_string_len, 0); @@ -1100,9 +1100,11 @@ Headers::Headers(Isolate* isolate, char* header_contents = *buf_ + (count_ * sizeof(nghttp2_nv)); nghttp2_nv* const nva = reinterpret_cast(*buf_); - CHECK_EQ(StringBytes::Write(isolate, - header_contents, header_string_len, - header_string, ASCII), header_string_len); + 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; From d02a8a74469a7e24f2cd64b4c291c9f70a1bcc23 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 10 Aug 2017 13:25:01 +0200 Subject: [PATCH 3/7] [squash] bump buffer size to 3000 bytes --- src/node_http2.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_http2.h b/src/node_http2.h index 9969a4552432b5..0290ac43fa1be8 100755 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -528,7 +528,7 @@ class Headers { private: size_t count_; - MaybeStackBuffer buf_; + MaybeStackBuffer buf_; }; } // namespace http2 From a9d9f6d9a1966e6af258e2e39b073a884303e21e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 10 Aug 2017 13:38:51 +0200 Subject: [PATCH 4/7] [squash] address alignment concern --- src/node_http2.cc | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index d930ac89e3a4f6..28661f9a668ba9 100755 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1094,12 +1094,18 @@ Headers::Headers(Isolate* isolate, } // Allocate a single buffer with count_ nghttp2_nv structs, followed - // by the raw header data as passed from JS. - buf_.AllocateSufficientStorage(count_ * sizeof(nghttp2_nv) + + // 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); - char* header_contents = *buf_ + (count_ * sizeof(nghttp2_nv)); - nghttp2_nv* const nva = reinterpret_cast(*buf_); + // 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, From eea382565e186fd64ddd6a1301537c98b5e94e17 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 10 Aug 2017 14:03:17 +0200 Subject: [PATCH 5/7] [squash] use arrays in test again --- test/parallel/test-http2-util-headers-list.js | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/test/parallel/test-http2-util-headers-list.js b/test/parallel/test-http2-util-headers-list.js index 9aba909bea5fd3..6531726c43783f 100644 --- a/test/parallel/test-http2-util-headers-list.js +++ b/test/parallel/test-http2-util-headers-list.js @@ -84,9 +84,8 @@ const { assert.deepStrictEqual( mapToHeaders(headers), - [ ':path_abc_:status_200_abc_1_xyz_1_xyz_2_xyz_3_xyz_4_bar_1_' - .replace(/_/g, '\0'), - 8 ] + [ [ ':path', 'abc', ':status', '200', 'abc', '1', 'xyz', '1', 'xyz', '2', + 'xyz', '3', 'xyz', '4', 'bar', '1', '' ].join('\0'), 8 ] ); } @@ -101,9 +100,8 @@ const { assert.deepStrictEqual( mapToHeaders(headers), - [ ':status_200_:path_abc_abc_1_xyz_1_xyz_2_xyz_3_xyz_4_' - .replace(/_/g, '\0'), - 7 ] + [ [ ':status', '200', ':path', 'abc', 'abc', '1', 'xyz', '1', 'xyz', '2', + 'xyz', '3', 'xyz', '4', '' ].join('\0'), 7 ] ); } @@ -119,9 +117,8 @@ const { assert.deepStrictEqual( mapToHeaders(headers), - [ ':status_200_:path_abc_abc_1_xyz_1_xyz_2_xyz_3_xyz_4_' - .replace(/_/g, '\0'), - 7 ] + [ [ ':status', '200', ':path', 'abc', 'abc', '1', 'xyz', '1', 'xyz', '2', + 'xyz', '3', 'xyz', '4', '' ].join('\0'), 7 ] ); } @@ -135,8 +132,9 @@ const { headers[':status'] = 200; assert.deepStrictEqual( - mapToHeaders(headers), - [ ':status_200_:path_abc_xyz_1_xyz_2_xyz_3_xyz_4_'.replace(/_/g, '\0'), 6 ] + mapToHeaders(headers) + [ [ ':status', '200', ':path', 'abc', 'xyz', '1', 'xyz', '2', 'xyz', '3', + 'xyz', '4', '' ].join('\0'), 6 ] ); } From b7080cf05c1252ebd68ba9ae9700dc271df44974 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 10 Aug 2017 14:06:54 +0200 Subject: [PATCH 6/7] [squash] whitespace nit --- lib/internal/http2/util.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js index 64e8fb573bc41f..5054d26e593aa3 100644 --- a/lib/internal/http2/util.js +++ b/lib/internal/http2/util.js @@ -428,7 +428,7 @@ function mapToHeaders(map, } } - return [ ret, count ]; + return [ret, count]; } class NghttpError extends Error { From 95964a1f19e511e2c9d849680bde758e2b0a7f82 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 10 Aug 2017 19:14:40 +0200 Subject: [PATCH 7/7] [squash] fix linting (oops) --- test/parallel/test-http2-util-headers-list.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-http2-util-headers-list.js b/test/parallel/test-http2-util-headers-list.js index 6531726c43783f..ac86e3473ea347 100644 --- a/test/parallel/test-http2-util-headers-list.js +++ b/test/parallel/test-http2-util-headers-list.js @@ -132,7 +132,7 @@ const { headers[':status'] = 200; assert.deepStrictEqual( - mapToHeaders(headers) + mapToHeaders(headers), [ [ ':status', '200', ':path', 'abc', 'xyz', '1', 'xyz', '2', 'xyz', '3', 'xyz', '4', '' ].join('\0'), 6 ] );