Merge pull request #1993 from ctiller/break-break-break-the-locks

Cache commonly used grpc-status mdelems

src/core/surface/call.c

@@ -820,7 +820,6 @@ static int fill_send_ops(grpc_call *call, grpc_transport_op *op) {
   grpc_ioreq_data data;
   grpc_metadata_batch mdb;
   size_t i;
-  char status_str[GPR_LTOA_MIN_BUFSIZE];
   GPR_ASSERT(op->send_ops == NULL);
 
   switch (call->write_state) {
@@ -865,13 +864,10 @@ static int fill_send_ops(grpc_call *call, grpc_transport_op *op) {
           /* send status */
           /* TODO(ctiller): cache common status values */
           data = call->request_data[GRPC_IOREQ_SEND_STATUS];
-          gpr_ltoa(data.send_status.code, status_str);
           grpc_metadata_batch_add_tail(
               &mdb, &call->status_link,
-              grpc_mdelem_from_metadata_strings(
-                  call->metadata_context,
-                  grpc_mdstr_ref(grpc_channel_get_status_string(call->channel)),
-                  grpc_mdstr_from_string(call->metadata_context, status_str)));
+              grpc_channel_get_reffed_status_elem(call->channel,
+                                                  data.send_status.code));
           if (data.send_status.details) {
             grpc_metadata_batch_add_tail(
                 &mdb, &call->details_link,

src/core/surface/channel.c

@@ -37,12 +37,20 @@
 #include <string.h>
 
 #include "src/core/iomgr/iomgr.h"
+#include "src/core/support/string.h"
 #include "src/core/surface/call.h"
 #include "src/core/surface/client.h"
 #include "src/core/surface/init.h"
 #include <grpc/support/alloc.h>
 #include <grpc/support/log.h>
 
+/** Cache grpc-status: X mdelems for X = 0..NUM_CACHED_STATUS_ELEMS.
+ *  Avoids needing to take a metadata context lock for sending status
+ *  if the status code is <= NUM_CACHED_STATUS_ELEMS.
+ *  Sized to allow the most commonly used codes to fit in
+ *  (OK, Cancelled, Unknown). */
+#define NUM_CACHED_STATUS_ELEMS 3
+
 typedef struct registered_call {
   grpc_mdelem *path;
   grpc_mdelem *authority;
@@ -54,10 +62,13 @@ struct grpc_channel {
   gpr_refcount refs;
   gpr_uint32 max_message_length;
   grpc_mdctx *metadata_context;
+  /** mdstr for the grpc-status key */
   grpc_mdstr *grpc_status_string;
   grpc_mdstr *grpc_message_string;
   grpc_mdstr *path_string;
   grpc_mdstr *authority_string;
+  /** mdelem for grpc-status: 0 thru grpc-status: 2 */
+  grpc_mdelem *grpc_status_elem[NUM_CACHED_STATUS_ELEMS];
 
   gpr_mu registered_call_mu;
   registered_call *registered_calls;
@@ -88,6 +99,13 @@ grpc_channel *grpc_channel_create_from_filters(
   channel->metadata_context = mdctx;
   channel->grpc_status_string = grpc_mdstr_from_string(mdctx, "grpc-status");
   channel->grpc_message_string = grpc_mdstr_from_string(mdctx, "grpc-message");
+  for (i = 0; i < NUM_CACHED_STATUS_ELEMS; i++) {
+    char buf[GPR_LTOA_MIN_BUFSIZE];
+    gpr_ltoa(i, buf);
+    channel->grpc_status_elem[i] = grpc_mdelem_from_metadata_strings(
+        mdctx, grpc_mdstr_ref(channel->grpc_status_string),
+        grpc_mdstr_from_string(mdctx, buf));
+  }
   channel->path_string = grpc_mdstr_from_string(mdctx, ":path");
   channel->authority_string = grpc_mdstr_from_string(mdctx, ":authority");
   grpc_channel_stack_init(filters, num_filters, args, channel->metadata_context,
@@ -175,7 +193,11 @@ void grpc_channel_internal_ref(grpc_channel *channel) {
 
 static void destroy_channel(void *p, int ok) {
   grpc_channel *channel = p;
+  size_t i;
   grpc_channel_stack_destroy(CHANNEL_STACK_FROM_CHANNEL(channel));
+  for (i = 0; i < NUM_CACHED_STATUS_ELEMS; i++) {
+    grpc_mdelem_unref(channel->grpc_status_elem[i]);
+  }
   grpc_mdstr_unref(channel->grpc_status_string);
   grpc_mdstr_unref(channel->grpc_message_string);
   grpc_mdstr_unref(channel->path_string);
@@ -235,6 +257,18 @@ grpc_mdstr *grpc_channel_get_status_string(grpc_channel *channel) {
   return channel->grpc_status_string;
 }
 
+grpc_mdelem *grpc_channel_get_reffed_status_elem(grpc_channel *channel, int i) {
+  if (i >= 0 && i < NUM_CACHED_STATUS_ELEMS) {
+    return grpc_mdelem_ref(channel->grpc_status_elem[i]);
+  } else {
+    char tmp[GPR_LTOA_MIN_BUFSIZE];
+    gpr_ltoa(i, tmp);
+    return grpc_mdelem_from_metadata_strings(
+        channel->metadata_context, grpc_mdstr_ref(channel->grpc_status_string),
+        grpc_mdstr_from_string(channel->metadata_context, tmp));
+  }
+}
+
 grpc_mdstr *grpc_channel_get_message_string(grpc_channel *channel) {
   return channel->grpc_message_string;
 }

src/core/surface/channel.h

@@ -40,8 +40,18 @@ grpc_channel *grpc_channel_create_from_filters(
     const grpc_channel_filter **filters, size_t count,
     const grpc_channel_args *args, grpc_mdctx *mdctx, int is_client);
 
+/** Get a (borrowed) pointer to this channels underlying channel stack */
 grpc_channel_stack *grpc_channel_get_channel_stack(grpc_channel *channel);
+
+/** Get a (borrowed) pointer to the channel wide metadata context */
 grpc_mdctx *grpc_channel_get_metadata_context(grpc_channel *channel);
+
+/** Get a grpc_mdelem of grpc-status: X where X is the numeric value of
+    status_code.
+
+    The returned elem is owned by the caller. */
+grpc_mdelem *grpc_channel_get_reffed_status_elem(grpc_channel *channel,
+                                                 int status_code);
 grpc_mdstr *grpc_channel_get_status_string(grpc_channel *channel);
 grpc_mdstr *grpc_channel_get_message_string(grpc_channel *channel);
 gpr_uint32 grpc_channel_get_max_message_length(grpc_channel *channel);

src/core/transport/metadata.c

@@ -120,7 +120,7 @@ static void unlock(grpc_mdctx *ctx) {
   if (ctx->refs == 0) {
     /* uncomment if you're having trouble diagnosing an mdelem leak to make
        things clearer (slows down destruction a lot, however) */
-    gc_mdtab(ctx);
+    /* gc_mdtab(ctx); */
     if (ctx->mdtab_count && ctx->mdtab_count == ctx->mdtab_free) {
       discard_metadata(ctx);
     }