From 6c49b3dad750168a5a5d72f777c29599dd0068cd Mon Sep 17 00:00:00 2001
From: Soheil Hassas Yeganeh <soheil@google.com>
Date: Mon, 5 Nov 2018 12:11:47 -0500
Subject: [PATCH] Eliminate function pointers in hpack_enc().

Using a conditional branch to set a function pointer will prevent the
compiler to inline the function and worse it would jump based on a
register value killing the pipline.

Here is a short example to demonstrate the variants:
https://godbolt.org/z/radfZg

Remove `add_nothing` and add a lambda when necessary to make sure
everything is inlined.
---
 .../chttp2/transport/hpack_encoder.cc         | 53 ++++++++++---------
 1 file changed, 28 insertions(+), 25 deletions(-)

diff --git a/src/core/ext/transport/chttp2/transport/hpack_encoder.cc b/src/core/ext/transport/chttp2/transport/hpack_encoder.cc
index 920d52770fc..dbe9df6ae38 100644
--- a/src/core/ext/transport/chttp2/transport/hpack_encoder.cc
+++ b/src/core/ext/transport/chttp2/transport/hpack_encoder.cc
@@ -212,10 +212,6 @@ static uint32_t prepare_space_for_new_elem(grpc_chttp2_hpack_compressor* c,
   return new_index;
 }
 
-/* dummy function */
-static void add_nothing(grpc_chttp2_hpack_compressor* c, grpc_mdelem elem,
-                        size_t elem_size) {}
-
 // Add a key to the dynamic table. Both key and value will be added to table at
 // the decoder.
 static void add_key_with_index(grpc_chttp2_hpack_compressor* c,
@@ -524,17 +520,22 @@ static void hpack_enc(grpc_chttp2_hpack_compressor* c, grpc_mdelem elem,
   uint32_t indices_key;
 
   /* should this elem be in the table? */
-  size_t decoder_space_usage =
+  const size_t decoder_space_usage =
       grpc_chttp2_get_size_in_hpack_table(elem, st->use_true_binary_metadata);
-  bool should_add_elem = elem_interned &&
-                         decoder_space_usage < MAX_DECODER_SPACE_USAGE &&
-                         c->filter_elems[HASH_FRAGMENT_1(elem_hash)] >=
-                             c->filter_elems_sum / ONE_ON_ADD_PROBABILITY;
-  void (*maybe_add)(grpc_chttp2_hpack_compressor*, grpc_mdelem, size_t) =
-      should_add_elem ? add_elem : add_nothing;
-  void (*emit)(grpc_chttp2_hpack_compressor*, uint32_t, grpc_mdelem,
-               framer_state*) =
-      should_add_elem ? emit_lithdr_incidx : emit_lithdr_noidx;
+  const bool should_add_elem = elem_interned &&
+                               decoder_space_usage < MAX_DECODER_SPACE_USAGE &&
+                               c->filter_elems[HASH_FRAGMENT_1(elem_hash)] >=
+                                   c->filter_elems_sum / ONE_ON_ADD_PROBABILITY;
+
+  auto emit_maybe_add = [&should_add_elem, &elem, &st, &c, &indices_key,
+                         &decoder_space_usage] {
+    if (should_add_elem) {
+      emit_lithdr_incidx(c, dynidx(c, indices_key), elem, st);
+      add_elem(c, elem, decoder_space_usage);
+    } else {
+      emit_lithdr_noidx(c, dynidx(c, indices_key), elem, st);
+    }
+  };
 
   /* no hits for the elem... maybe there's a key? */
   indices_key = c->indices_keys[HASH_FRAGMENT_2(key_hash)];
@@ -542,8 +543,7 @@ static void hpack_enc(grpc_chttp2_hpack_compressor* c, grpc_mdelem elem,
                     GRPC_MDKEY(elem)) &&
       indices_key > c->tail_remote_index) {
     /* HIT: key (first cuckoo hash) */
-    emit(c, dynidx(c, indices_key), elem, st);
-    maybe_add(c, elem, decoder_space_usage);
+    emit_maybe_add();
     return;
   }
 
@@ -552,20 +552,23 @@ static void hpack_enc(grpc_chttp2_hpack_compressor* c, grpc_mdelem elem,
                     GRPC_MDKEY(elem)) &&
       indices_key > c->tail_remote_index) {
     /* HIT: key (first cuckoo hash) */
-    emit(c, dynidx(c, indices_key), elem, st);
-    maybe_add(c, elem, decoder_space_usage);
+    emit_maybe_add();
     return;
   }
 
   /* no elem, key in the table... fall back to literal emission */
-  bool should_add_key =
+  const bool should_add_key =
       !elem_interned && decoder_space_usage < MAX_DECODER_SPACE_USAGE;
-  emit = (should_add_elem || should_add_key) ? emit_lithdr_incidx_v
-                                             : emit_lithdr_noidx_v;
-  maybe_add =
-      should_add_elem ? add_elem : (should_add_key ? add_key : add_nothing);
-  emit(c, 0, elem, st);
-  maybe_add(c, elem, decoder_space_usage);
+  if (should_add_elem || should_add_key) {
+    emit_lithdr_incidx_v(c, 0, elem, st);
+  } else {
+    emit_lithdr_noidx_v(c, 0, elem, st);
+  }
+  if (should_add_elem) {
+    add_elem(c, elem, decoder_space_usage);
+  } else if (should_add_key) {
+    add_key(c, elem, decoder_space_usage);
+  }
 }
 
 #define STRLEN_LIT(x) (sizeof(x) - 1)