[max-age] Add jitter to max idle, use absl bitgen for rng (#34225)

Co-authored-by: ctiller <ctiller@users.noreply.github.com>

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",

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 <stdint.h>
-#include <stdlib.h>
 
 #include <functional>
 #include <utility>
 
+#include "absl/base/thread_annotations.h"
+#include "absl/random/random.h"
 #include "absl/types/optional.h"
 
 #include <grpc/impl/channel_arg_names.h>
@@ -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
+  // A random jitter of +/-10% will be added to MAX_CONNECTION_AGE and
-  // connection storms. Note that the MAX_CONNECTION_AGE option without jitter
+  // MAX_CONNECTION_IDLE to spread out reconnection storms.
-  // 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.
   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 =
+    struct BitGen {
-        rand() * kMaxConnectionAgeJitter * 2.0 / RAND_MAX + 1.0 -
+      Mutex mu;
-        kMaxConnectionAgeJitter;
+      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<PerCpu<BitGen>> 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};
   }
 };
 

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

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

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

src/core/lib/experiments/rollouts.yaml

@@ -90,3 +90,5 @@
   default: false
 - name: keepalive_server_fix
   default: false
+- name: jitter_max_idle
+  default: true

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
+  {
+    ExecCtx exec_ctx;
     ASSERT_TRUE(CoreConfiguration::Get().channel_init().CreateStack(&builder));
+  }
   std::vector<std::string> filters;
   for (const auto& entry : *builder.mutable_stack()) {
     filters.push_back(entry->name);

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"],

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 <benchmark/benchmark.h>
+
+#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;
+}