From 4766fcb96d9459b7db8fb201d6ac44d6957b4df0 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Sat, 12 Aug 2017 16:49:31 +0200
Subject: [PATCH] http2: minor refactor of passing headers to JS

- Make `ExternalString::New` return a `MaybeLocal`. Failing is
  left up to the caller.
- Allow creating internalized strings for short header names
  to reduce memory consumption and increase performance.
- Use persistent storage for statically allocated header names.

PR-URL: https://github.com/nodejs/node/pull/14808
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
---
 benchmark/http2/headers.js | 13 +++++++++--
 src/env.h                  |  5 +++++
 src/node_http2.cc          |  6 ++++--
 src/node_http2.h           | 44 +++++++++++++++++++++++++++++---------
 4 files changed, 54 insertions(+), 14 deletions(-)

diff --git a/benchmark/http2/headers.js b/benchmark/http2/headers.js
index 09449d1e92..078e7a356a 100644
--- a/benchmark/http2/headers.js
+++ b/benchmark/http2/headers.js
@@ -5,7 +5,7 @@ const PORT = common.PORT;
 
 var bench = common.createBenchmark(main, {
   n: [1e3],
-  nheaders: [100, 1000],
+  nheaders: [0, 10, 100, 1000],
 }, { flags: ['--expose-http2', '--no-warnings'] });
 
 function main(conf) {
@@ -14,7 +14,16 @@ function main(conf) {
   const http2 = require('http2');
   const server = http2.createServer();
 
-  const headersObject = { ':path': '/' };
+  const headersObject = {
+    ':path': '/',
+    ':scheme': 'http',
+    'accept-encoding': 'gzip, deflate',
+    'accept-language': 'en',
+    'content-type': 'text/plain',
+    'referer': 'https://example.org/',
+    'user-agent': 'SuperBenchmarker 3000'
+  };
+
   for (var i = 0; i < nheaders; i++) {
     headersObject[`foo${i}`] = `some header value ${i}`;
   }
diff --git a/src/env.h b/src/env.h
index 7b17b75c49..2b299a6622 100644
--- a/src/env.h
+++ b/src/env.h
@@ -39,6 +39,9 @@
 #include <stdint.h>
 #include <vector>
 #include <stack>
+#include <unordered_map>
+
+struct nghttp2_rcbuf;
 
 namespace node {
 
@@ -341,6 +344,8 @@ class IsolateData {
 #undef VS
 #undef VP
 
+  std::unordered_map<nghttp2_rcbuf*, v8::Eternal<v8::String>> http2_static_strs;
+
  private:
 #define VP(PropertyName, StringValue) V(v8::Private, PropertyName)
 #define VS(PropertyName, StringValue) V(v8::String, PropertyName)
diff --git a/src/node_http2.cc b/src/node_http2.cc
index 16c7270342..9308e9e68e 100755
--- a/src/node_http2.cc
+++ b/src/node_http2.cc
@@ -901,8 +901,10 @@ void Http2Session::OnHeaders(Nghttp2Stream* stream,
     while (headers != nullptr && j < arraysize(argv) / 2) {
       nghttp2_header_list* item = headers;
       // The header name and value are passed as external one-byte strings
-      name_str = ExternalHeader::New(isolate, item->name);
-      value_str = ExternalHeader::New(isolate, item->value);
+      name_str =
+          ExternalHeader::New<true>(env(), item->name).ToLocalChecked();
+      value_str =
+          ExternalHeader::New<false>(env(), item->value).ToLocalChecked();
       argv[j * 2] = name_str;
       argv[j * 2 + 1] = value_str;
       headers = item->next;
diff --git a/src/node_http2.h b/src/node_http2.h
index 3ff2d33a7b..107e87a2dc 100755
--- a/src/node_http2.h
+++ b/src/node_http2.h
@@ -472,24 +472,48 @@ class ExternalHeader :
     return vec_.len;
   }
 
-  static Local<String> New(Isolate* isolate, nghttp2_rcbuf* buf) {
-    EscapableHandleScope scope(isolate);
+  static inline
+  MaybeLocal<String> GetInternalizedString(Environment* env,
+                                           const nghttp2_vec& vec) {
+    return String::NewFromOneByte(env->isolate(),
+                                  vec.base,
+                                  v8::NewStringType::kInternalized,
+                                  vec.len);
+  }
+
+  template<bool may_internalize>
+  static MaybeLocal<String> New(Environment* env, nghttp2_rcbuf* buf) {
+    if (nghttp2_rcbuf_is_static(buf)) {
+      auto& static_str_map = env->isolate_data()->http2_static_strs;
+      v8::Eternal<v8::String>& eternal = static_str_map[buf];
+      if (eternal.IsEmpty()) {
+        Local<String> str =
+            GetInternalizedString(env, nghttp2_rcbuf_get_buf(buf))
+                .ToLocalChecked();
+        eternal.Set(env->isolate(), str);
+        return str;
+      }
+      return eternal.Get(env->isolate());
+    }
+
     nghttp2_vec vec = nghttp2_rcbuf_get_buf(buf);
     if (vec.len == 0) {
       nghttp2_rcbuf_decref(buf);
-      return scope.Escape(String::Empty(isolate));
+      return String::Empty(env->isolate());
     }
 
-    ExternalHeader* h_str = new ExternalHeader(buf);
-    MaybeLocal<String> str = String::NewExternalOneByte(isolate, h_str);
-    isolate->AdjustAmountOfExternalAllocatedMemory(vec.len);
+    if (may_internalize && vec.len < 64) {
+      // This is a short header name, so there is a good chance V8 already has
+      // it internalized.
+      return GetInternalizedString(env, vec);
+    }
 
-    if (str.IsEmpty()) {
+    ExternalHeader* h_str = new ExternalHeader(buf);
+    MaybeLocal<String> str = String::NewExternalOneByte(env->isolate(), h_str);
+    if (str.IsEmpty())
       delete h_str;
-      return scope.Escape(String::Empty(isolate));
-    }
 
-    return scope.Escape(str.ToLocalChecked());
+    return str;
   }
 
  private: