Error arena optimization

src/core/lib/iomgr/error.c

@@ -35,6 +35,7 @@
 
 #include <string.h>
 
+#include <grpc/slice.h>
 #include <grpc/status.h>
 #include <grpc/support/alloc.h>
 #include <grpc/support/log.h>
@@ -47,46 +48,7 @@
 
 #include "src/core/lib/iomgr/error_internal.h"
 #include "src/core/lib/profiling/timers.h"
-
+#include "src/core/lib/slice/slice_internal.h"
-static void destroy_integer(void *key) {}
-
-static void *copy_integer(void *key) { return key; }
-
-static long compare_integers(void *key1, void *key2) {
-  return GPR_ICMP((uintptr_t)key1, (uintptr_t)key2);
-}
-
-static void destroy_string(void *str) { gpr_free(str); }
-
-static void *copy_string(void *str) { return gpr_strdup(str); }
-
-static void destroy_err(void *err) { GRPC_ERROR_UNREF(err); }
-
-static void *copy_err(void *err) { return GRPC_ERROR_REF(err); }
-
-static void destroy_time(void *tm) { gpr_free(tm); }
-
-static gpr_timespec *box_time(gpr_timespec tm) {
-  gpr_timespec *out = gpr_malloc(sizeof(*out));
-  *out = tm;
-  return out;
-}
-
-static void *copy_time(void *tm) { return box_time(*(gpr_timespec *)tm); }
-
-static const gpr_avl_vtable avl_vtable_ints = {destroy_integer, copy_integer,
-                                               compare_integers,
-                                               destroy_integer, copy_integer};
-
-static const gpr_avl_vtable avl_vtable_strs = {destroy_integer, copy_integer,
-                                               compare_integers, destroy_string,
-                                               copy_string};
-
-static const gpr_avl_vtable avl_vtable_times = {
-    destroy_integer, copy_integer, compare_integers, destroy_time, copy_time};
-
-static const gpr_avl_vtable avl_vtable_errs = {
-    destroy_integer, copy_integer, compare_integers, destroy_err, copy_err};
 
 static const char *error_int_name(grpc_error_ints key) {
   switch (key) {
@@ -120,6 +82,8 @@ static const char *error_int_name(grpc_error_ints key) {
       return "limit";
     case GRPC_ERROR_INT_OCCURRED_DURING_WRITE:
       return "occurred_during_write";
+    case GRPC_ERROR_INT_MAX:
+      GPR_UNREACHABLE_CODE(return "unknown");
   }
   GPR_UNREACHABLE_CODE(return "unknown");
 }
@@ -150,6 +114,8 @@ static const char *error_str_name(grpc_error_strs key) {
       return "filename";
     case GRPC_ERROR_STR_QUEUED_BUFFERS:
       return "queued_buffers";
+    case GRPC_ERROR_STR_MAX:
+      GPR_UNREACHABLE_CODE(return "unknown");
   }
   GPR_UNREACHABLE_CODE(return "unknown");
 }
@@ -158,6 +124,8 @@ static const char *error_time_name(grpc_error_times key) {
   switch (key) {
     case GRPC_ERROR_TIME_CREATED:
       return "created";
+    case GRPC_ERROR_TIME_MAX:
+      GPR_UNREACHABLE_CODE(return "unknown");
   }
   GPR_UNREACHABLE_CODE(return "unknown");
 }
@@ -184,12 +152,36 @@ grpc_error *grpc_error_ref(grpc_error *err) {
 }
 #endif
 
+static void unref_errs(grpc_error *err) {
+  uint8_t slot = err->first_err;
+  while (slot != UINT8_MAX) {
+    linked_error *lerr = (linked_error *)(err->arena + slot);
+    GRPC_ERROR_UNREF(lerr->err);
+    GPR_ASSERT(err->last_err == slot ? lerr->next == UINT8_MAX
+                                     : lerr->next != UINT8_MAX);
+    slot = lerr->next;
+  }
+}
+
+static void unref_slice(grpc_slice slice) {
+  grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT;
+  grpc_slice_unref_internal(&exec_ctx, slice);
+  grpc_exec_ctx_finish(&exec_ctx);
+}
+
+static void unref_strs(grpc_error *err) {
+  for (size_t which = 0; which < GRPC_ERROR_STR_MAX; ++which) {
+    uint8_t slot = err->strs[which];
+    if (slot != UINT8_MAX) {
+      unref_slice(*(grpc_slice *)(err->arena + slot));
+    }
+  }
+}
+
 static void error_destroy(grpc_error *err) {
   GPR_ASSERT(!grpc_error_is_special(err));
-  gpr_avl_unref(err->ints);
+  unref_errs(err);
-  gpr_avl_unref(err->strs);
+  unref_strs(err);
-  gpr_avl_unref(err->errs);
-  gpr_avl_unref(err->times);
   gpr_free((void *)gpr_atm_acq_load(&err->error_string));
   gpr_free(err);
 }
@@ -213,69 +205,188 @@ void grpc_error_unref(grpc_error *err) {
 }
 #endif
 
+static uint8_t get_placement(grpc_error **err, size_t size) {
+  GPR_ASSERT(*err);
+  uint8_t slots = (uint8_t)(size / sizeof(intptr_t));
+  if ((*err)->arena_size + slots > (*err)->arena_capacity) {
+    (*err)->arena_capacity = (uint8_t)(3 * (*err)->arena_capacity / 2);
+    *err = realloc(
+        *err, sizeof(grpc_error) + (*err)->arena_capacity * sizeof(intptr_t));
+  }
+  uint8_t placement = (*err)->arena_size;
+  (*err)->arena_size = (uint8_t)((*err)->arena_size + slots);
+  return placement;
+}
+
+static void internal_set_int(grpc_error **err, grpc_error_ints which,
+                             intptr_t value) {
+  // GPR_ASSERT((*err)->ints[which] == UINT8_MAX); // TODO, enforce this
+  uint8_t slot = (*err)->ints[which];
+  if (slot == UINT8_MAX) {
+    slot = get_placement(err, sizeof(value));
+  }
+  (*err)->ints[which] = slot;
+  (*err)->arena[slot] = value;
+}
+
+static void internal_set_str(grpc_error **err, grpc_error_strs which,
+                             grpc_slice value) {
+  // GPR_ASSERT((*err)->strs[which] == UINT8_MAX); // TODO, enforce this
+  uint8_t slot = (*err)->strs[which];
+  if (slot == UINT8_MAX) {
+    slot = get_placement(err, sizeof(value));
+  } else {
+    unref_slice(*(grpc_slice *)((*err)->arena + slot));
+  }
+  (*err)->strs[which] = slot;
+  memcpy((*err)->arena + slot, &value, sizeof(value));
+}
+
+static void internal_set_time(grpc_error **err, grpc_error_times which,
+                              gpr_timespec value) {
+  // GPR_ASSERT((*err)->times[which] == UINT8_MAX); // TODO, enforce this
+  uint8_t slot = (*err)->times[which];
+  if (slot == UINT8_MAX) {
+    slot = get_placement(err, sizeof(value));
+  }
+  (*err)->times[which] = slot;
+  memcpy((*err)->arena + slot, &value, sizeof(value));
+}
+
+static void internal_add_error(grpc_error **err, grpc_error *new) {
+  linked_error new_last = {new, UINT8_MAX};
+  uint8_t slot = get_placement(err, sizeof(linked_error));
+  if ((*err)->first_err == UINT8_MAX) {
+    GPR_ASSERT((*err)->last_err == UINT8_MAX);
+    (*err)->last_err = slot;
+    (*err)->first_err = slot;
+  } else {
+    GPR_ASSERT((*err)->last_err != UINT8_MAX);
+    linked_error *old_last = (linked_error *)((*err)->arena + (*err)->last_err);
+    old_last->next = slot;
+    (*err)->last_err = slot;
+  }
+  memcpy((*err)->arena + slot, &new_last, sizeof(linked_error));
+}
+
+#define SLOTS_PER_INT (sizeof(intptr_t) / sizeof(intptr_t))
+#define SLOTS_PER_STR (sizeof(grpc_slice) / sizeof(intptr_t))
+#define SLOTS_PER_TIME (sizeof(gpr_timespec) / sizeof(intptr_t))
+#define SLOTS_PER_LINKED_ERROR (sizeof(linked_error) / sizeof(intptr_t))
+
+// size of storing one int and two slices and a timespec. For line, desc, file,
+// and time created
+#define DEFAULT_ERROR_CAPACITY \
+  (SLOTS_PER_INT + (SLOTS_PER_STR * 2) + SLOTS_PER_TIME)
+
+// It is very common to include and extra int and string in an error
+#define SURPLUS_CAPACITY (2 * SLOTS_PER_INT + SLOTS_PER_TIME)
+
 grpc_error *grpc_error_create(const char *file, int line, const char *desc,
                               grpc_error **referencing,
                               size_t num_referencing) {
   GPR_TIMER_BEGIN("grpc_error_create", 0);
-  grpc_error *err = gpr_malloc(sizeof(*err));
+  uint8_t initial_arena_capacity = (uint8_t)(
+      DEFAULT_ERROR_CAPACITY +
+      (uint8_t)(num_referencing * SLOTS_PER_LINKED_ERROR) + SURPLUS_CAPACITY);
+  grpc_error *err =
+      gpr_malloc(sizeof(*err) + initial_arena_capacity * sizeof(intptr_t));
   if (err == NULL) {  // TODO(ctiller): make gpr_malloc return NULL
     return GRPC_ERROR_OOM;
   }
 #ifdef GRPC_ERROR_REFCOUNT_DEBUG
   gpr_log(GPR_DEBUG, "%p create [%s:%d]", err, file, line);
 #endif
-  err->ints = gpr_avl_add(gpr_avl_create(&avl_vtable_ints),
+
-                          (void *)(uintptr_t)GRPC_ERROR_INT_FILE_LINE,
+  err->arena_size = 0;
-                          (void *)(uintptr_t)line);
+  err->arena_capacity = initial_arena_capacity;
-  err->strs = gpr_avl_add(
+  err->first_err = UINT8_MAX;
-      gpr_avl_add(gpr_avl_create(&avl_vtable_strs),
+  err->last_err = UINT8_MAX;
-                  (void *)(uintptr_t)GRPC_ERROR_STR_FILE, gpr_strdup(file)),
+
-      (void *)(uintptr_t)GRPC_ERROR_STR_DESCRIPTION, gpr_strdup(desc));
+  memset(err->ints, UINT8_MAX, GRPC_ERROR_INT_MAX);
-  err->errs = gpr_avl_create(&avl_vtable_errs);
+  memset(err->strs, UINT8_MAX, GRPC_ERROR_STR_MAX);
-  err->next_err = 0;
+  memset(err->times, UINT8_MAX, GRPC_ERROR_TIME_MAX);
-  for (size_t i = 0; i < num_referencing; i++) {
+
+  internal_set_int(&err, GRPC_ERROR_INT_FILE_LINE, line);
+  internal_set_str(&err, GRPC_ERROR_STR_FILE,
+                   grpc_slice_from_static_string(file));
+  internal_set_str(
+      &err, GRPC_ERROR_STR_DESCRIPTION,
+      grpc_slice_from_copied_buffer(
+          desc,
+          strlen(desc) +
+              1));  // TODO, pull this up.  // TODO(ncteisen), pull this up.
+
+  for (size_t i = 0; i < num_referencing; ++i) {
     if (referencing[i] == GRPC_ERROR_NONE) continue;
-    err->errs = gpr_avl_add(err->errs, (void *)(err->next_err++),
+    internal_add_error(
-                            GRPC_ERROR_REF(referencing[i]));
+        &err,
+        GRPC_ERROR_REF(
+            referencing[i]));  // TODO(ncteisen), change ownership semantics
   }
-  err->times = gpr_avl_add(gpr_avl_create(&avl_vtable_times),
+
-                           (void *)(uintptr_t)GRPC_ERROR_TIME_CREATED,
+  internal_set_time(&err, GRPC_ERROR_TIME_CREATED, gpr_now(GPR_CLOCK_REALTIME));
-                           box_time(gpr_now(GPR_CLOCK_REALTIME)));
+
   gpr_atm_no_barrier_store(&err->error_string, 0);
   gpr_ref_init(&err->refs, 1);
   GPR_TIMER_END("grpc_error_create", 0);
   return err;
 }
 
+static void ref_strs(grpc_error *err) {
+  for (size_t i = 0; i < GRPC_ERROR_STR_MAX; ++i) {
+    uint8_t slot = err->strs[i];
+    if (slot != UINT8_MAX) {
+      grpc_slice_ref_internal(*(grpc_slice *)(err->arena + slot));
+    }
+  }
+}
+
+static void ref_errs(grpc_error *err) {
+  uint8_t slot = err->first_err;
+  while (slot != UINT8_MAX) {
+    linked_error *lerr = (linked_error *)(err->arena + slot);
+    GRPC_ERROR_REF(lerr->err);
+    slot = lerr->next;
+  }
+}
+
 static grpc_error *copy_error_and_unref(grpc_error *in) {
   GPR_TIMER_BEGIN("copy_error_and_unref", 0);
   grpc_error *out;
   if (grpc_error_is_special(in)) {
-    if (in == GRPC_ERROR_NONE)
-      out = grpc_error_set_int(GRPC_ERROR_CREATE("no error"),
-                               GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_OK);
-    else if (in == GRPC_ERROR_OOM)
-      out = GRPC_ERROR_CREATE("oom");
-    else if (in == GRPC_ERROR_CANCELLED)
-      out =
-          grpc_error_set_int(GRPC_ERROR_CREATE("cancelled"),
-                             GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_CANCELLED);
-    else
     out = GRPC_ERROR_CREATE("unknown");
+    if (in == GRPC_ERROR_NONE) {
+      internal_set_str(&out, GRPC_ERROR_STR_DESCRIPTION,
+                       grpc_slice_from_static_string("no error"));
+      internal_set_int(&out, GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_OK);
+    } else if (in == GRPC_ERROR_OOM) {
+      internal_set_str(&out, GRPC_ERROR_STR_DESCRIPTION,
+                       grpc_slice_from_static_string("oom"));
+    } else if (in == GRPC_ERROR_CANCELLED) {
+      internal_set_str(&out, GRPC_ERROR_STR_DESCRIPTION,
+                       grpc_slice_from_static_string("cancelled"));
+      internal_set_int(&out, GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_CANCELLED);
+    }
   } else if (gpr_ref_is_unique(&in->refs)) {
     return in;
   } else {
-    out = gpr_malloc(sizeof(*out));
+    uint8_t new_arena_capacity = in->arena_capacity;
+    // the returned err will be added to, so we ensure this is room to avoid
+    // unneeded allocations.
+    if (in->arena_capacity - in->arena_size < (uint8_t)SLOTS_PER_STR) {
+      new_arena_capacity = (uint8_t)(3 * new_arena_capacity / 2);
+    }
+    out = gpr_malloc(sizeof(*in) + new_arena_capacity * sizeof(intptr_t));
 #ifdef GRPC_ERROR_REFCOUNT_DEBUG
     gpr_log(GPR_DEBUG, "%p create copying %p", out, in);
 #endif
-    out->ints = gpr_avl_ref(in->ints);
+    memcpy(out, in, sizeof(*in) + in->arena_size * sizeof(intptr_t));
-    out->strs = gpr_avl_ref(in->strs);
+    out->arena_capacity = new_arena_capacity;
-    out->errs = gpr_avl_ref(in->errs);
-    out->times = gpr_avl_ref(in->times);
     gpr_atm_no_barrier_store(&out->error_string, 0);
-    out->next_err = in->next_err;
     gpr_ref_init(&out->refs, 1);
+    ref_strs(out);
+    ref_errs(out);
     GRPC_ERROR_UNREF(in);
   }
   GPR_TIMER_END("copy_error_and_unref", 0);
@@ -286,7 +397,7 @@ grpc_error *grpc_error_set_int(grpc_error *src, grpc_error_ints which,
                                intptr_t value) {
   GPR_TIMER_BEGIN("grpc_error_set_int", 0);
   grpc_error *new = copy_error_and_unref(src);
-  new->ints = gpr_avl_add(new->ints, (void *)(uintptr_t)which, (void *)value);
+  internal_set_int(&new, which, value);
   GPR_TIMER_END("grpc_error_set_int", 0);
   return new;
 }
@@ -304,7 +415,6 @@ static special_error_status_map error_status_map[] = {
 
 bool grpc_error_get_int(grpc_error *err, grpc_error_ints which, intptr_t *p) {
   GPR_TIMER_BEGIN("grpc_error_get_int", 0);
-  void *pp;
   if (grpc_error_is_special(err)) {
     if (which == GRPC_ERROR_INT_GRPC_STATUS) {
       for (size_t i = 0; i < GPR_ARRAY_SIZE(error_status_map); i++) {
@@ -318,8 +428,9 @@ bool grpc_error_get_int(grpc_error *err, grpc_error_ints which, intptr_t *p) {
     GPR_TIMER_END("grpc_error_get_int", 0);
     return false;
   }
-  if (gpr_avl_maybe_get(err->ints, (void *)(uintptr_t)which, &pp)) {
+  uint8_t slot = err->ints[which];
-    if (p != NULL) *p = (intptr_t)pp;
+  if (slot != UINT8_MAX) {
+    if (p != NULL) *p = err->arena[slot];
     GPR_TIMER_END("grpc_error_get_int", 0);
     return true;
   }
@@ -331,8 +442,9 @@ grpc_error *grpc_error_set_str(grpc_error *src, grpc_error_strs which,
                                const char *value) {
   GPR_TIMER_BEGIN("grpc_error_set_str", 0);
   grpc_error *new = copy_error_and_unref(src);
-  new->strs =
+  internal_set_str(&new, which,
-      gpr_avl_add(new->strs, (void *)(uintptr_t)which, gpr_strdup(value));
+                   grpc_slice_from_copied_buffer(
+                       value, strlen(value) + 1));  // TODO, pull this up.
   GPR_TIMER_END("grpc_error_set_str", 0);
   return new;
 }
@@ -348,13 +460,19 @@ const char *grpc_error_get_str(grpc_error *err, grpc_error_strs which) {
     }
     return NULL;
   }
-  return gpr_avl_get(err->strs, (void *)(uintptr_t)which);
+  uint8_t slot = err->strs[which];
+  if (slot != UINT8_MAX) {
+    return (const char *)GRPC_SLICE_START_PTR(
+        *(grpc_slice *)(err->arena + slot));
+  } else {
+    return NULL;
+  }
 }
 
 grpc_error *grpc_error_add_child(grpc_error *src, grpc_error *child) {
   GPR_TIMER_BEGIN("grpc_error_add_child", 0);
   grpc_error *new = copy_error_and_unref(src);
-  new->errs = gpr_avl_add(new->errs, (void *)(new->next_err++), child);
+  internal_add_error(&new, child);
   GPR_TIMER_END("grpc_error_add_child", 0);
   return new;
 }
@@ -374,42 +492,6 @@ typedef struct {
   size_t cap_kvs;
 } kv_pairs;
 
-static void append_kv(kv_pairs *kvs, char *key, char *value) {
-  if (kvs->num_kvs == kvs->cap_kvs) {
-    kvs->cap_kvs = GPR_MAX(3 * kvs->cap_kvs / 2, 4);
-    kvs->kvs = gpr_realloc(kvs->kvs, sizeof(*kvs->kvs) * kvs->cap_kvs);
-  }
-  kvs->kvs[kvs->num_kvs].key = key;
-  kvs->kvs[kvs->num_kvs].value = value;
-  kvs->num_kvs++;
-}
-
-static void collect_kvs(gpr_avl_node *node, char *key(void *k),
-                        char *fmt(void *v), kv_pairs *kvs) {
-  if (node == NULL) return;
-  append_kv(kvs, key(node->key), fmt(node->value));
-  collect_kvs(node->left, key, fmt, kvs);
-  collect_kvs(node->right, key, fmt, kvs);
-}
-
-static char *key_int(void *p) {
-  return gpr_strdup(error_int_name((grpc_error_ints)(uintptr_t)p));
-}
-
-static char *key_str(void *p) {
-  return gpr_strdup(error_str_name((grpc_error_strs)(uintptr_t)p));
-}
-
-static char *key_time(void *p) {
-  return gpr_strdup(error_time_name((grpc_error_times)(uintptr_t)p));
-}
-
-static char *fmt_int(void *p) {
-  char *s;
-  gpr_asprintf(&s, "%" PRIdPTR, (intptr_t)p);
-  return s;
-}
-
 static void append_chr(char c, char **s, size_t *sz, size_t *cap) {
   if (*sz == *cap) {
     *cap = GPR_MAX(8, 3 * *cap / 2);
@@ -461,6 +543,40 @@ static void append_esc_str(const char *str, char **s, size_t *sz, size_t *cap) {
   append_chr('"', s, sz, cap);
 }
 
+static void append_kv(kv_pairs *kvs, char *key, char *value) {
+  if (kvs->num_kvs == kvs->cap_kvs) {
+    kvs->cap_kvs = GPR_MAX(3 * kvs->cap_kvs / 2, 4);
+    kvs->kvs = gpr_realloc(kvs->kvs, sizeof(*kvs->kvs) * kvs->cap_kvs);
+  }
+  kvs->kvs[kvs->num_kvs].key = key;
+  kvs->kvs[kvs->num_kvs].value = value;
+  kvs->num_kvs++;
+}
+
+static char *key_int(grpc_error_ints which) {
+  return gpr_strdup(error_int_name(which));
+}
+
+static char *fmt_int(intptr_t p) {
+  char *s;
+  gpr_asprintf(&s, "%" PRIdPTR, p);
+  return s;
+}
+
+static void collect_ints_kvs(grpc_error *err, kv_pairs *kvs) {
+  for (size_t which = 0; which < GRPC_ERROR_INT_MAX; ++which) {
+    uint8_t slot = err->ints[which];
+    if (slot != UINT8_MAX) {
+      append_kv(kvs, key_int((grpc_error_ints)which),
+                fmt_int(err->arena[slot]));
+    }
+  }
+}
+
+static char *key_str(grpc_error_strs which) {
+  return gpr_strdup(error_str_name(which));
+}
+
 static char *fmt_str(void *p) {
   char *s = NULL;
   size_t sz = 0;
@@ -470,8 +586,22 @@ static char *fmt_str(void *p) {
   return s;
 }
 
-static char *fmt_time(void *p) {
+static void collect_strs_kvs(grpc_error *err, kv_pairs *kvs) {
-  gpr_timespec tm = *(gpr_timespec *)p;
+  for (size_t which = 0; which < GRPC_ERROR_STR_MAX; ++which) {
+    uint8_t slot = err->strs[which];
+    if (slot != UINT8_MAX) {
+      append_kv(
+          kvs, key_str((grpc_error_strs)which),
+          fmt_str(GRPC_SLICE_START_PTR(*(grpc_slice *)(err->arena + slot))));
+    }
+  }
+}
+
+static char *key_time(grpc_error_times which) {
+  return gpr_strdup(error_time_name(which));
+}
+
+static char *fmt_time(gpr_timespec tm) {
   char *out;
   char *pfx = "!!";
   switch (tm.clock_type) {
@@ -492,24 +622,37 @@ static char *fmt_time(void *p) {
   return out;
 }
 
-static void add_errs(gpr_avl_node *n, char **s, size_t *sz, size_t *cap,
+static void collect_times_kvs(grpc_error *err, kv_pairs *kvs) {
-                     bool *first) {
+  for (size_t which = 0; which < GRPC_ERROR_TIME_MAX; ++which) {
-  if (n == NULL) return;
+    uint8_t slot = err->times[which];
-  add_errs(n->left, s, sz, cap, first);
+    if (slot != UINT8_MAX) {
-  if (!*first) append_chr(',', s, sz, cap);
+      append_kv(kvs, key_time((grpc_error_times)which),
-  *first = false;
+                fmt_time(*(gpr_timespec *)(err->arena + slot)));
-  const char *e = grpc_error_string(n->value);
+    }
+  }
+}
+
+static void add_errs(grpc_error *err, char **s, size_t *sz, size_t *cap) {
+  uint8_t slot = err->first_err;
+  bool first = true;
+  while (slot != UINT8_MAX) {
+    linked_error *lerr = (linked_error *)(err->arena + slot);
+    if (!first) append_chr(',', s, sz, cap);
+    first = false;
+    const char *e = grpc_error_string(lerr->err);
     append_str(e, s, sz, cap);
-  add_errs(n->right, s, sz, cap, first);
+    GPR_ASSERT(err->last_err == slot ? lerr->next == UINT8_MAX
+                                     : lerr->next != UINT8_MAX);
+    slot = lerr->next;
+  }
 }
 
 static char *errs_string(grpc_error *err) {
   char *s = NULL;
   size_t sz = 0;
   size_t cap = 0;
-  bool first = true;
   append_chr('[', &s, &sz, &cap);
-  add_errs(err->errs.root, &s, &sz, &cap, &first);
+  add_errs(err, &s, &sz, &cap);
   append_chr(']', &s, &sz, &cap);
   append_chr(0, &s, &sz, &cap);
   return s;
@@ -557,10 +700,10 @@ const char *grpc_error_string(grpc_error *err) {
   kv_pairs kvs;
   memset(&kvs, 0, sizeof(kvs));
 
-  collect_kvs(err->ints.root, key_int, fmt_int, &kvs);
+  collect_ints_kvs(err, &kvs);
-  collect_kvs(err->strs.root, key_str, fmt_str, &kvs);
+  collect_strs_kvs(err, &kvs);
-  collect_kvs(err->times.root, key_time, fmt_time, &kvs);
+  collect_times_kvs(err, &kvs);
-  if (!gpr_avl_is_empty(err->errs)) {
+  if (err->first_err != UINT8_MAX) {
     append_kv(&kvs, gpr_strdup("referenced_errors"), errs_string(err));
   }
 

src/core/lib/iomgr/error.h

@@ -102,6 +102,9 @@ typedef enum {
   GRPC_ERROR_INT_LIMIT,
   /// chttp2: did the error occur while a write was in progress
   GRPC_ERROR_INT_OCCURRED_DURING_WRITE,
+
+  /// Must always be last
+  GRPC_ERROR_INT_MAX,
 } grpc_error_ints;
 
 typedef enum {
@@ -129,11 +132,17 @@ typedef enum {
   GRPC_ERROR_STR_KEY,
   /// value associated with the error
   GRPC_ERROR_STR_VALUE,
+
+  /// Must always be last
+  GRPC_ERROR_STR_MAX,
 } grpc_error_strs;
 
 typedef enum {
   /// timestamp of error creation
   GRPC_ERROR_TIME_CREATED,
+
+  /// Must always be last
+  GRPC_ERROR_TIME_MAX,
 } grpc_error_times;
 
 /// The following "special" errors can be propagated without allocating memory.
@@ -184,8 +193,6 @@ void grpc_error_unref(grpc_error *err);
 grpc_error *grpc_error_set_int(grpc_error *src, grpc_error_ints which,
                                intptr_t value) GRPC_MUST_USE_RESULT;
 bool grpc_error_get_int(grpc_error *error, grpc_error_ints which, intptr_t *p);
-grpc_error *grpc_error_set_time(grpc_error *src, grpc_error_times which,
-                                gpr_timespec value) GRPC_MUST_USE_RESULT;
 grpc_error *grpc_error_set_str(grpc_error *src, grpc_error_strs which,
                                const char *value) GRPC_MUST_USE_RESULT;
 /// Returns NULL if the specified string is not set.

src/core/lib/iomgr/error_internal.h

@@ -35,18 +35,28 @@
 #define GRPC_CORE_LIB_IOMGR_ERROR_INTERNAL_H
 
 #include <inttypes.h>
-#include <stdbool.h>
+#include <stdbool.h>  // TODO, do we need this?
 
-#include <grpc/support/avl.h>
+#include <grpc/support/sync.h>
+
+typedef struct linked_error linked_error;
+
+struct linked_error {
+  grpc_error *err;
+  uint8_t next;
+};
 
 struct grpc_error {
   gpr_refcount refs;
-  gpr_avl ints;
+  uint8_t ints[GRPC_ERROR_INT_MAX];
-  gpr_avl strs;
+  uint8_t strs[GRPC_ERROR_STR_MAX];
-  gpr_avl times;
+  uint8_t times[GRPC_ERROR_TIME_MAX];
-  gpr_avl errs;
+  uint8_t first_err;
-  uintptr_t next_err;
+  uint8_t last_err;
   gpr_atm error_string;
+  uint8_t arena_size;
+  uint8_t arena_capacity;
+  intptr_t arena[0];
 };
 
 bool grpc_error_is_special(grpc_error *err);

src/core/lib/transport/error_utils.c

@@ -44,12 +44,12 @@ static grpc_error *recursively_find_error_with_field(grpc_error *error,
   }
   if (grpc_error_is_special(error)) return NULL;
   // Otherwise, search through its children.
-  intptr_t key = 0;
+  uint8_t slot = error->first_err;
-  while (true) {
+  while (slot != UINT8_MAX) {
-    grpc_error *child_error = gpr_avl_get(error->errs, (void *)key++);
+    linked_error *lerr = (linked_error *)(error->arena + slot);
-    if (child_error == NULL) break;
+    grpc_error *result = recursively_find_error_with_field(lerr->err, which);
-    grpc_error *result = recursively_find_error_with_field(child_error, which);
+    if (result) return result;
-    if (result != NULL) return result;
+    slot = lerr->next;
   }
   return NULL;
 }
@@ -112,13 +112,13 @@ bool grpc_error_has_clear_grpc_status(grpc_error *error) {
   if (grpc_error_get_int(error, GRPC_ERROR_INT_GRPC_STATUS, NULL)) {
     return true;
   }
-  intptr_t key = 0;
+  uint8_t slot = error->first_err;
-  while (true) {
+  while (slot != UINT8_MAX) {
-    grpc_error *child_error = gpr_avl_get(error->errs, (void *)key++);
+    linked_error *lerr = (linked_error *)(error->arena + slot);
-    if (child_error == NULL) break;
+    if (grpc_error_has_clear_grpc_status(lerr->err)) {
-    if (grpc_error_has_clear_grpc_status(child_error)) {
       return true;
     }
+    slot = lerr->next;
   }
   return false;
 }

test/core/iomgr/error_test.c

@@ -43,30 +43,29 @@
 
 #include "test/core/util/test_config.h"
 
-static void test_set_get_int()
+static void test_set_get_int() {
-{
   grpc_error* error = GRPC_ERROR_CREATE("Test");
+  GPR_ASSERT(error);
   intptr_t i = 0;
   GPR_ASSERT(grpc_error_get_int(error, GRPC_ERROR_INT_FILE_LINE, &i));
   GPR_ASSERT(i);  // line set will never be 0
   GPR_ASSERT(!grpc_error_get_int(error, GRPC_ERROR_INT_ERRNO, &i));
   GPR_ASSERT(!grpc_error_get_int(error, GRPC_ERROR_INT_SIZE, &i));
 
-  intptr_t errno = 314;
+  intptr_t errnumber = 314;
-  error = grpc_error_set_int(error, GRPC_ERROR_INT_ERRNO, errno);
+  error = grpc_error_set_int(error, GRPC_ERROR_INT_ERRNO, errnumber);
   GPR_ASSERT(grpc_error_get_int(error, GRPC_ERROR_INT_ERRNO, &i));
-  GPR_ASSERT(i == errno);
+  GPR_ASSERT(i == errnumber);
 
-  intptr_t line = 555;
+  intptr_t http = 2;
-  error = grpc_error_set_int(error, GRPC_ERROR_INT_FILE_LINE, line);
+  error = grpc_error_set_int(error, GRPC_ERROR_INT_HTTP2_ERROR, http);
-  GPR_ASSERT(grpc_error_get_int(error, GRPC_ERROR_INT_FILE_LINE, &i));
+  GPR_ASSERT(grpc_error_get_int(error, GRPC_ERROR_INT_HTTP2_ERROR, &i));
-  GPR_ASSERT(i == line);
+  GPR_ASSERT(i == http);
 
   GRPC_ERROR_UNREF(error);
 }
 
-static void test_set_get_str()
+static void test_set_get_str() {
-{
   grpc_error* error = GRPC_ERROR_CREATE("Test");
 
   GPR_ASSERT(!grpc_error_get_str(error, GRPC_ERROR_STR_SYSCALL));
@@ -74,78 +73,146 @@ static void test_set_get_str()
 
   const char* c = grpc_error_get_str(error, GRPC_ERROR_STR_FILE);
   GPR_ASSERT(c);
-  GPR_ASSERT(!strcmp(c, "test/core/iomgr/error_test.c"));
+  GPR_ASSERT(strstr(c, "error_test.c"));  // __FILE__ expands differently on
+                                          // Windows. All should at least
+                                          // contain error_test.c
 
   c = grpc_error_get_str(error, GRPC_ERROR_STR_DESCRIPTION);
   GPR_ASSERT(c);
   GPR_ASSERT(!strcmp(c, "Test"));
 
-  error = grpc_error_set_str(error, GRPC_ERROR_STR_GRPC_MESSAGE, "message");
+  error =
+      grpc_error_set_str(error, GRPC_ERROR_STR_GRPC_MESSAGE, "longer message");
   c = grpc_error_get_str(error, GRPC_ERROR_STR_GRPC_MESSAGE);
   GPR_ASSERT(c);
-  GPR_ASSERT(!strcmp(c, "message"));
+  GPR_ASSERT(!strcmp(c, "longer message"));
-
-  error = grpc_error_set_str(error, GRPC_ERROR_STR_DESCRIPTION, "new desc");
-  c = grpc_error_get_str(error, GRPC_ERROR_STR_DESCRIPTION);
-  GPR_ASSERT(c);
-  GPR_ASSERT(!strcmp(c, "new desc"));
 
   GRPC_ERROR_UNREF(error);
 }
 
-static void test_copy_and_unref()
+static void test_copy_and_unref() {
-{
   // error1 has one ref
-  grpc_error* error1 = GRPC_ERROR_CREATE("Test");
+  grpc_error* error1 = grpc_error_set_str(
-  grpc_error* error2 = grpc_error_set_str(error1, GRPC_ERROR_STR_GRPC_MESSAGE, "message");
+      GRPC_ERROR_CREATE("Test"), GRPC_ERROR_STR_GRPC_MESSAGE, "message");
   const char* c = grpc_error_get_str(error1, GRPC_ERROR_STR_GRPC_MESSAGE);
   GPR_ASSERT(c);
   GPR_ASSERT(!strcmp(c, "message"));
-  c = grpc_error_get_str(error2, GRPC_ERROR_STR_GRPC_MESSAGE);
-  GPR_ASSERT(c);
-  GPR_ASSERT(!strcmp(c, "message"));
-  GPR_ASSERT(error1 == error2);
 
   // error 1 has two refs
   GRPC_ERROR_REF(error1);
   // this gives error3 a ref to the new error, and decrements error1 to one ref
-  grpc_error* error3 = grpc_error_set_str(error1, GRPC_ERROR_STR_DESCRIPTION, "reset");
+  grpc_error* error3 =
+      grpc_error_set_str(error1, GRPC_ERROR_STR_SYSCALL, "syscall");
   GPR_ASSERT(error3 != error1);  // should not be the same because of extra ref
   c = grpc_error_get_str(error3, GRPC_ERROR_STR_GRPC_MESSAGE);
   GPR_ASSERT(c);
   GPR_ASSERT(!strcmp(c, "message"));
 
-  c = grpc_error_get_str(error1, GRPC_ERROR_STR_DESCRIPTION);
+  // error 1 should not have a syscall but 3 should
+  GPR_ASSERT(!grpc_error_get_str(error1, GRPC_ERROR_STR_SYSCALL));
+  c = grpc_error_get_str(error3, GRPC_ERROR_STR_SYSCALL);
   GPR_ASSERT(c);
-  GPR_ASSERT(!strcmp(c, "Test"));
+  GPR_ASSERT(!strcmp(c, "syscall"));
-
-  c = grpc_error_get_str(error3, GRPC_ERROR_STR_DESCRIPTION);
-  GPR_ASSERT(c);
-  GPR_ASSERT(!strcmp(c, "reset"));
 
   GRPC_ERROR_UNREF(error1);
   GRPC_ERROR_UNREF(error3);
 }
 
-static void print_error_strings()
+static void test_create_referencing() {
-{
+  grpc_error* child = grpc_error_set_str(
-  grpc_error* error = grpc_error_set_int(GRPC_ERROR_CREATE("Error"),
+      GRPC_ERROR_CREATE("Child"), GRPC_ERROR_STR_GRPC_MESSAGE, "message");
-                                            GRPC_ERROR_INT_GRPC_STATUS,
+  grpc_error* parent = GRPC_ERROR_CREATE_REFERENCING("Parent", &child, 1);
+  GPR_ASSERT(parent);
+
+  GRPC_ERROR_UNREF(child);
+  GRPC_ERROR_UNREF(parent);
+}
+
+static void test_create_referencing_many() {
+  grpc_error* children[3];
+  children[0] = grpc_error_set_str(GRPC_ERROR_CREATE("Child1"),
+                                   GRPC_ERROR_STR_GRPC_MESSAGE, "message");
+  children[1] = grpc_error_set_int(GRPC_ERROR_CREATE("Child2"),
+                                   GRPC_ERROR_INT_HTTP2_ERROR, 5);
+  children[2] = grpc_error_set_str(GRPC_ERROR_CREATE("Child3"),
+                                   GRPC_ERROR_STR_GRPC_MESSAGE, "message 3");
+
+  grpc_error* parent = GRPC_ERROR_CREATE_REFERENCING("Parent", children, 3);
+  GPR_ASSERT(parent);
+
+  for (size_t i = 0; i < 3; ++i) {
+    GRPC_ERROR_UNREF(children[i]);
+  }
+  GRPC_ERROR_UNREF(parent);
+}
+
+static void print_error_string() {
+  grpc_error* error =
+      grpc_error_set_int(GRPC_ERROR_CREATE("Error"), GRPC_ERROR_INT_GRPC_STATUS,
                          GRPC_STATUS_UNIMPLEMENTED);
-  error = grpc_error_set_int(error, GRPC_ERROR_INT_GRPC_STATUS, 0);
   error = grpc_error_set_int(error, GRPC_ERROR_INT_SIZE, 666);
   error = grpc_error_set_str(error, GRPC_ERROR_STR_GRPC_MESSAGE, "message");
-  gpr_log(GPR_DEBUG, "%s", grpc_error_string(error));
+  // gpr_log(GPR_DEBUG, "%s", grpc_error_string(error));
+  GRPC_ERROR_UNREF(error);
+}
+
+static void print_error_string_reference() {
+  grpc_error* children[2];
+  children[0] = grpc_error_set_str(
+      grpc_error_set_int(GRPC_ERROR_CREATE("1"), GRPC_ERROR_INT_GRPC_STATUS,
+                         GRPC_STATUS_UNIMPLEMENTED),
+      GRPC_ERROR_STR_GRPC_MESSAGE, "message for child 1");
+  children[1] = grpc_error_set_str(
+      grpc_error_set_int(GRPC_ERROR_CREATE("2sd"), GRPC_ERROR_INT_GRPC_STATUS,
+                         GRPC_STATUS_INTERNAL),
+      GRPC_ERROR_STR_GRPC_MESSAGE, "message for child 2");
+
+  grpc_error* parent = GRPC_ERROR_CREATE_REFERENCING("Parent", children, 2);
+
+  gpr_log(GPR_DEBUG, "%s", grpc_error_string(parent));
+
+  for (size_t i = 0; i < 2; ++i) {
+    GRPC_ERROR_UNREF(children[i]);
+  }
+  GRPC_ERROR_UNREF(parent);
+}
+
+static void test_os_error() {
+  int fake_errno = 5;
+  const char* syscall = "syscall name";
+  grpc_error* error = GRPC_OS_ERROR(fake_errno, syscall);
+
+  intptr_t i = 0;
+  GPR_ASSERT(grpc_error_get_int(error, GRPC_ERROR_INT_ERRNO, &i));
+  GPR_ASSERT(i == fake_errno);
+
+  const char* c = grpc_error_get_str(error, GRPC_ERROR_STR_SYSCALL);
+  GPR_ASSERT(c);
+  GPR_ASSERT(!strcmp(c, syscall));
+  GRPC_ERROR_UNREF(error);
+}
+
+static void test_special() {
+  grpc_error* error = GRPC_ERROR_NONE;
+  error = grpc_error_add_child(error, GRPC_ERROR_CREATE("test child"));
+  intptr_t i;
+  GPR_ASSERT(grpc_error_get_int(error, GRPC_ERROR_INT_GRPC_STATUS, &i));
+  GPR_ASSERT(i == GRPC_STATUS_OK);
   GRPC_ERROR_UNREF(error);
 }
 
-int main(int argc, char **argv) {
+int main(int argc, char** argv) {
   grpc_test_init(argc, argv);
   grpc_init();
   test_set_get_int();
   test_set_get_str();
   test_copy_and_unref();
-  print_error_strings();
+  print_error_string();
+  print_error_string_reference();
+  test_os_error();
+  test_create_referencing();
+  test_create_referencing_many();
+  test_special();
   grpc_shutdown();
 
   return 0;

test/cpp/microbenchmarks/bm_error.cc

@@ -74,26 +74,6 @@ static void BM_ErrorCreateAndSetIntAndStr(benchmark::State& state) {
 }
 BENCHMARK(BM_ErrorCreateAndSetIntAndStr);
 
-static void BM_ErrorCreateAndSetIntLoop(benchmark::State& state) {
-  grpc_error* error = GRPC_ERROR_CREATE("Error");
-  int n = 0;
-  while (state.KeepRunning()) {
-    error = grpc_error_set_int(error, GRPC_ERROR_INT_GRPC_STATUS, n++);
-  }
-  GRPC_ERROR_UNREF(error);
-}
-BENCHMARK(BM_ErrorCreateAndSetIntLoop);
-
-static void BM_ErrorCreateAndSetStrLoop(benchmark::State& state) {
-  grpc_error* error = GRPC_ERROR_CREATE("Error");
-  const char* str = "hello";
-  while (state.KeepRunning()) {
-    error = grpc_error_set_str(error, GRPC_ERROR_STR_GRPC_MESSAGE, str);
-  }
-  GRPC_ERROR_UNREF(error);
-}
-BENCHMARK(BM_ErrorCreateAndSetStrLoop);
-
 static void BM_ErrorRefUnref(benchmark::State& state) {
   grpc_error* error = GRPC_ERROR_CREATE("Error");
   while (state.KeepRunning()) {