From c63f419c4900399a45f5de0f45aedc95fa331f80 Mon Sep 17 00:00:00 2001 From: Arjun Roy Date: Thu, 5 Sep 2019 12:52:44 -0700 Subject: [PATCH] Mark CH2 on_initial_header error path unlikely. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Yields slightly better unary and streaming performance for TCP: BM_UnaryPingPong/4096/4096 [polls/iter:3.00006 ] 27.1µs ± 2% 26.3µs ± 1% -2.77% (p=0.036 n=5+3) BM_UnaryPingPong/1/0 [polls/iter:3.00009 ] 21.7µs ± 2% 21.1µs ± 1% -2.88% (p=0.029 n=4+4) BM_UnaryPingPong/0/1 [polls/iter:3.00009 ] 21.8µs ± 2% 20.9µs ± 1% -4.32% (p=0.003 n=7+5) BM_UnaryPingPong/1/1 [polls/iter:3.00008 ] 22.0µs ± 1% 21.3µs ± 1% -3.15% (p=0.036 n=3+5) BM_UnaryPingPong/64/0 [polls/iter:3.00006 ] 22.0µs ± 1% 21.5µs ± 1% -2.19% (p=0.032 n=4+5) BM_UnaryPingPong/32768/0 [polls/iter:3.00007 ] 34.7µs ± 1% 34.1µs ± 0% -1.72% (p=0.017 n=7+3) BM_UnaryPingPong/0/262144 [polls/iter:3.00023 ] 160µs ± 1% 158µs ± 1% -1.29% (p=0.016 n=8+4) BM_UnaryPingPong/0/0 [polls/iter:3.00012 ] 20.8µs ± 1% 20.4µs ± 0% -1.89% (p=0.029 n=4+4) BM_UnaryPingPong/0/0 [polls/iter:3.00008 ] 22.1µs ± 4% 21.3µs ± 0% -3.88% (p=0.004 n=6+5) BM_UnaryPingPong/64/0 [polls/iter:3.00008 ] 23.2µs ± 2% 22.5µs ± 3% -3.07% (p=0.014 n=7+6) BM_UnaryPingPong/512/512 [polls/iter:3.0001 ] 23.5µs ± 2% 22.9µs ± 0% -2.85% (p=0.010 n=6+4) BM_UnaryPingPong/1/0 [polls/iter:3.00008 ] 22.5µs ± 1% 21.7µs ± 1% -3.35% (p=0.036 n=3+5) BM_UnaryPingPong/32768/32768 [polls/iter:3.0001 ] 48.6µs ± 1% 48.3µs ± 1% -0.58% (p=0.045 n=5+8) BM_UnaryPingPong/8/8 [polls/iter:3.00008 ] 22.0µs ± 1% 21.5µs ± 1% -2.35% (p=0.016 n=4+5) BM_UnaryPingPong/8/8 [polls/iter:3.00006 ] 22.4µs ± 3% 21.4µs ± 1% -4.05% (p=0.017 n=7+3) BM_UnaryPingPong/4096/0 [polls/iter:3.00007 ] 24.5µs ± 1% 23.9µs ± 1% -2.30% BM_UnaryPingPong/1/1 [polls/iter:3.0001 ] 22.9µs ± 2% 22.4µs ± 3% -2.04% (p=0.048 n=7+5) BM_UnaryPingPong/8/8 [polls/iter:3.0001 ] 23.0µs ± 2% 22.4µs ± 1% -2.75% (p=0.012 n=7+4) BM_UnaryPingPong/64/64 [polls/iter:3.00008 ] 23.5µs ± 2% 23.1µs ± 0% -2.10% (p=0.002 n=8+5) BM_UnaryPingPong/64/0 [polls/iter:3.00008 ] 22.1µs ± 2% 21.5µs ± 1% -2.93% (p=0.009 n=9+3) BM_UnaryPingPong/0/64 [polls/iter:3.00008 ] 22.2µs ± 1% 21.4µs ± 1% -3.51% (p=0.003 n=4+9) BM_UnaryPingPong/512/0 [polls/iter:3.00008 ] 22.4µs ± 2% 21.8µs ± 1% -2.75% (p=0.009 n=5+6) BM_UnaryPingPong/32768/0 [polls/iter:3.0001 ] 34.5µs ± 1% 34.0µs ± 1% -1.58% But, slightly worse performance for in-proc (about 2-3%). --- .../ext/transport/chttp2/transport/parsing.cc | 129 +++++++++++------- 1 file changed, 77 insertions(+), 52 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/parsing.cc b/src/core/ext/transport/chttp2/transport/parsing.cc index a39f6e83db7..e789006ed31 100644 --- a/src/core/ext/transport/chttp2/transport/parsing.cc +++ b/src/core/ext/transport/chttp2/transport/parsing.cc @@ -422,6 +422,76 @@ static bool is_nonzero_status(grpc_mdelem md) { !md_cmp(md, GRPC_MDELEM_GRPC_STATUS_0, GRPC_MDSTR_GRPC_STATUS); } +static void GPR_ATTRIBUTE_NOINLINE on_initial_header_log( + grpc_chttp2_transport* t, grpc_chttp2_stream* s, grpc_mdelem md) { + char* key = grpc_slice_to_c_string(GRPC_MDKEY(md)); + char* value = + grpc_dump_slice(GRPC_MDVALUE(md), GPR_DUMP_HEX | GPR_DUMP_ASCII); + gpr_log(GPR_INFO, "HTTP:%d:HDR:%s: %s: %s", s->id, + t->is_client ? "CLI" : "SVR", key, value); + gpr_free(key); + gpr_free(value); +} + +static grpc_error* GPR_ATTRIBUTE_NOINLINE handle_timeout(grpc_chttp2_stream* s, + grpc_mdelem md) { + 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 (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 GRPC_ERROR_NONE; +} + +static grpc_error* GPR_ATTRIBUTE_NOINLINE handle_metadata_size_limit_exceeded( + grpc_chttp2_transport* t, grpc_chttp2_stream* s, grpc_mdelem md, + size_t new_size, size_t metadata_size_limit) { + gpr_log(GPR_DEBUG, + "received initial metadata size exceeds limit (%" PRIuPTR + " vs. %" PRIuPTR ")", + new_size, metadata_size_limit); + grpc_chttp2_cancel_stream( + t, s, + grpc_error_set_int(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "received initial metadata size exceeds limit"), + GRPC_ERROR_INT_GRPC_STATUS, + GRPC_STATUS_RESOURCE_EXHAUSTED)); + grpc_chttp2_parsing_become_skip_parser(t); + s->seen_error = true; + GRPC_MDELEM_UNREF(md); + return GRPC_ERROR_NONE; +} + +static grpc_error* GPR_ATTRIBUTE_NOINLINE +handle_metadata_add_failure(grpc_chttp2_transport* t, grpc_chttp2_stream* s, + grpc_mdelem md, grpc_error* error) { + grpc_chttp2_cancel_stream(t, s, error); + grpc_chttp2_parsing_become_skip_parser(t); + s->seen_error = true; + GRPC_MDELEM_UNREF(md); + return GRPC_ERROR_NONE; +} + static grpc_error* on_initial_header(void* tp, grpc_mdelem md) { GPR_TIMER_SCOPE("on_initial_header", 0); @@ -430,45 +500,13 @@ static grpc_error* on_initial_header(void* tp, grpc_mdelem md) { GPR_DEBUG_ASSERT(s != nullptr); if (GRPC_TRACE_FLAG_ENABLED(grpc_http_trace)) { - char* key = grpc_slice_to_c_string(GRPC_MDKEY(md)); - char* value = - grpc_dump_slice(GRPC_MDVALUE(md), GPR_DUMP_HEX | GPR_DUMP_ASCII); - gpr_log(GPR_INFO, "HTTP:%d:HDR:%s: %s: %s", s->id, - t->is_client ? "CLI" : "SVR", key, value); - gpr_free(key); - gpr_free(value); + on_initial_header_log(t, s, md); } if (is_nonzero_status(md)) { // not GRPC_MDELEM_GRPC_STATUS_0? s->seen_error = true; } else if (md_key_cmp(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 (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 GRPC_ERROR_NONE; + return handle_timeout(s, md); } const size_t new_size = s->metadata_buffer[0].size + GRPC_MDELEM_LENGTH(md); @@ -476,29 +514,16 @@ static grpc_error* on_initial_header(void* tp, grpc_mdelem md) { t->settings[GRPC_ACKED_SETTINGS] [GRPC_CHTTP2_SETTINGS_MAX_HEADER_LIST_SIZE]; if (GPR_UNLIKELY(new_size > metadata_size_limit)) { - gpr_log(GPR_DEBUG, - "received initial metadata size exceeds limit (%" PRIuPTR - " vs. %" PRIuPTR ")", - new_size, metadata_size_limit); - grpc_chttp2_cancel_stream( - t, s, - grpc_error_set_int(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "received initial metadata size exceeds limit"), - GRPC_ERROR_INT_GRPC_STATUS, - GRPC_STATUS_RESOURCE_EXHAUSTED)); - grpc_chttp2_parsing_become_skip_parser(t); - s->seen_error = true; - GRPC_MDELEM_UNREF(md); + return handle_metadata_size_limit_exceeded(t, s, md, new_size, + metadata_size_limit); } else { grpc_error* error = grpc_chttp2_incoming_metadata_buffer_add(&s->metadata_buffer[0], md); - if (error != GRPC_ERROR_NONE) { - grpc_chttp2_cancel_stream(t, s, error); - grpc_chttp2_parsing_become_skip_parser(t); - s->seen_error = true; - GRPC_MDELEM_UNREF(md); + if (GPR_UNLIKELY(error != GRPC_ERROR_NONE)) { + return handle_metadata_add_failure(t, s, md, error); } } + // Not timeout-related metadata, and no error occurred. return GRPC_ERROR_NONE; }