diff --git a/bazel/experiments.bzl b/bazel/experiments.bzl index e959d615cd8..db2d283fb75 100644 --- a/bazel/experiments.bzl +++ b/bazel/experiments.bzl @@ -61,9 +61,6 @@ EXPERIMENTS = { ], }, "on": { - "flow_control_test": [ - "flow_control_fixes", - ], }, "opt": { }, diff --git a/src/core/ext/transport/chttp2/transport/flow_control.cc b/src/core/ext/transport/chttp2/transport/flow_control.cc index 4e765f97074..6d5399b297c 100644 --- a/src/core/ext/transport/chttp2/transport/flow_control.cc +++ b/src/core/ext/transport/chttp2/transport/flow_control.cc @@ -21,7 +21,6 @@ #include "src/core/ext/transport/chttp2/transport/flow_control.h" #include -#include #include #include @@ -278,45 +277,33 @@ void TransportFlowControl::UpdateSetting( uint32_t new_desired_value, FlowControlAction* action, FlowControlAction& (FlowControlAction::*set)(FlowControlAction::Urgency, uint32_t)) { - if (IsFlowControlFixesEnabled()) { - new_desired_value = - Clamp(new_desired_value, grpc_chttp2_settings_parameters[id].min_value, - grpc_chttp2_settings_parameters[id].max_value); - if (new_desired_value != *desired_value) { - if (grpc_flowctl_trace.enabled()) { - gpr_log(GPR_INFO, "[flowctl] UPDATE SETTING %s from %" PRId64 " to %d", - grpc_chttp2_settings_parameters[id].name, *desired_value, - new_desired_value); - } - // Reaching zero can only happen for initial window size, and if it occurs - // we really want to wake up writes and ensure all the queued stream - // window updates are flushed, since stream flow control operates - // differently at zero window size. - FlowControlAction::Urgency urgency = - FlowControlAction::Urgency::QUEUE_UPDATE; - if (*desired_value == 0 || new_desired_value == 0) { - urgency = FlowControlAction::Urgency::UPDATE_IMMEDIATELY; - } - *desired_value = new_desired_value; - (action->*set)(urgency, *desired_value); + new_desired_value = + Clamp(new_desired_value, grpc_chttp2_settings_parameters[id].min_value, + grpc_chttp2_settings_parameters[id].max_value); + if (new_desired_value != *desired_value) { + if (grpc_flowctl_trace.enabled()) { + gpr_log(GPR_INFO, "[flowctl] UPDATE SETTING %s from %" PRId64 " to %d", + grpc_chttp2_settings_parameters[id].name, *desired_value, + new_desired_value); } - } else { - int64_t delta = new_desired_value - *desired_value; - // TODO(ncteisen): tune this - if (delta != 0 && - (delta <= -*desired_value / 5 || delta >= *desired_value / 5)) { - *desired_value = new_desired_value; - (action->*set)(FlowControlAction::Urgency::QUEUE_UPDATE, - static_cast(*desired_value)); + // Reaching zero can only happen for initial window size, and if it occurs + // we really want to wake up writes and ensure all the queued stream + // window updates are flushed, since stream flow control operates + // differently at zero window size. + FlowControlAction::Urgency urgency = + FlowControlAction::Urgency::QUEUE_UPDATE; + if (*desired_value == 0 || new_desired_value == 0) { + urgency = FlowControlAction::Urgency::UPDATE_IMMEDIATELY; } + *desired_value = new_desired_value; + (action->*set)(urgency, *desired_value); } } FlowControlAction TransportFlowControl::SetAckedInitialWindow(uint32_t value) { acked_init_window_ = value; FlowControlAction action; - if (IsFlowControlFixesEnabled() && - acked_init_window_ != target_initial_window_size_) { + if (acked_init_window_ != target_initial_window_size_) { FlowControlAction::Urgency urgency = FlowControlAction::Urgency::QUEUE_UPDATE; if (acked_init_window_ == 0 || target_initial_window_size_ == 0) { @@ -330,68 +317,31 @@ FlowControlAction TransportFlowControl::SetAckedInitialWindow(uint32_t value) { FlowControlAction TransportFlowControl::PeriodicUpdate() { FlowControlAction action; if (enable_bdp_probe_) { - if (IsFlowControlFixesEnabled()) { - // get bdp estimate and update initial_window accordingly. - // target might change based on how much memory pressure we are under - // TODO(ncteisen): experiment with setting target to be huge under low - // memory pressure. - uint32_t target = static_cast(RoundUpToPowerOf2( - Clamp(IsMemoryPressureControllerEnabled() - ? TargetInitialWindowSizeBasedOnMemoryPressureAndBdp() - : pow(2, SmoothLogBdp(TargetLogBdp())), - 0.0, static_cast(kMaxInitialWindowSize)))); - if (target < kMinPositiveInitialWindowSize) target = 0; - if (g_test_only_transport_target_window_estimates_mocker != nullptr) { - // Hook for simulating unusual flow control situations in tests. - target = g_test_only_transport_target_window_estimates_mocker - ->ComputeNextTargetInitialWindowSizeFromPeriodicUpdate( - target_initial_window_size_ /* current target */); - } - // Though initial window 'could' drop to 0, we keep the floor at - // kMinInitialWindowSize - UpdateSetting(GRPC_CHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, - &target_initial_window_size_, target, &action, - &FlowControlAction::set_send_initial_window_update); - // we target the max of BDP or bandwidth in microseconds. - UpdateSetting(GRPC_CHTTP2_SETTINGS_MAX_FRAME_SIZE, &target_frame_size_, - target, &action, - &FlowControlAction::set_send_max_frame_size_update); - } else { - // get bdp estimate and update initial_window accordingly. - // target might change based on how much memory pressure we are under - // TODO(ncteisen): experiment with setting target to be huge under low - // memory pressure. - double target = IsMemoryPressureControllerEnabled() - ? TargetInitialWindowSizeBasedOnMemoryPressureAndBdp() - : pow(2, SmoothLogBdp(TargetLogBdp())); - if (g_test_only_transport_target_window_estimates_mocker != nullptr) { - // Hook for simulating unusual flow control situations in tests. - target = g_test_only_transport_target_window_estimates_mocker - ->ComputeNextTargetInitialWindowSizeFromPeriodicUpdate( - target_initial_window_size_ /* current target */); - } - // Though initial window 'could' drop to 0, we keep the floor at - // kMinInitialWindowSize - UpdateSetting(GRPC_CHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, - &target_initial_window_size_, - static_cast(Clamp( - target, static_cast(kMinInitialWindowSize), - static_cast(kMaxInitialWindowSize))), - &action, - &FlowControlAction::set_send_initial_window_update); - // get bandwidth estimate and update max_frame accordingly. - double bw_dbl = bdp_estimator_.EstimateBandwidth(); - // we target the max of BDP or bandwidth in microseconds. - UpdateSetting( - GRPC_CHTTP2_SETTINGS_MAX_FRAME_SIZE, &target_frame_size_, - static_cast( - Clamp(std::max(static_cast(Clamp( - bw_dbl, 0.0, static_cast(INT_MAX))) / - 1000, - static_cast(target_initial_window_size_)), - 16384, 16777215)), - &action, &FlowControlAction::set_send_max_frame_size_update); + // get bdp estimate and update initial_window accordingly. + // target might change based on how much memory pressure we are under + // TODO(ncteisen): experiment with setting target to be huge under low + // memory pressure. + uint32_t target = static_cast(RoundUpToPowerOf2( + Clamp(IsMemoryPressureControllerEnabled() + ? TargetInitialWindowSizeBasedOnMemoryPressureAndBdp() + : pow(2, SmoothLogBdp(TargetLogBdp())), + 0.0, static_cast(kMaxInitialWindowSize)))); + if (target < kMinPositiveInitialWindowSize) target = 0; + if (g_test_only_transport_target_window_estimates_mocker != nullptr) { + // Hook for simulating unusual flow control situations in tests. + target = g_test_only_transport_target_window_estimates_mocker + ->ComputeNextTargetInitialWindowSizeFromPeriodicUpdate( + target_initial_window_size_ /* current target */); } + // Though initial window 'could' drop to 0, we keep the floor at + // kMinInitialWindowSize + UpdateSetting(GRPC_CHTTP2_SETTINGS_INITIAL_WINDOW_SIZE, + &target_initial_window_size_, target, &action, + &FlowControlAction::set_send_initial_window_update); + // we target the max of BDP or bandwidth in microseconds. + UpdateSetting(GRPC_CHTTP2_SETTINGS_MAX_FRAME_SIZE, &target_frame_size_, + target, &action, + &FlowControlAction::set_send_max_frame_size_update); if (IsTcpFrameSizeTuningEnabled()) { // Advertise PREFERRED_RECEIVE_CRYPTO_FRAME_SIZE to peer. By advertising diff --git a/src/core/lib/experiments/experiments.cc b/src/core/lib/experiments/experiments.cc index 43e60e719f3..aecc7942a3c 100644 --- a/src/core/lib/experiments/experiments.cc +++ b/src/core/lib/experiments/experiments.cc @@ -31,8 +31,6 @@ const char* const description_peer_state_based_framing = "If set, the max sizes of frames sent to lower layers is controlled based " "on the peer's memory pressure which is reflected in its max http2 frame " "size."; -const char* const description_flow_control_fixes = - "Various fixes for flow control, max frame size setting."; const char* const description_memory_pressure_controller = "New memory pressure controller"; const char* const description_unconstrained_max_quota_buffer_size = @@ -75,7 +73,6 @@ const ExperimentMetadata g_experiment_metadata[] = { {"tcp_rcv_lowat", description_tcp_rcv_lowat, false, true}, {"peer_state_based_framing", description_peer_state_based_framing, false, true}, - {"flow_control_fixes", description_flow_control_fixes, true, true}, {"memory_pressure_controller", description_memory_pressure_controller, false, true}, {"unconstrained_max_quota_buffer_size", diff --git a/src/core/lib/experiments/experiments.h b/src/core/lib/experiments/experiments.h index f500e60adad..e43e20c7fbc 100644 --- a/src/core/lib/experiments/experiments.h +++ b/src/core/lib/experiments/experiments.h @@ -60,8 +60,6 @@ namespace grpc_core { inline bool IsTcpFrameSizeTuningEnabled() { return false; } inline bool IsTcpRcvLowatEnabled() { return false; } inline bool IsPeerStateBasedFramingEnabled() { return false; } -#define GRPC_EXPERIMENT_IS_INCLUDED_FLOW_CONTROL_FIXES -inline bool IsFlowControlFixesEnabled() { return true; } inline bool IsMemoryPressureControllerEnabled() { return false; } inline bool IsUnconstrainedMaxQuotaBufferSizeEnabled() { return false; } inline bool IsEventEngineClientEnabled() { return false; } @@ -86,52 +84,48 @@ inline bool IsTcpFrameSizeTuningEnabled() { return IsExperimentEnabled(0); } inline bool IsTcpRcvLowatEnabled() { return IsExperimentEnabled(1); } #define GRPC_EXPERIMENT_IS_INCLUDED_PEER_STATE_BASED_FRAMING inline bool IsPeerStateBasedFramingEnabled() { return IsExperimentEnabled(2); } -#define GRPC_EXPERIMENT_IS_INCLUDED_FLOW_CONTROL_FIXES -inline bool IsFlowControlFixesEnabled() { return IsExperimentEnabled(3); } #define GRPC_EXPERIMENT_IS_INCLUDED_MEMORY_PRESSURE_CONTROLLER inline bool IsMemoryPressureControllerEnabled() { - return IsExperimentEnabled(4); + return IsExperimentEnabled(3); } #define GRPC_EXPERIMENT_IS_INCLUDED_UNCONSTRAINED_MAX_QUOTA_BUFFER_SIZE inline bool IsUnconstrainedMaxQuotaBufferSizeEnabled() { - return IsExperimentEnabled(5); + return IsExperimentEnabled(4); } #define GRPC_EXPERIMENT_IS_INCLUDED_EVENT_ENGINE_CLIENT -inline bool IsEventEngineClientEnabled() { return IsExperimentEnabled(6); } +inline bool IsEventEngineClientEnabled() { return IsExperimentEnabled(5); } #define GRPC_EXPERIMENT_IS_INCLUDED_MONITORING_EXPERIMENT -inline bool IsMonitoringExperimentEnabled() { return IsExperimentEnabled(7); } +inline bool IsMonitoringExperimentEnabled() { return IsExperimentEnabled(6); } #define GRPC_EXPERIMENT_IS_INCLUDED_PROMISE_BASED_CLIENT_CALL -inline bool IsPromiseBasedClientCallEnabled() { return IsExperimentEnabled(8); } +inline bool IsPromiseBasedClientCallEnabled() { return IsExperimentEnabled(7); } #define GRPC_EXPERIMENT_IS_INCLUDED_FREE_LARGE_ALLOCATOR -inline bool IsFreeLargeAllocatorEnabled() { return IsExperimentEnabled(9); } +inline bool IsFreeLargeAllocatorEnabled() { return IsExperimentEnabled(8); } #define GRPC_EXPERIMENT_IS_INCLUDED_PROMISE_BASED_SERVER_CALL -inline bool IsPromiseBasedServerCallEnabled() { - return IsExperimentEnabled(10); -} +inline bool IsPromiseBasedServerCallEnabled() { return IsExperimentEnabled(9); } #define GRPC_EXPERIMENT_IS_INCLUDED_TRANSPORT_SUPPLIES_CLIENT_LATENCY inline bool IsTransportSuppliesClientLatencyEnabled() { - return IsExperimentEnabled(11); + return IsExperimentEnabled(10); } #define GRPC_EXPERIMENT_IS_INCLUDED_EVENT_ENGINE_LISTENER -inline bool IsEventEngineListenerEnabled() { return IsExperimentEnabled(12); } +inline bool IsEventEngineListenerEnabled() { return IsExperimentEnabled(11); } #define GRPC_EXPERIMENT_IS_INCLUDED_SCHEDULE_CANCELLATION_OVER_WRITE inline bool IsScheduleCancellationOverWriteEnabled() { - return IsExperimentEnabled(13); + return IsExperimentEnabled(12); } #define GRPC_EXPERIMENT_IS_INCLUDED_TRACE_RECORD_CALLOPS -inline bool IsTraceRecordCallopsEnabled() { return IsExperimentEnabled(14); } +inline bool IsTraceRecordCallopsEnabled() { return IsExperimentEnabled(13); } #define GRPC_EXPERIMENT_IS_INCLUDED_EVENT_ENGINE_DNS -inline bool IsEventEngineDnsEnabled() { return IsExperimentEnabled(15); } +inline bool IsEventEngineDnsEnabled() { return IsExperimentEnabled(14); } #define GRPC_EXPERIMENT_IS_INCLUDED_WORK_STEALING -inline bool IsWorkStealingEnabled() { return IsExperimentEnabled(16); } +inline bool IsWorkStealingEnabled() { return IsExperimentEnabled(15); } #define GRPC_EXPERIMENT_IS_INCLUDED_CLIENT_PRIVACY -inline bool IsClientPrivacyEnabled() { return IsExperimentEnabled(17); } +inline bool IsClientPrivacyEnabled() { return IsExperimentEnabled(16); } #define GRPC_EXPERIMENT_IS_INCLUDED_CANARY_CLIENT_PRIVACY -inline bool IsCanaryClientPrivacyEnabled() { return IsExperimentEnabled(18); } +inline bool IsCanaryClientPrivacyEnabled() { return IsExperimentEnabled(17); } #define GRPC_EXPERIMENT_IS_INCLUDED_SERVER_PRIVACY -inline bool IsServerPrivacyEnabled() { return IsExperimentEnabled(19); } +inline bool IsServerPrivacyEnabled() { return IsExperimentEnabled(18); } -constexpr const size_t kNumExperiments = 20; +constexpr const size_t kNumExperiments = 19; extern const ExperimentMetadata g_experiment_metadata[kNumExperiments]; #endif diff --git a/src/core/lib/experiments/experiments.yaml b/src/core/lib/experiments/experiments.yaml index d203a3d8d8a..8c01dafd35c 100644 --- a/src/core/lib/experiments/experiments.yaml +++ b/src/core/lib/experiments/experiments.yaml @@ -64,12 +64,6 @@ expiry: 2023/06/01 owner: vigneshbabu@google.com test_tags: ["flow_control_test"] -- name: flow_control_fixes - description: Various fixes for flow control, max frame size setting. - default: true - expiry: 2023/06/01 - owner: ctiller@google.com - test_tags: ["flow_control_test"] - name: memory_pressure_controller description: New memory pressure controller default: false