Merge pull request #523 from ctiller/sopb

Avoid four allocations per stream

Makefile

@@ -69,7 +69,7 @@ CC_msan = clang
 CXX_msan = clang++-libc++
 LD_msan = clang
 LDXX_msan = clang++-libc++
-CPPFLAGS_msan = -O1 -fsanitize=memory -fno-omit-frame-pointer -DGTEST_HAS_TR1_TUPLE=0 -DGTEST_USE_OWN_TR1_TUPLE=1
+CPPFLAGS_msan = -O1 -fsanitize=memory -fsanitize-memory-track-origins -fno-omit-frame-pointer -DGTEST_HAS_TR1_TUPLE=0 -DGTEST_USE_OWN_TR1_TUPLE=1
 OPENSSL_CFLAGS_msan = -DPURIFY
 OPENSSL_CONFIG_msan = no-asm
 LDFLAGS_msan = -fsanitize=memory -DGTEST_HAS_TR1_TUPLE=0 -DGTEST_USE_OWN_TR1_TUPLE=1

src/core/transport/chttp2_transport.c

@@ -684,10 +684,11 @@ static void unlock(transport *t) {
   int i;
   pending_goaway *goaways = NULL;
   grpc_endpoint *ep = t->ep;
-  grpc_stream_op_buffer nuke_now = t->nuke_later_sopb;
+  grpc_stream_op_buffer nuke_now;
   
-  if (nuke_now.nops) {
-    memset(&t->nuke_later_sopb, 0, sizeof(t->nuke_later_sopb));
+  grpc_sopb_init(&nuke_now);
+  if (t->nuke_later_sopb.nops) {
+    grpc_sopb_swap(&nuke_now, &t->nuke_later_sopb);
   }
 
   /* see if we need to trigger a write - and if so, get the data ready */
@@ -757,9 +758,7 @@ static void unlock(transport *t) {
     unref_transport(t);
   }
 
-  if (nuke_now.nops) {
-    grpc_sopb_destroy(&nuke_now);
-  }
+  grpc_sopb_destroy(&nuke_now);
 
   gpr_free(goaways);
 }
@@ -1698,13 +1697,10 @@ static grpc_stream_state compute_state(gpr_uint8 write_closed,
 
 static int prepare_callbacks(transport *t) {
   stream *s;
-  grpc_stream_op_buffer temp_sopb;
   int n = 0;
   while ((s = stream_list_remove_head(t, PENDING_CALLBACKS))) {
     int execute = 1;
-    temp_sopb = s->parser.incoming_sopb;
-    s->parser.incoming_sopb = s->callback_sopb;
-    s->callback_sopb = temp_sopb;
+    grpc_sopb_swap(&s->parser.incoming_sopb, &s->callback_sopb);
 
     s->callback_state = compute_state(s->sent_write_closed, s->read_closed);
     if (s->callback_state == GRPC_STREAM_CLOSED) {

src/core/transport/stream_op.c

@@ -38,23 +38,19 @@
 
 #include <string.h>
 
-/* Initial number of operations to allocate */
-#define INITIAL_SLOTS 8
 /* Exponential growth function: Given x, return a larger x.
-   Currently we grow by 1.5 times upon reallocation.
-   Assumes INITIAL_SLOTS > 1 */
+   Currently we grow by 1.5 times upon reallocation. */
 #define GROW(x) (3 * (x) / 2)
 
 void grpc_sopb_init(grpc_stream_op_buffer *sopb) {
-  sopb->ops = gpr_malloc(sizeof(grpc_stream_op) * INITIAL_SLOTS);
-  GPR_ASSERT(sopb->ops);
+  sopb->ops = sopb->inlined_ops;
   sopb->nops = 0;
-  sopb->capacity = INITIAL_SLOTS;
+  sopb->capacity = GRPC_SOPB_INLINE_ELEMENTS;
 }
 
 void grpc_sopb_destroy(grpc_stream_op_buffer *sopb) {
   grpc_stream_ops_unref_owned_objects(sopb->ops, sopb->nops);
-  gpr_free(sopb->ops);
+  if (sopb->ops != sopb->inlined_ops) gpr_free(sopb->ops);
 }
 
 void grpc_sopb_reset(grpc_stream_op_buffer *sopb) {
@@ -62,6 +58,19 @@ void grpc_sopb_reset(grpc_stream_op_buffer *sopb) {
   sopb->nops = 0;
 }
 
+void grpc_sopb_swap(grpc_stream_op_buffer *a, grpc_stream_op_buffer *b) {
+  grpc_stream_op_buffer temp = *a;
+  *a = *b;
+  *b = temp;
+
+  if (a->ops == b->inlined_ops) {
+    a->ops = a->inlined_ops;
+  }
+  if (b->ops == a->inlined_ops) {
+    b->ops = b->inlined_ops;
+  }
+}
+
 void grpc_stream_ops_unref_owned_objects(grpc_stream_op *ops, size_t nops) {
   size_t i;
   for (i = 0; i < nops; i++) {
@@ -84,17 +93,21 @@ void grpc_stream_ops_unref_owned_objects(grpc_stream_op *ops, size_t nops) {
   }
 }
 
-static void expand(grpc_stream_op_buffer *sopb) {
-  sopb->capacity = GROW(sopb->capacity);
-  sopb->ops = gpr_realloc(sopb->ops, sizeof(grpc_stream_op) * sopb->capacity);
-  GPR_ASSERT(sopb->ops);
+static void expandto(grpc_stream_op_buffer *sopb, size_t new_capacity) {
+  sopb->capacity = new_capacity;
+  if (sopb->ops == sopb->inlined_ops) {
+    sopb->ops = gpr_malloc(sizeof(grpc_stream_op) * new_capacity);
+    memcpy(sopb->ops, sopb->inlined_ops, sopb->nops * sizeof(grpc_stream_op));
+  } else {
+    sopb->ops = gpr_realloc(sopb->ops, sizeof(grpc_stream_op) * new_capacity);
+  }
 }
 
 static grpc_stream_op *add(grpc_stream_op_buffer *sopb) {
   grpc_stream_op *out;
 
   if (sopb->nops == sopb->capacity) {
-    expand(sopb);
+    expandto(sopb, GROW(sopb->capacity));
   }
   out = sopb->ops + sopb->nops;
   sopb->nops++;
@@ -152,12 +165,7 @@ void grpc_sopb_append(grpc_stream_op_buffer *sopb, grpc_stream_op *ops,
   size_t new_nops = orig_nops + nops;
 
   if (new_nops > sopb->capacity) {
-    size_t new_capacity = GROW(sopb->capacity);
-    if (new_capacity < new_nops) {
-      new_capacity = new_nops;
-    }
-    sopb->ops = gpr_realloc(sopb->ops, sizeof(grpc_stream_op) * new_capacity);
-    sopb->capacity = new_capacity;
+    expandto(sopb, GPR_MAX(GROW(sopb->capacity), new_nops));
   }
 
   memcpy(sopb->ops + orig_nops, ops, sizeof(grpc_stream_op) * nops);

src/core/transport/stream_op.h

@@ -40,6 +40,9 @@
 #include <grpc/support/time.h>
 #include "src/core/transport/metadata.h"
 
+/* this many stream ops are inlined into a sopb before allocating */
+#define GRPC_SOPB_INLINE_ELEMENTS 16
+
 /* Operations that can be performed on a stream.
    Used by grpc_stream_op. */
 typedef enum grpc_stream_op_code {
@@ -96,6 +99,7 @@ typedef struct grpc_stream_op_buffer {
   grpc_stream_op *ops;
   size_t nops;
   size_t capacity;
+  grpc_stream_op inlined_ops[GRPC_SOPB_INLINE_ELEMENTS];
 } grpc_stream_op_buffer;
 
 /* Initialize a stream op buffer */
@@ -104,6 +108,8 @@ void grpc_sopb_init(grpc_stream_op_buffer *sopb);
 void grpc_sopb_destroy(grpc_stream_op_buffer *sopb);
 /* Reset a sopb to no elements */
 void grpc_sopb_reset(grpc_stream_op_buffer *sopb);
+/* Swap two sopbs */
+void grpc_sopb_swap(grpc_stream_op_buffer *a, grpc_stream_op_buffer *b);
 
 void grpc_stream_ops_unref_owned_objects(grpc_stream_op *ops, size_t nops);
 

templates/Makefile.template

@@ -86,7 +86,7 @@ CC_msan = clang
 CXX_msan = clang++-libc++
 LD_msan = clang
 LDXX_msan = clang++-libc++
-CPPFLAGS_msan = -O1 -fsanitize=memory -fno-omit-frame-pointer -DGTEST_HAS_TR1_TUPLE=0 -DGTEST_USE_OWN_TR1_TUPLE=1
+CPPFLAGS_msan = -O1 -fsanitize=memory -fsanitize-memory-track-origins -fno-omit-frame-pointer -DGTEST_HAS_TR1_TUPLE=0 -DGTEST_USE_OWN_TR1_TUPLE=1
 OPENSSL_CFLAGS_msan = -DPURIFY
 OPENSSL_CONFIG_msan = no-asm
 LDFLAGS_msan = -fsanitize=memory -DGTEST_HAS_TR1_TUPLE=0 -DGTEST_USE_OWN_TR1_TUPLE=1