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 <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>

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);
+  });
+}

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 {

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> context,
+                 Local<Array> headers) {
+  CHECK_EQ(headers->Length(), 2);
+  Local<Value> header_string = headers->Get(context, 0).ToLocalChecked();
+  Local<Value> header_count = headers->Get(context, 1).ToLocalChecked();
+  CHECK(header_string->IsString());
+  CHECK(header_count->IsUint32());
+  count_ = header_count.As<Uint32>()->Value();
+  int header_string_len = header_string.As<String>()->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<char*>(
+      ROUND_UP(reinterpret_cast<uintptr_t>(*buf_), alignof(nghttp2_nv)));
+  char* header_contents = start + (count_ * sizeof(nghttp2_nv));
+  nghttp2_nv* const nva = reinterpret_cast<nghttp2_nv*>(start);
+
+  CHECK_LE(header_contents + header_string_len, *buf_ + buf_.length());
+  CHECK_EQ(header_string.As<String>()
+              ->WriteOneByte(reinterpret_cast<uint8_t*>(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<uint8_t*>(p);
+    nva[n].namelen = strlen(p);
+    p += nva[n].namelen + 1;
+    nva[n].value = reinterpret_cast<uint8_t*>(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<Object> target,
                 Local<Value> unused,
                 Local<Context> context,

src/node_http2.h

@@ -515,54 +515,20 @@ class ExternalHeader :
 
 class Headers {
  public:
-  Headers(Isolate* isolate, Local<Context> context, Local<Array> headers) {
+  Headers(Isolate* isolate, Local<Context> context, Local<Array> headers);
-    headers_.AllocateSufficientStorage(headers->Length());
+  ~Headers() {}
-    Local<Value> item;
-    Local<Array> header;
-
-    for (size_t n = 0; n < headers->Length(); n++) {
-      item = headers->Get(context, n).ToLocalChecked();
-      CHECK(item->IsArray());
-      header = item.As<Array>();
-      Local<Value> key = header->Get(context, 0).ToLocalChecked();
-      Local<Value> 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<Value> flag = header->Get(context, 2).ToLocalChecked();
-      if (flag->BooleanValue(context).ToChecked())
-        headers_[n].flags |= NGHTTP2_NV_FLAG_NO_INDEX;
-      uint8_t* buf = Malloc<uint8_t>(keylen + valuelen);
-      headers_[n].name = buf;
-      headers_[n].value = buf + keylen;
-      headers_[n].namelen =
-          StringBytes::Write(isolate,
-                            reinterpret_cast<char*>(headers_[n].name),
-                            keylen, key, ASCII);
-      headers_[n].valuelen =
-          StringBytes::Write(isolate,
-                            reinterpret_cast<char*>(headers_[n].value),
-                            valuelen, value, ASCII);
-    }
-  }
-
-  ~Headers() {
-    for (size_t n = 0; n < headers_.length(); n++)
-      free(headers_[n].name);
-  }
 
   nghttp2_nv* operator*() {
-    return *headers_;
+    return reinterpret_cast<nghttp2_nv*>(*buf_);
   }
 
   size_t length() const {
-    return headers_.length();
+    return count_;
   }
 
  private:
-  MaybeStackBuffer<nghttp2_nv> headers_;
+  size_t count_;
+  MaybeStackBuffer<char, 3000> buf_;
 };
 
 }  // namespace http2

test/parallel/test-http2-util-headers-list.js

@@ -82,16 +82,11 @@ const {
     'BAR': [1]
   };
 
-  assert.deepStrictEqual(mapToHeaders(headers), [
+  assert.deepStrictEqual(
-    [ ':path', 'abc' ],
+    mapToHeaders(headers),
-    [ ':status', '200' ],
+    [ [ ':path', 'abc', ':status', '200', 'abc', '1', 'xyz', '1', 'xyz', '2',
-    [ 'abc', '1' ],
+        'xyz', '3', 'xyz', '4', 'bar', '1', '' ].join('\0'), 8 ]
-    [ 'xyz', '1' ],
+  );
-    [ 'xyz', '2' ],
-    [ 'xyz', '3' ],
-    [ 'xyz', '4' ],
-    [ 'bar', '1' ]
-  ]);
 }
 
 {
@@ -103,15 +98,11 @@ const {
     'xyz': [1, 2, 3, 4]
   };
 
-  assert.deepStrictEqual(mapToHeaders(headers), [
+  assert.deepStrictEqual(
-    [ ':status', '200' ],
+    mapToHeaders(headers),
-    [ ':path', 'abc' ],
+    [ [ ':status', '200', ':path', 'abc', 'abc', '1', 'xyz', '1', 'xyz', '2',
-    [ 'abc', '1' ],
+        'xyz', '3', 'xyz', '4', '' ].join('\0'), 7 ]
-    [ 'xyz', '1' ],
+  );
-    [ 'xyz', '2' ],
-    [ 'xyz', '3' ],
-    [ 'xyz', '4' ]
-  ]);
 }
 
 {
@@ -124,15 +115,11 @@ const {
     [Symbol('test')]: 1 // Symbol keys are ignored
   };
 
-  assert.deepStrictEqual(mapToHeaders(headers), [
+  assert.deepStrictEqual(
-    [ ':status', '200' ],
+    mapToHeaders(headers),
-    [ ':path', 'abc' ],
+    [ [ ':status', '200', ':path', 'abc', 'abc', '1', 'xyz', '1', 'xyz', '2',
-    [ 'abc', '1' ],
+        'xyz', '3', 'xyz', '4', '' ].join('\0'), 7 ]
-    [ 'xyz', '1' ],
+  );
-    [ 'xyz', '2' ],
-    [ 'xyz', '3' ],
-    [ 'xyz', '4' ]
-  ]);
 }
 
 {
@@ -144,14 +131,11 @@ const {
   headers.foo = [];
   headers[':status'] = 200;
 
-  assert.deepStrictEqual(mapToHeaders(headers), [
+  assert.deepStrictEqual(
-    [ ':status', '200' ],
+    mapToHeaders(headers),
-    [ ':path', 'abc' ],
+    [ [ ':status', '200', ':path', 'abc', 'xyz', '1', 'xyz', '2', 'xyz', '3',
-    [ 'xyz', '1' ],
+        'xyz', '4', '' ].join('\0'), 6 ]
-    [ 'xyz', '2' ],
+  );
-    [ 'xyz', '3' ],
-    [ 'xyz', '4' ]
-  ]);
 }
 
 // The following are not allowed to have multiple values