diff --git a/src/core/ext/transport/chttp2/transport/hpack_parser.cc b/src/core/ext/transport/chttp2/transport/hpack_parser.cc index efbd997e994..f83f67ed195 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_parser.cc +++ b/src/core/ext/transport/chttp2/transport/hpack_parser.cc @@ -779,6 +779,7 @@ static grpc_error* parse_indexed_field(grpc_chttp2_hpack_parser* p, const uint8_t* cur, const uint8_t* end) { p->dynamic_table_update_allowed = 0; p->index = (*cur) & 0x7f; + p->md_for_index.payload = 0; /* Invalidate cached md when index changes. */ return finish_indexed_field(p, cur + 1, end); } @@ -791,17 +792,32 @@ static grpc_error* parse_indexed_field_x(grpc_chttp2_hpack_parser* p, p->dynamic_table_update_allowed = 0; p->next_state = and_then; p->index = 0x7f; + p->md_for_index.payload = 0; /* Invalidate cached md when index changes. */ p->parsing.value = &p->index; return parse_value0(p, cur + 1, end); } +/* When finishing with a header, get the cached md element for this index. + This is set in parse_value_string(). We ensure (in debug mode) that the + cached metadata corresponds with the index we are examining. */ +static grpc_mdelem get_precomputed_md_for_idx(grpc_chttp2_hpack_parser* p) { + GPR_DEBUG_ASSERT(p->md_for_index.payload != 0); + GPR_DEBUG_ASSERT(static_cast(p->index) == p->precomputed_md_index); + grpc_mdelem md = p->md_for_index; + GPR_DEBUG_ASSERT(!GRPC_MDISNULL(md)); /* handled in string parsing */ + p->md_for_index.payload = 0; /* Invalidate cached md when index changes. */ +#ifndef NDEBUG + p->precomputed_md_index = -1; +#endif + return md; +} + /* finish a literal header with incremental indexing */ static grpc_error* finish_lithdr_incidx(grpc_chttp2_hpack_parser* p, const uint8_t* cur, const uint8_t* end) { - grpc_mdelem md = grpc_chttp2_hptbl_lookup(&p->table, p->index); - GPR_ASSERT(!GRPC_MDISNULL(md)); /* handled in string parsing */ GRPC_STATS_INC_HPACK_RECV_LITHDR_INCIDX(); + grpc_mdelem md = get_precomputed_md_for_idx(p); grpc_error* err = on_hdr( p, grpc_mdelem_from_slices(grpc_slice_ref_internal(GRPC_MDKEY(md)), take_string(p, &p->value, true))); @@ -829,6 +845,7 @@ static grpc_error* parse_lithdr_incidx(grpc_chttp2_hpack_parser* p, p->dynamic_table_update_allowed = 0; p->next_state = and_then; p->index = (*cur) & 0x3f; + p->md_for_index.payload = 0; /* Invalidate cached md when index changes. */ return parse_string_prefix(p, cur + 1, end); } @@ -842,6 +859,7 @@ static grpc_error* parse_lithdr_incidx_x(grpc_chttp2_hpack_parser* p, p->dynamic_table_update_allowed = 0; p->next_state = and_then; p->index = 0x3f; + p->md_for_index.payload = 0; /* Invalidate cached md when index changes. */ p->parsing.value = &p->index; return parse_value0(p, cur + 1, end); } @@ -862,9 +880,8 @@ static grpc_error* parse_lithdr_incidx_v(grpc_chttp2_hpack_parser* p, static grpc_error* finish_lithdr_notidx(grpc_chttp2_hpack_parser* p, const uint8_t* cur, const uint8_t* end) { - grpc_mdelem md = grpc_chttp2_hptbl_lookup(&p->table, p->index); - GPR_ASSERT(!GRPC_MDISNULL(md)); /* handled in string parsing */ GRPC_STATS_INC_HPACK_RECV_LITHDR_NOTIDX(); + grpc_mdelem md = get_precomputed_md_for_idx(p); grpc_error* err = on_hdr( p, grpc_mdelem_from_slices(grpc_slice_ref_internal(GRPC_MDKEY(md)), take_string(p, &p->value, false))); @@ -892,6 +909,7 @@ static grpc_error* parse_lithdr_notidx(grpc_chttp2_hpack_parser* p, p->dynamic_table_update_allowed = 0; p->next_state = and_then; p->index = (*cur) & 0xf; + p->md_for_index.payload = 0; /* Invalidate cached md when index changes. */ return parse_string_prefix(p, cur + 1, end); } @@ -905,6 +923,7 @@ static grpc_error* parse_lithdr_notidx_x(grpc_chttp2_hpack_parser* p, p->dynamic_table_update_allowed = 0; p->next_state = and_then; p->index = 0xf; + p->md_for_index.payload = 0; /* Invalidate cached md when index changes. */ p->parsing.value = &p->index; return parse_value0(p, cur + 1, end); } @@ -925,9 +944,8 @@ static grpc_error* parse_lithdr_notidx_v(grpc_chttp2_hpack_parser* p, static grpc_error* finish_lithdr_nvridx(grpc_chttp2_hpack_parser* p, const uint8_t* cur, const uint8_t* end) { - grpc_mdelem md = grpc_chttp2_hptbl_lookup(&p->table, p->index); - GPR_ASSERT(!GRPC_MDISNULL(md)); /* handled in string parsing */ GRPC_STATS_INC_HPACK_RECV_LITHDR_NVRIDX(); + grpc_mdelem md = get_precomputed_md_for_idx(p); grpc_error* err = on_hdr( p, grpc_mdelem_from_slices(grpc_slice_ref_internal(GRPC_MDKEY(md)), take_string(p, &p->value, false))); @@ -955,6 +973,7 @@ static grpc_error* parse_lithdr_nvridx(grpc_chttp2_hpack_parser* p, p->dynamic_table_update_allowed = 0; p->next_state = and_then; p->index = (*cur) & 0xf; + p->md_for_index.payload = 0; /* Invalidate cached md when index changes. */ return parse_string_prefix(p, cur + 1, end); } @@ -968,6 +987,7 @@ static grpc_error* parse_lithdr_nvridx_x(grpc_chttp2_hpack_parser* p, p->dynamic_table_update_allowed = 0; p->next_state = and_then; p->index = 0xf; + p->md_for_index.payload = 0; /* Invalidate cached md when index changes. */ p->parsing.value = &p->index; return parse_value0(p, cur + 1, end); } @@ -1007,6 +1027,7 @@ static grpc_error* parse_max_tbl_size(grpc_chttp2_hpack_parser* p, } p->dynamic_table_update_allowed--; p->index = (*cur) & 0x1f; + p->md_for_index.payload = 0; /* Invalidate cached md when index changes. */ return finish_max_tbl_size(p, cur + 1, end); } @@ -1025,6 +1046,7 @@ static grpc_error* parse_max_tbl_size_x(grpc_chttp2_hpack_parser* p, p->dynamic_table_update_allowed--; p->next_state = and_then; p->index = 0x1f; + p->md_for_index.payload = 0; /* Invalidate cached md when index changes. */ p->parsing.value = &p->index; return parse_value0(p, cur + 1, end); } @@ -1499,6 +1521,23 @@ static bool is_binary_literal_header(grpc_chttp2_hpack_parser* p) { : p->key.data.referenced); } +/* Cache the metadata for the given index during initial parsing. This avoids a + pointless recomputation of the metadata when finishing a header. We read the + cached value in get_precomputed_md_for_idx(). */ +static void set_precomputed_md_idx(grpc_chttp2_hpack_parser* p, + grpc_mdelem md) { + GPR_DEBUG_ASSERT(p->md_for_index.payload == 0); + GPR_DEBUG_ASSERT(p->precomputed_md_index == -1); + p->md_for_index = md; +#ifndef NDEBUG + p->precomputed_md_index = p->index; +#endif +} + +/* Determines if a metadata element key associated with the current parser index + is a binary indexed header during string parsing. We'll need to revisit this + metadata when we're done parsing, so we cache the metadata for this index + here using set_precomputed_md_idx(). */ 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); @@ -1519,6 +1558,7 @@ static grpc_error* is_binary_indexed_header(grpc_chttp2_hpack_parser* p, * interned. * 4. Both static and interned element slices have non-null refcounts. */ *is = grpc_is_refcounted_slice_binary_header(GRPC_MDKEY(elem)); + set_precomputed_md_idx(p, elem); return GRPC_ERROR_NONE; } @@ -1557,6 +1597,18 @@ void grpc_chttp2_hpack_parser_init(grpc_chttp2_hpack_parser* p) { p->value.data.copied.str = nullptr; p->value.data.copied.capacity = 0; p->value.data.copied.length = 0; + /* Cached metadata for the current index the parser is handling. This is set + to 0 initially, invalidated when the index changes, and invalidated when it + is read (by get_precomputed_md_for_idx()). It is set during string parsing, + by set_precomputed_md_idx() - which is called by parse_value_string(). + The goal here is to avoid recomputing the metadata for the index when + finishing with a header as well as the initial parse. */ + p->md_for_index.payload = 0; +#ifndef NDEBUG + /* In debug mode, this ensures that the cached metadata we're reading is in + * fact correct for the index we are examining. */ + p->precomputed_md_index = -1; +#endif p->dynamic_table_update_allowed = 2; p->last_error = GRPC_ERROR_NONE; grpc_chttp2_hptbl_init(&p->table); diff --git a/src/core/ext/transport/chttp2/transport/hpack_parser.h b/src/core/ext/transport/chttp2/transport/hpack_parser.h index 3dc8e13bea2..c5691244028 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_parser.h +++ b/src/core/ext/transport/chttp2/transport/hpack_parser.h @@ -69,6 +69,14 @@ struct grpc_chttp2_hpack_parser { grpc_chttp2_hpack_parser_string value; /* parsed index */ uint32_t index; + /* When we parse a value string, we determine the metadata element for a + specific index, which we need again when we're finishing up with that + header. To avoid calculating the metadata element for that index a second + time at that stage, we cache (and invalidate) the element here. */ + grpc_mdelem md_for_index; +#ifndef NDEBUG + int64_t precomputed_md_index; +#endif /* length of source bytes for the currently parsing string */ uint32_t strlen; /* number of source bytes read for the currently parsing string */