diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index f3967bfe247..0e3eccb1a19 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -1219,42 +1219,26 @@ void grpc_chttp2_complete_closure_step(grpc_chttp2_transport* t, write_state_name(t->write_state)); } if (error != GRPC_ERROR_NONE) { -#ifdef GRPC_ERROR_IS_ABSEIL_STATUS - grpc_error_handle cl_err = - grpc_core::internal::StatusMoveFromHeapPtr(closure->error_data.error); -#else - grpc_error_handle cl_err = - reinterpret_cast(closure->error_data.error); -#endif - if (cl_err == GRPC_ERROR_NONE) { - cl_err = GRPC_ERROR_CREATE_FROM_STATIC_STRING( + if (closure->error_data.error == GRPC_ERROR_NONE) { + closure->error_data.error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( "Error in HTTP transport completing operation"); - cl_err = grpc_error_set_str(cl_err, GRPC_ERROR_STR_TARGET_ADDRESS, - t->peer_string); + closure->error_data.error = + grpc_error_set_str(closure->error_data.error, + GRPC_ERROR_STR_TARGET_ADDRESS, t->peer_string); } - cl_err = grpc_error_add_child(cl_err, error); -#ifdef GRPC_ERROR_IS_ABSEIL_STATUS - closure->error_data.error = grpc_core::internal::StatusAllocHeapPtr(cl_err); -#else - closure->error_data.error = reinterpret_cast(cl_err); -#endif + closure->error_data.error = + grpc_error_add_child(closure->error_data.error, error); } if (closure->next_data.scratch < CLOSURE_BARRIER_FIRST_REF_BIT) { if ((t->write_state == GRPC_CHTTP2_WRITE_STATE_IDLE) || !(closure->next_data.scratch & CLOSURE_BARRIER_MAY_COVER_WRITE)) { // Using GRPC_CLOSURE_SCHED instead of GRPC_CLOSURE_RUN to avoid running // closures earlier than when it is safe to do so. -#ifdef GRPC_ERROR_IS_ABSEIL_STATUS - grpc_error_handle run_error = - grpc_core::internal::StatusMoveFromHeapPtr(closure->error_data.error); -#else - grpc_error_handle run_error = - reinterpret_cast(closure->error_data.error); -#endif - closure->error_data.error = 0; - grpc_core::ExecCtx::Run(DEBUG_LOCATION, closure, run_error); + grpc_core::ExecCtx::Run(DEBUG_LOCATION, closure, + closure->error_data.error); } else { - grpc_closure_list_append(&t->run_after_write, closure); + grpc_closure_list_append(&t->run_after_write, closure, + closure->error_data.error); } } } @@ -1402,7 +1386,7 @@ static void perform_stream_op_locked(void* stream_op, // This batch has send ops. Use final_data as a barrier until enqueue time; // the initial counter is dropped at the end of this function. on_complete->next_data.scratch = CLOSURE_BARRIER_FIRST_REF_BIT; - on_complete->error_data.error = 0; + on_complete->error_data.error = GRPC_ERROR_NONE; } if (op->cancel_stream) { diff --git a/src/core/lib/gprpp/status_helper.cc b/src/core/lib/gprpp/status_helper.cc index 5fde9ca9904..fa538dc54c6 100644 --- a/src/core/lib/gprpp/status_helper.cc +++ b/src/core/lib/gprpp/status_helper.cc @@ -379,8 +379,32 @@ absl::Status StatusFromProto(google_rpc_Status* msg) { return status; } +uintptr_t StatusAllocPtr(absl::Status s) { + // This relies the fact that absl::Status has only one member, StatusRep* + // so the sizeof(absl::Status) has the same size of intptr_t and StatusRep* + // can be stolen using placement allocation. + static_assert(sizeof(intptr_t) == sizeof(absl::Status), + "absl::Status should be as big as intptr_t"); + // This does two things; + // 1. Copies StatusRep* of absl::Status to ptr + // 2. Increases the counter of StatusRep if it's not inlined + uintptr_t ptr; + new (&ptr) absl::Status(s); + return ptr; +} + +void StatusFreePtr(uintptr_t ptr) { + // Decreases the counter of StatusRep if it's not inlined. + reinterpret_cast(&ptr)->~Status(); +} + +absl::Status StatusGetFromPtr(uintptr_t ptr) { + // Constructs Status from ptr having the address of StatusRep. + return *reinterpret_cast(&ptr); +} + uintptr_t StatusAllocHeapPtr(absl::Status s) { - if (s.ok()) return 0; + if (s.ok()) return kOkStatusPtr; absl::Status* ptr = new absl::Status(s); return reinterpret_cast(ptr); } @@ -391,24 +415,13 @@ void StatusFreeHeapPtr(uintptr_t ptr) { } absl::Status StatusGetFromHeapPtr(uintptr_t ptr) { - if (ptr == 0) { + if (ptr == kOkStatusPtr) { return absl::OkStatus(); } else { return *reinterpret_cast(ptr); } } -absl::Status StatusMoveFromHeapPtr(uintptr_t ptr) { - if (ptr == 0) { - return absl::OkStatus(); - } else { - absl::Status* s = reinterpret_cast(ptr); - absl::Status ret = std::move(*s); - delete s; - return ret; - } -} - } // namespace internal } // namespace grpc_core diff --git a/src/core/lib/gprpp/status_helper.h b/src/core/lib/gprpp/status_helper.h index 1587bad4f68..801736d24ed 100644 --- a/src/core/lib/gprpp/status_helper.h +++ b/src/core/lib/gprpp/status_helper.h @@ -160,6 +160,22 @@ google_rpc_Status* StatusToProto(const absl::Status& status, /// This is for internal implementation & test only absl::Status StatusFromProto(google_rpc_Status* msg) GRPC_MUST_USE_RESULT; +/// The same value of internal::StatusAllocPtr(absl::OkStatus()) +static constexpr uintptr_t kOkStatusPtr = 0; + +/// Returns ptr where the given status is copied into. +/// This ptr can be used to get Status later and should be freed by +/// StatusFreePtr. This shouldn't be used except migration purpose. +uintptr_t StatusAllocPtr(absl::Status s); + +/// Frees the allocated status at ptr. +/// This shouldn't be used except migration purpose. +void StatusFreePtr(uintptr_t ptr); + +/// Get the status from ptr. +/// This shouldn't be used except migration purpose. +absl::Status StatusGetFromPtr(uintptr_t ptr); + /// Returns ptr that is allocated in the heap memory and the given status is /// copied into. This ptr can be used to get Status later and should be /// freed by StatusFreeHeapPtr. This can be 0 in case of OkStatus. @@ -171,9 +187,6 @@ void StatusFreeHeapPtr(uintptr_t ptr); /// Get the status from a heap ptr. absl::Status StatusGetFromHeapPtr(uintptr_t ptr); -/// Move the status from a heap ptr. (GetFrom & FreeHeap) -absl::Status StatusMoveFromHeapPtr(uintptr_t ptr); - } // namespace internal } // namespace grpc_core diff --git a/src/core/lib/iomgr/call_combiner.cc b/src/core/lib/iomgr/call_combiner.cc index 720fd47a279..d2c40d38e43 100644 --- a/src/core/lib/iomgr/call_combiner.cc +++ b/src/core/lib/iomgr/call_combiner.cc @@ -150,11 +150,7 @@ void CallCombiner::Start(grpc_closure* closure, grpc_error_handle error, gpr_log(GPR_INFO, " QUEUING"); } // Queue was not empty, so add closure to queue. -#ifdef GRPC_ERROR_IS_ABSEIL_STATUS - closure->error_data.error = internal::StatusAllocHeapPtr(error); -#else - closure->error_data.error = reinterpret_cast(error); -#endif + closure->error_data.error = error; queue_.Push( reinterpret_cast(closure)); } @@ -189,19 +185,12 @@ void CallCombiner::Stop(DEBUG_ARGS const char* reason) { } continue; } -#ifdef GRPC_ERROR_IS_ABSEIL_STATUS - grpc_error_handle error = - internal::StatusMoveFromHeapPtr(closure->error_data.error); -#else - grpc_error_handle error = - reinterpret_cast(closure->error_data.error); -#endif - closure->error_data.error = 0; if (GRPC_TRACE_FLAG_ENABLED(grpc_call_combiner_trace)) { gpr_log(GPR_INFO, " EXECUTING FROM QUEUE: closure=%p error=%s", - closure, grpc_error_std_string(error).c_str()); + closure, + grpc_error_std_string(closure->error_data.error).c_str()); } - ScheduleClosure(closure, error); + ScheduleClosure(closure, closure->error_data.error); break; } } else if (GRPC_TRACE_FLAG_ENABLED(grpc_call_combiner_trace)) { diff --git a/src/core/lib/iomgr/closure.h b/src/core/lib/iomgr/closure.h index 0b79368a9f2..b429c9e3ebd 100644 --- a/src/core/lib/iomgr/closure.h +++ b/src/core/lib/iomgr/closure.h @@ -72,7 +72,7 @@ struct grpc_closure { /** Once queued, the result of the closure. Before then: scratch space */ union { - uintptr_t error; + grpc_error_handle error; uintptr_t scratch; } error_data; @@ -98,7 +98,7 @@ inline grpc_closure* grpc_closure_init(grpc_closure* closure, #endif closure->cb = cb; closure->cb_arg = cb_arg; - closure->error_data.error = 0; + closure->error_data.error = GRPC_ERROR_NONE; #ifndef NDEBUG closure->scheduled = false; closure->file_initiated = nullptr; @@ -172,12 +172,16 @@ inline void grpc_closure_list_init(grpc_closure_list* closure_list) { } /** add \a closure to the end of \a list + and set \a closure's result to \a error Returns true if \a list becomes non-empty */ inline bool grpc_closure_list_append(grpc_closure_list* closure_list, - grpc_closure* closure) { + grpc_closure* closure, + grpc_error_handle error) { if (closure == nullptr) { + GRPC_ERROR_UNREF(error); return false; } + closure->error_data.error = error; closure->next_data.next = nullptr; bool was_empty = (closure_list->head == nullptr); if (was_empty) { @@ -189,36 +193,12 @@ inline bool grpc_closure_list_append(grpc_closure_list* closure_list, return was_empty; } -/** add \a closure to the end of \a list - and set \a closure's result to \a error - Returns true if \a list becomes non-empty */ -inline bool grpc_closure_list_append(grpc_closure_list* closure_list, - grpc_closure* closure, - grpc_error_handle error) { - if (closure == nullptr) { - GRPC_ERROR_UNREF(error); - return false; - } -#ifdef GRPC_ERROR_IS_ABSEIL_STATUS - closure->error_data.error = grpc_core::internal::StatusAllocHeapPtr(error); -#else - closure->error_data.error = reinterpret_cast(error); -#endif - return grpc_closure_list_append(closure_list, closure); -} - /** force all success bits in \a list to false */ inline void grpc_closure_list_fail_all(grpc_closure_list* list, grpc_error_handle forced_failure) { for (grpc_closure* c = list->head; c != nullptr; c = c->next_data.next) { - if (c->error_data.error == 0) { -#ifdef GRPC_ERROR_IS_ABSEIL_STATUS - c->error_data.error = - grpc_core::internal::StatusAllocHeapPtr(forced_failure); -#else - c->error_data.error = - reinterpret_cast(GRPC_ERROR_REF(forced_failure)); -#endif + if (c->error_data.error == GRPC_ERROR_NONE) { + c->error_data.error = GRPC_ERROR_REF(forced_failure); } } GRPC_ERROR_UNREF(forced_failure); diff --git a/src/core/lib/iomgr/combiner.cc b/src/core/lib/iomgr/combiner.cc index a51364ca4b7..1b5add88cd2 100644 --- a/src/core/lib/iomgr/combiner.cc +++ b/src/core/lib/iomgr/combiner.cc @@ -149,11 +149,7 @@ static void combiner_exec(grpc_core::Combiner* lock, grpc_closure* cl, } GPR_ASSERT(last & STATE_UNORPHANED); // ensure lock has not been destroyed assert(cl->cb); -#ifdef GRPC_ERROR_IS_ABSEIL_STATUS - cl->error_data.error = grpc_core::internal::StatusAllocHeapPtr(error); -#else - cl->error_data.error = reinterpret_cast(error); -#endif + cl->error_data.error = error; lock->queue.Push(cl->next_data.mpscq_node.get()); } @@ -225,21 +221,12 @@ bool grpc_combiner_continue_exec_ctx() { return true; } grpc_closure* cl = reinterpret_cast(n); + grpc_error_handle cl_err = cl->error_data.error; #ifndef NDEBUG cl->scheduled = false; #endif -#ifdef GRPC_ERROR_IS_ABSEIL_STATUS - grpc_error_handle cl_err = - grpc_core::internal::StatusMoveFromHeapPtr(cl->error_data.error); - cl->error_data.error = 0; - cl->cb(cl->cb_arg, std::move(cl_err)); -#else - grpc_error_handle cl_err = - reinterpret_cast(cl->error_data.error); - cl->error_data.error = 0; cl->cb(cl->cb_arg, cl_err); GRPC_ERROR_UNREF(cl_err); -#endif } else { grpc_closure* c = lock->final_list.head; GPR_ASSERT(c != nullptr); @@ -249,21 +236,12 @@ bool grpc_combiner_continue_exec_ctx() { GRPC_COMBINER_TRACE( gpr_log(GPR_INFO, "C:%p execute_final[%d] c=%p", lock, loops, c)); grpc_closure* next = c->next_data.next; + grpc_error_handle error = c->error_data.error; #ifndef NDEBUG c->scheduled = false; #endif -#ifdef GRPC_ERROR_IS_ABSEIL_STATUS - grpc_error_handle error = - grpc_core::internal::StatusMoveFromHeapPtr(c->error_data.error); - c->error_data.error = 0; - c->cb(c->cb_arg, std::move(error)); -#else - grpc_error_handle error = - reinterpret_cast(c->error_data.error); - c->error_data.error = 0; c->cb(c->cb_arg, error); GRPC_ERROR_UNREF(error); -#endif c = next; } } diff --git a/src/core/lib/iomgr/exec_ctx.cc b/src/core/lib/iomgr/exec_ctx.cc index ef4248112e0..ed480d8f742 100644 --- a/src/core/lib/iomgr/exec_ctx.cc +++ b/src/core/lib/iomgr/exec_ctx.cc @@ -27,7 +27,7 @@ #include "src/core/lib/iomgr/error.h" #include "src/core/lib/profiling/timers.h" -static void exec_ctx_run(grpc_closure* closure) { +static void exec_ctx_run(grpc_closure* closure, grpc_error_handle error) { #ifndef NDEBUG closure->scheduled = false; if (grpc_trace_closure.enabled()) { @@ -37,27 +37,18 @@ static void exec_ctx_run(grpc_closure* closure) { closure->line_initiated); } #endif -#ifdef GRPC_ERROR_IS_ABSEIL_STATUS - grpc_error_handle error = - grpc_core::internal::StatusMoveFromHeapPtr(closure->error_data.error); - closure->error_data.error = 0; - closure->cb(closure->cb_arg, std::move(error)); -#else - grpc_error_handle error = - reinterpret_cast(closure->error_data.error); - closure->error_data.error = 0; closure->cb(closure->cb_arg, error); - GRPC_ERROR_UNREF(error); -#endif #ifndef NDEBUG if (grpc_trace_closure.enabled()) { gpr_log(GPR_DEBUG, "closure %p finished", closure); } #endif + GRPC_ERROR_UNREF(error); } -static void exec_ctx_sched(grpc_closure* closure) { - grpc_closure_list_append(grpc_core::ExecCtx::Get()->closure_list(), closure); +static void exec_ctx_sched(grpc_closure* closure, grpc_error_handle error) { + grpc_closure_list_append(grpc_core::ExecCtx::Get()->closure_list(), closure, + error); } static gpr_timespec g_start_time; @@ -160,8 +151,9 @@ bool ExecCtx::Flush() { closure_list_.head = closure_list_.tail = nullptr; while (c != nullptr) { grpc_closure* next = c->next_data.next; + grpc_error_handle error = c->error_data.error; did_something = true; - exec_ctx_run(c); + exec_ctx_run(c, error); c = next; } } else if (!grpc_combiner_continue_exec_ctx()) { @@ -203,12 +195,7 @@ void ExecCtx::Run(const DebugLocation& location, grpc_closure* closure, closure->run = false; GPR_ASSERT(closure->cb != nullptr); #endif -#ifdef GRPC_ERROR_IS_ABSEIL_STATUS - closure->error_data.error = internal::StatusAllocHeapPtr(error); -#else - closure->error_data.error = reinterpret_cast(error); -#endif - exec_ctx_sched(closure); + exec_ctx_sched(closure, error); } void ExecCtx::RunList(const DebugLocation& location, grpc_closure_list* list) { @@ -231,7 +218,7 @@ void ExecCtx::RunList(const DebugLocation& location, grpc_closure_list* list) { c->run = false; GPR_ASSERT(c->cb != nullptr); #endif - exec_ctx_sched(c); + exec_ctx_sched(c, c->error_data.error); c = next; } list->head = list->tail = nullptr; diff --git a/src/core/lib/iomgr/executor.cc b/src/core/lib/iomgr/executor.cc index b8986d9f804..bf62288b0ab 100644 --- a/src/core/lib/iomgr/executor.cc +++ b/src/core/lib/iomgr/executor.cc @@ -114,6 +114,7 @@ size_t Executor::RunClosures(const char* executor_name, grpc_closure* c = list.head; while (c != nullptr) { grpc_closure* next = c->next_data.next; + grpc_error_handle error = c->error_data.error; #ifndef NDEBUG EXECUTOR_TRACE("(%s) run %p [created by %s:%d]", executor_name, c, c->file_created, c->line_created); @@ -121,18 +122,8 @@ size_t Executor::RunClosures(const char* executor_name, #else EXECUTOR_TRACE("(%s) run %p", executor_name, c); #endif -#ifdef GRPC_ERROR_IS_ABSEIL_STATUS - grpc_error_handle error = - internal::StatusMoveFromHeapPtr(c->error_data.error); - c->error_data.error = 0; - c->cb(c->cb_arg, std::move(error)); -#else - grpc_error_handle error = - reinterpret_cast(c->error_data.error); - c->error_data.error = 0; c->cb(c->cb_arg, error); GRPC_ERROR_UNREF(error); -#endif c = next; n++; ExecCtx::Get()->Flush(); diff --git a/test/core/gprpp/status_helper_test.cc b/test/core/gprpp/status_helper_test.cc index 18ed3fffb4f..9e153867041 100644 --- a/test/core/gprpp/status_helper_test.cc +++ b/test/core/gprpp/status_helper_test.cc @@ -150,22 +150,23 @@ TEST(StatusUtilTest, ComplexErrorWithChildrenToString) { t); } -TEST(StatusUtilTest, AllocHeapPtr) { +TEST(StatusUtilTest, AllocPtr) { absl::Status statuses[] = {absl::OkStatus(), absl::CancelledError(), absl::AbortedError("Message")}; for (const auto& s : statuses) { - uintptr_t p = internal::StatusAllocHeapPtr(s); - EXPECT_EQ(s, internal::StatusGetFromHeapPtr(p)); - internal::StatusFreeHeapPtr(p); + uintptr_t p = internal::StatusAllocPtr(s); + EXPECT_EQ(s, internal::StatusGetFromPtr(p)); + internal::StatusFreePtr(p); } } -TEST(StatusUtilTest, MoveHeapPtr) { +TEST(StatusUtilTest, AllocHeapPtr) { absl::Status statuses[] = {absl::OkStatus(), absl::CancelledError(), absl::AbortedError("Message")}; for (const auto& s : statuses) { uintptr_t p = internal::StatusAllocHeapPtr(s); - EXPECT_EQ(s, internal::StatusMoveFromHeapPtr(p)); + EXPECT_EQ(s, internal::StatusGetFromHeapPtr(p)); + internal::StatusFreeHeapPtr(p); } }