From bff4dd1b2d6af9e467243b4424999a3c307c0c0c Mon Sep 17 00:00:00 2001 From: Arjun Roy Date: Mon, 3 Jun 2019 13:45:20 -0700 Subject: [PATCH] Fast-path for no-error case for grpc_error_get_status. For each client side call, we execute grpc_error_get_status; in the common case that there is no error, we save roughly 30 instructions and a call to strlen. --- .../filters/http/client/http_client_filter.cc | 2 +- .../filters/http/client_authority_filter.cc | 4 +- .../chttp2/transport/hpack_parser.cc | 10 ++-- .../transport/chttp2/transport/hpack_table.cc | 6 ++- .../ext/transport/inproc/inproc_transport.cc | 2 +- src/core/lib/iomgr/error.cc | 24 ++++++--- src/core/lib/slice/slice.cc | 49 +++++++++---------- src/core/lib/slice/slice_internal.h | 15 ++++++ src/core/lib/transport/error_utils.cc | 11 ++++- .../run_tests/sanity/core_banned_functions.py | 1 + 10 files changed, 78 insertions(+), 46 deletions(-) diff --git a/src/core/ext/filters/http/client/http_client_filter.cc b/src/core/ext/filters/http/client/http_client_filter.cc index 4ef6c1f610e..d014460b34e 100644 --- a/src/core/ext/filters/http/client/http_client_filter.cc +++ b/src/core/ext/filters/http/client/http_client_filter.cc @@ -558,7 +558,7 @@ static grpc_slice user_agent_from_args(const grpc_channel_args* args, tmp = gpr_strvec_flatten(&v, nullptr); gpr_strvec_destroy(&v); - result = grpc_slice_intern(grpc_slice_from_static_string(tmp)); + result = grpc_slice_intern(grpc_slice_from_static_string_internal(tmp)); gpr_free(tmp); return result; diff --git a/src/core/ext/filters/http/client_authority_filter.cc b/src/core/ext/filters/http/client_authority_filter.cc index 85b30bc13ca..4bd666ecfee 100644 --- a/src/core/ext/filters/http/client_authority_filter.cc +++ b/src/core/ext/filters/http/client_authority_filter.cc @@ -101,8 +101,8 @@ grpc_error* init_channel_elem(grpc_channel_element* elem, return GRPC_ERROR_CREATE_FROM_STATIC_STRING( "GRPC_ARG_DEFAULT_AUTHORITY channel arg. must be a string"); } - chand->default_authority = - grpc_slice_intern(grpc_slice_from_static_string(default_authority_str)); + chand->default_authority = grpc_slice_intern( + grpc_slice_from_static_string_internal(default_authority_str)); chand->default_authority_mdelem = grpc_mdelem_create( GRPC_MDSTR_AUTHORITY, chand->default_authority, nullptr); GPR_ASSERT(!args->is_last); diff --git a/src/core/ext/transport/chttp2/transport/hpack_parser.cc b/src/core/ext/transport/chttp2/transport/hpack_parser.cc index 7d5c39e5144..8db0c96cc52 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_parser.cc +++ b/src/core/ext/transport/chttp2/transport/hpack_parser.cc @@ -670,7 +670,7 @@ static grpc_slice take_string(grpc_chttp2_hpack_parser* p, str->copied = true; str->data.referenced = grpc_empty_slice(); } else if (intern) { - s = grpc_slice_intern(grpc_slice_from_static_buffer( + s = grpc_slice_intern(grpc_slice_from_static_buffer_internal( str->data.copied.str, str->data.copied.length)); } else { s = grpc_slice_from_copied_buffer(str->data.copied.str, @@ -1496,14 +1496,14 @@ static grpc_error* parse_key_string(grpc_chttp2_hpack_parser* p, static bool is_binary_literal_header(grpc_chttp2_hpack_parser* p) { /* We know that either argument here is a reference counter slice. - * 1. If a result of grpc_slice_from_static_buffer, the refcount is set to - * NoopRefcount. + * 1. If a result of grpc_slice_from_static_buffer_internal, the refcount is + * set to kNoopRefcount. * 2. If it's p->key.data.referenced, then p->key.copied was set to false, * which occurs in begin_parse_string() - where the refcount is set to * p->current_slice_refcount, which is not null. */ return grpc_is_refcounted_slice_binary_header( - p->key.copied ? grpc_slice_from_static_buffer(p->key.data.copied.str, - p->key.data.copied.length) + p->key.copied ? grpc_slice_from_static_buffer_internal( + p->key.data.copied.str, p->key.data.copied.length) : p->key.data.referenced); } diff --git a/src/core/ext/transport/chttp2/transport/hpack_table.cc b/src/core/ext/transport/chttp2/transport/hpack_table.cc index f9e97cc5566..9d1ac4b370f 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_table.cc +++ b/src/core/ext/transport/chttp2/transport/hpack_table.cc @@ -29,6 +29,7 @@ #include "src/core/lib/debug/trace.h" #include "src/core/lib/gpr/murmur_hash.h" +#include "src/core/lib/slice/slice_internal.h" #include "src/core/lib/surface/validate_metadata.h" #include "src/core/lib/transport/static_metadata.h" @@ -182,9 +183,10 @@ void grpc_chttp2_hptbl_init(grpc_chttp2_hptbl* tbl) { memset(tbl->ents, 0, sizeof(*tbl->ents) * tbl->cap_entries); for (i = 1; i <= GRPC_CHTTP2_LAST_STATIC_ENTRY; i++) { tbl->static_ents[i - 1] = grpc_mdelem_from_slices( - grpc_slice_intern(grpc_slice_from_static_string(static_table[i].key)), grpc_slice_intern( - grpc_slice_from_static_string(static_table[i].value))); + grpc_slice_from_static_string_internal(static_table[i].key)), + grpc_slice_intern( + grpc_slice_from_static_string_internal(static_table[i].value))); } } diff --git a/src/core/ext/transport/inproc/inproc_transport.cc b/src/core/ext/transport/inproc/inproc_transport.cc index 8da89851a69..8cb9ac4a2fd 100644 --- a/src/core/ext/transport/inproc/inproc_transport.cc +++ b/src/core/ext/transport/inproc/inproc_transport.cc @@ -1203,7 +1203,7 @@ void inproc_transports_create(grpc_transport** server_transport, */ void grpc_inproc_transport_init(void) { grpc_core::ExecCtx exec_ctx; - g_empty_slice = grpc_slice_from_static_buffer(nullptr, 0); + g_empty_slice = grpc_slice_from_static_buffer_internal(nullptr, 0); grpc_slice key_tmp = grpc_slice_from_static_string(":path"); g_fake_path_key = grpc_slice_intern(key_tmp); diff --git a/src/core/lib/iomgr/error.cc b/src/core/lib/iomgr/error.cc index ebec9dc704a..eb44c9a2c90 100644 --- a/src/core/lib/iomgr/error.cc +++ b/src/core/lib/iomgr/error.cc @@ -447,13 +447,17 @@ grpc_error* grpc_error_set_int(grpc_error* src, grpc_error_ints which, typedef struct { grpc_status_code code; const char* msg; + size_t len; } special_error_status_map; -static const special_error_status_map error_status_map[] = { - {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 + +const special_error_status_map error_status_map[] = { + {GRPC_STATUS_OK, "", 0}, // GRPC_ERROR_NONE + {GRPC_STATUS_INVALID_ARGUMENT, "", 0}, // GRPC_ERROR_RESERVED_1 + {GRPC_STATUS_RESOURCE_EXHAUSTED, "Out of memory", + strlen("Out of memory")}, // GRPC_ERROR_OOM + {GRPC_STATUS_INVALID_ARGUMENT, "", 0}, // GRPC_ERROR_RESERVED_2 + {GRPC_STATUS_CANCELLED, "Cancelled", + strlen("Cancelled")}, // GRPC_ERROR_CANCELLED }; bool grpc_error_get_int(grpc_error* err, grpc_error_ints which, intptr_t* p) { @@ -483,8 +487,12 @@ bool grpc_error_get_str(grpc_error* err, grpc_error_strs which, grpc_slice* str) { if (grpc_error_is_special(err)) { if (which != GRPC_ERROR_STR_GRPC_MESSAGE) return false; - *str = grpc_slice_from_static_string( - error_status_map[reinterpret_cast(err)].msg); + const special_error_status_map& msg = + error_status_map[reinterpret_cast(err)]; + str->refcount = &grpc_core::kNoopRefcount; + str->data.refcounted.bytes = + reinterpret_cast(const_cast(msg.msg)); + str->data.refcounted.length = msg.len; return true; } uint8_t slot = err->strs[which]; diff --git a/src/core/lib/slice/slice.cc b/src/core/lib/slice/slice.cc index eebf66bb882..57862518feb 100644 --- a/src/core/lib/slice/slice.cc +++ b/src/core/lib/slice/slice.cc @@ -66,32 +66,13 @@ void grpc_slice_unref(grpc_slice slice) { } } +namespace grpc_core { + /* grpc_slice_from_static_string support structure - a refcount that does nothing */ -static grpc_slice_refcount NoopRefcount = - grpc_slice_refcount(grpc_slice_refcount::Type::NOP); - -size_t grpc_slice_memory_usage(grpc_slice s) { - if (s.refcount == nullptr || s.refcount == &NoopRefcount) { - return 0; - } else { - return s.data.refcounted.length; - } -} - -grpc_slice grpc_slice_from_static_buffer(const void* s, size_t len) { - grpc_slice slice; - slice.refcount = &NoopRefcount; - slice.data.refcounted.bytes = (uint8_t*)s; - slice.data.refcounted.length = len; - return slice; -} - -grpc_slice grpc_slice_from_static_string(const char* s) { - return grpc_slice_from_static_buffer(s, strlen(s)); -} - -namespace grpc_core { +grpc_slice_refcount kNoopRefcount(grpc_slice_refcount::Type::NOP); +static_assert(std::is_trivially_destructible::value, + "kNoopRefcount must be trivially destructible."); /* grpc_slice_new support structures - we create a refcount object extended with the user provided data pointer & destroy function */ @@ -122,6 +103,22 @@ class NewSliceRefcount { } // namespace grpc_core +size_t grpc_slice_memory_usage(grpc_slice s) { + if (s.refcount == nullptr || s.refcount == &grpc_core::kNoopRefcount) { + return 0; + } else { + return s.data.refcounted.length; + } +} + +grpc_slice grpc_slice_from_static_buffer(const void* s, size_t len) { + return grpc_slice_from_static_buffer_internal(s, len); +} + +grpc_slice grpc_slice_from_static_string(const char* s) { + return grpc_slice_from_static_buffer_internal(s, strlen(s)); +} + grpc_slice grpc_slice_new_with_user_data(void* p, size_t len, void (*destroy)(void*), void* user_data) { @@ -375,10 +372,10 @@ grpc_slice grpc_slice_split_tail_maybe_ref(grpc_slice* source, size_t split, switch (ref_whom) { case GRPC_SLICE_REF_TAIL: tail.refcount = source->refcount->sub_refcount(); - source->refcount = &NoopRefcount; + source->refcount = &grpc_core::kNoopRefcount; break; case GRPC_SLICE_REF_HEAD: - tail.refcount = &NoopRefcount; + tail.refcount = &grpc_core::kNoopRefcount; source->refcount = source->refcount->sub_refcount(); break; case GRPC_SLICE_REF_BOTH: diff --git a/src/core/lib/slice/slice_internal.h b/src/core/lib/slice/slice_internal.h index c6943fe6563..54badeb9ab9 100644 --- a/src/core/lib/slice/slice_internal.h +++ b/src/core/lib/slice/slice_internal.h @@ -171,6 +171,8 @@ struct grpc_slice_refcount { namespace grpc_core { +extern grpc_slice_refcount kNoopRefcount; + struct InternedSliceRefcount { static void Destroy(void* arg) { auto* rc = static_cast(arg); @@ -312,4 +314,17 @@ grpc_slice grpc_slice_from_moved_string(grpc_core::UniquePtr p); // 0. All other slices will return the size of the allocated chars. size_t grpc_slice_memory_usage(grpc_slice s); +inline grpc_slice grpc_slice_from_static_buffer_internal(const void* s, + size_t len) { + grpc_slice slice; + slice.refcount = &grpc_core::kNoopRefcount; + slice.data.refcounted.bytes = (uint8_t*)s; + slice.data.refcounted.length = len; + return slice; +} + +inline grpc_slice grpc_slice_from_static_string_internal(const char* s) { + return grpc_slice_from_static_buffer_internal(s, strlen(s)); +} + #endif /* GRPC_CORE_LIB_SLICE_SLICE_INTERNAL_H */ diff --git a/src/core/lib/transport/error_utils.cc b/src/core/lib/transport/error_utils.cc index eb4e8c3a282..5be98c9f04f 100644 --- a/src/core/lib/transport/error_utils.cc +++ b/src/core/lib/transport/error_utils.cc @@ -22,6 +22,7 @@ #include #include "src/core/lib/iomgr/error_internal.h" +#include "src/core/lib/slice/slice_internal.h" #include "src/core/lib/transport/status_conversion.h" static grpc_error* recursively_find_error_with_field(grpc_error* error, @@ -52,7 +53,15 @@ void grpc_error_get_status(grpc_error* error, grpc_millis deadline, if (GPR_LIKELY(error == GRPC_ERROR_NONE)) { if (code != nullptr) *code = GRPC_STATUS_OK; if (slice != nullptr) { - grpc_error_get_str(error, GRPC_ERROR_STR_GRPC_MESSAGE, slice); + // Normally, we call grpc_error_get_str( + // error, GRPC_ERROR_STR_GRPC_MESSAGE, slice). + // We can fastpath since we know that: + // 1) Error is null + // 2) which == GRPC_ERROR_STR_GRPC_MESSAGE + // 3) The resulting slice is statically known. + // 4) Said resulting slice is of length 0 (""). + // This means 3 movs, instead of 10s of instructions and a strlen. + *slice = grpc_slice_from_static_string_internal(""); } if (http_error != nullptr) { *http_error = GRPC_HTTP2_NO_ERROR; diff --git a/tools/run_tests/sanity/core_banned_functions.py b/tools/run_tests/sanity/core_banned_functions.py index ce9ff0dae21..a9c986b3ac2 100755 --- a/tools/run_tests/sanity/core_banned_functions.py +++ b/tools/run_tests/sanity/core_banned_functions.py @@ -23,6 +23,7 @@ os.chdir(os.path.join(os.path.dirname(sys.argv[0]), '../../..')) # map of banned function signature to whitelist BANNED_EXCEPT = { + 'grpc_slice_from_static_buffer(': ['src/core/lib/slice/slice.cc'], 'grpc_resource_quota_ref(': ['src/core/lib/iomgr/resource_quota.cc'], 'grpc_resource_quota_unref(': ['src/core/lib/iomgr/resource_quota.cc', 'src/core/lib/surface/server.cc'],