From ee603bf172d9905d172883106b8c64bf15beba34 Mon Sep 17 00:00:00 2001 From: Arjun Roy Date: Wed, 4 Sep 2019 20:05:38 -0700 Subject: [PATCH] Better codegenfor validate_filtered_metadata. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit validate_filtered_metadata() performs several checks to see if a call must be failed. Failure is the unlikely case; to that end, failing branches are marked unlikely, and the specific code handling failure cases is refactored into explicitly un-inlined helper methods. This will prevent us from unnecessarily clobbering registers and give us a straight-line codepath for the success case. Results: BM_UnaryPingPong/8/0 [polls/iter:3.00008 ] 22.5µs ± 0% 21.6µs ± 0% -4.02% (p=0.036 n=5+3) BM_UnaryPingPong/64/64 [polls/iter:3.00008 ] 23.4µs ± 1% 23.0µs ± 1% -1.63% (p=0.010 n=6+4) BM_UnaryPingPong/32768/0 [polls/iter:3.00007 ] 34.4µs ± 1% 34.1µs ± 0% -0.99% (p=0.024 n=6+3) BM_UnaryPingPong/0/0 [polls/iter:0 ] 6.36µs ± 5% 6.16µs ± 2% -3.26% (p=0.013 n=20+19) BM_UnaryPingPong/1/1 [polls/iter:0 ] 6.62µs ± 6% 6.50µs ± 4% -1.72% (p=0.049 n=20+20) BM_UnaryPingPong/512/0 [polls/iter:0 ] 6.67µs ± 6% 6.59µs ± 2% -1.29% (p=0.047 n=20+19) BM_UnaryPingPong/4096/0 [polls/iter:0 ] 7.68µs ± 1% 7.65µs ± 2% -0.46% (p=0.031 n=18+18) BM_UnaryPingPong/0/262144 [polls/iter:0 ] 86.0µs ± 2% 85.3µs ± 2% -0.77% (p=0.046 n=19+19) BM_UnaryPingPong/0/0 [polls/iter:0 ] 6.28µs ± 5% 6.00µs ± 2% -4.37% (p=0.000 n=20+20) BM_UnaryPingPong/1/0 [polls/iter:0 ] 6.39µs ± 6% 6.20µs ± 2% -3.03% (p=0.001 n=20+19) BM_UnaryPingPong/0/1 [polls/iter:0 ] 6.36µs ± 6% 6.17µs ± 1% -3.00% (p=0.006 n=20+17) BM_UnaryPingPong/1/1 [polls/iter:0 ] 6.59µs ± 5% 6.30µs ± 2% -4.37% (p=0.000 n=20+19) BM_UnaryPingPong/8/0 [polls/iter:0 ] 6.37µs ± 5% 6.20µs ± 2% -2.76% (p=0.001 n=20+20) BM_UnaryPingPong/0/8 [polls/iter:0 ] 6.36µs ± 5% 6.17µs ± 2% -2.95% (p=0.001 n=20+19) BM_UnaryPingPong/8/8 [polls/iter:0 ] 6.45µs ± 7% 6.27µs ± 1% -2.72% (p=0.002 n=20+18) BM_UnaryPingPong/64/0 [polls/iter:0 ] 6.46µs ± 6% 6.31µs ± 1% -2.39% (p=0.001 n=20+20) BM_UnaryPingPong/512/0 [polls/iter:0 ] 6.62µs ± 6% 6.43µs ± 2% -2.92% (p=0.000 n=20+18) BM_UnaryPingPong/0/512 [polls/iter:0 ] 6.58µs ± 7% 6.41µs ± 1% -2.57% (p=0.002 n=20+17) BM_UnaryPingPong/512/512 [polls/iter:0 ] 6.88µs ± 7% 6.76µs ± 2% -1.81% (p=0.047 n=20+19) BM_UnaryPingPong/4096/0 [polls/iter:0 ] 7.57µs ± 3% 7.49µs ± 2% -0.99% (p=0.007 n=20+20) BM_UnaryPingPong/0/4096 [polls/iter:0 ] 7.66µs ± 5% 7.50µs ± 2% -2.15% (p=0.003 n=20+20) BM_UnaryPingPong/32768/0 [polls/iter:0 ] 15.8µs ± 2% 15.7µs ± 1% -0.75% (p=0.001 n=20+19) BM_UnaryPingPong/0/32768 [polls/iter:0 ] 16.1µs ± 2% 16.0µs ± 2% -0.84% (p=0.002 n=20+20) BM_UnaryPingPong/32768/32768 [polls/iter:0 ] 25.5µs ± 1% 25.4µs ± 1% -0.42% (p=0.011 n=20+19) BM_UnaryPingPong, 2>, NoOpMutator>/0/0 [polls/iter:0 ] 7.99µs ± 5% 7.85µs ± 2% -1.81% (p=0.028 n=20+20) BM_UnaryPingPong, 1>>/0/0 [polls/iter:0 ] 7.07µs ± 6% 7.14µs ± 5% +0.95% (p=0.007 n=19+18) BM_UnaryPingPong, 1>, NoOpMutator>/0/0 [polls/iter:0 ] 6.95µs ± 5% 7.02µs ± 3% +0.94% (p=0.017 n=18+19) BM_UnaryPingPong, 1>, NoOpMutator>/0/0 [polls/iter:0 ] 7.10µs ± 2% 7.19µs ± 2% +1.31% (p=0.000 n=16+20) BM_UnaryPingPong, 1>>/0/0 [polls/iter:0 ] 6.89µs ± 2% 7.00µs ± 3% +1.61% (p=0.000 n=17+19) BM_UnaryPingPong/512/512 [polls/iter:3.00007 ] 24.1µs ± 1% 23.7µs ± 1% -1.77% (p=0.024 n=6+3) BM_UnaryPingPong/1/0 [polls/iter:3.00009 ] 21.5µs ± 1% 20.9µs ± 0% -2.78% (p=0.024 n=6+3) BM_UnaryPingPong/4096/0 [polls/iter:3.00005 ] 24.4µs ± 2% 23.9µs ± 2% -2.16% (p=0.020 n=9+4) BM_UnaryPingPong/32768/0 [polls/iter:3.0001 ] 35.3µs ± 1% 34.8µs ± 1% -1.45% (p=0.008 n=5+5) BM_UnaryPingPong/0/0 [polls/iter:3.00008 ] 19.5µs ± 1% 19.1µs ± 1% -2.30% (p=0.016 n=4+5) BM_UnaryPingPong/0/32768 [polls/iter:3.0001 ] 35.4µs ± 1% 34.7µs ± 1% -1.77% (p=0.016 n=4+5) --- src/core/lib/compression/compression.cc | 3 +- .../lib/compression/compression_internal.h | 8 ++ src/core/lib/surface/call.cc | 127 +++++++++++------- 3 files changed, 89 insertions(+), 49 deletions(-) diff --git a/src/core/lib/compression/compression.cc b/src/core/lib/compression/compression.cc index 2f35e5fa03f..8a0abca8dd7 100644 --- a/src/core/lib/compression/compression.cc +++ b/src/core/lib/compression/compression.cc @@ -127,7 +127,8 @@ void grpc_compression_options_disable_algorithm( int grpc_compression_options_is_algorithm_enabled( const grpc_compression_options* opts, grpc_compression_algorithm algorithm) { - return GPR_BITGET(opts->enabled_algorithms_bitset, algorithm); + return grpc_compression_options_is_algorithm_enabled_internal(opts, + algorithm); } grpc_slice grpc_compression_algorithm_slice( diff --git a/src/core/lib/compression/compression_internal.h b/src/core/lib/compression/compression_internal.h index 73947a2c34d..49afb941d73 100644 --- a/src/core/lib/compression/compression_internal.h +++ b/src/core/lib/compression/compression_internal.h @@ -23,6 +23,8 @@ #include +#include "src/core/lib/gpr/useful.h" + #ifdef __cplusplus extern "C" { #endif @@ -85,4 +87,10 @@ int grpc_stream_compression_algorithm_parse( } #endif +inline int grpc_compression_options_is_algorithm_enabled_internal( + const grpc_compression_options* opts, + grpc_compression_algorithm algorithm) { + return GPR_BITGET(opts->enabled_algorithms_bitset, algorithm); +} + #endif /* GRPC_CORE_LIB_COMPRESSION_COMPRESSION_INTERNAL_H */ diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index 1331e57ab0c..3c6a5d098ae 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -1365,64 +1365,95 @@ static void receiving_stream_ready_in_call_combiner(void* bctlp, receiving_stream_ready(bctlp, error); } +static void GPR_ATTRIBUTE_NOINLINE +handle_both_stream_and_msg_compression_set(grpc_call* call) { + char* error_msg = nullptr; + gpr_asprintf(&error_msg, + "Incoming stream has both stream compression (%d) and message " + "compression (%d).", + call->incoming_stream_compression_algorithm, + call->incoming_message_compression_algorithm); + gpr_log(GPR_ERROR, "%s", error_msg); + cancel_with_status(call, GRPC_STATUS_INTERNAL, error_msg); + gpr_free(error_msg); +} + +static void GPR_ATTRIBUTE_NOINLINE +handle_error_parsing_compression_algorithm(grpc_call* call) { + char* error_msg = nullptr; + gpr_asprintf(&error_msg, + "Error in incoming message compression (%d) or stream " + "compression (%d).", + call->incoming_stream_compression_algorithm, + call->incoming_message_compression_algorithm); + cancel_with_status(call, GRPC_STATUS_INTERNAL, error_msg); + gpr_free(error_msg); +} + +static void GPR_ATTRIBUTE_NOINLINE handle_invalid_compression( + grpc_call* call, grpc_compression_algorithm compression_algorithm) { + char* error_msg = nullptr; + gpr_asprintf(&error_msg, "Invalid compression algorithm value '%d'.", + compression_algorithm); + gpr_log(GPR_ERROR, "%s", error_msg); + cancel_with_status(call, GRPC_STATUS_UNIMPLEMENTED, error_msg); + gpr_free(error_msg); +} + +static void GPR_ATTRIBUTE_NOINLINE handle_compression_algorithm_disabled( + grpc_call* call, grpc_compression_algorithm compression_algorithm) { + char* error_msg = nullptr; + const char* algo_name = nullptr; + grpc_compression_algorithm_name(compression_algorithm, &algo_name); + gpr_asprintf(&error_msg, "Compression algorithm '%s' is disabled.", + algo_name); + gpr_log(GPR_ERROR, "%s", error_msg); + cancel_with_status(call, GRPC_STATUS_UNIMPLEMENTED, error_msg); + gpr_free(error_msg); +} + +static void GPR_ATTRIBUTE_NOINLINE handle_compression_algorithm_not_accepted( + grpc_call* call, grpc_compression_algorithm compression_algorithm) { + const char* algo_name = nullptr; + grpc_compression_algorithm_name(compression_algorithm, &algo_name); + gpr_log(GPR_ERROR, + "Compression algorithm ('%s') not present in the bitset of " + "accepted encodings ('0x%x')", + algo_name, call->encodings_accepted_by_peer); +} + static void validate_filtered_metadata(batch_control* bctl) { grpc_compression_algorithm compression_algorithm; grpc_call* call = bctl->call; - if (call->incoming_stream_compression_algorithm != - GRPC_STREAM_COMPRESS_NONE && - call->incoming_message_compression_algorithm != - GRPC_MESSAGE_COMPRESS_NONE) { - char* error_msg = nullptr; - gpr_asprintf(&error_msg, - "Incoming stream has both stream compression (%d) and message " - "compression (%d).", - call->incoming_stream_compression_algorithm, - call->incoming_message_compression_algorithm); - gpr_log(GPR_ERROR, "%s", error_msg); - cancel_with_status(call, GRPC_STATUS_INTERNAL, error_msg); - gpr_free(error_msg); + if (GPR_UNLIKELY(call->incoming_stream_compression_algorithm != + GRPC_STREAM_COMPRESS_NONE && + call->incoming_message_compression_algorithm != + GRPC_MESSAGE_COMPRESS_NONE)) { + handle_both_stream_and_msg_compression_set(call); } else if ( - grpc_compression_algorithm_from_message_stream_compression_algorithm( - &compression_algorithm, call->incoming_message_compression_algorithm, - call->incoming_stream_compression_algorithm) == 0) { - char* error_msg = nullptr; - gpr_asprintf(&error_msg, - "Error in incoming message compression (%d) or stream " - "compression (%d).", - call->incoming_stream_compression_algorithm, - call->incoming_message_compression_algorithm); - cancel_with_status(call, GRPC_STATUS_INTERNAL, error_msg); - gpr_free(error_msg); + GPR_UNLIKELY( + grpc_compression_algorithm_from_message_stream_compression_algorithm( + &compression_algorithm, + call->incoming_message_compression_algorithm, + call->incoming_stream_compression_algorithm) == 0)) { + handle_error_parsing_compression_algorithm(call); } else { - char* error_msg = nullptr; const grpc_compression_options compression_options = grpc_channel_compression_options(call->channel); - if (compression_algorithm >= GRPC_COMPRESS_ALGORITHMS_COUNT) { - gpr_asprintf(&error_msg, "Invalid compression algorithm value '%d'.", - compression_algorithm); - gpr_log(GPR_ERROR, "%s", error_msg); - cancel_with_status(call, GRPC_STATUS_UNIMPLEMENTED, error_msg); - } else if (grpc_compression_options_is_algorithm_enabled( - &compression_options, compression_algorithm) == 0) { + if (GPR_UNLIKELY(compression_algorithm >= GRPC_COMPRESS_ALGORITHMS_COUNT)) { + handle_invalid_compression(call, compression_algorithm); + } else if (GPR_UNLIKELY( + grpc_compression_options_is_algorithm_enabled_internal( + &compression_options, compression_algorithm) == 0)) { /* check if algorithm is supported by current channel config */ - const char* algo_name = nullptr; - grpc_compression_algorithm_name(compression_algorithm, &algo_name); - gpr_asprintf(&error_msg, "Compression algorithm '%s' is disabled.", - algo_name); - gpr_log(GPR_ERROR, "%s", error_msg); - cancel_with_status(call, GRPC_STATUS_UNIMPLEMENTED, error_msg); + handle_compression_algorithm_disabled(call, compression_algorithm); } - gpr_free(error_msg); - - GPR_ASSERT(call->encodings_accepted_by_peer != 0); - if (!GPR_BITGET(call->encodings_accepted_by_peer, compression_algorithm)) { + /* GRPC_COMPRESS_NONE is always set. */ + GPR_DEBUG_ASSERT(call->encodings_accepted_by_peer != 0); + if (GPR_UNLIKELY(!GPR_BITGET(call->encodings_accepted_by_peer, + compression_algorithm))) { if (GRPC_TRACE_FLAG_ENABLED(grpc_compression_trace)) { - const char* algo_name = nullptr; - grpc_compression_algorithm_name(compression_algorithm, &algo_name); - gpr_log(GPR_ERROR, - "Compression algorithm ('%s') not present in the bitset of " - "accepted encodings ('0x%x')", - algo_name, call->encodings_accepted_by_peer); + handle_compression_algorithm_not_accepted(call, compression_algorithm); } } }