From fd42ceb1ff3d0a80487ee81d86afbde97b19fe42 Mon Sep 17 00:00:00 2001 From: yang-g Date: Tue, 10 Oct 2017 16:55:13 -0700 Subject: [PATCH 1/5] Use key in dynamic table if available --- .../chttp2/transport/hpack_encoder.cc | 146 ++++++++++-------- src/core/lib/transport/metadata.cc | 7 +- src/core/lib/transport/metadata.h | 3 +- .../transport/chttp2/hpack_encoder_test.c | 99 ++++++++---- test/core/transport/metadata_test.c | 4 +- 5 files changed, 156 insertions(+), 103 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/hpack_encoder.cc b/src/core/ext/transport/chttp2/transport/hpack_encoder.cc index 17b8c4ab85e..e667d8829a8 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_encoder.cc +++ b/src/core/ext/transport/chttp2/transport/hpack_encoder.cc @@ -180,14 +180,12 @@ static void evict_entry(grpc_chttp2_hpack_compressor *c) { /* add an element to the decoder table */ static void add_elem(grpc_exec_ctx *exec_ctx, grpc_chttp2_hpack_compressor *c, - grpc_mdelem elem) { - GPR_ASSERT(GRPC_MDELEM_IS_INTERNED(elem)); - + grpc_mdelem elem, size_t elem_size, bool only_add_key) { uint32_t key_hash = grpc_slice_hash(GRPC_MDKEY(elem)); - uint32_t value_hash = grpc_slice_hash(GRPC_MDVALUE(elem)); - uint32_t elem_hash = GRPC_MDSTR_KV_HASH(key_hash, value_hash); + uint32_t value_hash = only_add_key ? 0 : grpc_slice_hash(GRPC_MDVALUE(elem)); + uint32_t elem_hash = + only_add_key ? 0 : GRPC_MDSTR_KV_HASH(key_hash, value_hash); uint32_t new_index = c->tail_remote_index + c->table_elems + 1; - size_t elem_size = grpc_mdelem_get_size_in_hpack_table(elem); GPR_ASSERT(elem_size < 65536); @@ -209,37 +207,7 @@ static void add_elem(grpc_exec_ctx *exec_ctx, grpc_chttp2_hpack_compressor *c, c->table_size = (uint16_t)(c->table_size + elem_size); c->table_elems++; - /* Store this element into {entries,indices}_elem */ - if (grpc_mdelem_eq(c->entries_elems[HASH_FRAGMENT_2(elem_hash)], elem)) { - /* already there: update with new index */ - c->indices_elems[HASH_FRAGMENT_2(elem_hash)] = new_index; - } else if (grpc_mdelem_eq(c->entries_elems[HASH_FRAGMENT_3(elem_hash)], - elem)) { - /* already there (cuckoo): update with new index */ - c->indices_elems[HASH_FRAGMENT_3(elem_hash)] = new_index; - } else if (GRPC_MDISNULL(c->entries_elems[HASH_FRAGMENT_2(elem_hash)])) { - /* not there, but a free element: add */ - c->entries_elems[HASH_FRAGMENT_2(elem_hash)] = GRPC_MDELEM_REF(elem); - c->indices_elems[HASH_FRAGMENT_2(elem_hash)] = new_index; - } else if (GRPC_MDISNULL(c->entries_elems[HASH_FRAGMENT_3(elem_hash)])) { - /* not there (cuckoo), but a free element: add */ - c->entries_elems[HASH_FRAGMENT_3(elem_hash)] = GRPC_MDELEM_REF(elem); - c->indices_elems[HASH_FRAGMENT_3(elem_hash)] = new_index; - } else if (c->indices_elems[HASH_FRAGMENT_2(elem_hash)] < - c->indices_elems[HASH_FRAGMENT_3(elem_hash)]) { - /* not there: replace oldest */ - GRPC_MDELEM_UNREF(exec_ctx, c->entries_elems[HASH_FRAGMENT_2(elem_hash)]); - c->entries_elems[HASH_FRAGMENT_2(elem_hash)] = GRPC_MDELEM_REF(elem); - c->indices_elems[HASH_FRAGMENT_2(elem_hash)] = new_index; - } else { - /* not there: replace oldest */ - GRPC_MDELEM_UNREF(exec_ctx, c->entries_elems[HASH_FRAGMENT_3(elem_hash)]); - c->entries_elems[HASH_FRAGMENT_3(elem_hash)] = GRPC_MDELEM_REF(elem); - c->indices_elems[HASH_FRAGMENT_3(elem_hash)] = new_index; - } - - /* do exactly the same for the key (so we can find by that again too) */ - + /* Store the key into {entries,indices}_keys */ if (grpc_slice_eq(c->entries_keys[HASH_FRAGMENT_2(key_hash)], GRPC_MDKEY(elem))) { c->indices_keys[HASH_FRAGMENT_2(key_hash)] = new_index; @@ -270,6 +238,37 @@ static void add_elem(grpc_exec_ctx *exec_ctx, grpc_chttp2_hpack_compressor *c, grpc_slice_ref_internal(GRPC_MDKEY(elem)); c->indices_keys[HASH_FRAGMENT_3(key_hash)] = new_index; } + + if (!only_add_key) { + /* Store this element into {entries,indices}_elem */ + if (grpc_mdelem_eq(c->entries_elems[HASH_FRAGMENT_2(elem_hash)], elem)) { + /* already there: update with new index */ + c->indices_elems[HASH_FRAGMENT_2(elem_hash)] = new_index; + } else if (grpc_mdelem_eq(c->entries_elems[HASH_FRAGMENT_3(elem_hash)], + elem)) { + /* already there (cuckoo): update with new index */ + c->indices_elems[HASH_FRAGMENT_3(elem_hash)] = new_index; + } else if (GRPC_MDISNULL(c->entries_elems[HASH_FRAGMENT_2(elem_hash)])) { + /* not there, but a free element: add */ + c->entries_elems[HASH_FRAGMENT_2(elem_hash)] = GRPC_MDELEM_REF(elem); + c->indices_elems[HASH_FRAGMENT_2(elem_hash)] = new_index; + } else if (GRPC_MDISNULL(c->entries_elems[HASH_FRAGMENT_3(elem_hash)])) { + /* not there (cuckoo), but a free element: add */ + c->entries_elems[HASH_FRAGMENT_3(elem_hash)] = GRPC_MDELEM_REF(elem); + c->indices_elems[HASH_FRAGMENT_3(elem_hash)] = new_index; + } else if (c->indices_elems[HASH_FRAGMENT_2(elem_hash)] < + c->indices_elems[HASH_FRAGMENT_3(elem_hash)]) { + /* not there: replace oldest */ + GRPC_MDELEM_UNREF(exec_ctx, c->entries_elems[HASH_FRAGMENT_2(elem_hash)]); + c->entries_elems[HASH_FRAGMENT_2(elem_hash)] = GRPC_MDELEM_REF(elem); + c->indices_elems[HASH_FRAGMENT_2(elem_hash)] = new_index; + } else { + /* not there: replace oldest */ + GRPC_MDELEM_UNREF(exec_ctx, c->entries_elems[HASH_FRAGMENT_3(elem_hash)]); + c->entries_elems[HASH_FRAGMENT_3(elem_hash)] = GRPC_MDELEM_REF(elem); + c->indices_elems[HASH_FRAGMENT_3(elem_hash)] = new_index; + } + } } static void emit_indexed(grpc_exec_ctx *exec_ctx, @@ -430,9 +429,14 @@ static void hpack_enc(grpc_exec_ctx *exec_ctx, grpc_chttp2_hpack_compressor *c, "Reserved header (colon-prefixed) happening after regular ones."); } - if (GRPC_TRACER_ON(grpc_http_trace) && !GRPC_MDELEM_IS_INTERNED(elem)) { + if (GRPC_TRACER_ON(grpc_http_trace)) { char *k = grpc_slice_to_c_string(GRPC_MDKEY(elem)); - char *v = grpc_slice_to_c_string(GRPC_MDVALUE(elem)); + char *v = NULL; + if (grpc_is_binary_header(GRPC_MDKEY(elem))) { + v = grpc_dump_slice(GRPC_MDVALUE(elem), GPR_DUMP_HEX); + } else { + v = grpc_slice_to_c_string(GRPC_MDVALUE(elem)); + } gpr_log( GPR_DEBUG, "Encode: '%s: %s', elem_interned=%d [%d], k_interned=%d, v_interned=%d", @@ -442,7 +446,12 @@ static void hpack_enc(grpc_exec_ctx *exec_ctx, grpc_chttp2_hpack_compressor *c, gpr_free(k); gpr_free(v); } - if (!GRPC_MDELEM_IS_INTERNED(elem)) { + + bool elem_interned = GRPC_MDELEM_IS_INTERNED(elem); + bool key_interned = elem_interned || grpc_slice_is_interned(GRPC_MDKEY(elem)); + + // Key is not interned, emit literals. + if (!key_interned) { emit_lithdr_noidx_v(exec_ctx, c, elem, st); return; } @@ -452,37 +461,46 @@ static void hpack_enc(grpc_exec_ctx *exec_ctx, grpc_chttp2_hpack_compressor *c, uint32_t elem_hash; size_t decoder_space_usage; uint32_t indices_key; - int should_add_elem; + bool should_add_elem; + bool should_add_key; key_hash = grpc_slice_hash(GRPC_MDKEY(elem)); - value_hash = grpc_slice_hash(GRPC_MDVALUE(elem)); - elem_hash = GRPC_MDSTR_KV_HASH(key_hash, value_hash); - inc_filter(HASH_FRAGMENT_1(elem_hash), &c->filter_elems_sum, c->filter_elems); + if (elem_interned) { + value_hash = grpc_slice_hash(GRPC_MDVALUE(elem)); + elem_hash = GRPC_MDSTR_KV_HASH(key_hash, value_hash); - /* is this elem currently in the decoders table? */ + inc_filter(HASH_FRAGMENT_1(elem_hash), &c->filter_elems_sum, + c->filter_elems); - if (grpc_mdelem_eq(c->entries_elems[HASH_FRAGMENT_2(elem_hash)], elem) && - c->indices_elems[HASH_FRAGMENT_2(elem_hash)] > c->tail_remote_index) { - /* HIT: complete element (first cuckoo hash) */ - emit_indexed(exec_ctx, c, - dynidx(c, c->indices_elems[HASH_FRAGMENT_2(elem_hash)]), st); - return; - } + /* is this elem currently in the decoders table? */ - if (grpc_mdelem_eq(c->entries_elems[HASH_FRAGMENT_3(elem_hash)], elem) && - c->indices_elems[HASH_FRAGMENT_3(elem_hash)] > c->tail_remote_index) { - /* HIT: complete element (second cuckoo hash) */ - emit_indexed(exec_ctx, c, - dynidx(c, c->indices_elems[HASH_FRAGMENT_3(elem_hash)]), st); - return; + if (grpc_mdelem_eq(c->entries_elems[HASH_FRAGMENT_2(elem_hash)], elem) && + c->indices_elems[HASH_FRAGMENT_2(elem_hash)] > c->tail_remote_index) { + /* HIT: complete element (first cuckoo hash) */ + emit_indexed(exec_ctx, c, + dynidx(c, c->indices_elems[HASH_FRAGMENT_2(elem_hash)]), st); + return; + } + + if (grpc_mdelem_eq(c->entries_elems[HASH_FRAGMENT_3(elem_hash)], elem) && + c->indices_elems[HASH_FRAGMENT_3(elem_hash)] > c->tail_remote_index) { + /* HIT: complete element (second cuckoo hash) */ + emit_indexed(exec_ctx, c, + dynidx(c, c->indices_elems[HASH_FRAGMENT_3(elem_hash)]), st); + return; + } } /* should this elem be in the table? */ - decoder_space_usage = grpc_mdelem_get_size_in_hpack_table(elem); - should_add_elem = decoder_space_usage < MAX_DECODER_SPACE_USAGE && + decoder_space_usage = + grpc_mdelem_get_size_in_hpack_table(elem, st->use_true_binary_metadata); + 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; + should_add_key = + !elem_interned && decoder_space_usage < MAX_DECODER_SPACE_USAGE; /* no hits for the elem... maybe there's a key? */ @@ -493,7 +511,7 @@ static void hpack_enc(grpc_exec_ctx *exec_ctx, grpc_chttp2_hpack_compressor *c, /* HIT: key (first cuckoo hash) */ if (should_add_elem) { emit_lithdr_incidx(exec_ctx, c, dynidx(c, indices_key), elem, st); - add_elem(exec_ctx, c, elem); + add_elem(exec_ctx, c, elem, decoder_space_usage, false); return; } else { emit_lithdr_noidx(exec_ctx, c, dynidx(c, indices_key), elem, st); @@ -509,7 +527,7 @@ static void hpack_enc(grpc_exec_ctx *exec_ctx, grpc_chttp2_hpack_compressor *c, /* HIT: key (first cuckoo hash) */ if (should_add_elem) { emit_lithdr_incidx(exec_ctx, c, dynidx(c, indices_key), elem, st); - add_elem(exec_ctx, c, elem); + add_elem(exec_ctx, c, elem, decoder_space_usage, false); return; } else { emit_lithdr_noidx(exec_ctx, c, dynidx(c, indices_key), elem, st); @@ -520,9 +538,9 @@ static void hpack_enc(grpc_exec_ctx *exec_ctx, grpc_chttp2_hpack_compressor *c, /* no elem, key in the table... fall back to literal emission */ - if (should_add_elem) { + if (should_add_elem || should_add_key) { emit_lithdr_incidx_v(exec_ctx, c, elem, st); - add_elem(exec_ctx, c, elem); + add_elem(exec_ctx, c, elem, decoder_space_usage, should_add_key); return; } else { emit_lithdr_noidx_v(exec_ctx, c, elem, st); diff --git a/src/core/lib/transport/metadata.cc b/src/core/lib/transport/metadata.cc index 5455b2481bd..2392f26c0b1 100644 --- a/src/core/lib/transport/metadata.cc +++ b/src/core/lib/transport/metadata.cc @@ -352,11 +352,14 @@ static size_t get_base64_encoded_size(size_t raw_length) { return raw_length / 3 * 4 + tail_xtra[raw_length % 3]; } -size_t grpc_mdelem_get_size_in_hpack_table(grpc_mdelem elem) { +size_t grpc_mdelem_get_size_in_hpack_table(grpc_mdelem elem, + bool use_true_binary_metadata) { size_t overhead_and_key = 32 + GRPC_SLICE_LENGTH(GRPC_MDKEY(elem)); size_t value_len = GRPC_SLICE_LENGTH(GRPC_MDVALUE(elem)); if (grpc_is_binary_header(GRPC_MDKEY(elem))) { - return overhead_and_key + get_base64_encoded_size(value_len); + return overhead_and_key + (use_true_binary_metadata + ? value_len + 1 + : get_base64_encoded_size(value_len)); } else { return overhead_and_key + value_len; } diff --git a/src/core/lib/transport/metadata.h b/src/core/lib/transport/metadata.h index 9f82225dc39..3f1032ab8ad 100644 --- a/src/core/lib/transport/metadata.h +++ b/src/core/lib/transport/metadata.h @@ -132,7 +132,8 @@ grpc_mdelem grpc_mdelem_create( bool grpc_mdelem_eq(grpc_mdelem a, grpc_mdelem b); -size_t grpc_mdelem_get_size_in_hpack_table(grpc_mdelem elem); +size_t grpc_mdelem_get_size_in_hpack_table(grpc_mdelem elem, + bool use_true_binary_metadata); /* Mutator and accessor for grpc_mdelem user data. The destructor function is used as a type tag and is checked during user_data fetch. */ diff --git a/test/core/transport/chttp2/hpack_encoder_test.c b/test/core/transport/chttp2/hpack_encoder_test.c index ed51dd18592..a2af83b6cb9 100644 --- a/test/core/transport/chttp2/hpack_encoder_test.c +++ b/test/core/transport/chttp2/hpack_encoder_test.c @@ -43,10 +43,15 @@ void **to_delete = NULL; size_t num_to_delete = 0; size_t cap_to_delete = 0; +typedef struct { + bool eof; + bool use_true_binary_metadata; + bool only_intern_key; +} verify_params; + /* verify that the output generated by encoding the stream matches the hexstring passed in */ -static void verify(grpc_exec_ctx *exec_ctx, size_t window_available, bool eof, - bool use_true_binary_metadata, size_t expect_window_used, +static void verify(grpc_exec_ctx *exec_ctx, const verify_params params, const char *expected, size_t nheaders, ...) { grpc_slice_buffer output; grpc_slice merged; @@ -66,9 +71,13 @@ static void verify(grpc_exec_ctx *exec_ctx, size_t window_available, bool eof, e[i - 1].next = &e[i]; e[i].prev = &e[i - 1]; } + grpc_slice value_slice = grpc_slice_from_static_string(value); + if (!params.only_intern_key) { + value_slice = grpc_slice_intern(value_slice); + } e[i].md = grpc_mdelem_from_slices( exec_ctx, grpc_slice_intern(grpc_slice_from_static_string(key)), - grpc_slice_intern(grpc_slice_from_static_string(value))); + value_slice); } e[0].prev = NULL; e[nheaders - 1].next = NULL; @@ -90,8 +99,8 @@ static void verify(grpc_exec_ctx *exec_ctx, size_t window_available, bool eof, memset(&stats, 0, sizeof(stats)); grpc_encode_header_options hopt = { .stream_id = 0xdeadbeef, - .is_eof = eof, - .use_true_binary_metadata = use_true_binary_metadata, + .is_eof = params.eof, + .use_true_binary_metadata = params.use_true_binary_metadata, .max_frame_size = 16384, .stats = &stats, }; @@ -119,28 +128,27 @@ static void verify(grpc_exec_ctx *exec_ctx, size_t window_available, bool eof, static void test_basic_headers(grpc_exec_ctx *exec_ctx) { int i; - verify(exec_ctx, 0, false, false, 0, "000005 0104 deadbeef 40 0161 0161", 1, - "a", "a"); - verify(exec_ctx, 0, false, false, 0, "000001 0104 deadbeef be", 1, "a", "a"); - verify(exec_ctx, 0, false, false, 0, "000001 0104 deadbeef be", 1, "a", "a"); - verify(exec_ctx, 0, false, false, 0, "000006 0104 deadbeef be 40 0162 0163", - 2, "a", "a", "b", "c"); - verify(exec_ctx, 0, false, false, 0, "000002 0104 deadbeef bf be", 2, "a", - "a", "b", "c"); - verify(exec_ctx, 0, false, false, 0, "000004 0104 deadbeef 7f 00 0164", 1, - "a", "d"); + verify_params params = { + .eof = false, .use_true_binary_metadata = false, .only_intern_key = false, + }; + verify(exec_ctx, params, "000005 0104 deadbeef 40 0161 0161", 1, "a", "a"); + verify(exec_ctx, params, "000001 0104 deadbeef be", 1, "a", "a"); + verify(exec_ctx, params, "000001 0104 deadbeef be", 1, "a", "a"); + verify(exec_ctx, params, "000006 0104 deadbeef be 40 0162 0163", 2, "a", "a", + "b", "c"); + verify(exec_ctx, params, "000002 0104 deadbeef bf be", 2, "a", "a", "b", "c"); + verify(exec_ctx, params, "000004 0104 deadbeef 7f 00 0164", 1, "a", "d"); /* flush out what's there to make a few values look very popular */ for (i = 0; i < 350; i++) { - verify(exec_ctx, 0, false, false, 0, "000003 0104 deadbeef c0 bf be", 3, - "a", "a", "b", "c", "a", "d"); + verify(exec_ctx, params, "000003 0104 deadbeef c0 bf be", 3, "a", "a", "b", + "c", "a", "d"); } - verify(exec_ctx, 0, false, false, 0, "000006 0104 deadbeef c0 00 016b 0176", - 2, "a", "a", "k", "v"); + verify(exec_ctx, params, "000006 0104 deadbeef c0 00 016b 0176", 2, "a", "a", + "k", "v"); /* this could be 000004 0104 deadbeef 0f 30 0176 also */ - verify(exec_ctx, 0, false, false, 0, "000004 0104 deadbeef 0f 2f 0176", 1, - "a", "v"); + verify(exec_ctx, params, "000004 0104 deadbeef 0f 2f 0176", 1, "a", "v"); } static void encode_int_to_str(int i, char *p) { @@ -156,6 +164,10 @@ static void test_decode_table_overflow(grpc_exec_ctx *exec_ctx) { char key[3], value[3]; char *expect; + verify_params params = { + .eof = false, .use_true_binary_metadata = false, .only_intern_key = false, + }; + for (i = 0; i < 114; i++) { encode_int_to_str(i, key); encode_int_to_str(i + 1, value); @@ -174,27 +186,28 @@ static void test_decode_table_overflow(grpc_exec_ctx *exec_ctx) { } if (i > 0) { - verify(exec_ctx, 0, false, false, 0, expect, 2, "aa", "ba", key, value); + verify(exec_ctx, params, expect, 2, "aa", "ba", key, value); } else { - verify(exec_ctx, 0, false, false, 0, expect, 1, key, value); + verify(exec_ctx, params, expect, 1, key, value); } gpr_free(expect); } /* if the above passes, then we must have just knocked this pair out of the decoder stack, and so we'll be forced to re-encode it */ - verify(exec_ctx, 0, false, false, 0, "000007 0104 deadbeef 40 026161 026261", - 1, "aa", "ba"); + verify(exec_ctx, params, "000007 0104 deadbeef 40 026161 026261", 1, "aa", + "ba"); } static void verify_table_size_change_match_elem_size(grpc_exec_ctx *exec_ctx, const char *key, - const char *value) { + const char *value, + bool use_true_binary) { grpc_slice_buffer output; grpc_mdelem elem = grpc_mdelem_from_slices( exec_ctx, grpc_slice_intern(grpc_slice_from_static_string(key)), grpc_slice_intern(grpc_slice_from_static_string(value))); - size_t elem_size = grpc_mdelem_get_size_in_hpack_table(elem); + size_t elem_size = grpc_mdelem_get_size_in_hpack_table(elem, use_true_binary); size_t initial_table_size = g_compressor.table_size; grpc_linked_mdelem *e = gpr_malloc(sizeof(*e)); grpc_metadata_batch b; @@ -209,11 +222,12 @@ static void verify_table_size_change_match_elem_size(grpc_exec_ctx *exec_ctx, grpc_transport_one_way_stats stats; memset(&stats, 0, sizeof(stats)); - grpc_encode_header_options hopt = {.stream_id = 0xdeadbeef, - .is_eof = false, - .use_true_binary_metadata = false, - .max_frame_size = 16384, - .stats = &stats}; + grpc_encode_header_options hopt = { + .stream_id = 0xdeadbeef, + .is_eof = false, + .use_true_binary_metadata = use_true_binary, + .max_frame_size = 16384, + .stats = &stats}; grpc_chttp2_encode_header(exec_ctx, &g_compressor, NULL, 0, &b, &hopt, &output); grpc_slice_buffer_destroy_internal(exec_ctx, &output); @@ -224,8 +238,24 @@ static void verify_table_size_change_match_elem_size(grpc_exec_ctx *exec_ctx, } static void test_encode_header_size(grpc_exec_ctx *exec_ctx) { - verify_table_size_change_match_elem_size(exec_ctx, "hello", "world"); - verify_table_size_change_match_elem_size(exec_ctx, "hello-bin", "world"); + verify_table_size_change_match_elem_size(exec_ctx, "hello", "world", false); + verify_table_size_change_match_elem_size(exec_ctx, "hello-bin", "world", + false); + verify_table_size_change_match_elem_size(exec_ctx, "true-binary-bin", + "I_am_true_binary_value", true); +} + +static void test_interned_key_indexed(grpc_exec_ctx *exec_ctx) { + int i; + verify_params params = { + .eof = false, .use_true_binary_metadata = false, .only_intern_key = true, + }; + verify(exec_ctx, params, "000009 0104 deadbeef 40 0161 0162 0f2f 0163", 2, + "a", "b", "a", "c"); + for (i = 0; i < 10; i++) { + verify(exec_ctx, params, "000008 0104 deadbeef 0f2f 0162 0f2f 0163", 2, "a", + "b", "a", "c"); + } } static void run_test(void (*test)(grpc_exec_ctx *exec_ctx), const char *name) { @@ -245,6 +275,7 @@ int main(int argc, char **argv) { TEST(test_basic_headers); TEST(test_decode_table_overflow); TEST(test_encode_header_size); + TEST(test_interned_key_indexed); grpc_shutdown(); for (i = 0; i < num_to_delete; i++) { gpr_free(to_delete[i]); diff --git a/test/core/transport/metadata_test.c b/test/core/transport/metadata_test.c index cb06fce30bd..f7124d29a7e 100644 --- a/test/core/transport/metadata_test.c +++ b/test/core/transport/metadata_test.c @@ -302,7 +302,7 @@ static void verify_ascii_header_size(grpc_exec_ctx *exec_ctx, const char *key, grpc_mdelem elem = grpc_mdelem_from_slices( exec_ctx, maybe_intern(grpc_slice_from_static_string(key), intern_key), maybe_intern(grpc_slice_from_static_string(value), intern_value)); - size_t elem_size = grpc_mdelem_get_size_in_hpack_table(elem); + size_t elem_size = grpc_mdelem_get_size_in_hpack_table(elem, false); size_t expected_size = 32 + strlen(key) + strlen(value); GPR_ASSERT(expected_size == elem_size); GRPC_MDELEM_UNREF(exec_ctx, elem); @@ -316,7 +316,7 @@ static void verify_binary_header_size(grpc_exec_ctx *exec_ctx, const char *key, maybe_intern(grpc_slice_from_static_buffer(value, value_len), intern_value)); GPR_ASSERT(grpc_is_binary_header(GRPC_MDKEY(elem))); - size_t elem_size = grpc_mdelem_get_size_in_hpack_table(elem); + size_t elem_size = grpc_mdelem_get_size_in_hpack_table(elem, false); grpc_slice value_slice = grpc_slice_from_copied_buffer((const char *)value, value_len); grpc_slice base64_encoded = grpc_chttp2_base64_encode(value_slice); From b7cc8b0b8de063a37ccbeffd21f4225d48161cbf Mon Sep 17 00:00:00 2001 From: yang-g Date: Wed, 11 Oct 2017 15:50:58 -0700 Subject: [PATCH 2/5] Resolve comments --- .../chttp2/transport/hpack_encoder.cc | 204 ++++++++++-------- 1 file changed, 117 insertions(+), 87 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/hpack_encoder.cc b/src/core/ext/transport/chttp2/transport/hpack_encoder.cc index e667d8829a8..0ea50e394b2 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_encoder.cc +++ b/src/core/ext/transport/chttp2/transport/hpack_encoder.cc @@ -178,22 +178,19 @@ static void evict_entry(grpc_chttp2_hpack_compressor *c) { c->table_elems--; } -/* add an element to the decoder table */ -static void add_elem(grpc_exec_ctx *exec_ctx, grpc_chttp2_hpack_compressor *c, - grpc_mdelem elem, size_t elem_size, bool only_add_key) { - uint32_t key_hash = grpc_slice_hash(GRPC_MDKEY(elem)); - uint32_t value_hash = only_add_key ? 0 : grpc_slice_hash(GRPC_MDVALUE(elem)); - uint32_t elem_hash = - only_add_key ? 0 : GRPC_MDSTR_KV_HASH(key_hash, value_hash); +// Reserve space in table for the new element, evict entries if needed. +// Return the new index of the element. Return 0 to indicate not adding to +// table. +static uint32_t prepare_space_for_new_elem(grpc_chttp2_hpack_compressor *c, + size_t elem_size) { uint32_t new_index = c->tail_remote_index + c->table_elems + 1; - GPR_ASSERT(elem_size < 65536); if (elem_size > c->max_table_size) { while (c->table_size > 0) { evict_entry(c); } - return; + return 0; } /* Reserve space for this element in the remote table: if this overflows @@ -207,6 +204,25 @@ static void add_elem(grpc_exec_ctx *exec_ctx, grpc_chttp2_hpack_compressor *c, c->table_size = (uint16_t)(c->table_size + elem_size); c->table_elems++; + return new_index; +} + +/* dummy function */ +static void add_nothing(grpc_exec_ctx *exec_ctx, + 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_exec_ctx *exec_ctx, + grpc_chttp2_hpack_compressor *c, + grpc_mdelem elem, uint32_t new_index) { + if (new_index == 0) { + return; + } + + uint32_t key_hash = grpc_slice_hash(GRPC_MDKEY(elem)); + /* Store the key into {entries,indices}_keys */ if (grpc_slice_eq(c->entries_keys[HASH_FRAGMENT_2(key_hash)], GRPC_MDKEY(elem))) { @@ -238,37 +254,63 @@ static void add_elem(grpc_exec_ctx *exec_ctx, grpc_chttp2_hpack_compressor *c, grpc_slice_ref_internal(GRPC_MDKEY(elem)); c->indices_keys[HASH_FRAGMENT_3(key_hash)] = new_index; } +} - if (!only_add_key) { - /* Store this element into {entries,indices}_elem */ - if (grpc_mdelem_eq(c->entries_elems[HASH_FRAGMENT_2(elem_hash)], elem)) { - /* already there: update with new index */ - c->indices_elems[HASH_FRAGMENT_2(elem_hash)] = new_index; - } else if (grpc_mdelem_eq(c->entries_elems[HASH_FRAGMENT_3(elem_hash)], - elem)) { - /* already there (cuckoo): update with new index */ - c->indices_elems[HASH_FRAGMENT_3(elem_hash)] = new_index; - } else if (GRPC_MDISNULL(c->entries_elems[HASH_FRAGMENT_2(elem_hash)])) { - /* not there, but a free element: add */ - c->entries_elems[HASH_FRAGMENT_2(elem_hash)] = GRPC_MDELEM_REF(elem); - c->indices_elems[HASH_FRAGMENT_2(elem_hash)] = new_index; - } else if (GRPC_MDISNULL(c->entries_elems[HASH_FRAGMENT_3(elem_hash)])) { - /* not there (cuckoo), but a free element: add */ - c->entries_elems[HASH_FRAGMENT_3(elem_hash)] = GRPC_MDELEM_REF(elem); - c->indices_elems[HASH_FRAGMENT_3(elem_hash)] = new_index; - } else if (c->indices_elems[HASH_FRAGMENT_2(elem_hash)] < - c->indices_elems[HASH_FRAGMENT_3(elem_hash)]) { - /* not there: replace oldest */ - GRPC_MDELEM_UNREF(exec_ctx, c->entries_elems[HASH_FRAGMENT_2(elem_hash)]); - c->entries_elems[HASH_FRAGMENT_2(elem_hash)] = GRPC_MDELEM_REF(elem); - c->indices_elems[HASH_FRAGMENT_2(elem_hash)] = new_index; - } else { - /* not there: replace oldest */ - GRPC_MDELEM_UNREF(exec_ctx, c->entries_elems[HASH_FRAGMENT_3(elem_hash)]); - c->entries_elems[HASH_FRAGMENT_3(elem_hash)] = GRPC_MDELEM_REF(elem); - c->indices_elems[HASH_FRAGMENT_3(elem_hash)] = new_index; - } +/* add an element to the decoder table */ +static void add_elem_with_index(grpc_exec_ctx *exec_ctx, + grpc_chttp2_hpack_compressor *c, + grpc_mdelem elem, uint32_t new_index) { + if (new_index == 0) { + return; } + GPR_ASSERT(GRPC_MDELEM_IS_INTERNED(elem)); + + uint32_t key_hash = grpc_slice_hash(GRPC_MDKEY(elem)); + uint32_t value_hash = grpc_slice_hash(GRPC_MDVALUE(elem)); + uint32_t elem_hash = GRPC_MDSTR_KV_HASH(key_hash, value_hash); + + /* Store this element into {entries,indices}_elem */ + if (grpc_mdelem_eq(c->entries_elems[HASH_FRAGMENT_2(elem_hash)], elem)) { + /* already there: update with new index */ + c->indices_elems[HASH_FRAGMENT_2(elem_hash)] = new_index; + } else if (grpc_mdelem_eq(c->entries_elems[HASH_FRAGMENT_3(elem_hash)], + elem)) { + /* already there (cuckoo): update with new index */ + c->indices_elems[HASH_FRAGMENT_3(elem_hash)] = new_index; + } else if (GRPC_MDISNULL(c->entries_elems[HASH_FRAGMENT_2(elem_hash)])) { + /* not there, but a free element: add */ + c->entries_elems[HASH_FRAGMENT_2(elem_hash)] = GRPC_MDELEM_REF(elem); + c->indices_elems[HASH_FRAGMENT_2(elem_hash)] = new_index; + } else if (GRPC_MDISNULL(c->entries_elems[HASH_FRAGMENT_3(elem_hash)])) { + /* not there (cuckoo), but a free element: add */ + c->entries_elems[HASH_FRAGMENT_3(elem_hash)] = GRPC_MDELEM_REF(elem); + c->indices_elems[HASH_FRAGMENT_3(elem_hash)] = new_index; + } else if (c->indices_elems[HASH_FRAGMENT_2(elem_hash)] < + c->indices_elems[HASH_FRAGMENT_3(elem_hash)]) { + /* not there: replace oldest */ + GRPC_MDELEM_UNREF(exec_ctx, c->entries_elems[HASH_FRAGMENT_2(elem_hash)]); + c->entries_elems[HASH_FRAGMENT_2(elem_hash)] = GRPC_MDELEM_REF(elem); + c->indices_elems[HASH_FRAGMENT_2(elem_hash)] = new_index; + } else { + /* not there: replace oldest */ + GRPC_MDELEM_UNREF(exec_ctx, c->entries_elems[HASH_FRAGMENT_3(elem_hash)]); + c->entries_elems[HASH_FRAGMENT_3(elem_hash)] = GRPC_MDELEM_REF(elem); + c->indices_elems[HASH_FRAGMENT_3(elem_hash)] = new_index; + } + + add_key_with_index(exec_ctx, c, elem, new_index); +} + +static void add_elem(grpc_exec_ctx *exec_ctx, grpc_chttp2_hpack_compressor *c, + grpc_mdelem elem, size_t elem_size) { + uint32_t new_index = prepare_space_for_new_elem(c, elem_size); + add_elem_with_index(exec_ctx, c, elem, new_index); +} + +static void add_key(grpc_exec_ctx *exec_ctx, grpc_chttp2_hpack_compressor *c, + grpc_mdelem elem, size_t elem_size) { + uint32_t new_index = prepare_space_for_new_elem(c, elem_size); + add_key_with_index(exec_ctx, c, elem, new_index); } static void emit_indexed(grpc_exec_ctx *exec_ctx, @@ -362,7 +404,9 @@ static void emit_lithdr_noidx(grpc_exec_ctx *exec_ctx, static void emit_lithdr_incidx_v(grpc_exec_ctx *exec_ctx, grpc_chttp2_hpack_compressor *c, - grpc_mdelem elem, framer_state *st) { + uint32_t unused_index, grpc_mdelem elem, + framer_state *st) { + GPR_ASSERT(unused_index == 0); GRPC_STATS_INC_HPACK_SEND_LITHDR_INCIDX_V(exec_ctx); GRPC_STATS_INC_HPACK_SEND_UNCOMPRESSED(exec_ctx); uint32_t len_key = (uint32_t)GRPC_SLICE_LENGTH(GRPC_MDKEY(elem)); @@ -384,7 +428,9 @@ static void emit_lithdr_incidx_v(grpc_exec_ctx *exec_ctx, static void emit_lithdr_noidx_v(grpc_exec_ctx *exec_ctx, grpc_chttp2_hpack_compressor *c, - grpc_mdelem elem, framer_state *st) { + uint32_t unused_index, grpc_mdelem elem, + framer_state *st) { + GPR_ASSERT(unused_index == 0); GRPC_STATS_INC_HPACK_SEND_LITHDR_NOTIDX_V(exec_ctx); GRPC_STATS_INC_HPACK_SEND_UNCOMPRESSED(exec_ctx); uint32_t len_key = (uint32_t)GRPC_SLICE_LENGTH(GRPC_MDKEY(elem)); @@ -452,22 +498,15 @@ static void hpack_enc(grpc_exec_ctx *exec_ctx, grpc_chttp2_hpack_compressor *c, // Key is not interned, emit literals. if (!key_interned) { - emit_lithdr_noidx_v(exec_ctx, c, elem, st); + emit_lithdr_noidx_v(exec_ctx, c, 0, elem, st); return; } - uint32_t key_hash; - uint32_t value_hash; - uint32_t elem_hash; - size_t decoder_space_usage; - uint32_t indices_key; - bool should_add_elem; - bool should_add_key; - - key_hash = grpc_slice_hash(GRPC_MDKEY(elem)); + uint32_t key_hash = grpc_slice_hash(GRPC_MDKEY(elem)); + uint32_t elem_hash = 0; if (elem_interned) { - value_hash = grpc_slice_hash(GRPC_MDVALUE(elem)); + uint32_t value_hash = grpc_slice_hash(GRPC_MDVALUE(elem)); elem_hash = GRPC_MDSTR_KV_HASH(key_hash, value_hash); inc_filter(HASH_FRAGMENT_1(elem_hash), &c->filter_elems_sum, @@ -492,32 +531,31 @@ static void hpack_enc(grpc_exec_ctx *exec_ctx, grpc_chttp2_hpack_compressor *c, } } + uint32_t indices_key; + /* should this elem be in the table? */ - decoder_space_usage = + size_t decoder_space_usage = grpc_mdelem_get_size_in_hpack_table(elem, st->use_true_binary_metadata); - 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; - should_add_key = - !elem_interned && decoder_space_usage < MAX_DECODER_SPACE_USAGE; + 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_exec_ctx *, grpc_chttp2_hpack_compressor *, + grpc_mdelem, size_t) = + should_add_elem ? add_elem : add_nothing; + void (*emit)(grpc_exec_ctx *, grpc_chttp2_hpack_compressor *, uint32_t, + grpc_mdelem, framer_state *) = + should_add_elem ? emit_lithdr_incidx : emit_lithdr_noidx; /* no hits for the elem... maybe there's a key? */ - indices_key = c->indices_keys[HASH_FRAGMENT_2(key_hash)]; if (grpc_slice_eq(c->entries_keys[HASH_FRAGMENT_2(key_hash)], GRPC_MDKEY(elem)) && indices_key > c->tail_remote_index) { /* HIT: key (first cuckoo hash) */ - if (should_add_elem) { - emit_lithdr_incidx(exec_ctx, c, dynidx(c, indices_key), elem, st); - add_elem(exec_ctx, c, elem, decoder_space_usage, false); - return; - } else { - emit_lithdr_noidx(exec_ctx, c, dynidx(c, indices_key), elem, st); - return; - } - GPR_UNREACHABLE_CODE(return ); + emit(exec_ctx, c, dynidx(c, indices_key), elem, st); + maybe_add(exec_ctx, c, elem, decoder_space_usage); + return; } indices_key = c->indices_keys[HASH_FRAGMENT_3(key_hash)]; @@ -525,28 +563,20 @@ static void hpack_enc(grpc_exec_ctx *exec_ctx, grpc_chttp2_hpack_compressor *c, GRPC_MDKEY(elem)) && indices_key > c->tail_remote_index) { /* HIT: key (first cuckoo hash) */ - if (should_add_elem) { - emit_lithdr_incidx(exec_ctx, c, dynidx(c, indices_key), elem, st); - add_elem(exec_ctx, c, elem, decoder_space_usage, false); - return; - } else { - emit_lithdr_noidx(exec_ctx, c, dynidx(c, indices_key), elem, st); - return; - } - GPR_UNREACHABLE_CODE(return ); + emit(exec_ctx, c, dynidx(c, indices_key), elem, st); + maybe_add(exec_ctx, c, elem, decoder_space_usage); + return; } /* no elem, key in the table... fall back to literal emission */ - - if (should_add_elem || should_add_key) { - emit_lithdr_incidx_v(exec_ctx, c, elem, st); - add_elem(exec_ctx, c, elem, decoder_space_usage, should_add_key); - return; - } else { - emit_lithdr_noidx_v(exec_ctx, c, elem, st); - return; - } - GPR_UNREACHABLE_CODE(return ); + 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(exec_ctx, c, 0, elem, st); + maybe_add(exec_ctx, c, elem, decoder_space_usage); } #define STRLEN_LIT(x) (sizeof(x) - 1) From 377636f4d2ac7e6e4e318aa215b06f38fd022582 Mon Sep 17 00:00:00 2001 From: yang-g Date: Mon, 2 Oct 2017 21:10:58 -0700 Subject: [PATCH 3/5] Make hpack micro bm more representative --- test/cpp/microbenchmarks/bm_chttp2_hpack.cc | 149 +++++++++++++++----- test/cpp/microbenchmarks/helpers.cc | 7 + test/cpp/microbenchmarks/helpers.h | 3 + 3 files changed, 124 insertions(+), 35 deletions(-) diff --git a/test/cpp/microbenchmarks/bm_chttp2_hpack.cc b/test/cpp/microbenchmarks/bm_chttp2_hpack.cc index adbfa4d7967..92d0da8f69d 100644 --- a/test/cpp/microbenchmarks/bm_chttp2_hpack.cc +++ b/test/cpp/microbenchmarks/bm_chttp2_hpack.cc @@ -34,6 +34,15 @@ extern "C" { auto &force_library_initialization = Library::get(); +static grpc_slice MakeSlice(std::vector bytes) { + grpc_slice s = grpc_slice_malloc(bytes.size()); + uint8_t *p = GRPC_SLICE_START_PTR(s); + for (auto b : bytes) { + *p++ = b; + } + return s; +} + //////////////////////////////////////////////////////////////////////////////// // HPACK encoder // @@ -52,6 +61,48 @@ static void BM_HpackEncoderInitDestroy(benchmark::State &state) { } BENCHMARK(BM_HpackEncoderInitDestroy); +static void BM_HpackEncoderEncodeDeadline(benchmark::State &state) { + TrackCounters track_counters; + grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; + + grpc_metadata_batch b; + grpc_metadata_batch_init(&b); + b.deadline = gpr_time_add(gpr_now(GPR_CLOCK_REALTIME), + gpr_time_from_seconds(30, GPR_TIMESPAN)); + + grpc_chttp2_hpack_compressor c; + grpc_chttp2_hpack_compressor_init(&c); + grpc_transport_one_way_stats stats; + memset(&stats, 0, sizeof(stats)); + grpc_slice_buffer outbuf; + grpc_slice_buffer_init(&outbuf); + while (state.KeepRunning()) { + grpc_encode_header_options hopt = { + static_cast(state.iterations()), + true, + false, + (size_t)1024, + &stats, + }; + grpc_chttp2_encode_header(&exec_ctx, &c, NULL, 0, &b, &hopt, &outbuf); + grpc_slice_buffer_reset_and_unref_internal(&exec_ctx, &outbuf); + grpc_exec_ctx_flush(&exec_ctx); + } + grpc_metadata_batch_destroy(&exec_ctx, &b); + grpc_chttp2_hpack_compressor_destroy(&exec_ctx, &c); + grpc_slice_buffer_destroy_internal(&exec_ctx, &outbuf); + grpc_exec_ctx_finish(&exec_ctx); + + std::ostringstream label; + label << "framing_bytes/iter:" << (static_cast(stats.framing_bytes) / + static_cast(state.iterations())) + << " header_bytes/iter:" << (static_cast(stats.header_bytes) / + static_cast(state.iterations())); + track_counters.AddLabel(label.str()); + track_counters.Finish(state); +} +BENCHMARK(BM_HpackEncoderEncodeDeadline); + template static void BM_HpackEncoderEncodeHeader(benchmark::State &state) { TrackCounters track_counters; @@ -104,7 +155,7 @@ static void BM_HpackEncoderEncodeHeader(benchmark::State &state) { static_cast(state.iterations())) << " header_bytes/iter:" << (static_cast(stats.header_bytes) / static_cast(state.iterations())); - state.SetLabel(label.str()); + track_counters.AddLabel(label.str()); track_counters.Finish(state); } @@ -206,10 +257,24 @@ class RepresentativeClientInitialMetadata { GRPC_MDELEM_SCHEME_HTTP, GRPC_MDELEM_METHOD_POST, grpc_mdelem_from_slices( exec_ctx, GRPC_MDSTR_PATH, - grpc_slice_intern(grpc_slice_from_static_string("/foo/bar"))), + grpc_slice_from_static_string("/grpc.test.FooService/BarMethod")), grpc_mdelem_from_slices(exec_ctx, GRPC_MDSTR_AUTHORITY, grpc_slice_intern(grpc_slice_from_static_string( "foo.test.google.fr:1234"))), + grpc_mdelem_from_slices( + exec_ctx, GRPC_MDSTR_GRPC_TRACE_BIN, + grpc_slice_from_static_string("\x00\x01\x02\x03\x04\x05\x06\x07\x08" + "\x09\x0a\x0b\x0c\x0d\x0e\x0f" + "\x10\x11\x12\x13\x14\x15\x16\x17\x18" + "\x19\x1a\x1b\x1c\x1d\x1e\x1f" + "\x20\x21\x22\x23\x24\x25\x26\x27\x28" + "\x29\x2a\x2b\x2c\x2d\x2e\x2f" + "\x30")), + grpc_mdelem_from_slices( + exec_ctx, GRPC_MDSTR_GRPC_TAGS_BIN, + grpc_slice_from_static_string("\x00\x01\x02\x03\x04\x05\x06\x07\x08" + "\x09\x0a\x0b\x0c\x0d\x0e\x0f" + "\x10\x11\x12\x13")), GRPC_MDELEM_GRPC_ACCEPT_ENCODING_IDENTITY_COMMA_DEFLATE_COMMA_GZIP, GRPC_MDELEM_TE_TRAILERS, GRPC_MDELEM_CONTENT_TYPE_APPLICATION_SLASH_GRPC, @@ -359,11 +424,13 @@ static void BM_HpackParserParseHeader(benchmark::State &state) { p.on_header = UnrefHeader; p.on_header_user_data = nullptr; for (auto slice : init_slices) { - grpc_chttp2_hpack_parser_parse(&exec_ctx, &p, slice); + GPR_ASSERT(GRPC_ERROR_NONE == + grpc_chttp2_hpack_parser_parse(&exec_ctx, &p, slice)); } while (state.KeepRunning()) { for (auto slice : benchmark_slices) { - grpc_chttp2_hpack_parser_parse(&exec_ctx, &p, slice); + GPR_ASSERT(GRPC_ERROR_NONE == + grpc_chttp2_hpack_parser_parse(&exec_ctx, &p, slice)); } grpc_exec_ctx_flush(&exec_ctx); } @@ -376,15 +443,6 @@ static void BM_HpackParserParseHeader(benchmark::State &state) { namespace hpack_parser_fixtures { -static grpc_slice MakeSlice(std::vector bytes) { - grpc_slice s = grpc_slice_malloc(bytes.size()); - uint8_t *p = GRPC_SLICE_START_PTR(s); - for (auto b : bytes) { - *p++ = b; - } - return s; -} - class EmptyBatch { public: static std::vector GetInitSlices() { return {}; } @@ -545,30 +603,51 @@ class NonIndexedBinaryElem<100, false> { class RepresentativeClientInitialMetadata { public: static std::vector GetInitSlices() { - return {grpc_slice_from_static_string( - // generated with: - // ``` - // tools/codegen/core/gen_header_frame.py --compression inc --no_framing - // < test/core/bad_client/tests/simple_request.headers - // ``` - "@\x05:path\x08/foo/bar" - "@\x07:scheme\x04http" - "@\x07:method\x04POST" - "@\x0a:authority\x09localhost" - "@\x0c" - "content-type\x10" - "application/grpc" - "@\x14grpc-accept-encoding\x15identity,deflate,gzip" - "@\x02te\x08trailers" - "@\x0auser-agent\"bad-client grpc-c/0.12.0.0 (linux)")}; + return {MakeSlice( + {0x40, 0x07, ':', 's', 'c', 'h', 'e', 'm', 'e', 0x04, 'h', 't', + 't', 'p', 0x40, 0x07, ':', 'm', 'e', 't', 'h', 'o', 'd', 0x04, + 'P', 'O', 'S', 'T', 0x00, 0x05, ':', 'p', 'a', 't', 'h', 0x1f, + '/', 'g', 'r', 'p', 'c', '.', 't', 'e', 's', 't', '.', 'F', + 'o', 'o', 'S', 'e', 'r', 'v', 'i', 'c', 'e', '/', 'B', 'a', + 'r', 'M', 'e', 't', 'h', 'o', 'd', 0x40, 0x0a, ':', 'a', 'u', + 't', 'h', 'o', 'r', 'i', 't', 'y', 0x09, 'l', 'o', 'c', 'a', + 'l', 'h', 'o', 's', 't', 0x00, 0x0e, 'g', 'r', 'p', 'c', '-', + 't', 'r', 'a', 'c', 'e', '-', 'b', 'i', 'n', 0x31, 0x00, 0x01, + 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, + 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, 0x19, + 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f, 0x20, 0x21, 0x22, 0x23, 0x24, 0x25, + 0x26, 0x27, 0x28, 0x29, 0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f, 0x30, 0x00, + 0x0d, 'g', 'r', 'p', 'c', '-', 't', 'a', 'g', 's', '-', 'b', + 'i', 'n', 0x14, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, + 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13, 0x40, + 0x0c, 'c', 'o', 'n', 't', 'e', 'n', 't', '-', 't', 'y', 'p', + 'e', 0x10, 'a', 'p', 'p', 'l', 'i', 'c', 'a', 't', 'i', 'o', + 'n', '/', 'g', 'r', 'p', 'c', 0x40, 0x14, 'g', 'r', 'p', 'c', + '-', 'a', 'c', 'c', 'e', 'p', 't', '-', 'e', 'n', 'c', 'o', + 'd', 'i', 'n', 'g', 0x15, 'i', 'd', 'e', 'n', 't', 'i', 't', + 'y', ',', 'd', 'e', 'f', 'l', 'a', 't', 'e', ',', 'g', 'z', + 'i', 'p', 0x40, 0x02, 't', 'e', 0x08, 't', 'r', 'a', 'i', 'l', + 'e', 'r', 's', 0x40, 0x0a, 'u', 's', 'e', 'r', '-', 'a', 'g', + 'e', 'n', 't', 0x22, 'b', 'a', 'd', '-', 'c', 'l', 'i', 'e', + 'n', 't', ' ', 'g', 'r', 'p', 'c', '-', 'c', '/', '0', '.', + '1', '2', '.', '0', '.', '0', ' ', '(', 'l', 'i', 'n', 'u', + 'x', ')'})}; } static std::vector GetBenchmarkSlices() { - // generated with: - // ``` - // tools/codegen/core/gen_header_frame.py --compression pre --no_framing - // --hex < test/core/bad_client/tests/simple_request.headers - // ``` - return {MakeSlice({0xc5, 0xc4, 0xc3, 0xc2, 0xc1, 0xc0, 0xbf, 0xbe})}; + return {MakeSlice( + {0xc4, 0xc3, 0x00, 0x05, ':', 'p', 'a', 't', 'h', 0x1f, '/', 'g', + 'r', 'p', 'c', '.', 't', 'e', 's', 't', '.', 'F', 'o', 'o', + 'S', 'e', 'r', 'v', 'i', 'c', 'e', '/', 'B', 'a', 'r', 'M', + 'e', 't', 'h', 'o', 'd', 0xc2, 0x00, 0x0e, 'g', 'r', 'p', 'c', + '-', 't', 'r', 'a', 'c', 'e', '-', 'b', 'i', 'n', 0x31, 0x00, + 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, + 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, + 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f, 0x20, 0x21, 0x22, 0x23, 0x24, + 0x25, 0x26, 0x27, 0x28, 0x29, 0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f, 0x30, + 0x00, 0x0d, 'g', 'r', 'p', 'c', '-', 't', 'a', 'g', 's', '-', + 'b', 'i', 'n', 0x14, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, + 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13, + 0xc1, 0xc0, 0xbf, 0xbe})}; } }; diff --git a/test/cpp/microbenchmarks/helpers.cc b/test/cpp/microbenchmarks/helpers.cc index b0caa48cd08..6802a0aa99b 100644 --- a/test/cpp/microbenchmarks/helpers.cc +++ b/test/cpp/microbenchmarks/helpers.cc @@ -20,6 +20,9 @@ void TrackCounters::Finish(benchmark::State &state) { std::ostringstream out; + for (const auto &l : labels_) { + out << l << ' '; + } AddToLabel(out, state); std::string label = out.str(); if (label.length() && label[0] == ' ') { @@ -28,6 +31,10 @@ void TrackCounters::Finish(benchmark::State &state) { state.SetLabel(label.c_str()); } +void TrackCounters::AddLabel(const grpc::string &label) { + labels_.push_back(label); +} + void TrackCounters::AddToLabel(std::ostream &out, benchmark::State &state) { grpc_stats_data stats_end; grpc_stats_collect(&stats_end); diff --git a/test/cpp/microbenchmarks/helpers.h b/test/cpp/microbenchmarks/helpers.h index 07dd6117098..b6cea7c3170 100644 --- a/test/cpp/microbenchmarks/helpers.h +++ b/test/cpp/microbenchmarks/helpers.h @@ -20,6 +20,7 @@ #define TEST_CPP_MICROBENCHMARKS_COUNTERS_H #include +#include extern "C" { #include @@ -65,10 +66,12 @@ class TrackCounters { public: TrackCounters() { grpc_stats_collect(&stats_begin_); } virtual void Finish(benchmark::State& state); + virtual void AddLabel(const grpc::string& label); virtual void AddToLabel(std::ostream& out, benchmark::State& state); private: grpc_stats_data stats_begin_; + std::vector labels_; #ifdef GPR_LOW_LEVEL_COUNTERS const size_t mu_locks_at_start_ = gpr_atm_no_barrier_load(&gpr_mu_locks); const size_t atm_cas_at_start_ = From c94c7cc5b5d6f745cda257d739bfa9a1b16aa204 Mon Sep 17 00:00:00 2001 From: yang-g Date: Tue, 3 Oct 2017 09:35:38 -0700 Subject: [PATCH 4/5] restore existing fixtures --- test/cpp/microbenchmarks/bm_chttp2_hpack.cc | 63 +++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/test/cpp/microbenchmarks/bm_chttp2_hpack.cc b/test/cpp/microbenchmarks/bm_chttp2_hpack.cc index 92d0da8f69d..8807fa8f490 100644 --- a/test/cpp/microbenchmarks/bm_chttp2_hpack.cc +++ b/test/cpp/microbenchmarks/bm_chttp2_hpack.cc @@ -250,6 +250,31 @@ class SingleNonInternedBinaryElem { }; class RepresentativeClientInitialMetadata { + public: + static constexpr bool kEnableTrueBinary = true; + static std::vector GetElems(grpc_exec_ctx *exec_ctx) { + return { + GRPC_MDELEM_SCHEME_HTTP, GRPC_MDELEM_METHOD_POST, + grpc_mdelem_from_slices( + exec_ctx, GRPC_MDSTR_PATH, + grpc_slice_intern(grpc_slice_from_static_string("/foo/bar"))), + grpc_mdelem_from_slices(exec_ctx, GRPC_MDSTR_AUTHORITY, + grpc_slice_intern(grpc_slice_from_static_string( + "foo.test.google.fr:1234"))), + GRPC_MDELEM_GRPC_ACCEPT_ENCODING_IDENTITY_COMMA_DEFLATE_COMMA_GZIP, + GRPC_MDELEM_TE_TRAILERS, + GRPC_MDELEM_CONTENT_TYPE_APPLICATION_SLASH_GRPC, + grpc_mdelem_from_slices( + exec_ctx, GRPC_MDSTR_USER_AGENT, + grpc_slice_intern(grpc_slice_from_static_string( + "grpc-c/3.0.0-dev (linux; chttp2; green)")))}; + } +}; + +// This fixture reflects how initial metadata are sent by a production client, +// with non-indexed :path and binary headers. The metadata here are the same as +// the corresponding parser benchmark below. +class MoreRepresentativeClientInitialMetadata { public: static constexpr bool kEnableTrueBinary = true; static std::vector GetElems(grpc_exec_ctx *exec_ctx) { @@ -381,6 +406,9 @@ BENCHMARK_TEMPLATE(BM_HpackEncoderEncodeHeader, SingleNonInternedElem) BENCHMARK_TEMPLATE(BM_HpackEncoderEncodeHeader, RepresentativeClientInitialMetadata) ->Args({0, 16384}); +BENCHMARK_TEMPLATE(BM_HpackEncoderEncodeHeader, + MoreRepresentativeClientInitialMetadata) + ->Args({0, 16384}); BENCHMARK_TEMPLATE(BM_HpackEncoderEncodeHeader, RepresentativeServerInitialMetadata) ->Args({0, 16384}); @@ -601,6 +629,39 @@ class NonIndexedBinaryElem<100, false> { }; class RepresentativeClientInitialMetadata { + public: + static std::vector GetInitSlices() { + return {grpc_slice_from_static_string( + // generated with: + // ``` + // tools/codegen/core/gen_header_frame.py --compression inc --no_framing + // < test/core/bad_client/tests/simple_request.headers + // ``` + "@\x05:path\x08/foo/bar" + "@\x07:scheme\x04http" + "@\x07:method\x04POST" + "@\x0a:authority\x09localhost" + "@\x0c" + "content-type\x10" + "application/grpc" + "@\x14grpc-accept-encoding\x15identity,deflate,gzip" + "@\x02te\x08trailers" + "@\x0auser-agent\"bad-client grpc-c/0.12.0.0 (linux)")}; + } + static std::vector GetBenchmarkSlices() { + // generated with: + // ``` + // tools/codegen/core/gen_header_frame.py --compression pre --no_framing + // --hex < test/core/bad_client/tests/simple_request.headers + // ``` + return {MakeSlice({0xc5, 0xc4, 0xc3, 0xc2, 0xc1, 0xc0, 0xbf, 0xbe})}; + } +}; + +// This fixture reflects how initial metadata are sent by a production client, +// with non-indexed :path and binary headers. The metadata here are the same as +// the corresponding encoder benchmark above. +class MoreRepresentativeClientInitialMetadata { public: static std::vector GetInitSlices() { return {MakeSlice( @@ -724,6 +785,8 @@ BENCHMARK_TEMPLATE(BM_HpackParserParseHeader, NonIndexedBinaryElem<31, true>); BENCHMARK_TEMPLATE(BM_HpackParserParseHeader, NonIndexedBinaryElem<100, true>); BENCHMARK_TEMPLATE(BM_HpackParserParseHeader, RepresentativeClientInitialMetadata); +BENCHMARK_TEMPLATE(BM_HpackParserParseHeader, + MoreRepresentativeClientInitialMetadata); BENCHMARK_TEMPLATE(BM_HpackParserParseHeader, RepresentativeServerInitialMetadata); BENCHMARK_TEMPLATE(BM_HpackParserParseHeader, From c010d1d18af34df5fb2a1269f47e0487ff6f8bc6 Mon Sep 17 00:00:00 2001 From: yang-g Date: Fri, 13 Oct 2017 10:14:29 -0700 Subject: [PATCH 5/5] Update benchmark according to new encoding method --- test/cpp/microbenchmarks/bm_chttp2_hpack.cc | 34 +++++++++------------ 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/test/cpp/microbenchmarks/bm_chttp2_hpack.cc b/test/cpp/microbenchmarks/bm_chttp2_hpack.cc index 8807fa8f490..5428cc47e75 100644 --- a/test/cpp/microbenchmarks/bm_chttp2_hpack.cc +++ b/test/cpp/microbenchmarks/bm_chttp2_hpack.cc @@ -64,11 +64,11 @@ BENCHMARK(BM_HpackEncoderInitDestroy); static void BM_HpackEncoderEncodeDeadline(benchmark::State &state) { TrackCounters track_counters; grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; + grpc_millis saved_now = grpc_exec_ctx_now(&exec_ctx); grpc_metadata_batch b; grpc_metadata_batch_init(&b); - b.deadline = gpr_time_add(gpr_now(GPR_CLOCK_REALTIME), - gpr_time_from_seconds(30, GPR_TIMESPAN)); + b.deadline = saved_now + 30 * 1000; grpc_chttp2_hpack_compressor c; grpc_chttp2_hpack_compressor_init(&c); @@ -280,9 +280,9 @@ class MoreRepresentativeClientInitialMetadata { static std::vector GetElems(grpc_exec_ctx *exec_ctx) { return { GRPC_MDELEM_SCHEME_HTTP, GRPC_MDELEM_METHOD_POST, - grpc_mdelem_from_slices( - exec_ctx, GRPC_MDSTR_PATH, - grpc_slice_from_static_string("/grpc.test.FooService/BarMethod")), + grpc_mdelem_from_slices(exec_ctx, GRPC_MDSTR_PATH, + grpc_slice_intern(grpc_slice_from_static_string( + "/grpc.test.FooService/BarMethod"))), grpc_mdelem_from_slices(exec_ctx, GRPC_MDSTR_AUTHORITY, grpc_slice_intern(grpc_slice_from_static_string( "foo.test.google.fr:1234"))), @@ -667,17 +667,17 @@ class MoreRepresentativeClientInitialMetadata { return {MakeSlice( {0x40, 0x07, ':', 's', 'c', 'h', 'e', 'm', 'e', 0x04, 'h', 't', 't', 'p', 0x40, 0x07, ':', 'm', 'e', 't', 'h', 'o', 'd', 0x04, - 'P', 'O', 'S', 'T', 0x00, 0x05, ':', 'p', 'a', 't', 'h', 0x1f, + 'P', 'O', 'S', 'T', 0x40, 0x05, ':', 'p', 'a', 't', 'h', 0x1f, '/', 'g', 'r', 'p', 'c', '.', 't', 'e', 's', 't', '.', 'F', 'o', 'o', 'S', 'e', 'r', 'v', 'i', 'c', 'e', '/', 'B', 'a', 'r', 'M', 'e', 't', 'h', 'o', 'd', 0x40, 0x0a, ':', 'a', 'u', 't', 'h', 'o', 'r', 'i', 't', 'y', 0x09, 'l', 'o', 'c', 'a', - 'l', 'h', 'o', 's', 't', 0x00, 0x0e, 'g', 'r', 'p', 'c', '-', + 'l', 'h', 'o', 's', 't', 0x40, 0x0e, 'g', 'r', 'p', 'c', '-', 't', 'r', 'a', 'c', 'e', '-', 'b', 'i', 'n', 0x31, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f, 0x20, 0x21, 0x22, 0x23, 0x24, 0x25, - 0x26, 0x27, 0x28, 0x29, 0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f, 0x30, 0x00, + 0x26, 0x27, 0x28, 0x29, 0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f, 0x30, 0x40, 0x0d, 'g', 'r', 'p', 'c', '-', 't', 'a', 'g', 's', '-', 'b', 'i', 'n', 0x14, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13, 0x40, @@ -696,19 +696,13 @@ class MoreRepresentativeClientInitialMetadata { } static std::vector GetBenchmarkSlices() { return {MakeSlice( - {0xc4, 0xc3, 0x00, 0x05, ':', 'p', 'a', 't', 'h', 0x1f, '/', 'g', - 'r', 'p', 'c', '.', 't', 'e', 's', 't', '.', 'F', 'o', 'o', - 'S', 'e', 'r', 'v', 'i', 'c', 'e', '/', 'B', 'a', 'r', 'M', - 'e', 't', 'h', 'o', 'd', 0xc2, 0x00, 0x0e, 'g', 'r', 'p', 'c', - '-', 't', 'r', 'a', 'c', 'e', '-', 'b', 'i', 'n', 0x31, 0x00, + {0xc7, 0xc6, 0xc5, 0xc4, 0x7f, 0x04, 0x31, 0x00, 0x01, 0x02, 0x03, 0x04, + 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, + 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, 0x19, 0x1a, 0x1b, 0x1c, + 0x1d, 0x1e, 0x1f, 0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28, + 0x29, 0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f, 0x30, 0x7f, 0x03, 0x14, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, - 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, - 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f, 0x20, 0x21, 0x22, 0x23, 0x24, - 0x25, 0x26, 0x27, 0x28, 0x29, 0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f, 0x30, - 0x00, 0x0d, 'g', 'r', 'p', 'c', '-', 't', 'a', 'g', 's', '-', - 'b', 'i', 'n', 0x14, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, - 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13, - 0xc1, 0xc0, 0xbf, 0xbe})}; + 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13, 0xc1, 0xc0, 0xbf, 0xbe})}; } };