Merge pull request #20014 from arjunroy/on_hdr_nobranch

s/branch/tail_call/ for CH2 on_hdr().

src/core/ext/transport/chttp2/transport/hpack_parser.cc

@@ -655,12 +655,7 @@ static grpc_error* on_hdr(grpc_chttp2_hpack_parser* p, grpc_mdelem md) {
     grpc_error* err = grpc_chttp2_hptbl_add(&p->table, md);
     if (GPR_UNLIKELY(err != GRPC_ERROR_NONE)) return err;
   }
-  if (GPR_UNLIKELY(p->on_header == nullptr)) {
-    GRPC_MDELEM_UNREF(md);
-    return GRPC_ERROR_CREATE_FROM_STATIC_STRING("on_header callback not set");
-  }
-  p->on_header(p->on_header_user_data, md);
-  return GRPC_ERROR_NONE;
+  return p->on_header(p->on_header_user_data, md);
 }
 
 static grpc_core::UnmanagedMemorySlice take_string_extern(
@@ -765,23 +760,26 @@ static grpc_error* parse_stream_dep0(grpc_chttp2_hpack_parser* p,
   return parse_stream_dep1(p, cur + 1, end);
 }
 
+static grpc_error* GPR_ATTRIBUTE_NOINLINE
+on_invalid_hpack_idx(grpc_chttp2_hpack_parser* p) {
+  return grpc_error_set_int(
+      grpc_error_set_int(
+          GRPC_ERROR_CREATE_FROM_STATIC_STRING("Invalid HPACK index received"),
+          GRPC_ERROR_INT_INDEX, static_cast<intptr_t>(p->index)),
+      GRPC_ERROR_INT_SIZE, static_cast<intptr_t>(p->table.num_ents));
+}
+
 /* emit an indexed field; jumps to begin the next field on completion */
 static grpc_error* finish_indexed_field(grpc_chttp2_hpack_parser* p,
                                         const uint8_t* cur,
                                         const uint8_t* end) {
-  grpc_mdelem md = grpc_chttp2_hptbl_lookup(&p->table, p->index);
-  if (GRPC_MDISNULL(md)) {
-    return grpc_error_set_int(
-        grpc_error_set_int(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-                               "Invalid HPACK index received"),
-                           GRPC_ERROR_INT_INDEX,
-                           static_cast<intptr_t>(p->index)),
-        GRPC_ERROR_INT_SIZE, static_cast<intptr_t>(p->table.num_ents));
+  grpc_mdelem md = grpc_chttp2_hptbl_lookup<true>(&p->table, p->index);
+  if (GPR_UNLIKELY(GRPC_MDISNULL(md))) {
+    return on_invalid_hpack_idx(p);
   }
-  GRPC_MDELEM_REF(md);
   GRPC_STATS_INC_HPACK_RECV_INDEXED();
   grpc_error* err = on_hdr<false>(p, md);
-  if (err != GRPC_ERROR_NONE) return err;
+  if (GPR_UNLIKELY(err != GRPC_ERROR_NONE)) return err;
   return parse_begin(p, cur, end);
 }
 
@@ -1557,13 +1555,8 @@ static void set_precomputed_md_idx(grpc_chttp2_hpack_parser* p,
 static grpc_error* is_binary_indexed_header(grpc_chttp2_hpack_parser* p,
                                             bool* is) {
   grpc_mdelem elem = grpc_chttp2_hptbl_lookup(&p->table, p->index);
-  if (GRPC_MDISNULL(elem)) {
-    return grpc_error_set_int(
-        grpc_error_set_int(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
-                               "Invalid HPACK index received"),
-                           GRPC_ERROR_INT_INDEX,
-                           static_cast<intptr_t>(p->index)),
-        GRPC_ERROR_INT_SIZE, static_cast<intptr_t>(p->table.num_ents));
+  if (GPR_UNLIKELY(GRPC_MDISNULL(elem))) {
+    return on_invalid_hpack_idx(p);
   }
   /* We know that GRPC_MDKEY(elem) points to a reference counted slice since:
    * 1. elem was a result of grpc_chttp2_hptbl_lookup
@@ -1599,10 +1592,16 @@ static grpc_error* parse_value_string_with_literal_key(
   return parse_value_string(p, cur, end, is_binary_literal_header(p));
 }
 
+/* "Uninitialized" header parser to save us a branch in on_hdr().  */
+static grpc_error* on_header_uninitialized(void* user_data, grpc_mdelem md) {
+  GRPC_MDELEM_UNREF(md);
+  return GRPC_ERROR_CREATE_FROM_STATIC_STRING("on_header callback not set");
+}
+
 /* PUBLIC INTERFACE */
 
 void grpc_chttp2_hpack_parser_init(grpc_chttp2_hpack_parser* p) {
-  p->on_header = nullptr;
+  p->on_header = on_header_uninitialized;
   p->on_header_user_data = nullptr;
   p->state = parse_begin;
   p->key.data.referenced = grpc_empty_slice();
@@ -1750,7 +1749,7 @@ grpc_error* grpc_chttp2_header_parser_parse(void* hpack_parser,
         grpc_chttp2_mark_stream_closed(t, s, true, false, GRPC_ERROR_NONE);
       }
     }
-    parser->on_header = nullptr;
+    parser->on_header = on_header_uninitialized;
     parser->on_header_user_data = nullptr;
     parser->is_boundary = 0xde;
     parser->is_eof = 0xde;

src/core/ext/transport/chttp2/transport/hpack_parser.h

@@ -46,7 +46,7 @@ typedef struct {
 
 struct grpc_chttp2_hpack_parser {
   /* user specified callback for each header output */
-  void (*on_header)(void* user_data, grpc_mdelem md);
+  grpc_error* (*on_header)(void* user_data, grpc_mdelem md);
   void* on_header_user_data;
 
   grpc_error* last_error;

src/core/ext/transport/chttp2/transport/hpack_table.cc

@@ -44,19 +44,34 @@ void grpc_chttp2_hptbl_destroy(grpc_chttp2_hptbl* tbl) {
   tbl->ents = nullptr;
 }
 
-grpc_mdelem grpc_chttp2_hptbl_lookup_dynamic_index(const grpc_chttp2_hptbl* tbl,
-                                                   uint32_t tbl_index) {
+template <bool take_ref>
+static grpc_mdelem lookup_dynamic_index(const grpc_chttp2_hptbl* tbl,
+                                        uint32_t tbl_index) {
   /* Not static - find the value in the list of valid entries */
   tbl_index -= (GRPC_CHTTP2_LAST_STATIC_ENTRY + 1);
   if (tbl_index < tbl->num_ents) {
     uint32_t offset =
         (tbl->num_ents - 1u - tbl_index + tbl->first_ent) % tbl->cap_entries;
-    return tbl->ents[offset];
+    grpc_mdelem md = tbl->ents[offset];
+    if (take_ref) {
+      GRPC_MDELEM_REF(md);
+    }
+    return md;
   }
   /* Invalid entry: return error */
   return GRPC_MDNULL;
 }
 
+grpc_mdelem grpc_chttp2_hptbl_lookup_dynamic_index(const grpc_chttp2_hptbl* tbl,
+                                                   uint32_t tbl_index) {
+  return lookup_dynamic_index<false>(tbl, tbl_index);
+}
+
+grpc_mdelem grpc_chttp2_hptbl_lookup_ref_dynamic_index(
+    const grpc_chttp2_hptbl* tbl, uint32_t tbl_index) {
+  return lookup_dynamic_index<true>(tbl, tbl_index);
+}
+
 /* Evict one element from the table */
 static void evict1(grpc_chttp2_hptbl* tbl) {
   grpc_mdelem first_ent = tbl->ents[tbl->first_ent];

src/core/ext/transport/chttp2/transport/hpack_table.h

@@ -95,6 +95,9 @@ grpc_error* grpc_chttp2_hptbl_set_current_table_size(grpc_chttp2_hptbl* tbl,
 /* lookup a table entry based on its hpack index */
 grpc_mdelem grpc_chttp2_hptbl_lookup_dynamic_index(const grpc_chttp2_hptbl* tbl,
                                                    uint32_t tbl_index);
+grpc_mdelem grpc_chttp2_hptbl_lookup_ref_dynamic_index(
+    const grpc_chttp2_hptbl* tbl, uint32_t tbl_index);
+template <bool take_ref = false>
 inline grpc_mdelem grpc_chttp2_hptbl_lookup(const grpc_chttp2_hptbl* tbl,
                                             uint32_t index) {
   /* Static table comes first, just return an entry from it.
@@ -103,9 +106,15 @@ inline grpc_mdelem grpc_chttp2_hptbl_lookup(const grpc_chttp2_hptbl* tbl,
      must follow the hpack standard. If that changes, we *must* not rely on
      reading the core static metadata table here; at that point we'd need our
      own singleton static metadata in the correct order. */
-  return index <= GRPC_CHTTP2_LAST_STATIC_ENTRY
-             ? grpc_static_mdelem_manifested()[index - 1]
-             : grpc_chttp2_hptbl_lookup_dynamic_index(tbl, index);
+  if (index <= GRPC_CHTTP2_LAST_STATIC_ENTRY) {
+    return grpc_static_mdelem_manifested()[index - 1];
+  } else {
+    if (take_ref) {
+      return grpc_chttp2_hptbl_lookup_ref_dynamic_index(tbl, index);
+    } else {
+      return grpc_chttp2_hptbl_lookup_dynamic_index(tbl, index);
+    }
+  }
 }
 /* add a table entry to the index */
 grpc_error* grpc_chttp2_hptbl_add(grpc_chttp2_hptbl* tbl,

src/core/ext/transport/chttp2/transport/parsing.cc

@@ -318,7 +318,10 @@ static grpc_error* skip_parser(void* parser, grpc_chttp2_transport* t,
   return GRPC_ERROR_NONE;
 }
 
-static void skip_header(void* tp, grpc_mdelem md) { GRPC_MDELEM_UNREF(md); }
+static grpc_error* skip_header(void* tp, grpc_mdelem md) {
+  GRPC_MDELEM_UNREF(md);
+  return GRPC_ERROR_NONE;
+}
 
 static grpc_error* init_skip_frame_parser(grpc_chttp2_transport* t,
                                           int is_header) {
@@ -419,7 +422,7 @@ static bool is_nonzero_status(grpc_mdelem md) {
          !md_cmp(md, GRPC_MDELEM_GRPC_STATUS_0, GRPC_MDSTR_GRPC_STATUS);
 }
 
-static void on_initial_header(void* tp, grpc_mdelem md) {
+static grpc_error* on_initial_header(void* tp, grpc_mdelem md) {
   GPR_TIMER_SCOPE("on_initial_header", 0);
 
   grpc_chttp2_transport* t = static_cast<grpc_chttp2_transport*>(tp);
@@ -465,7 +468,7 @@ static void on_initial_header(void* tp, grpc_mdelem md) {
           &s->metadata_buffer[0], grpc_core::ExecCtx::Get()->Now() + timeout);
     }
     GRPC_MDELEM_UNREF(md);
-    return;
+    return GRPC_ERROR_NONE;
   }
 
   const size_t new_size = s->metadata_buffer[0].size + GRPC_MDELEM_LENGTH(md);
@@ -496,9 +499,10 @@ static void on_initial_header(void* tp, grpc_mdelem md) {
       GRPC_MDELEM_UNREF(md);
     }
   }
+  return GRPC_ERROR_NONE;
 }
 
-static void on_trailing_header(void* tp, grpc_mdelem md) {
+static grpc_error* on_trailing_header(void* tp, grpc_mdelem md) {
   GPR_TIMER_SCOPE("on_trailing_header", 0);
 
   grpc_chttp2_transport* t = static_cast<grpc_chttp2_transport*>(tp);
@@ -547,6 +551,7 @@ static void on_trailing_header(void* tp, grpc_mdelem md) {
       GRPC_MDELEM_UNREF(md);
     }
   }
+  return GRPC_ERROR_NONE;
 }
 
 static grpc_error* init_header_frame_parser(grpc_chttp2_transport* t,

test/core/transport/chttp2/hpack_parser_fuzzer_test.cc

@@ -30,7 +30,10 @@
 bool squelch = true;
 bool leak_check = true;
 
-static void onhdr(void* ud, grpc_mdelem md) { GRPC_MDELEM_UNREF(md); }
+static grpc_error* onhdr(void* ud, grpc_mdelem md) {
+  GRPC_MDELEM_UNREF(md);
+  return GRPC_ERROR_NONE;
+}
 static void dont_log(gpr_log_func_args* args) {}
 
 extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {

test/core/transport/chttp2/hpack_parser_test.cc

@@ -34,7 +34,7 @@ typedef struct {
   va_list args;
 } test_checker;
 
-static void onhdr(void* ud, grpc_mdelem md) {
+static grpc_error* onhdr(void* ud, grpc_mdelem md) {
   const char *ekey, *evalue;
   test_checker* chk = static_cast<test_checker*>(ud);
   ekey = va_arg(chk->args, char*);
@@ -44,6 +44,7 @@ static void onhdr(void* ud, grpc_mdelem md) {
   GPR_ASSERT(grpc_slice_str_cmp(GRPC_MDKEY(md), ekey) == 0);
   GPR_ASSERT(grpc_slice_str_cmp(GRPC_MDVALUE(md), evalue) == 0);
   GRPC_MDELEM_UNREF(md);
+  return GRPC_ERROR_NONE;
 }
 
 static void test_vector(grpc_chttp2_hpack_parser* parser,

test/cpp/microbenchmarks/bm_chttp2_hpack.cc

@@ -450,11 +450,12 @@ static void BM_HpackParserInitDestroy(benchmark::State& state) {
 }
 BENCHMARK(BM_HpackParserInitDestroy);
 
-static void UnrefHeader(void* user_data, grpc_mdelem md) {
+static grpc_error* UnrefHeader(void* user_data, grpc_mdelem md) {
   GRPC_MDELEM_UNREF(md);
+  return GRPC_ERROR_NONE;
 }
 
-template <class Fixture, void (*OnHeader)(void*, grpc_mdelem)>
+template <class Fixture, grpc_error* (*OnHeader)(void*, grpc_mdelem)>
 static void BM_HpackParserParseHeader(benchmark::State& state) {
   TrackCounters track_counters;
   grpc_core::ExecCtx exec_ctx;
@@ -781,7 +782,7 @@ class RepresentativeServerTrailingMetadata {
 static void free_timeout(void* p) { gpr_free(p); }
 
 // Benchmark the current on_initial_header implementation
-static void OnInitialHeader(void* user_data, grpc_mdelem md) {
+static grpc_error* OnInitialHeader(void* user_data, grpc_mdelem md) {
   // Setup for benchmark. This will bloat the absolute values of this benchmark
   grpc_chttp2_incoming_metadata_buffer buffer(
       static_cast<grpc_core::Arena*>(user_data));
@@ -827,10 +828,11 @@ static void OnInitialHeader(void* user_data, grpc_mdelem md) {
       GPR_ASSERT(0);
     }
   }
+  return GRPC_ERROR_NONE;
 }
 
 // Benchmark timeout handling
-static void OnHeaderTimeout(void* user_data, grpc_mdelem md) {
+static grpc_error* OnHeaderTimeout(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));
@@ -858,6 +860,7 @@ static void OnHeaderTimeout(void* user_data, grpc_mdelem md) {
   } else {
     GPR_ASSERT(0);
   }
+  return GRPC_ERROR_NONE;
 }
 
 // Send the same deadline repeatedly