Merge pull request #19672 from arjunroy/stream_find_debug_assert

Reduced ops for grpc_chttp2_stream_map_find().

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*>(
-      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*>(
       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);

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*>(
+      grpc_chttp2_stream_map_find(&t->stream_map, id));
+}
 grpc_chttp2_stream* grpc_chttp2_parsing_accept_stream(grpc_chttp2_transport* t,
                                                       uint32_t id);
 

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<uint32_t*>(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<uint32_t*>(
           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 <bool strict_find>
 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<true>(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<false>(map, key);
   return pvalue != nullptr ? *pvalue : nullptr;
 }
 

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);