add experiment

BUILD

@@ -3005,6 +3005,7 @@ grpc_cc_library(
     ],
     external_deps = [
         "absl/base:core_headers",
+        "absl/cleanup",
         "absl/container:flat_hash_set",
         "absl/container:inlined_vector",
         "absl/functional:any_invocable",
@@ -3064,6 +3065,7 @@ grpc_cc_library(
         "//src/core:dual_ref_counted",
         "//src/core:env",
         "//src/core:error",
+        "//src/core:experiments",
         "//src/core:gpr_atm",
         "//src/core:grpc_backend_metric_data",
         "//src/core:grpc_deadline_filter",

src/core/BUILD

@@ -4929,6 +4929,7 @@ grpc_cc_library(
     language = "c++",
     deps = [
         "channel_args",
+        "experiments",
         "grpc_backend_metric_data",
         "grpc_lb_subchannel_list",
         "json",

src/core/ext/filters/client_channel/client_channel.cc

@@ -29,6 +29,7 @@
 #include <utility>
 #include <vector>
 
+#include "absl/cleanup/cleanup.h"
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
 #include "absl/strings/cord.h"
@@ -68,6 +69,7 @@
 #include "src/core/lib/channel/status_util.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/gpr/useful.h"
 #include "src/core/lib/gprpp/debug_location.h"
 #include "src/core/lib/gprpp/status_helper.h"
@@ -643,10 +645,30 @@ class ClientChannel::SubchannelWrapper : public SubchannelInterface {
               "chand=%p: destroying subchannel wrapper %p for subchannel %p",
               chand_, this, subchannel_.get());
     }
+    if (!IsClientChannelSubchannelWrapperWorkSerializerOrphanEnabled()) {
+      chand_->subchannel_wrappers_.erase(this);
+      if (chand_->channelz_node_ != nullptr) {
+        auto* subchannel_node = subchannel_->channelz_node();
+        if (subchannel_node != nullptr) {
+          auto it =
+              chand_->subchannel_refcount_map_.find(subchannel_.get());
+          GPR_ASSERT(it != chand_->subchannel_refcount_map_.end());
+          --it->second;
+          if (it->second == 0) {
+            chand_->channelz_node_->RemoveChildSubchannel(
+                subchannel_node->uuid());
+            chand_->subchannel_refcount_map_.erase(it);
+          }
+        }
+      }
+    }
     GRPC_CHANNEL_STACK_UNREF(chand_->owning_stack_, "SubchannelWrapper");
   }
 
   void Orphan() override {
+    if (!IsClientChannelSubchannelWrapperWorkSerializerOrphanEnabled()) {
+      return;
+    }
     // Make sure we clean up the channel's subchannel maps inside the
     // WorkSerializer.
     // Ref held by callback.
@@ -740,13 +762,17 @@ class ClientChannel::SubchannelWrapper : public SubchannelInterface {
         : watcher_(std::move(watcher)), parent_(std::move(parent)) {}
 
     ~WatcherWrapper() override {
-      auto* parent = parent_.release();  // ref owned by lambda
-      parent->chand_->work_serializer_->Run(
-          [parent]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(
-              *parent_->chand_->work_serializer_) {
-            parent->Unref(DEBUG_LOCATION, "WatcherWrapper");
-          },
-          DEBUG_LOCATION);
+      if (!IsClientChannelSubchannelWrapperWorkSerializerOrphanEnabled()) {
+        auto* parent = parent_.release();  // ref owned by lambda
+        parent->chand_->work_serializer_->Run(
+            [parent]() ABSL_EXCLUSIVE_LOCKS_REQUIRED(
+                *parent_->chand_->work_serializer_) {
+              parent->Unref(DEBUG_LOCATION, "WatcherWrapper");
+            },
+            DEBUG_LOCATION);
+        return;
+      }
+      parent_.reset(DEBUG_LOCATION, "WatcherWrapper");
     }
 
     void OnConnectivityStateChange(
@@ -2781,6 +2807,38 @@ void ClientChannel::LoadBalancedCall::AddCallToLbQueuedCallsLocked() {
 
 absl::optional<absl::Status> ClientChannel::LoadBalancedCall::PickSubchannel(
     bool was_queued) {
+  // We may accumulate multiple pickers here, because if a picker says
+  // to queue the call, we check again to see if the picker has been
+  // updated before we queue it.
+  // We need to unref pickers in the WorkSerializer.
+  std::vector<RefCountedPtr<LoadBalancingPolicy::SubchannelPicker>> pickers;
+  auto cleanup = absl::MakeCleanup([&]() {
+    if (IsClientChannelSubchannelWrapperWorkSerializerOrphanEnabled()) {
+      return;
+    }
+    chand_->work_serializer_->Run(
+        [pickers = std::move(pickers)]() mutable {
+          for (auto& picker : pickers) {
+            picker.reset(DEBUG_LOCATION, "PickSubchannel");
+          }
+        },
+        DEBUG_LOCATION);
+  });
+  absl::AnyInvocable<void(RefCountedPtr<LoadBalancingPolicy::SubchannelPicker>)>
+      set_picker;
+  if (!IsClientChannelSubchannelWrapperWorkSerializerOrphanEnabled()) {
+    set_picker =
+        [&](RefCountedPtr<LoadBalancingPolicy::SubchannelPicker> picker) {
+          pickers.emplace_back(std::move(picker));
+        };
+
+  } else {
+    pickers.emplace_back();
+    set_picker =
+        [&](RefCountedPtr<LoadBalancingPolicy::SubchannelPicker> picker) {
+          pickers[0] = std::move(picker);
+        };
+  }
   // Grab mutex and take a ref to the picker.
   if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_lb_call_trace)) {
     gpr_log(GPR_INFO, "chand=%p lb_call=%p: grabbing LB mutex to get picker",
@@ -2789,26 +2847,26 @@ absl::optional<absl::Status> ClientChannel::LoadBalancedCall::PickSubchannel(
   RefCountedPtr<LoadBalancingPolicy::SubchannelPicker> picker;
   {
     MutexLock lock(&chand_->lb_mu_);
-    picker = chand_->picker_;
+    set_picker(chand_->picker_);
   }
   while (true) {
     // Do pick.
     if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_lb_call_trace)) {
       gpr_log(GPR_INFO, "chand=%p lb_call=%p: performing pick with picker=%p",
-              chand_, this, picker.get());
+              chand_, this, pickers.back().get());
     }
     grpc_error_handle error;
-    bool pick_complete = PickSubchannelImpl(picker.get(), &error);
+    bool pick_complete = PickSubchannelImpl(pickers.back().get(), &error);
     if (!pick_complete) {
       MutexLock lock(&chand_->lb_mu_);
       // If picker has been swapped out since we grabbed it, try again.
-      if (picker != chand_->picker_) {
+      if (pickers.back() != chand_->picker_) {
         if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_lb_call_trace)) {
           gpr_log(GPR_INFO,
                   "chand=%p lb_call=%p: pick not complete, but picker changed",
                   chand_, this);
         }
-        picker = chand_->picker_;
+        set_picker(chand_->picker_);
         continue;
       }
       // Otherwise queue the pick to try again later when we get a new picker.

src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/weighted_round_robin.cc

@@ -54,6 +54,7 @@
 #include "src/core/lib/debug/stats.h"
 #include "src/core/lib/debug/stats_data.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/orphanable.h"
 #include "src/core/lib/gprpp/ref_counted.h"
@@ -608,7 +609,9 @@ void WeightedRoundRobin::Picker::BuildSchedulerAndStartTimerLocked() {
   // Start timer.
   WeakRefCountedPtr<Picker> self = WeakRef();
   timer_handle_ = wrr_->channel_control_helper()->GetEventEngine()->RunAfter(
-      config_->weight_update_period(), [self = std::move(self)]() mutable {
+      config_->weight_update_period(),
+      [self = std::move(self),
+       work_serializer = wrr_->work_serializer()]() mutable {
         ApplicationCallbackExecCtx callback_exec_ctx;
         ExecCtx exec_ctx;
         {
@@ -621,6 +624,11 @@ void WeightedRoundRobin::Picker::BuildSchedulerAndStartTimerLocked() {
             self->BuildSchedulerAndStartTimerLocked();
           }
         }
+        if (!IsClientChannelSubchannelWrapperWorkSerializerOrphanEnabled()) {
+          // Release the picker ref inside the WorkSerializer.
+          work_serializer->Run([self = std::move(self)]() {}, DEBUG_LOCATION);
+          return;
+        }
         self.reset();
       });
 }

src/core/lib/experiments/experiments.cc

@@ -105,6 +105,13 @@ const char* const description_jitter_max_idle =
     "only on max connection age, but it seems like this could smooth out some "
     "herding problems.";
 const char* const additional_constraints_jitter_max_idle = "{}";
+const char* const
+    description_client_channel_subchannel_wrapper_work_serializer_orphan =
+        "Client channel subchannel wrapper hops into WorkSerializer at "
+        "Orphan() time, rather than requiring callers to do it.";
+const char* const
+    additional_constraints_client_channel_subchannel_wrapper_work_serializer_orphan =
+        "{}";
 }  // namespace
 
 namespace grpc_core {
@@ -158,6 +165,10 @@ const ExperimentMetadata g_experiment_metadata[] = {
      additional_constraints_lazier_stream_updates, true, true},
     {"jitter_max_idle", description_jitter_max_idle,
      additional_constraints_jitter_max_idle, true, true},
+    {"client_channel_subchannel_wrapper_work_serializer_orphan",
+     description_client_channel_subchannel_wrapper_work_serializer_orphan,
+     additional_constraints_client_channel_subchannel_wrapper_work_serializer_orphan,
+     true, true},
 };
 
 }  // namespace grpc_core
@@ -247,6 +258,13 @@ const char* const description_jitter_max_idle =
     "only on max connection age, but it seems like this could smooth out some "
     "herding problems.";
 const char* const additional_constraints_jitter_max_idle = "{}";
+const char* const
+    description_client_channel_subchannel_wrapper_work_serializer_orphan =
+        "Client channel subchannel wrapper hops into WorkSerializer at "
+        "Orphan() time, rather than requiring callers to do it.";
+const char* const
+    additional_constraints_client_channel_subchannel_wrapper_work_serializer_orphan =
+        "{}";
 }  // namespace
 
 namespace grpc_core {
@@ -300,6 +318,10 @@ const ExperimentMetadata g_experiment_metadata[] = {
      additional_constraints_lazier_stream_updates, true, true},
     {"jitter_max_idle", description_jitter_max_idle,
      additional_constraints_jitter_max_idle, true, true},
+    {"client_channel_subchannel_wrapper_work_serializer_orphan",
+     description_client_channel_subchannel_wrapper_work_serializer_orphan,
+     additional_constraints_client_channel_subchannel_wrapper_work_serializer_orphan,
+     true, true},
 };
 
 }  // namespace grpc_core
@@ -389,6 +411,13 @@ const char* const description_jitter_max_idle =
     "only on max connection age, but it seems like this could smooth out some "
     "herding problems.";
 const char* const additional_constraints_jitter_max_idle = "{}";
+const char* const
+    description_client_channel_subchannel_wrapper_work_serializer_orphan =
+        "Client channel subchannel wrapper hops into WorkSerializer at "
+        "Orphan() time, rather than requiring callers to do it.";
+const char* const
+    additional_constraints_client_channel_subchannel_wrapper_work_serializer_orphan =
+        "{}";
 }  // namespace
 
 namespace grpc_core {
@@ -442,6 +471,10 @@ const ExperimentMetadata g_experiment_metadata[] = {
      additional_constraints_lazier_stream_updates, true, true},
     {"jitter_max_idle", description_jitter_max_idle,
      additional_constraints_jitter_max_idle, true, true},
+    {"client_channel_subchannel_wrapper_work_serializer_orphan",
+     description_client_channel_subchannel_wrapper_work_serializer_orphan,
+     additional_constraints_client_channel_subchannel_wrapper_work_serializer_orphan,
+     true, true},
 };
 
 }  // namespace grpc_core

src/core/lib/experiments/experiments.h

@@ -87,6 +87,10 @@ inline bool IsKeepaliveServerFixEnabled() { return false; }
 inline bool IsLazierStreamUpdatesEnabled() { return true; }
 #define GRPC_EXPERIMENT_IS_INCLUDED_JITTER_MAX_IDLE
 inline bool IsJitterMaxIdleEnabled() { return true; }
+#define GRPC_EXPERIMENT_IS_INCLUDED_CLIENT_CHANNEL_SUBCHANNEL_WRAPPER_WORK_SERIALIZER_ORPHAN
+inline bool IsClientChannelSubchannelWrapperWorkSerializerOrphanEnabled() {
+  return true;
+}
 
 #elif defined(GPR_WINDOWS)
 inline bool IsTcpFrameSizeTuningEnabled() { return false; }
@@ -117,6 +121,10 @@ inline bool IsKeepaliveServerFixEnabled() { return false; }
 inline bool IsLazierStreamUpdatesEnabled() { return true; }
 #define GRPC_EXPERIMENT_IS_INCLUDED_JITTER_MAX_IDLE
 inline bool IsJitterMaxIdleEnabled() { return true; }
+#define GRPC_EXPERIMENT_IS_INCLUDED_CLIENT_CHANNEL_SUBCHANNEL_WRAPPER_WORK_SERIALIZER_ORPHAN
+inline bool IsClientChannelSubchannelWrapperWorkSerializerOrphanEnabled() {
+  return true;
+}
 
 #else
 inline bool IsTcpFrameSizeTuningEnabled() { return false; }
@@ -147,6 +155,10 @@ inline bool IsKeepaliveServerFixEnabled() { return false; }
 inline bool IsLazierStreamUpdatesEnabled() { return true; }
 #define GRPC_EXPERIMENT_IS_INCLUDED_JITTER_MAX_IDLE
 inline bool IsJitterMaxIdleEnabled() { return true; }
+#define GRPC_EXPERIMENT_IS_INCLUDED_CLIENT_CHANNEL_SUBCHANNEL_WRAPPER_WORK_SERIALIZER_ORPHAN
+inline bool IsClientChannelSubchannelWrapperWorkSerializerOrphanEnabled() {
+  return true;
+}
 #endif
 
 #else
@@ -202,8 +214,12 @@ inline bool IsKeepaliveServerFixEnabled() { return IsExperimentEnabled(20); }
 inline bool IsLazierStreamUpdatesEnabled() { return IsExperimentEnabled(21); }
 #define GRPC_EXPERIMENT_IS_INCLUDED_JITTER_MAX_IDLE
 inline bool IsJitterMaxIdleEnabled() { return IsExperimentEnabled(22); }
+#define GRPC_EXPERIMENT_IS_INCLUDED_CLIENT_CHANNEL_SUBCHANNEL_WRAPPER_WORK_SERIALIZER_ORPHAN
+inline bool IsClientChannelSubchannelWrapperWorkSerializerOrphanEnabled() {
+  return IsExperimentEnabled(23);
+}
 
-constexpr const size_t kNumExperiments = 23;
+constexpr const size_t kNumExperiments = 24;
 extern const ExperimentMetadata g_experiment_metadata[kNumExperiments];
 
 #endif

src/core/lib/experiments/experiments.yaml

@@ -185,3 +185,10 @@
   owner: [email protected]
   test_tags: []
   allow_in_fuzzing_config: true
+- name: client_channel_subchannel_wrapper_work_serializer_orphan
+  description:
+    Client channel subchannel wrapper hops into WorkSerializer at
+    Orphan() time, rather than requiring callers to do it.
+  expiry: 2023/11/15
+  owner: [email protected]
+  test_tags: ["cpp_lb_end2end_test", "xds_end2end_test"]

src/core/lib/experiments/rollouts.yaml

@@ -94,3 +94,5 @@
   default: true
 - name: jitter_max_idle
   default: true
+- name: client_channel_subchannel_wrapper_work_serializer_orphan
+  default: true