Merge pull request #13045 from yang-g/hpack_parse

Only allocate cached_timeout when md is interned

3 files changed, 101 insertions, 13 deletions

diff --git a/src/core/ext/transport/chttp2/transport/hpack_parser.cc b/src/core/ext/transport/chttp2/transport/hpack_parser.cc
index 3d1df19bc3..7c17229122 100644
--- a/src/core/ext/transport/chttp2/transport/hpack_parser.cc
+++ b/src/core/ext/transport/chttp2/transport/hpack_parser.cc
@@ -33,6 +33,7 @@
 #include "src/core/lib/debug/stats.h"
 #include "src/core/lib/profiling/timers.h"
 #include "src/core/lib/slice/slice_internal.h"
+#include "src/core/lib/slice/slice_string_helpers.h"
 #include "src/core/lib/support/string.h"
 #include "src/core/lib/transport/http2_errors.h"
 
@@ -650,9 +651,14 @@ static const uint8_t inverse_base64[256] = {
 /* emission helpers */
 static grpc_error *on_hdr(grpc_exec_ctx *exec_ctx, grpc_chttp2_hpack_parser *p,
                           grpc_mdelem md, int add_to_table) {
-  if (GRPC_TRACER_ON(grpc_http_trace) && !GRPC_MDELEM_IS_INTERNED(md)) {
+  if (GRPC_TRACER_ON(grpc_http_trace)) {
     char *k = grpc_slice_to_c_string(GRPC_MDKEY(md));
-    char *v = grpc_slice_to_c_string(GRPC_MDVALUE(md));
+    char *v = NULL;
+    if (grpc_is_binary_header(GRPC_MDKEY(md))) {
+      v = grpc_dump_slice(GRPC_MDVALUE(md), GPR_DUMP_HEX);
+    } else {
+      v = grpc_slice_to_c_string(GRPC_MDVALUE(md));
+    }
     gpr_log(
         GPR_DEBUG,
         "Decode: '%s: %s', elem_interned=%d [%d], k_interned=%d, v_interned=%d",
diff --git a/src/core/ext/transport/chttp2/transport/parsing.cc b/src/core/ext/transport/chttp2/transport/parsing.cc
index ace71f8b4d..efa5791d3f 100644
--- a/src/core/ext/transport/chttp2/transport/parsing.cc
+++ b/src/core/ext/transport/chttp2/transport/parsing.cc
@@ -435,19 +435,21 @@ static void on_initial_header(grpc_exec_ctx *exec_ctx, void *tp,
     grpc_millis *cached_timeout =
         static_cast<grpc_millis *>(grpc_mdelem_get_user_data(md, free_timeout));
     grpc_millis timeout;
-    if (cached_timeout == NULL) {
-      /* not already parsed: parse it now, and store the result away */
-      cached_timeout = (grpc_millis *)gpr_malloc(sizeof(grpc_millis));
-      if (!grpc_http2_decode_timeout(GRPC_MDVALUE(md), cached_timeout)) {
+    if (cached_timeout != NULL) {
+      timeout = *cached_timeout;
+    } else {
+      if (!grpc_http2_decode_timeout(GRPC_MDVALUE(md), &timeout)) {
         char *val = grpc_slice_to_c_string(GRPC_MDVALUE(md));
         gpr_log(GPR_ERROR, "Ignoring bad timeout value '%s'", val);
         gpr_free(val);
-        *cached_timeout = GRPC_MILLIS_INF_FUTURE;
+        timeout = GRPC_MILLIS_INF_FUTURE;
+      }
+      if (GRPC_MDELEM_IS_INTERNED(md)) {
+        /* store the result */
+        cached_timeout = (grpc_millis *)gpr_malloc(sizeof(grpc_millis));
+        *cached_timeout = timeout;
+        grpc_mdelem_set_user_data(md, free_timeout, cached_timeout);
       }
-      timeout = *cached_timeout;
-      grpc_mdelem_set_user_data(md, free_timeout, cached_timeout);
-    } else {
-      timeout = *cached_timeout;
     }
     if (timeout != GRPC_MILLIS_INF_FUTURE) {
       grpc_chttp2_incoming_metadata_buffer_set_deadline(
diff --git a/test/cpp/microbenchmarks/bm_chttp2_hpack.cc b/test/cpp/microbenchmarks/bm_chttp2_hpack.cc
index 5428cc47e7..f813bb7b64 100644
--- a/test/cpp/microbenchmarks/bm_chttp2_hpack.cc
+++ b/test/cpp/microbenchmarks/bm_chttp2_hpack.cc
@@ -28,6 +28,7 @@ extern "C" {
 #include "src/core/lib/slice/slice_internal.h"
 #include "src/core/lib/slice/slice_string_helpers.h"
 #include "src/core/lib/transport/static_metadata.h"
+#include "src/core/lib/transport/timeout_encoding.h"
 }
 #include "test/cpp/microbenchmarks/helpers.h"
 #include "third_party/benchmark/include/benchmark/benchmark.h"
@@ -441,7 +442,8 @@ static void UnrefHeader(grpc_exec_ctx *exec_ctx, void *user_data,
   GRPC_MDELEM_UNREF(exec_ctx, md);
 }
 
-template <class Fixture>
+template <class Fixture,
+          void (*OnHeader)(grpc_exec_ctx *, void *, grpc_mdelem) = UnrefHeader>
 static void BM_HpackParserParseHeader(benchmark::State &state) {
   TrackCounters track_counters;
   grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT;
@@ -449,7 +451,7 @@ static void BM_HpackParserParseHeader(benchmark::State &state) {
   std::vector<grpc_slice> benchmark_slices = Fixture::GetBenchmarkSlices();
   grpc_chttp2_hpack_parser p;
   grpc_chttp2_hpack_parser_init(&exec_ctx, &p);
-  p.on_header = UnrefHeader;
+  p.on_header = OnHeader;
   p.on_header_user_data = nullptr;
   for (auto slice : init_slices) {
     GPR_ASSERT(GRPC_ERROR_NONE ==
@@ -759,6 +761,81 @@ class RepresentativeServerTrailingMetadata {
   }
 };
 
+static void free_timeout(void *p) { gpr_free(p); }
+
+// New implementation.
+static void OnHeaderNew(grpc_exec_ctx *exec_ctx, void *user_data,
+                        grpc_mdelem md) {
+  if (grpc_slice_eq(GRPC_MDKEY(md), GRPC_MDSTR_GRPC_TIMEOUT)) {
+    grpc_millis *cached_timeout =
+        static_cast<grpc_millis *>(grpc_mdelem_get_user_data(md, free_timeout));
+    grpc_millis timeout;
+    if (cached_timeout != NULL) {
+      timeout = *cached_timeout;
+    } else {
+      if (!grpc_http2_decode_timeout(GRPC_MDVALUE(md), &timeout)) {
+        char *val = grpc_slice_to_c_string(GRPC_MDVALUE(md));
+        gpr_log(GPR_ERROR, "Ignoring bad timeout value '%s'", val);
+        gpr_free(val);
+        timeout = GRPC_MILLIS_INF_FUTURE;
+      }
+      if (GRPC_MDELEM_IS_INTERNED(md)) {
+        /* not already parsed: parse it now, and store the
+         * result away */
+        cached_timeout = (grpc_millis *)gpr_malloc(sizeof(grpc_millis));
+        *cached_timeout = timeout;
+        grpc_mdelem_set_user_data(md, free_timeout, cached_timeout);
+      }
+    }
+    benchmark::DoNotOptimize(timeout);
+    GRPC_MDELEM_UNREF(exec_ctx, md);
+  } else {
+    GPR_ASSERT(0);
+  }
+}
+
+// Current implementation.
+static void OnHeaderOld(grpc_exec_ctx *exec_ctx, void *user_data,
+                        grpc_mdelem md) {
+  if (grpc_slice_eq(GRPC_MDKEY(md), GRPC_MDSTR_GRPC_TIMEOUT)) {
+    grpc_millis *cached_timeout =
+        static_cast<grpc_millis *>(grpc_mdelem_get_user_data(md, free_timeout));
+    grpc_millis timeout;
+    if (cached_timeout == NULL) {
+      /* not already parsed: parse it now, and store the result away */
+      cached_timeout = (grpc_millis *)gpr_malloc(sizeof(grpc_millis));
+      if (!grpc_http2_decode_timeout(GRPC_MDVALUE(md), cached_timeout)) {
+        char *val = grpc_slice_to_c_string(GRPC_MDVALUE(md));
+        gpr_log(GPR_ERROR, "Ignoring bad timeout value '%s'", val);
+        gpr_free(val);
+        *cached_timeout = GRPC_MILLIS_INF_FUTURE;
+      }
+      timeout = *cached_timeout;
+      grpc_mdelem_set_user_data(md, free_timeout, cached_timeout);
+    } else {
+      timeout = *cached_timeout;
+    }
+    benchmark::DoNotOptimize(timeout);
+    GRPC_MDELEM_UNREF(exec_ctx, md);
+  } else {
+    GPR_ASSERT(0);
+  }
+}
+
+// Send the same deadline repeatedly
+class SameDeadline {
+ public:
+  static std::vector<grpc_slice> GetInitSlices() {
+    return {
+        grpc_slice_from_static_string("@\x0cgrpc-timeout\x03"
+                                      "30S")};
+  }
+  static std::vector<grpc_slice> GetBenchmarkSlices() {
+    // Use saved key and literal value.
+    return {MakeSlice({0x0f, 0x2f, 0x03, '3', '0', 'S'})};
+  }
+};
+
 BENCHMARK_TEMPLATE(BM_HpackParserParseHeader, EmptyBatch);
 BENCHMARK_TEMPLATE(BM_HpackParserParseHeader, IndexedSingleStaticElem);
 BENCHMARK_TEMPLATE(BM_HpackParserParseHeader, AddIndexedSingleStaticElem);
@@ -786,6 +863,9 @@ BENCHMARK_TEMPLATE(BM_HpackParserParseHeader,
 BENCHMARK_TEMPLATE(BM_HpackParserParseHeader,
                    RepresentativeServerTrailingMetadata);
 
+BENCHMARK_TEMPLATE(BM_HpackParserParseHeader, SameDeadline, OnHeaderOld);
+BENCHMARK_TEMPLATE(BM_HpackParserParseHeader, SameDeadline, OnHeaderNew);
+
 }  // namespace hpack_parser_fixtures
 
 BENCHMARK_MAIN();