From f65208af02714799faa6ba3ceeae5c73d0bf6dfa Mon Sep 17 00:00:00 2001 From: Arjun Roy Date: Mon, 29 Apr 2019 15:59:20 -0700 Subject: [PATCH] Added slice equality when static fastpath. --- BUILD | 1 + BUILD.gn | 2 + build.yaml | 1 + gRPC-C++.podspec | 2 + gRPC-Core.podspec | 2 + grpc.gemspec | 1 + package.xml | 1 + .../ext/transport/chttp2/transport/parsing.cc | 96 ++++++++----------- .../cronet/transport/cronet_transport.cc | 14 +-- src/core/lib/compression/compression.cc | 21 ++-- .../lib/compression/compression_internal.cc | 24 +++-- .../lib/compression/stream_compression.cc | 5 +- src/core/lib/slice/slice.cc | 25 +++++ src/core/lib/slice/slice_intern.cc | 10 +- src/core/lib/slice/slice_utils.h | 50 ++++++++++ src/core/lib/surface/call.cc | 6 +- src/core/lib/transport/metadata.h | 3 +- src/core/lib/transport/static_metadata.cc | 4 +- src/core/lib/transport/static_metadata.h | 17 ++-- tools/codegen/core/gen_static_metadata.py | 14 +-- tools/doxygen/Doxyfile.c++.internal | 1 + tools/doxygen/Doxyfile.core.internal | 1 + .../generated/sources_and_headers.json | 2 + 23 files changed, 198 insertions(+), 105 deletions(-) create mode 100644 src/core/lib/slice/slice_utils.h diff --git a/BUILD b/BUILD index 5c05c9e4549..750ebb2a48d 100644 --- a/BUILD +++ b/BUILD @@ -997,6 +997,7 @@ grpc_cc_library( "src/core/lib/slice/slice_hash_table.h", "src/core/lib/slice/slice_internal.h", "src/core/lib/slice/slice_string_helpers.h", + "src/core/lib/slice/slice_utils.h", "src/core/lib/slice/slice_weak_hash_table.h", "src/core/lib/surface/api_trace.h", "src/core/lib/surface/call.h", diff --git a/BUILD.gn b/BUILD.gn index 47b2747065d..dcd5bc880eb 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -739,6 +739,7 @@ config("grpc_config") { "src/core/lib/slice/slice_internal.h", "src/core/lib/slice/slice_string_helpers.cc", "src/core/lib/slice/slice_string_helpers.h", + "src/core/lib/slice/slice_utils.h", "src/core/lib/slice/slice_weak_hash_table.h", "src/core/lib/surface/api_trace.cc", "src/core/lib/surface/api_trace.h", @@ -1283,6 +1284,7 @@ config("grpc_config") { "src/core/lib/slice/slice_hash_table.h", "src/core/lib/slice/slice_internal.h", "src/core/lib/slice/slice_string_helpers.h", + "src/core/lib/slice/slice_utils.h", "src/core/lib/slice/slice_weak_hash_table.h", "src/core/lib/surface/api_trace.h", "src/core/lib/surface/call.h", diff --git a/build.yaml b/build.yaml index fabb041b386..c46d5ab423f 100644 --- a/build.yaml +++ b/build.yaml @@ -525,6 +525,7 @@ filegroups: - src/core/lib/slice/slice_hash_table.h - src/core/lib/slice/slice_internal.h - src/core/lib/slice/slice_string_helpers.h + - src/core/lib/slice/slice_utils.h - src/core/lib/slice/slice_weak_hash_table.h - src/core/lib/surface/api_trace.h - src/core/lib/surface/call.h diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index 1dc2258538a..316fd349da9 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -518,6 +518,7 @@ Pod::Spec.new do |s| 'src/core/lib/slice/slice_hash_table.h', 'src/core/lib/slice/slice_internal.h', 'src/core/lib/slice/slice_string_helpers.h', + 'src/core/lib/slice/slice_utils.h', 'src/core/lib/slice/slice_weak_hash_table.h', 'src/core/lib/surface/api_trace.h', 'src/core/lib/surface/call.h', @@ -721,6 +722,7 @@ Pod::Spec.new do |s| 'src/core/lib/slice/slice_hash_table.h', 'src/core/lib/slice/slice_internal.h', 'src/core/lib/slice/slice_string_helpers.h', + 'src/core/lib/slice/slice_utils.h', 'src/core/lib/slice/slice_weak_hash_table.h', 'src/core/lib/surface/api_trace.h', 'src/core/lib/surface/call.h', diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index cbf0a4b2924..c36ba13000f 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -494,6 +494,7 @@ Pod::Spec.new do |s| 'src/core/lib/slice/slice_hash_table.h', 'src/core/lib/slice/slice_internal.h', 'src/core/lib/slice/slice_string_helpers.h', + 'src/core/lib/slice/slice_utils.h', 'src/core/lib/slice/slice_weak_hash_table.h', 'src/core/lib/surface/api_trace.h', 'src/core/lib/surface/call.h', @@ -1147,6 +1148,7 @@ Pod::Spec.new do |s| 'src/core/lib/slice/slice_hash_table.h', 'src/core/lib/slice/slice_internal.h', 'src/core/lib/slice/slice_string_helpers.h', + 'src/core/lib/slice/slice_utils.h', 'src/core/lib/slice/slice_weak_hash_table.h', 'src/core/lib/surface/api_trace.h', 'src/core/lib/surface/call.h', diff --git a/grpc.gemspec b/grpc.gemspec index 6e229a5ab71..03b0116e149 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -428,6 +428,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/slice/slice_hash_table.h ) s.files += %w( src/core/lib/slice/slice_internal.h ) s.files += %w( src/core/lib/slice/slice_string_helpers.h ) + s.files += %w( src/core/lib/slice/slice_utils.h ) s.files += %w( src/core/lib/slice/slice_weak_hash_table.h ) s.files += %w( src/core/lib/surface/api_trace.h ) s.files += %w( src/core/lib/surface/call.h ) diff --git a/package.xml b/package.xml index 04d9f35cf03..5c97e0e307b 100644 --- a/package.xml +++ b/package.xml @@ -433,6 +433,7 @@ + diff --git a/src/core/ext/transport/chttp2/transport/parsing.cc b/src/core/ext/transport/chttp2/transport/parsing.cc index 15648c06fcd..1aa6d971684 100644 --- a/src/core/ext/transport/chttp2/transport/parsing.cc +++ b/src/core/ext/transport/chttp2/transport/parsing.cc @@ -28,6 +28,7 @@ #include "src/core/lib/profiling/timers.h" #include "src/core/lib/slice/slice_string_helpers.h" +#include "src/core/lib/slice/slice_utils.h" #include "src/core/lib/transport/http2_errors.h" #include "src/core/lib/transport/static_metadata.h" #include "src/core/lib/transport/status_conversion.h" @@ -410,53 +411,42 @@ static void on_initial_header(void* tp, grpc_mdelem md) { gpr_free(value); } - if (GRPC_MDELEM_STORAGE(md) == GRPC_MDELEM_STORAGE_STATIC) { - // We don't use grpc_mdelem_eq here to avoid executing additional - // instructions. The reasoning is if the payload is not equal, we already - // know that the metadata elements are not equal because the md is - // confirmed to be static. If we had used grpc_mdelem_eq here, then if the - // payloads are not equal, grpc_mdelem_eq executes more instructions to - // determine if they're equal or not. - if (md.payload == GRPC_MDELEM_GRPC_STATUS_1.payload || - md.payload == GRPC_MDELEM_GRPC_STATUS_2.payload) { - s->seen_error = true; - } - } else { - if (grpc_slice_eq(GRPC_MDKEY(md), GRPC_MDSTR_GRPC_STATUS) && - !grpc_mdelem_eq(md, GRPC_MDELEM_GRPC_STATUS_0)) { - /* TODO(ctiller): check for a status like " 0" */ - s->seen_error = true; - } - - if (grpc_slice_eq(GRPC_MDKEY(md), GRPC_MDSTR_GRPC_TIMEOUT)) { - grpc_millis* cached_timeout = static_cast( - grpc_mdelem_get_user_data(md, free_timeout)); - grpc_millis timeout; - if (cached_timeout != nullptr) { - timeout = *cached_timeout; - } else { - if (GPR_UNLIKELY( - !grpc_http2_decode_timeout(GRPC_MDVALUE(md), &timeout))) { - char* val = grpc_slice_to_c_string(GRPC_MDVALUE(md)); - gpr_log(GPR_ERROR, "Ignoring bad timeout value '%s'", val); - gpr_free(val); - timeout = GRPC_MILLIS_INF_FUTURE; - } - if (GRPC_MDELEM_IS_INTERNED(md)) { - /* store the result */ - cached_timeout = - static_cast(gpr_malloc(sizeof(grpc_millis))); - *cached_timeout = timeout; - grpc_mdelem_set_user_data(md, free_timeout, cached_timeout); - } + // If md.payload == GRPC_MDELEM_GRPC_STATUS_1 or GRPC_MDELEM_GRPC_STATUS_2, + // then we have seen an error. In fact, if it is a GRPC_STATUS and it's + // not equal to GRPC_MDELEM_GRPC_STATUS_0, then we have seen an error. + if (grpc_slice_eq_static_interned(GRPC_MDKEY(md), GRPC_MDSTR_GRPC_STATUS) && + !grpc_mdelem_static_value_eq(md, GRPC_MDELEM_GRPC_STATUS_0)) { + /* TODO(ctiller): check for a status like " 0" */ + s->seen_error = true; + } else if (grpc_slice_eq_static_interned(GRPC_MDKEY(md), + GRPC_MDSTR_GRPC_TIMEOUT)) { + grpc_millis* cached_timeout = + static_cast(grpc_mdelem_get_user_data(md, free_timeout)); + grpc_millis timeout; + if (cached_timeout != nullptr) { + timeout = *cached_timeout; + } else { + if (GPR_UNLIKELY( + !grpc_http2_decode_timeout(GRPC_MDVALUE(md), &timeout))) { + char* val = grpc_slice_to_c_string(GRPC_MDVALUE(md)); + gpr_log(GPR_ERROR, "Ignoring bad timeout value '%s'", val); + gpr_free(val); + timeout = GRPC_MILLIS_INF_FUTURE; } - if (timeout != GRPC_MILLIS_INF_FUTURE) { - grpc_chttp2_incoming_metadata_buffer_set_deadline( - &s->metadata_buffer[0], grpc_core::ExecCtx::Get()->Now() + timeout); + if (GRPC_MDELEM_IS_INTERNED(md)) { + /* store the result */ + cached_timeout = + static_cast(gpr_malloc(sizeof(grpc_millis))); + *cached_timeout = timeout; + grpc_mdelem_set_user_data(md, free_timeout, cached_timeout); } - GRPC_MDELEM_UNREF(md); - return; } + if (timeout != GRPC_MILLIS_INF_FUTURE) { + grpc_chttp2_incoming_metadata_buffer_set_deadline( + &s->metadata_buffer[0], grpc_core::ExecCtx::Get()->Now() + timeout); + } + GRPC_MDELEM_UNREF(md); + return; } const size_t new_size = s->metadata_buffer[0].size + GRPC_MDELEM_LENGTH(md); @@ -506,19 +496,11 @@ static void on_trailing_header(void* tp, grpc_mdelem md) { gpr_free(value); } - if (GRPC_MDELEM_STORAGE(md) == GRPC_MDELEM_STORAGE_STATIC) { - // We don't use grpc_mdelem_eq here to avoid executing additional - // instructions. The reasoning is if the payload is not equal, we already - // know that the metadata elements are not equal because the md is - // confirmed to be static. If we had used grpc_mdelem_eq here, then if the - // payloads are not equal, grpc_mdelem_eq executes more instructions to - // determine if they're equal or not. - if (md.payload == GRPC_MDELEM_GRPC_STATUS_1.payload || - md.payload == GRPC_MDELEM_GRPC_STATUS_2.payload) { - s->seen_error = true; - } - } else if (grpc_slice_eq(GRPC_MDKEY(md), GRPC_MDSTR_GRPC_STATUS) && - !grpc_mdelem_eq(md, GRPC_MDELEM_GRPC_STATUS_0)) { + // If md.payload == GRPC_MDELEM_GRPC_STATUS_1 or GRPC_MDELEM_GRPC_STATUS_2, + // then we have seen an error. In fact, if it is a GRPC_STATUS and it's + // not equal to GRPC_MDELEM_GRPC_STATUS_0, then we have seen an error. + if (grpc_slice_eq_static_interned(GRPC_MDKEY(md), GRPC_MDSTR_GRPC_STATUS) && + !grpc_mdelem_static_value_eq(md, GRPC_MDELEM_GRPC_STATUS_0)) { /* TODO(ctiller): check for a status like " 0" */ s->seen_error = true; } diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.cc b/src/core/ext/transport/cronet/transport/cronet_transport.cc index 320b5297251..413de807638 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.cc +++ b/src/core/ext/transport/cronet/transport/cronet_transport.cc @@ -753,15 +753,16 @@ static void convert_metadata_to_cronet_headers( } else { value = grpc_slice_to_c_string(GRPC_MDVALUE(mdelem)); } - if (grpc_slice_eq(GRPC_MDKEY(mdelem), GRPC_MDSTR_SCHEME) || - grpc_slice_eq(GRPC_MDKEY(mdelem), GRPC_MDSTR_AUTHORITY)) { + if (grpc_slice_eq_static_interned(GRPC_MDKEY(mdelem), GRPC_MDSTR_SCHEME) || + grpc_slice_eq_static_interned(GRPC_MDKEY(mdelem), + GRPC_MDSTR_AUTHORITY)) { /* Cronet populates these fields on its own */ gpr_free(key); gpr_free(value); continue; } - if (grpc_slice_eq(GRPC_MDKEY(mdelem), GRPC_MDSTR_METHOD)) { - if (grpc_slice_eq(GRPC_MDVALUE(mdelem), GRPC_MDSTR_PUT)) { + if (grpc_slice_eq_static_interned(GRPC_MDKEY(mdelem), GRPC_MDSTR_METHOD)) { + if (grpc_slice_eq_static_interned(GRPC_MDVALUE(mdelem), GRPC_MDSTR_PUT)) { *method = "PUT"; } else { /* POST method in default*/ @@ -771,7 +772,7 @@ static void convert_metadata_to_cronet_headers( gpr_free(value); continue; } - if (grpc_slice_eq(GRPC_MDKEY(mdelem), GRPC_MDSTR_PATH)) { + if (grpc_slice_eq_static_interned(GRPC_MDKEY(mdelem), GRPC_MDSTR_PATH)) { /* Create URL by appending :path value to the hostname */ gpr_asprintf(pp_url, "https://%s%s", host, value); gpr_free(key); @@ -803,7 +804,8 @@ static void parse_grpc_header(const uint8_t* data, int* length, static bool header_has_authority(grpc_linked_mdelem* head) { while (head != nullptr) { - if (grpc_slice_eq(GRPC_MDKEY(head->md), GRPC_MDSTR_AUTHORITY)) { + if (grpc_slice_eq_static_interned(GRPC_MDKEY(head->md), + GRPC_MDSTR_AUTHORITY)) { return true; } head = head->next; diff --git a/src/core/lib/compression/compression.cc b/src/core/lib/compression/compression.cc index 9139fa04ee5..a3a069d9266 100644 --- a/src/core/lib/compression/compression.cc +++ b/src/core/lib/compression/compression.cc @@ -26,6 +26,7 @@ #include "src/core/lib/compression/algorithm_metadata.h" #include "src/core/lib/compression/compression_internal.h" #include "src/core/lib/gpr/useful.h" +#include "src/core/lib/slice/slice_utils.h" #include "src/core/lib/surface/api_trace.h" #include "src/core/lib/transport/static_metadata.h" @@ -42,16 +43,17 @@ int grpc_compression_algorithm_is_stream(grpc_compression_algorithm algorithm) { int grpc_compression_algorithm_parse(grpc_slice name, grpc_compression_algorithm* algorithm) { - if (grpc_slice_eq(name, GRPC_MDSTR_IDENTITY)) { + if (grpc_slice_eq_static_interned(name, GRPC_MDSTR_IDENTITY)) { *algorithm = GRPC_COMPRESS_NONE; return 1; - } else if (grpc_slice_eq(name, GRPC_MDSTR_DEFLATE)) { + } else if (grpc_slice_eq_static_interned(name, GRPC_MDSTR_DEFLATE)) { *algorithm = GRPC_COMPRESS_DEFLATE; return 1; - } else if (grpc_slice_eq(name, GRPC_MDSTR_GZIP)) { + } else if (grpc_slice_eq_static_interned(name, GRPC_MDSTR_GZIP)) { *algorithm = GRPC_COMPRESS_GZIP; return 1; - } else if (grpc_slice_eq(name, GRPC_MDSTR_STREAM_SLASH_GZIP)) { + } else if (grpc_slice_eq_static_interned(name, + GRPC_MDSTR_STREAM_SLASH_GZIP)) { *algorithm = GRPC_COMPRESS_STREAM_GZIP; return 1; } else { @@ -148,10 +150,13 @@ grpc_slice grpc_compression_algorithm_slice( grpc_compression_algorithm grpc_compression_algorithm_from_slice( const grpc_slice& str) { - if (grpc_slice_eq(str, GRPC_MDSTR_IDENTITY)) return GRPC_COMPRESS_NONE; - if (grpc_slice_eq(str, GRPC_MDSTR_DEFLATE)) return GRPC_COMPRESS_DEFLATE; - if (grpc_slice_eq(str, GRPC_MDSTR_GZIP)) return GRPC_COMPRESS_GZIP; - if (grpc_slice_eq(str, GRPC_MDSTR_STREAM_SLASH_GZIP)) + if (grpc_slice_eq_static_interned(str, GRPC_MDSTR_IDENTITY)) + return GRPC_COMPRESS_NONE; + if (grpc_slice_eq_static_interned(str, GRPC_MDSTR_DEFLATE)) + return GRPC_COMPRESS_DEFLATE; + if (grpc_slice_eq_static_interned(str, GRPC_MDSTR_GZIP)) + return GRPC_COMPRESS_GZIP; + if (grpc_slice_eq_static_interned(str, GRPC_MDSTR_STREAM_SLASH_GZIP)) return GRPC_COMPRESS_STREAM_GZIP; return GRPC_COMPRESS_ALGORITHMS_COUNT; } diff --git a/src/core/lib/compression/compression_internal.cc b/src/core/lib/compression/compression_internal.cc index 65a36de4290..e0d73ef6d83 100644 --- a/src/core/lib/compression/compression_internal.cc +++ b/src/core/lib/compression/compression_internal.cc @@ -26,6 +26,7 @@ #include "src/core/lib/compression/algorithm_metadata.h" #include "src/core/lib/compression/compression_internal.h" #include "src/core/lib/gpr/useful.h" +#include "src/core/lib/slice/slice_utils.h" #include "src/core/lib/surface/api_trace.h" #include "src/core/lib/transport/static_metadata.h" @@ -33,18 +34,21 @@ grpc_message_compression_algorithm grpc_message_compression_algorithm_from_slice(const grpc_slice& str) { - if (grpc_slice_eq(str, GRPC_MDSTR_IDENTITY)) + if (grpc_slice_eq_static_interned(str, GRPC_MDSTR_IDENTITY)) return GRPC_MESSAGE_COMPRESS_NONE; - if (grpc_slice_eq(str, GRPC_MDSTR_DEFLATE)) + if (grpc_slice_eq_static_interned(str, GRPC_MDSTR_DEFLATE)) return GRPC_MESSAGE_COMPRESS_DEFLATE; - if (grpc_slice_eq(str, GRPC_MDSTR_GZIP)) return GRPC_MESSAGE_COMPRESS_GZIP; + if (grpc_slice_eq_static_interned(str, GRPC_MDSTR_GZIP)) + return GRPC_MESSAGE_COMPRESS_GZIP; return GRPC_MESSAGE_COMPRESS_ALGORITHMS_COUNT; } grpc_stream_compression_algorithm grpc_stream_compression_algorithm_from_slice( const grpc_slice& str) { - if (grpc_slice_eq(str, GRPC_MDSTR_IDENTITY)) return GRPC_STREAM_COMPRESS_NONE; - if (grpc_slice_eq(str, GRPC_MDSTR_GZIP)) return GRPC_STREAM_COMPRESS_GZIP; + if (grpc_slice_eq_static_interned(str, GRPC_MDSTR_IDENTITY)) + return GRPC_STREAM_COMPRESS_NONE; + if (grpc_slice_eq_static_interned(str, GRPC_MDSTR_GZIP)) + return GRPC_STREAM_COMPRESS_GZIP; return GRPC_STREAM_COMPRESS_ALGORITHMS_COUNT; } @@ -244,13 +248,13 @@ grpc_message_compression_algorithm grpc_message_compression_algorithm_for_level( int grpc_message_compression_algorithm_parse( grpc_slice value, grpc_message_compression_algorithm* algorithm) { - if (grpc_slice_eq(value, GRPC_MDSTR_IDENTITY)) { + if (grpc_slice_eq_static_interned(value, GRPC_MDSTR_IDENTITY)) { *algorithm = GRPC_MESSAGE_COMPRESS_NONE; return 1; - } else if (grpc_slice_eq(value, GRPC_MDSTR_DEFLATE)) { + } else if (grpc_slice_eq_static_interned(value, GRPC_MDSTR_DEFLATE)) { *algorithm = GRPC_MESSAGE_COMPRESS_DEFLATE; return 1; - } else if (grpc_slice_eq(value, GRPC_MDSTR_GZIP)) { + } else if (grpc_slice_eq_static_interned(value, GRPC_MDSTR_GZIP)) { *algorithm = GRPC_MESSAGE_COMPRESS_GZIP; return 1; } else { @@ -263,10 +267,10 @@ int grpc_message_compression_algorithm_parse( int grpc_stream_compression_algorithm_parse( grpc_slice value, grpc_stream_compression_algorithm* algorithm) { - if (grpc_slice_eq(value, GRPC_MDSTR_IDENTITY)) { + if (grpc_slice_eq_static_interned(value, GRPC_MDSTR_IDENTITY)) { *algorithm = GRPC_STREAM_COMPRESS_NONE; return 1; - } else if (grpc_slice_eq(value, GRPC_MDSTR_GZIP)) { + } else if (grpc_slice_eq_static_interned(value, GRPC_MDSTR_GZIP)) { *algorithm = GRPC_STREAM_COMPRESS_GZIP; return 1; } else { diff --git a/src/core/lib/compression/stream_compression.cc b/src/core/lib/compression/stream_compression.cc index 46cb3daf4cc..e0857988643 100644 --- a/src/core/lib/compression/stream_compression.cc +++ b/src/core/lib/compression/stream_compression.cc @@ -22,6 +22,7 @@ #include "src/core/lib/compression/stream_compression.h" #include "src/core/lib/compression/stream_compression_gzip.h" +#include "src/core/lib/slice/slice_utils.h" extern const grpc_stream_compression_vtable grpc_stream_compression_identity_vtable; @@ -65,11 +66,11 @@ void grpc_stream_compression_context_destroy( int grpc_stream_compression_method_parse( grpc_slice value, bool is_compress, grpc_stream_compression_method* method) { - if (grpc_slice_eq(value, GRPC_MDSTR_IDENTITY)) { + if (grpc_slice_eq_static_interned(value, GRPC_MDSTR_IDENTITY)) { *method = is_compress ? GRPC_STREAM_COMPRESSION_IDENTITY_COMPRESS : GRPC_STREAM_COMPRESSION_IDENTITY_DECOMPRESS; return 1; - } else if (grpc_slice_eq(value, GRPC_MDSTR_GZIP)) { + } else if (grpc_slice_eq_static_interned(value, GRPC_MDSTR_GZIP)) { *method = is_compress ? GRPC_STREAM_COMPRESSION_GZIP_COMPRESS : GRPC_STREAM_COMPRESSION_GZIP_DECOMPRESS; return 1; diff --git a/src/core/lib/slice/slice.cc b/src/core/lib/slice/slice.cc index 2bf2b0f499f..75acb28e99c 100644 --- a/src/core/lib/slice/slice.cc +++ b/src/core/lib/slice/slice.cc @@ -410,6 +410,31 @@ int grpc_slice_eq(grpc_slice a, grpc_slice b) { return grpc_slice_default_eq_impl(a, b); } +int grpc_slice_differs_refcounted(const grpc_slice& a, + const grpc_slice& b_not_inline) { + size_t a_len; + const uint8_t* a_ptr; + if (a.refcount) { + a_len = a.data.refcounted.length; + a_ptr = a.data.refcounted.bytes; + } else { + a_len = a.data.inlined.length; + a_ptr = &a.data.inlined.bytes[0]; + } + if (a_len != b_not_inline.data.refcounted.length) { + return true; + } + if (a_len == 0) { + return false; + } + // This check *must* occur after the a_len == 0 check + // to retain compatibility with grpc_slice_eq. + if (a_ptr == nullptr) { + return true; + } + return memcmp(a_ptr, b_not_inline.data.refcounted.bytes, a_len); +} + int grpc_slice_cmp(grpc_slice a, grpc_slice b) { int d = static_cast(GRPC_SLICE_LENGTH(a) - GRPC_SLICE_LENGTH(b)); if (d != 0) return d; diff --git a/src/core/lib/slice/slice_intern.cc b/src/core/lib/slice/slice_intern.cc index 0f190c186fa..15909d9b96f 100644 --- a/src/core/lib/slice/slice_intern.cc +++ b/src/core/lib/slice/slice_intern.cc @@ -19,6 +19,7 @@ #include #include "src/core/lib/slice/slice_internal.h" +#include "src/core/lib/slice/slice_utils.h" #include #include @@ -143,7 +144,8 @@ grpc_slice grpc_slice_maybe_static_intern(grpc_slice slice, static_metadata_hash_ent ent = static_metadata_hash[(hash + i) % GPR_ARRAY_SIZE(static_metadata_hash)]; if (ent.hash == hash && ent.idx < GRPC_STATIC_MDSTR_COUNT && - grpc_slice_eq(grpc_static_slice_table[ent.idx], slice)) { + grpc_slice_eq_static_interned(slice, + grpc_static_slice_table[ent.idx])) { *returned_slice_is_different = true; return grpc_static_slice_table[ent.idx]; } @@ -170,7 +172,8 @@ grpc_slice grpc_slice_intern(grpc_slice slice) { static_metadata_hash_ent ent = static_metadata_hash[(hash + i) % GPR_ARRAY_SIZE(static_metadata_hash)]; if (ent.hash == hash && ent.idx < GRPC_STATIC_MDSTR_COUNT && - grpc_slice_eq(grpc_static_slice_table[ent.idx], slice)) { + grpc_slice_eq_static_interned(slice, + grpc_static_slice_table[ent.idx])) { return grpc_static_slice_table[ent.idx]; } } @@ -183,7 +186,8 @@ grpc_slice grpc_slice_intern(grpc_slice slice) { /* search for an existing string */ size_t idx = TABLE_IDX(hash, shard->capacity); for (s = shard->strs[idx]; s; s = s->bucket_next) { - if (s->hash == hash && grpc_slice_eq(slice, materialize(s))) { + if (s->hash == hash && + grpc_slice_eq_static_interned(slice, materialize(s))) { if (s->refcnt.RefIfNonZero()) { gpr_mu_unlock(&shard->mu); return materialize(s); diff --git a/src/core/lib/slice/slice_utils.h b/src/core/lib/slice/slice_utils.h new file mode 100644 index 00000000000..7589bf8f0f7 --- /dev/null +++ b/src/core/lib/slice/slice_utils.h @@ -0,0 +1,50 @@ +/* + * + * Copyright 2019 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#ifndef GRPC_CORE_LIB_SLICE_SLICE_UTILS_H +#define GRPC_CORE_LIB_SLICE_SLICE_UTILS_H + +#include + +#include + +// When we compare two slices, and we know the latter is not inlined, we can +// short circuit our comparison operator. We specifically use differs() +// semantics instead of equals() semantics due to more favourable code +// generation when using differs(). Specifically, we may use the output of +// grpc_slice_differs_refcounted for control flow. If we use differs() +// semantics, we end with a tailcall to memcmp(). If we use equals() semantics, +// we need to invert the result that memcmp provides us, which costs several +// instructions to do so. If we're using the result for control flow (i.e. +// branching based on the output) then we're just performing the extra +// operations to invert the result pointlessly. Concretely, we save 6 ops on +// x86-64/clang with differs(). +int grpc_slice_differs_refcounted(const grpc_slice& a, + const grpc_slice& b_not_inline); +// When we compare two slices, and we *know* that one of them is static or +// interned, we can short circuit our slice equality function. The second slice +// here must be static or interned; slice a can be any slice, inlined or not. +inline bool grpc_slice_eq_static_interned(const grpc_slice& a, + const grpc_slice& b_static_interned) { + if (a.refcount == b_static_interned.refcount) { + return true; + } + return !grpc_slice_differs_refcounted(a, b_static_interned); +} + +#endif /* GRPC_CORE_LIB_SLICE_SLICE_UTILS_H */ diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index 275111f3213..254476f47e3 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -42,8 +42,8 @@ #include "src/core/lib/gprpp/ref_counted.h" #include "src/core/lib/iomgr/timer.h" #include "src/core/lib/profiling/timers.h" -#include "src/core/lib/slice/slice_internal.h" #include "src/core/lib/slice/slice_string_helpers.h" +#include "src/core/lib/slice/slice_utils.h" #include "src/core/lib/surface/api_trace.h" #include "src/core/lib/surface/call.h" #include "src/core/lib/surface/call_test_only.h" @@ -349,8 +349,8 @@ grpc_error* grpc_call_create(const grpc_call_create_args* args, MAX_SEND_EXTRA_METADATA_COUNT); for (size_t i = 0; i < args->add_initial_metadata_count; i++) { call->send_extra_metadata[i].md = args->add_initial_metadata[i]; - if (grpc_slice_eq(GRPC_MDKEY(args->add_initial_metadata[i]), - GRPC_MDSTR_PATH)) { + if (grpc_slice_eq_static_interned( + GRPC_MDKEY(args->add_initial_metadata[i]), GRPC_MDSTR_PATH)) { path = grpc_slice_ref_internal( GRPC_MDVALUE(args->add_initial_metadata[i])); } diff --git a/src/core/lib/transport/metadata.h b/src/core/lib/transport/metadata.h index c0d1ab36351..3ff5a35dd2d 100644 --- a/src/core/lib/transport/metadata.h +++ b/src/core/lib/transport/metadata.h @@ -30,6 +30,7 @@ #include "src/core/lib/gpr/useful.h" #include "src/core/lib/gprpp/atomic.h" #include "src/core/lib/gprpp/sync.h" +#include "src/core/lib/slice/slice_utils.h" extern grpc_core::DebugOnlyTraceFlag grpc_trace_metadata; @@ -138,7 +139,7 @@ bool grpc_mdelem_eq(grpc_mdelem a, grpc_mdelem b); * grpc_mdelem_eq and remove unnecessary checks. */ inline bool grpc_mdelem_static_value_eq(grpc_mdelem a, grpc_mdelem b_static) { if (a.payload == b_static.payload) return true; - return grpc_slice_eq(GRPC_MDVALUE(a), GRPC_MDVALUE(b_static)); + return grpc_slice_eq_static_interned(GRPC_MDVALUE(a), GRPC_MDVALUE(b_static)); } /* Mutator and accessor for grpc_mdelem user data. The destructor function diff --git a/src/core/lib/transport/static_metadata.cc b/src/core/lib/transport/static_metadata.cc index fc5bb647573..61ee1d46f77 100644 --- a/src/core/lib/transport/static_metadata.cc +++ b/src/core/lib/transport/static_metadata.cc @@ -384,9 +384,9 @@ static const uint8_t elem_idxs[] = { 52, 53, 54, 76, 69, 55, 56, 70, 58, 78, 80, 81, 82, 83, 59, 64, 60, 75, 72, 255, 85, 255, 255, 68, 255, 255, 0}; -grpc_mdelem grpc_static_mdelem_for_static_strings(int a, int b) { +grpc_mdelem grpc_static_mdelem_for_static_strings(intptr_t a, intptr_t b) { if (a == -1 || b == -1) return GRPC_MDNULL; - uint32_t k = (uint32_t)(a * 107 + b); + uint32_t k = static_cast(a * 107 + b); uint32_t h = elems_phash(k); return h < GPR_ARRAY_SIZE(elem_keys) && elem_keys[h] == k && elem_idxs[h] != 255 diff --git a/src/core/lib/transport/static_metadata.h b/src/core/lib/transport/static_metadata.h index 88293ae0613..a3f23de0953 100644 --- a/src/core/lib/transport/static_metadata.h +++ b/src/core/lib/transport/static_metadata.h @@ -29,6 +29,8 @@ #include +#include + #include "src/core/lib/transport/metadata.h" #define GRPC_STATIC_MDSTR_COUNT 107 @@ -263,7 +265,8 @@ extern grpc_slice_refcount (slice).refcount->GetType() == grpc_slice_refcount::Type::STATIC) #define GRPC_STATIC_METADATA_INDEX(static_slice) \ - ((int)((static_slice).refcount - grpc_static_metadata_refcounts)) + (static_cast( \ + ((static_slice).refcount - grpc_static_metadata_refcounts))) #define GRPC_STATIC_MDELEM_COUNT 86 extern grpc_mdelem_data grpc_static_mdelem_table[GRPC_STATIC_MDELEM_COUNT]; @@ -527,7 +530,7 @@ extern uintptr_t grpc_static_mdelem_user_data[GRPC_STATIC_MDELEM_COUNT]; #define GRPC_MDELEM_ACCEPT_ENCODING_IDENTITY_COMMA_GZIP \ (GRPC_MAKE_MDELEM(&grpc_static_mdelem_table[85], GRPC_MDELEM_STORAGE_STATIC)) -grpc_mdelem grpc_static_mdelem_for_static_strings(int a, int b); +grpc_mdelem grpc_static_mdelem_for_static_strings(intptr_t a, intptr_t b); typedef enum { GRPC_BATCH_PATH, GRPC_BATCH_METHOD, @@ -586,11 +589,11 @@ typedef union { } named; } grpc_metadata_batch_callouts; -#define GRPC_BATCH_INDEX_OF(slice) \ - (GRPC_IS_STATIC_METADATA_STRING((slice)) \ - ? (grpc_metadata_batch_callouts_index)GPR_CLAMP( \ - GRPC_STATIC_METADATA_INDEX((slice)), 0, \ - GRPC_BATCH_CALLOUTS_COUNT) \ +#define GRPC_BATCH_INDEX_OF(slice) \ + (GRPC_IS_STATIC_METADATA_STRING((slice)) \ + ? static_cast( \ + GPR_CLAMP(GRPC_STATIC_METADATA_INDEX((slice)), 0, \ + static_cast(GRPC_BATCH_CALLOUTS_COUNT))) \ : GRPC_BATCH_CALLOUTS_COUNT) extern const uint8_t grpc_static_accept_encoding_metadata[8]; diff --git a/tools/codegen/core/gen_static_metadata.py b/tools/codegen/core/gen_static_metadata.py index 5545e871117..d66fdc3e835 100755 --- a/tools/codegen/core/gen_static_metadata.py +++ b/tools/codegen/core/gen_static_metadata.py @@ -376,6 +376,8 @@ print >> H, '#define GRPC_CORE_LIB_TRANSPORT_STATIC_METADATA_H' print >> H print >> H, '#include ' print >> H +print >> H, '#include ' +print >> H print >> H, '#include "src/core/lib/transport/metadata.h"' print >> H print >> C, '#include ' @@ -433,8 +435,8 @@ for i, elem in enumerate(all_strs): print >> C, '};' print >> C print >> H, '#define GRPC_STATIC_METADATA_INDEX(static_slice) \\' -print >> H, (' ((int)((static_slice).refcount - ' - 'grpc_static_metadata_refcounts))') +print >> H, (' (static_cast(((static_slice).refcount - ' + 'grpc_static_metadata_refcounts)))') print >> H print >> D, '# hpack fuzzing dictionary' @@ -537,10 +539,10 @@ print >> C, 'static const uint8_t elem_idxs[] = {%s};' % ','.join( '%d' % i for i in idxs) print >> C -print >> H, 'grpc_mdelem grpc_static_mdelem_for_static_strings(int a, int b);' -print >> C, 'grpc_mdelem grpc_static_mdelem_for_static_strings(int a, int b) {' +print >> H, 'grpc_mdelem grpc_static_mdelem_for_static_strings(intptr_t a, intptr_t b);' +print >> C, 'grpc_mdelem grpc_static_mdelem_for_static_strings(intptr_t a, intptr_t b) {' print >> C, ' if (a == -1 || b == -1) return GRPC_MDNULL;' -print >> C, ' uint32_t k = (uint32_t)(a * %d + b);' % len(all_strs) +print >> C, ' uint32_t k = static_cast(a * %d + b);' % len(all_strs) print >> C, ' uint32_t h = elems_phash(k);' print >> C, ' return h < GPR_ARRAY_SIZE(elem_keys) && elem_keys[h] == k && elem_idxs[h] != 255 ? GRPC_MAKE_MDELEM(&grpc_static_mdelem_table[elem_idxs[h]], GRPC_MDELEM_STORAGE_STATIC) : GRPC_MDNULL;' print >> C, '}' @@ -566,7 +568,7 @@ print >> H, ' } named;' print >> H, '} grpc_metadata_batch_callouts;' print >> H print >> H, '#define GRPC_BATCH_INDEX_OF(slice) \\' -print >> H, ' (GRPC_IS_STATIC_METADATA_STRING((slice)) ? (grpc_metadata_batch_callouts_index)GPR_CLAMP(GRPC_STATIC_METADATA_INDEX((slice)), 0, GRPC_BATCH_CALLOUTS_COUNT) : GRPC_BATCH_CALLOUTS_COUNT)' +print >> H, ' (GRPC_IS_STATIC_METADATA_STRING((slice)) ? static_cast(GPR_CLAMP(GRPC_STATIC_METADATA_INDEX((slice)), 0, static_cast(GRPC_BATCH_CALLOUTS_COUNT))) : GRPC_BATCH_CALLOUTS_COUNT)' print >> H print >> H, 'extern const uint8_t grpc_static_accept_encoding_metadata[%d];' % ( diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index ab0968b3252..489e76efd89 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -1188,6 +1188,7 @@ src/core/lib/slice/percent_encoding.h \ src/core/lib/slice/slice_hash_table.h \ src/core/lib/slice/slice_internal.h \ src/core/lib/slice/slice_string_helpers.h \ +src/core/lib/slice/slice_utils.h \ src/core/lib/slice/slice_weak_hash_table.h \ src/core/lib/surface/api_trace.h \ src/core/lib/surface/call.h \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index 293a6e1f9c1..f7c268a8105 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -1451,6 +1451,7 @@ src/core/lib/slice/slice_intern.cc \ src/core/lib/slice/slice_internal.h \ src/core/lib/slice/slice_string_helpers.cc \ src/core/lib/slice/slice_string_helpers.h \ +src/core/lib/slice/slice_utils.h \ src/core/lib/slice/slice_weak_hash_table.h \ src/core/lib/surface/README.md \ src/core/lib/surface/api_trace.cc \ diff --git a/tools/run_tests/generated/sources_and_headers.json b/tools/run_tests/generated/sources_and_headers.json index de8015ac52a..e67465d1602 100644 --- a/tools/run_tests/generated/sources_and_headers.json +++ b/tools/run_tests/generated/sources_and_headers.json @@ -8574,6 +8574,7 @@ "src/core/lib/slice/slice_hash_table.h", "src/core/lib/slice/slice_internal.h", "src/core/lib/slice/slice_string_helpers.h", + "src/core/lib/slice/slice_utils.h", "src/core/lib/slice/slice_weak_hash_table.h", "src/core/lib/surface/api_trace.h", "src/core/lib/surface/call.h", @@ -8730,6 +8731,7 @@ "src/core/lib/slice/slice_hash_table.h", "src/core/lib/slice/slice_internal.h", "src/core/lib/slice/slice_string_helpers.h", + "src/core/lib/slice/slice_utils.h", "src/core/lib/slice/slice_weak_hash_table.h", "src/core/lib/surface/api_trace.h", "src/core/lib/surface/call.h",