Merge pull request #16932 from soheilhy/worktree-error

Optimize error handling for special cases.

5 files changed, 53 insertions, 41 deletions

diff --git a/src/core/ext/transport/chttp2/transport/parsing.cc b/src/core/ext/transport/chttp2/transport/parsing.cc
index f532b084c9..1ff96d3cd3 100644
--- a/src/core/ext/transport/chttp2/transport/parsing.cc
+++ b/src/core/ext/transport/chttp2/transport/parsing.cc
@@ -368,6 +368,7 @@ static grpc_error* init_data_frame_parser(grpc_chttp2_transport* t) {
         &s->data_parser, t->incoming_frame_flags, s->id, s);
   }
 error_handler:
+  intptr_t unused;
   if (err == GRPC_ERROR_NONE) {
     t->incoming_stream = s;
     /* t->parser = grpc_chttp2_data_parser_parse;*/
@@ -375,7 +376,7 @@ error_handler:
     t->parser_data = &s->data_parser;
     t->ping_state.last_ping_sent_time = GRPC_MILLIS_INF_PAST;
     return GRPC_ERROR_NONE;
-  } else if (grpc_error_get_int(err, GRPC_ERROR_INT_STREAM_ID, nullptr)) {
+  } else if (grpc_error_get_int(err, GRPC_ERROR_INT_STREAM_ID, &unused)) {
     /* handle stream errors by closing the stream */
     if (s != nullptr) {
       grpc_chttp2_mark_stream_closed(t, s, true, false, err);
@@ -756,9 +757,10 @@ static grpc_error* parse_frame_slice(grpc_chttp2_transport* t, grpc_slice slice,
                                      int is_last) {
   grpc_chttp2_stream* s = t->incoming_stream;
   grpc_error* err = t->parser(t->parser_data, t, s, slice, is_last);
+  intptr_t unused;
   if (GPR_LIKELY(err == GRPC_ERROR_NONE)) {
     return err;
-  } else if (grpc_error_get_int(err, GRPC_ERROR_INT_STREAM_ID, nullptr)) {
+  } else if (grpc_error_get_int(err, GRPC_ERROR_INT_STREAM_ID, &unused)) {
     if (grpc_http_trace.enabled()) {
       const char* msg = grpc_error_string(err);
       gpr_log(GPR_ERROR, "%s", msg);
diff --git a/src/core/lib/iomgr/error.cc b/src/core/lib/iomgr/error.cc
index 146a539027..6ae077fd54 100644
--- a/src/core/lib/iomgr/error.cc
+++ b/src/core/lib/iomgr/error.cc
@@ -121,14 +121,8 @@ static const char* error_time_name(grpc_error_times key) {
   GPR_UNREACHABLE_CODE(return "unknown");
 }
 
-bool grpc_error_is_special(grpc_error* err) {
-  return err == GRPC_ERROR_NONE || err == GRPC_ERROR_OOM ||
-         err == GRPC_ERROR_CANCELLED;
-}
-
 #ifndef NDEBUG
-grpc_error* grpc_error_ref(grpc_error* err, const char* file, int line) {
-  if (grpc_error_is_special(err)) return err;
+grpc_error* grpc_error_do_ref(grpc_error* err, const char* file, int line) {
   if (grpc_trace_error_refcount.enabled()) {
     gpr_log(GPR_DEBUG, "%p: %" PRIdPTR " -> %" PRIdPTR " [%s:%d]", err,
             gpr_atm_no_barrier_load(&err->atomics.refs.count),
@@ -138,8 +132,7 @@ grpc_error* grpc_error_ref(grpc_error* err, const char* file, int line) {
   return err;
 }
 #else
-grpc_error* grpc_error_ref(grpc_error* err) {
-  if (grpc_error_is_special(err)) return err;
+grpc_error* grpc_error_do_ref(grpc_error* err) {
   gpr_ref(&err->atomics.refs);
   return err;
 }
@@ -177,8 +170,7 @@ static void error_destroy(grpc_error* err) {
 }
 
 #ifndef NDEBUG
-void grpc_error_unref(grpc_error* err, const char* file, int line) {
-  if (grpc_error_is_special(err)) return;
+void grpc_error_do_unref(grpc_error* err, const char* file, int line) {
   if (grpc_trace_error_refcount.enabled()) {
     gpr_log(GPR_DEBUG, "%p: %" PRIdPTR " -> %" PRIdPTR " [%s:%d]", err,
             gpr_atm_no_barrier_load(&err->atomics.refs.count),
@@ -189,8 +181,7 @@ void grpc_error_unref(grpc_error* err, const char* file, int line) {
   }
 }
 #else
-void grpc_error_unref(grpc_error* err) {
-  if (grpc_error_is_special(err)) return;
+void grpc_error_do_unref(grpc_error* err) {
   if (gpr_unref(&err->atomics.refs)) {
     error_destroy(err);
   }
@@ -450,26 +441,23 @@ grpc_error* grpc_error_set_int(grpc_error* src, grpc_error_ints which,
 }
 
 typedef struct {
-  grpc_error* error;
   grpc_status_code code;
   const char* msg;
 } special_error_status_map;
 static const special_error_status_map error_status_map[] = {
-    {GRPC_ERROR_NONE, GRPC_STATUS_OK, ""},
-    {GRPC_ERROR_CANCELLED, GRPC_STATUS_CANCELLED, "Cancelled"},
-    {GRPC_ERROR_OOM, GRPC_STATUS_RESOURCE_EXHAUSTED, "Out of memory"},
+    {GRPC_STATUS_OK, ""},                               // GRPC_ERROR_NONE
+    {GRPC_STATUS_INVALID_ARGUMENT, ""},                 // GRPC_ERROR_RESERVED_1
+    {GRPC_STATUS_RESOURCE_EXHAUSTED, "Out of memory"},  // GRPC_ERROR_OOM
+    {GRPC_STATUS_INVALID_ARGUMENT, ""},                 // GRPC_ERROR_RESERVED_2
+    {GRPC_STATUS_CANCELLED, "Cancelled"},               // GRPC_ERROR_CANCELLED
 };
 
 bool grpc_error_get_int(grpc_error* err, grpc_error_ints which, intptr_t* p) {
   GPR_TIMER_SCOPE("grpc_error_get_int", 0);
   if (grpc_error_is_special(err)) {
-    for (size_t i = 0; i < GPR_ARRAY_SIZE(error_status_map); i++) {
-      if (error_status_map[i].error == err) {
-        if (which != GRPC_ERROR_INT_GRPC_STATUS) return false;
-        if (p != nullptr) *p = error_status_map[i].code;
-        return true;
-      }
-    }
+    if (which != GRPC_ERROR_INT_GRPC_STATUS) return false;
+    *p = error_status_map[reinterpret_cast<size_t>(err)].code;
+    return true;
   }
   uint8_t slot = err->ints[which];
   if (slot != UINT8_MAX) {
@@ -490,13 +478,10 @@ grpc_error* grpc_error_set_str(grpc_error* src, grpc_error_strs which,
 bool grpc_error_get_str(grpc_error* err, grpc_error_strs which,
                         grpc_slice* str) {
   if (grpc_error_is_special(err)) {
-    for (size_t i = 0; i < GPR_ARRAY_SIZE(error_status_map); i++) {
-      if (error_status_map[i].error == err) {
-        if (which != GRPC_ERROR_STR_GRPC_MESSAGE) return false;
-        *str = grpc_slice_from_static_string(error_status_map[i].msg);
-        return true;
-      }
-    }
+    if (which != GRPC_ERROR_STR_GRPC_MESSAGE) return false;
+    *str = grpc_slice_from_static_string(
+        error_status_map[reinterpret_cast<size_t>(err)].msg);
+    return true;
   }
   uint8_t slot = err->strs[which];
   if (slot != UINT8_MAX) {
diff --git a/src/core/lib/iomgr/error.h b/src/core/lib/iomgr/error.h
index 49f4029bc2..cb740d5b01 100644
--- a/src/core/lib/iomgr/error.h
+++ b/src/core/lib/iomgr/error.h
@@ -120,8 +120,15 @@ typedef enum {
 /// polling engines) can safely use the lower bit for themselves.
 
 #define GRPC_ERROR_NONE ((grpc_error*)NULL)
+#define GRPC_ERROR_RESERVED_1 ((grpc_error*)1)
 #define GRPC_ERROR_OOM ((grpc_error*)2)
+#define GRPC_ERROR_RESERVED_2 ((grpc_error*)3)
 #define GRPC_ERROR_CANCELLED ((grpc_error*)4)
+#define GRPC_ERROR_SPECIAL_MAX GRPC_ERROR_CANCELLED
+
+inline bool grpc_error_is_special(struct grpc_error* err) {
+  return err <= GRPC_ERROR_SPECIAL_MAX;
+}
 
 // debug only toggles that allow for a sanity to check that ensures we will
 // never create any errors in the per-RPC hotpath.
@@ -158,19 +165,37 @@ grpc_error* grpc_error_create(const char* file, int line, grpc_slice desc,
                     errs, count)
 
 #ifndef NDEBUG
-grpc_error* grpc_error_ref(grpc_error* err, const char* file, int line);
-void grpc_error_unref(grpc_error* err, const char* file, int line);
+grpc_error* grpc_error_do_ref(grpc_error* err, const char* file, int line);
+void grpc_error_do_unref(grpc_error* err, const char* file, int line);
+inline grpc_error* grpc_error_ref(grpc_error* err, const char* file, int line) {
+  if (grpc_error_is_special(err)) return err;
+  return grpc_error_do_ref(err, file, line);
+}
+inline void grpc_error_unref(grpc_error* err, const char* file, int line) {
+  if (grpc_error_is_special(err)) return;
+  grpc_error_do_unref(err, file, line);
+}
 #define GRPC_ERROR_REF(err) grpc_error_ref(err, __FILE__, __LINE__)
 #define GRPC_ERROR_UNREF(err) grpc_error_unref(err, __FILE__, __LINE__)
 #else
-grpc_error* grpc_error_ref(grpc_error* err);
-void grpc_error_unref(grpc_error* err);
+grpc_error* grpc_error_do_ref(grpc_error* err);
+void grpc_error_do_unref(grpc_error* err);
+inline grpc_error* grpc_error_ref(grpc_error* err) {
+  if (grpc_error_is_special(err)) return err;
+  return grpc_error_do_ref(err);
+}
+inline void grpc_error_unref(grpc_error* err) {
+  if (grpc_error_is_special(err)) return;
+  grpc_error_do_unref(err);
+}
 #define GRPC_ERROR_REF(err) grpc_error_ref(err)
 #define GRPC_ERROR_UNREF(err) grpc_error_unref(err)
 #endif
 
 grpc_error* grpc_error_set_int(grpc_error* src, grpc_error_ints which,
                                intptr_t value) GRPC_MUST_USE_RESULT;
+/// It is an error to pass nullptr as `p`. Caller should allocate a dummy
+/// intptr_t for `p`, even if the value of `p` is not used.
 bool grpc_error_get_int(grpc_error* error, grpc_error_ints which, intptr_t* p);
 /// This call takes ownership of the slice; the error is responsible for
 /// eventually unref-ing it.
diff --git a/src/core/lib/iomgr/error_internal.h b/src/core/lib/iomgr/error_internal.h
index 7fde347abd..8027396019 100644
--- a/src/core/lib/iomgr/error_internal.h
+++ b/src/core/lib/iomgr/error_internal.h
@@ -58,6 +58,4 @@ struct grpc_error {
   intptr_t arena[0];
 };
 
-bool grpc_error_is_special(struct grpc_error* err);
-
 #endif /* GRPC_CORE_LIB_IOMGR_ERROR_INTERNAL_H */
diff --git a/src/core/lib/transport/error_utils.cc b/src/core/lib/transport/error_utils.cc
index 2eff8b2916..558f1d494c 100644
--- a/src/core/lib/transport/error_utils.cc
+++ b/src/core/lib/transport/error_utils.cc
@@ -26,8 +26,9 @@
 
 static grpc_error* recursively_find_error_with_field(grpc_error* error,
                                                      grpc_error_ints which) {
+  intptr_t unused;
   // If the error itself has a status code, return it.
-  if (grpc_error_get_int(error, which, nullptr)) {
+  if (grpc_error_get_int(error, which, &unused)) {
     return error;
   }
   if (grpc_error_is_special(error)) return nullptr;
@@ -102,7 +103,8 @@ void grpc_error_get_status(grpc_error* error, grpc_millis deadline,
 }
 
 bool grpc_error_has_clear_grpc_status(grpc_error* error) {
-  if (grpc_error_get_int(error, GRPC_ERROR_INT_GRPC_STATUS, nullptr)) {
+  intptr_t unused;
+  if (grpc_error_get_int(error, GRPC_ERROR_INT_GRPC_STATUS, &unused)) {
     return true;
   }
   uint8_t slot = error->first_err;