diff --git a/src/core/BUILD b/src/core/BUILD index d996c7a01d9..b459ced46f6 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -3709,6 +3709,8 @@ grpc_cc_library( "ext/filters/channel_idle/channel_idle_filter.h", ], external_deps = [ + "absl/base:core_headers", + "absl/random", "absl/status", "absl/status:statusor", "absl/types:optional", @@ -3723,9 +3725,12 @@ grpc_cc_library( "closure", "error", "exec_ctx_wakeup_scheduler", + "experiments", "http2_errors", "idle_filter_state", "loop", + "no_destruct", + "per_cpu", "poll", "single_set_ptr", "sleep", diff --git a/src/core/ext/filters/channel_idle/channel_idle_filter.cc b/src/core/ext/filters/channel_idle/channel_idle_filter.cc index 5a7d98efc47..7056e3c8675 100644 --- a/src/core/ext/filters/channel_idle/channel_idle_filter.cc +++ b/src/core/ext/filters/channel_idle/channel_idle_filter.cc @@ -20,11 +20,12 @@ #include "src/core/ext/filters/channel_idle/channel_idle_filter.h" #include -#include #include #include +#include "absl/base/thread_annotations.h" +#include "absl/random/random.h" #include "absl/types/optional.h" #include @@ -35,9 +36,13 @@ #include "src/core/lib/channel/promise_based_filter.h" #include "src/core/lib/config/core_configuration.h" #include "src/core/lib/debug/trace.h" +#include "src/core/lib/experiments/experiments.h" #include "src/core/lib/gprpp/debug_location.h" +#include "src/core/lib/gprpp/no_destruct.h" #include "src/core/lib/gprpp/orphanable.h" +#include "src/core/lib/gprpp/per_cpu.h" #include "src/core/lib/gprpp/status_helper.h" +#include "src/core/lib/gprpp/sync.h" #include "src/core/lib/iomgr/closure.h" #include "src/core/lib/iomgr/error.h" #include "src/core/lib/iomgr/exec_ctx.h" @@ -95,10 +100,8 @@ struct MaxAgeFilter::Config { max_connection_idle != Duration::Infinity(); } - // A random jitter of +/-10% will be added to MAX_CONNECTION_AGE to spread out - // connection storms. Note that the MAX_CONNECTION_AGE option without jitter - // would not create connection storms by itself, but if there happened to be a - // connection storm it could cause it to repeat at a fixed period. + // A random jitter of +/-10% will be added to MAX_CONNECTION_AGE and + // MAX_CONNECTION_IDLE to spread out reconnection storms. static Config FromChannelArgs(const ChannelArgs& args) { const Duration args_max_age = args.GetDurationFromIntMillis(GRPC_ARG_MAX_CONNECTION_AGE_MS) @@ -111,12 +114,22 @@ struct MaxAgeFilter::Config { .value_or(kDefaultMaxConnectionAgeGrace); // generate a random number between 1 - kMaxConnectionAgeJitter and // 1 + kMaxConnectionAgeJitter - const double multiplier = - rand() * kMaxConnectionAgeJitter * 2.0 / RAND_MAX + 1.0 - - kMaxConnectionAgeJitter; + struct BitGen { + Mutex mu; + absl::BitGen bit_gen ABSL_GUARDED_BY(mu); + double MakeUniformDouble(double min, double max) { + MutexLock lock(&mu); + return absl::Uniform(bit_gen, min, max); + } + }; + static NoDestruct> bit_gen(PerCpuOptions().SetMaxShards(8)); + const double multiplier = bit_gen->this_cpu().MakeUniformDouble( + 1.0 - kMaxConnectionAgeJitter, 1.0 + kMaxConnectionAgeJitter); // GRPC_MILLIS_INF_FUTURE - 0.5 converts the value to float, so that result // will not be cast to int implicitly before the comparison. - return Config{args_max_age * multiplier, args_max_idle, args_max_age_grace}; + return Config{args_max_age * multiplier, + args_max_idle * (IsJitterMaxIdleEnabled() ? multiplier : 1.0), + args_max_age_grace}; } }; diff --git a/src/core/lib/experiments/experiments.cc b/src/core/lib/experiments/experiments.cc index c45f0cf90e4..06c2e2c8dcd 100644 --- a/src/core/lib/experiments/experiments.cc +++ b/src/core/lib/experiments/experiments.cc @@ -96,6 +96,11 @@ const char* const description_keepalive_server_fix = "Allows overriding keepalive_permit_without_calls for servers. Refer " "https://github.com/grpc/grpc/pull/33917 for more information."; const char* const additional_constraints_keepalive_server_fix = "{}"; +const char* const description_jitter_max_idle = + "Enable jitter on connection max idle times. Historically this jitter was " + "only on max connection age, but it seems like this could smooth out some " + "herding problems."; +const char* const additional_constraints_jitter_max_idle = "{}"; } // namespace namespace grpc_core { @@ -145,6 +150,8 @@ const ExperimentMetadata g_experiment_metadata[] = { additional_constraints_keepalive_fix, false, false}, {"keepalive_server_fix", description_keepalive_server_fix, additional_constraints_keepalive_server_fix, false, false}, + {"jitter_max_idle", description_jitter_max_idle, + additional_constraints_jitter_max_idle, true, true}, }; } // namespace grpc_core @@ -225,6 +232,11 @@ const char* const description_keepalive_server_fix = "Allows overriding keepalive_permit_without_calls for servers. Refer " "https://github.com/grpc/grpc/pull/33917 for more information."; const char* const additional_constraints_keepalive_server_fix = "{}"; +const char* const description_jitter_max_idle = + "Enable jitter on connection max idle times. Historically this jitter was " + "only on max connection age, but it seems like this could smooth out some " + "herding problems."; +const char* const additional_constraints_jitter_max_idle = "{}"; } // namespace namespace grpc_core { @@ -274,6 +286,8 @@ const ExperimentMetadata g_experiment_metadata[] = { additional_constraints_keepalive_fix, false, false}, {"keepalive_server_fix", description_keepalive_server_fix, additional_constraints_keepalive_server_fix, false, false}, + {"jitter_max_idle", description_jitter_max_idle, + additional_constraints_jitter_max_idle, true, true}, }; } // namespace grpc_core @@ -354,6 +368,11 @@ const char* const description_keepalive_server_fix = "Allows overriding keepalive_permit_without_calls for servers. Refer " "https://github.com/grpc/grpc/pull/33917 for more information."; const char* const additional_constraints_keepalive_server_fix = "{}"; +const char* const description_jitter_max_idle = + "Enable jitter on connection max idle times. Historically this jitter was " + "only on max connection age, but it seems like this could smooth out some " + "herding problems."; +const char* const additional_constraints_jitter_max_idle = "{}"; } // namespace namespace grpc_core { @@ -403,6 +422,8 @@ const ExperimentMetadata g_experiment_metadata[] = { additional_constraints_keepalive_fix, false, false}, {"keepalive_server_fix", description_keepalive_server_fix, additional_constraints_keepalive_server_fix, false, false}, + {"jitter_max_idle", description_jitter_max_idle, + additional_constraints_jitter_max_idle, true, true}, }; } // namespace grpc_core diff --git a/src/core/lib/experiments/experiments.h b/src/core/lib/experiments/experiments.h index 7bbd1cb732e..b43028b9615 100644 --- a/src/core/lib/experiments/experiments.h +++ b/src/core/lib/experiments/experiments.h @@ -83,6 +83,8 @@ inline bool IsServerPrivacyEnabled() { return false; } inline bool IsUniqueMetadataStringsEnabled() { return true; } inline bool IsKeepaliveFixEnabled() { return false; } inline bool IsKeepaliveServerFixEnabled() { return false; } +#define GRPC_EXPERIMENT_IS_INCLUDED_JITTER_MAX_IDLE +inline bool IsJitterMaxIdleEnabled() { return true; } #elif defined(GPR_WINDOWS) inline bool IsTcpFrameSizeTuningEnabled() { return false; } @@ -109,6 +111,8 @@ inline bool IsServerPrivacyEnabled() { return false; } inline bool IsUniqueMetadataStringsEnabled() { return true; } inline bool IsKeepaliveFixEnabled() { return false; } inline bool IsKeepaliveServerFixEnabled() { return false; } +#define GRPC_EXPERIMENT_IS_INCLUDED_JITTER_MAX_IDLE +inline bool IsJitterMaxIdleEnabled() { return true; } #else inline bool IsTcpFrameSizeTuningEnabled() { return false; } @@ -135,6 +139,8 @@ inline bool IsServerPrivacyEnabled() { return false; } inline bool IsUniqueMetadataStringsEnabled() { return true; } inline bool IsKeepaliveFixEnabled() { return false; } inline bool IsKeepaliveServerFixEnabled() { return false; } +#define GRPC_EXPERIMENT_IS_INCLUDED_JITTER_MAX_IDLE +inline bool IsJitterMaxIdleEnabled() { return true; } #endif #else @@ -186,8 +192,10 @@ inline bool IsUniqueMetadataStringsEnabled() { return IsExperimentEnabled(18); } inline bool IsKeepaliveFixEnabled() { return IsExperimentEnabled(19); } #define GRPC_EXPERIMENT_IS_INCLUDED_KEEPALIVE_SERVER_FIX inline bool IsKeepaliveServerFixEnabled() { return IsExperimentEnabled(20); } +#define GRPC_EXPERIMENT_IS_INCLUDED_JITTER_MAX_IDLE +inline bool IsJitterMaxIdleEnabled() { return IsExperimentEnabled(21); } -constexpr const size_t kNumExperiments = 21; +constexpr const size_t kNumExperiments = 22; 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 b7cf97f0127..7d2b2752571 100644 --- a/src/core/lib/experiments/experiments.yaml +++ b/src/core/lib/experiments/experiments.yaml @@ -169,3 +169,12 @@ owner: yashkt@google.com test_tags: [] allow_in_fuzzing_config: false +- name: jitter_max_idle + description: + Enable jitter on connection max idle times. + Historically this jitter was only on max connection age, but it seems like + this could smooth out some herding problems. + expiry: 2023/11/21 + owner: ctiller@google.com + test_tags: [] + allow_in_fuzzing_config: true diff --git a/src/core/lib/experiments/rollouts.yaml b/src/core/lib/experiments/rollouts.yaml index df4dbc8c50a..9ae080be143 100644 --- a/src/core/lib/experiments/rollouts.yaml +++ b/src/core/lib/experiments/rollouts.yaml @@ -90,3 +90,5 @@ default: false - name: keepalive_server_fix default: false +- name: jitter_max_idle + default: true diff --git a/test/core/xds/xds_channel_stack_modifier_test.cc b/test/core/xds/xds_channel_stack_modifier_test.cc index 8da455e365f..063092745c5 100644 --- a/test/core/xds/xds_channel_stack_modifier_test.cc +++ b/test/core/xds/xds_channel_stack_modifier_test.cc @@ -31,6 +31,7 @@ #include "src/core/lib/channel/channel_stack.h" #include "src/core/lib/channel/channel_stack_builder_impl.h" #include "src/core/lib/config/core_configuration.h" +#include "src/core/lib/iomgr/exec_ctx.h" #include "src/core/lib/surface/channel_init.h" #include "src/core/lib/surface/channel_stack_type.h" #include "src/core/lib/transport/transport_fwd.h" @@ -99,7 +100,10 @@ TEST(XdsChannelStackModifierTest, XdsHttpFiltersInsertion) { builder.SetTransport(&fake_transport); // Construct channel stack and verify that the test filters were successfully // added - ASSERT_TRUE(CoreConfiguration::Get().channel_init().CreateStack(&builder)); + { + ExecCtx exec_ctx; + ASSERT_TRUE(CoreConfiguration::Get().channel_init().CreateStack(&builder)); + } std::vector filters; for (const auto& entry : *builder.mutable_stack()) { filters.push_back(entry->name); diff --git a/test/cpp/microbenchmarks/BUILD b/test/cpp/microbenchmarks/BUILD index 67431304ceb..b4ef81a56ae 100644 --- a/test/cpp/microbenchmarks/BUILD +++ b/test/cpp/microbenchmarks/BUILD @@ -42,6 +42,20 @@ grpc_cc_test( ], ) +grpc_cc_test( + name = "bm_rng", + srcs = ["bm_rng.cc"], + external_deps = [ + "benchmark", + "absl/container:btree", + ], + deps = [ + "//:grpc++", + "//src/core:channel_args", + "//test/core/util:grpc_test_util", + ], +) + grpc_cc_test( name = "bm_exec_ctx", srcs = ["bm_exec_ctx.cc"], diff --git a/test/cpp/microbenchmarks/bm_rng.cc b/test/cpp/microbenchmarks/bm_rng.cc new file mode 100644 index 00000000000..8797ee633a6 --- /dev/null +++ b/test/cpp/microbenchmarks/bm_rng.cc @@ -0,0 +1,65 @@ +// +// +// Copyright 2017 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. +// +// + +// Benchmark arenas + +#include + +#include "absl/random/random.h" + +#include "src/core/lib/gprpp/sync.h" + +static void BM_OneRngFromFreshBitSet(benchmark::State& state) { + for (auto _ : state) { + benchmark::DoNotOptimize(absl::Uniform(absl::BitGen(), 0.0, 1.0)); + } +} +BENCHMARK(BM_OneRngFromFreshBitSet); + +static void BM_OneRngFromReusedBitSet(benchmark::State& state) { + absl::BitGen bitgen; + for (auto _ : state) { + benchmark::DoNotOptimize(absl::Uniform(bitgen, 0.0, 1.0)); + } +} +BENCHMARK(BM_OneRngFromReusedBitSet); + +static void BM_OneRngFromReusedBitSetWithMutex(benchmark::State& state) { + struct Data { + grpc_core::Mutex mu; + absl::BitGen bitgen ABSL_GUARDED_BY(mu); + }; + Data data; + for (auto _ : state) { + grpc_core::MutexLock lock(&data.mu); + benchmark::DoNotOptimize(absl::Uniform(data.bitgen, 0.0, 1.0)); + } +} +BENCHMARK(BM_OneRngFromReusedBitSetWithMutex); + +// Some distros have RunSpecifiedBenchmarks under the benchmark namespace, +// and others do not. This allows us to support both modes. +namespace benchmark { +void RunTheBenchmarksNamespaced() { RunSpecifiedBenchmarks(); } +} // namespace benchmark + +int main(int argc, char** argv) { + ::benchmark::Initialize(&argc, argv); + benchmark::RunTheBenchmarksNamespaced(); + return 0; +}