From b2cda1e1853bcf17cf3fc0449f9065698e6f60cb Mon Sep 17 00:00:00 2001 From: Arjun Roy Date: Wed, 17 Jul 2019 11:17:54 -0700 Subject: [PATCH] Reduced ops for grpc_chttp2_stream_map_find(). Several asserts in grpc_chttp2_stream_map_find() can be converted to debug asserts. This PR also templatizes the internal find() method to have it be strict in the delete case (which saves some branches). --- .../chttp2/transport/chttp2_transport.cc | 8 +--- .../ext/transport/chttp2/transport/internal.h | 7 ++- .../transport/chttp2/transport/stream_map.cc | 46 +++++++++++-------- test/core/transport/chttp2/stream_map_test.cc | 24 ---------- 4 files changed, 34 insertions(+), 51 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index aff4506577e..16146569886 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -785,12 +785,6 @@ static void destroy_stream(grpc_transport* gt, grpc_stream* gs, GRPC_ERROR_NONE); } -grpc_chttp2_stream* grpc_chttp2_parsing_lookup_stream(grpc_chttp2_transport* t, - uint32_t id) { - return static_cast( - grpc_chttp2_stream_map_find(&t->stream_map, id)); -} - grpc_chttp2_stream* grpc_chttp2_parsing_accept_stream(grpc_chttp2_transport* t, uint32_t id) { if (t->channel_callback.accept_stream == nullptr) { @@ -2069,7 +2063,7 @@ static void remove_stream(grpc_chttp2_transport* t, uint32_t id, grpc_error* error) { grpc_chttp2_stream* s = static_cast( grpc_chttp2_stream_map_delete(&t->stream_map, id)); - GPR_ASSERT(s); + GPR_DEBUG_ASSERT(s); if (t->incoming_stream == s) { t->incoming_stream = nullptr; grpc_chttp2_parsing_become_skip_parser(t); diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h index 732bc38a0ac..643cd25b8bd 100644 --- a/src/core/ext/transport/chttp2/transport/internal.h +++ b/src/core/ext/transport/chttp2/transport/internal.h @@ -745,8 +745,11 @@ void grpc_chttp2_act_on_flowctl_action( /********* End of Flow Control ***************/ -grpc_chttp2_stream* grpc_chttp2_parsing_lookup_stream(grpc_chttp2_transport* t, - uint32_t id); +inline grpc_chttp2_stream* grpc_chttp2_parsing_lookup_stream( + grpc_chttp2_transport* t, uint32_t id) { + return static_cast( + grpc_chttp2_stream_map_find(&t->stream_map, id)); +} grpc_chttp2_stream* grpc_chttp2_parsing_accept_stream(grpc_chttp2_transport* t, uint32_t id); diff --git a/src/core/ext/transport/chttp2/transport/stream_map.cc b/src/core/ext/transport/chttp2/transport/stream_map.cc index f300e2356c9..647214b94a0 100644 --- a/src/core/ext/transport/chttp2/transport/stream_map.cc +++ b/src/core/ext/transport/chttp2/transport/stream_map.cc @@ -27,7 +27,7 @@ void grpc_chttp2_stream_map_init(grpc_chttp2_stream_map* map, size_t initial_capacity) { - GPR_ASSERT(initial_capacity > 1); + GPR_DEBUG_ASSERT(initial_capacity > 1); map->keys = static_cast(gpr_malloc(sizeof(uint32_t) * initial_capacity)); map->values = @@ -63,9 +63,17 @@ void grpc_chttp2_stream_map_add(grpc_chttp2_stream_map* map, uint32_t key, uint32_t* keys = map->keys; void** values = map->values; + // The first assertion ensures that the table is monotonically increasing. GPR_ASSERT(count == 0 || keys[count - 1] < key); - GPR_ASSERT(value); - GPR_ASSERT(grpc_chttp2_stream_map_find(map, key) == nullptr); + GPR_DEBUG_ASSERT(value); + // Asserting that the key is not already in the map can be a debug assertion. + // Why: we're already checking that the map elements are monotonically + // increasing. If we re-add a key, i.e. if the key is already present, then + // either it is the most recently added key in the map (in which case the + // first assertion fails due to key == last_key) or there is a more recently + // added (larger) key at the end of the map: in which case the first assertion + // still fails due to key < last_key. + GPR_DEBUG_ASSERT(grpc_chttp2_stream_map_find(map, key) == nullptr); if (count == capacity) { if (map->free > capacity / 4) { @@ -74,7 +82,7 @@ void grpc_chttp2_stream_map_add(grpc_chttp2_stream_map* map, uint32_t key, } else { /* resize when less than 25% of the table is free, because compaction won't help much */ - map->capacity = capacity = 3 * capacity / 2; + map->capacity = capacity = 2 * capacity; map->keys = keys = static_cast( gpr_realloc(keys, capacity * sizeof(uint32_t))); map->values = values = @@ -87,6 +95,7 @@ void grpc_chttp2_stream_map_add(grpc_chttp2_stream_map* map, uint32_t key, map->count = count + 1; } +template static void** find(grpc_chttp2_stream_map* map, uint32_t key) { size_t min_idx = 0; size_t max_idx = map->count; @@ -95,7 +104,8 @@ static void** find(grpc_chttp2_stream_map* map, uint32_t key) { void** values = map->values; uint32_t mid_key; - if (max_idx == 0) return nullptr; + GPR_DEBUG_ASSERT(!strict_find || max_idx > 0); + if (!strict_find && max_idx == 0) return nullptr; while (min_idx < max_idx) { /* find the midpoint, avoiding overflow */ @@ -112,28 +122,28 @@ static void** find(grpc_chttp2_stream_map* map, uint32_t key) { } } + GPR_DEBUG_ASSERT(!strict_find); return nullptr; } void* grpc_chttp2_stream_map_delete(grpc_chttp2_stream_map* map, uint32_t key) { - void** pvalue = find(map, key); - void* out = nullptr; - if (pvalue != nullptr) { - out = *pvalue; - *pvalue = nullptr; - map->free += (out != nullptr); - /* recognize complete emptyness and ensure we can skip - * defragmentation later */ - if (map->free == map->count) { - map->free = map->count = 0; - } - GPR_ASSERT(grpc_chttp2_stream_map_find(map, key) == nullptr); + void** pvalue = find(map, key); + GPR_DEBUG_ASSERT(pvalue != nullptr); + void* out = *pvalue; + GPR_DEBUG_ASSERT(out != nullptr); + *pvalue = nullptr; + map->free++; + /* recognize complete emptyness and ensure we can skip + defragmentation later */ + if (map->free == map->count) { + map->free = map->count = 0; } + GPR_DEBUG_ASSERT(grpc_chttp2_stream_map_find(map, key) == nullptr); return out; } void* grpc_chttp2_stream_map_find(grpc_chttp2_stream_map* map, uint32_t key) { - void** pvalue = find(map, key); + void** pvalue = find(map, key); return pvalue != nullptr ? *pvalue : nullptr; } diff --git a/test/core/transport/chttp2/stream_map_test.cc b/test/core/transport/chttp2/stream_map_test.cc index a36c496eb1e..3452c86cdc9 100644 --- a/test/core/transport/chttp2/stream_map_test.cc +++ b/test/core/transport/chttp2/stream_map_test.cc @@ -43,29 +43,6 @@ static void test_empty_find(void) { grpc_chttp2_stream_map_destroy(&map); } -/* test it's safe to delete twice */ -static void test_double_deletion(void) { - grpc_chttp2_stream_map map; - - LOG_TEST("test_double_deletion"); - - grpc_chttp2_stream_map_init(&map, 8); - GPR_ASSERT(0 == grpc_chttp2_stream_map_size(&map)); - grpc_chttp2_stream_map_add(&map, 1, (void*)1); - GPR_ASSERT((void*)1 == grpc_chttp2_stream_map_find(&map, 1)); - GPR_ASSERT(1 == grpc_chttp2_stream_map_size(&map)); - GPR_ASSERT((void*)1 == grpc_chttp2_stream_map_delete(&map, 1)); - GPR_ASSERT(0 == grpc_chttp2_stream_map_size(&map)); - GPR_ASSERT(nullptr == grpc_chttp2_stream_map_find(&map, 1)); - GPR_ASSERT(nullptr == grpc_chttp2_stream_map_delete(&map, 1)); - GPR_ASSERT(nullptr == grpc_chttp2_stream_map_find(&map, 1)); - GPR_ASSERT(nullptr == grpc_chttp2_stream_map_delete(&map, 1)); - GPR_ASSERT(nullptr == grpc_chttp2_stream_map_find(&map, 1)); - GPR_ASSERT(nullptr == grpc_chttp2_stream_map_delete(&map, 1)); - GPR_ASSERT(nullptr == grpc_chttp2_stream_map_find(&map, 1)); - grpc_chttp2_stream_map_destroy(&map); -} - /* test add & lookup */ static void test_basic_add_find(uint32_t n) { grpc_chttp2_stream_map map; @@ -197,7 +174,6 @@ int main(int argc, char** argv) { test_no_op(); test_empty_find(); - test_double_deletion(); while (n < 100000) { test_basic_add_find(n);