From 1c8698758d79a3d9c20382cd11ed1db1f14463ef Mon Sep 17 00:00:00 2001 From: ncteisen Date: Mon, 1 May 2017 11:37:23 -0700 Subject: [PATCH 1/3] Set frame size to B/us --- .../chttp2/transport/chttp2_transport.c | 40 +++++++++++++------ src/core/lib/transport/bdp_estimator.c | 5 +++ src/core/lib/transport/bdp_estimator.h | 2 + .../microbenchmarks/bm_fullstack_trickle.cc | 5 ++- 4 files changed, 39 insertions(+), 13 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index 30738080ee7..8ed34301e67 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -2139,15 +2139,8 @@ static void end_all_the_calls(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, static void update_bdp(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, double bdp_dbl) { - int32_t bdp; - const int32_t kMinBDP = 128; - if (bdp_dbl <= kMinBDP) { - bdp = kMinBDP; - } else if (bdp_dbl > INT32_MAX) { - bdp = INT32_MAX; - } else { - bdp = (int32_t)(bdp_dbl); - } + // initial window size bounded [1,2^31-1], but we set the min to 128. + int32_t bdp = GPR_CLAMP((int32_t)bdp_dbl, 128, INT32_MAX); int64_t delta = (int64_t)bdp - (int64_t)t->settings[GRPC_LOCAL_SETTINGS] @@ -2159,9 +2152,27 @@ static void update_bdp(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, gpr_log(GPR_DEBUG, "%s: update initial window size to %d", t->peer_string, (int)bdp); } - push_setting(exec_ctx, t, GRPC_CHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, - (uint32_t)bdp); - push_setting(exec_ctx, t, GRPC_CHTTP2_SETTINGS_MAX_FRAME_SIZE, (uint32_t)bdp); + push_setting(exec_ctx, t, GRPC_CHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, (uint32_t)bdp); +} + +// TODO(ncteisen): combine this logic with above func +static void update_frame(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, + double bw_dbl) { + int32_t target = (int32_t)bw_dbl / 1000000; + // frame size is bounded [2^14,2^24-1] + int32_t frame_size = GPR_CLAMP(target, 16384, 16777215); + int64_t delta = (int64_t)frame_size - + (int64_t)t->settings[GRPC_LOCAL_SETTINGS] + [GRPC_CHTTP2_SETTINGS_MAX_FRAME_SIZE]; +// gpr_log(GPR_DEBUG, "delta: %lld, frame_size %u, low: %d, high: %d", delta, frame_size, (uint32_t)(-frame_size / 10), (uint32_t)(frame_size / 10)); + if (delta == 0 || (delta > -frame_size / 10 && delta < frame_size / 10)) { + return; + } + if (grpc_bdp_estimator_trace) { + gpr_log(GPR_DEBUG, "%s: update max_frame size to %d", t->peer_string, + (int)frame_size); + } + push_setting(exec_ctx, t, GRPC_CHTTP2_SETTINGS_MAX_FRAME_SIZE, (uint32_t)frame_size); } static grpc_error *try_http_parsing(grpc_exec_ctx *exec_ctx, @@ -2300,6 +2311,11 @@ static void read_action_locked(grpc_exec_ctx *exec_ctx, void *tp, update_bdp(exec_ctx, t, pow(2, log2_bdp_guess)); t->last_pid_update = now; } + + double bw = -1; + if (grpc_bdp_estimator_get_bw(&t->bdp_estimator, &bw)) { + update_frame(exec_ctx, t, bw); + } } GRPC_CHTTP2_UNREF_TRANSPORT(exec_ctx, t, "keep_reading"); } else { diff --git a/src/core/lib/transport/bdp_estimator.c b/src/core/lib/transport/bdp_estimator.c index 536694214e2..ad89dee51a1 100644 --- a/src/core/lib/transport/bdp_estimator.c +++ b/src/core/lib/transport/bdp_estimator.c @@ -53,6 +53,11 @@ bool grpc_bdp_estimator_get_estimate(grpc_bdp_estimator *estimator, return true; } +bool grpc_bdp_estimator_get_bw(grpc_bdp_estimator *estimator, double *bw) { + *bw = estimator->bw_est; + return true; +} + bool grpc_bdp_estimator_add_incoming_bytes(grpc_bdp_estimator *estimator, int64_t num_bytes) { estimator->accumulator += num_bytes; diff --git a/src/core/lib/transport/bdp_estimator.h b/src/core/lib/transport/bdp_estimator.h index 752bee1abda..d1de398acc6 100644 --- a/src/core/lib/transport/bdp_estimator.h +++ b/src/core/lib/transport/bdp_estimator.h @@ -63,6 +63,8 @@ void grpc_bdp_estimator_init(grpc_bdp_estimator *estimator, const char *name); // Returns true if a reasonable estimate could be obtained bool grpc_bdp_estimator_get_estimate(grpc_bdp_estimator *estimator, int64_t *estimate); +// Returns true if a reasonable estimate could be obtained +bool grpc_bdp_estimator_get_bw(grpc_bdp_estimator *estimator, double *bw); // Returns true if the user should schedule a ping bool grpc_bdp_estimator_add_incoming_bytes(grpc_bdp_estimator *estimator, int64_t num_bytes); diff --git a/test/cpp/microbenchmarks/bm_fullstack_trickle.cc b/test/cpp/microbenchmarks/bm_fullstack_trickle.cc index fc99b06dbb2..01d2023fa63 100644 --- a/test/cpp/microbenchmarks/bm_fullstack_trickle.cc +++ b/test/cpp/microbenchmarks/bm_fullstack_trickle.cc @@ -100,6 +100,8 @@ class TrickledCHTTP2 : public EndpointPairFixture { } void AddToLabel(std::ostream& out, benchmark::State& state) { + grpc_chttp2_transport* client = + reinterpret_cast(client_transport_); out << " writes/iter:" << ((double)stats_.num_writes / (double)state.iterations()) << " cli_transport_stalls/iter:" @@ -115,7 +117,8 @@ class TrickledCHTTP2 : public EndpointPairFixture { (double)state.iterations()) << " svr_stream_stalls/iter:" << ((double)server_stats_.streams_stalled_due_to_stream_flow_control / - (double)state.iterations()); + (double)state.iterations()) + << " cli_bw_est:" << (double)client->bdp_estimator.bw_est; } void Log(int64_t iteration) { From 2a7cf878d2e42791ec76b8fd2bca2d385a7636ec Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 2 May 2017 14:59:39 -0700 Subject: [PATCH 2/3] Set frame size to max(bdp, bw in ms) --- .../transport/chttp2/transport/chttp2_transport.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index 8ed34301e67..1278fb42a36 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -2155,16 +2155,15 @@ static void update_bdp(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, push_setting(exec_ctx, t, GRPC_CHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, (uint32_t)bdp); } -// TODO(ncteisen): combine this logic with above func static void update_frame(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, - double bw_dbl) { - int32_t target = (int32_t)bw_dbl / 1000000; + double bw_dbl, double bdp_dbl) { + int32_t bdp = GPR_CLAMP((int32_t)bdp_dbl, 128, INT32_MAX); + int32_t target = GPR_MAX((int32_t)bw_dbl / 1000, bdp); // frame size is bounded [2^14,2^24-1] int32_t frame_size = GPR_CLAMP(target, 16384, 16777215); int64_t delta = (int64_t)frame_size - (int64_t)t->settings[GRPC_LOCAL_SETTINGS] [GRPC_CHTTP2_SETTINGS_MAX_FRAME_SIZE]; -// gpr_log(GPR_DEBUG, "delta: %lld, frame_size %u, low: %d, high: %d", delta, frame_size, (uint32_t)(-frame_size / 10), (uint32_t)(frame_size / 10)); if (delta == 0 || (delta > -frame_size / 10 && delta < frame_size / 10)) { return; } @@ -2291,6 +2290,7 @@ static void read_action_locked(grpc_exec_ctx *exec_ctx, void *tp, } int64_t estimate = -1; + double bdp_guess = -1; if (grpc_bdp_estimator_get_estimate(&t->bdp_estimator, &estimate)) { double target = 1 + log2((double)estimate); double memory_pressure = grpc_resource_quota_get_memory_pressure( @@ -2308,13 +2308,14 @@ static void read_action_locked(grpc_exec_ctx *exec_ctx, void *tp, } double log2_bdp_guess = grpc_pid_controller_update(&t->pid_controller, bdp_error, dt); - update_bdp(exec_ctx, t, pow(2, log2_bdp_guess)); + bdp_guess = pow(2, log2_bdp_guess); + update_bdp(exec_ctx, t, bdp_guess); t->last_pid_update = now; } double bw = -1; if (grpc_bdp_estimator_get_bw(&t->bdp_estimator, &bw)) { - update_frame(exec_ctx, t, bw); + update_frame(exec_ctx, t, bw, bdp_guess); } } GRPC_CHTTP2_UNREF_TRANSPORT(exec_ctx, t, "keep_reading"); From 8450876c76f155c2970e0f6e08affb9d336580fd Mon Sep 17 00:00:00 2001 From: ncteisen Date: Mon, 8 May 2017 18:09:05 -0700 Subject: [PATCH 3/3] clang --- src/core/ext/transport/chttp2/transport/chttp2_transport.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index 1278fb42a36..49e80aaee93 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -2152,7 +2152,8 @@ static void update_bdp(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, gpr_log(GPR_DEBUG, "%s: update initial window size to %d", t->peer_string, (int)bdp); } - push_setting(exec_ctx, t, GRPC_CHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, (uint32_t)bdp); + push_setting(exec_ctx, t, GRPC_CHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, + (uint32_t)bdp); } static void update_frame(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, @@ -2171,7 +2172,8 @@ static void update_frame(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, gpr_log(GPR_DEBUG, "%s: update max_frame size to %d", t->peer_string, (int)frame_size); } - push_setting(exec_ctx, t, GRPC_CHTTP2_SETTINGS_MAX_FRAME_SIZE, (uint32_t)frame_size); + push_setting(exec_ctx, t, GRPC_CHTTP2_SETTINGS_MAX_FRAME_SIZE, + (uint32_t)frame_size); } static grpc_error *try_http_parsing(grpc_exec_ctx *exec_ctx,