[RegisteredMethod] Remove experiment (#36181)

Closes #36181 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36181 from yashykt:RemoveRegisteredMethodLookupInTransport 3709bfa1cf PiperOrigin-RevId: 619695276
1 year ago · 95c4f32b23
parent 3d82c522fa
commit 95c4f32b23
7 changed files with 17 additions and 94 deletions
--- a/bazel/experiments.bzl
+++ b/bazel/experiments.bzl
@ -35,8 +35,7 @@ EXPERIMENT_ENABLES = {
    "promise_based_client_call": "event_engine_client,event_engine_listener,promise_based_client_call",
    "promise_based_server_call": "promise_based_server_call",
    "chaotic_good": "chaotic_good,event_engine_client,event_engine_listener,promise_based_client_call,promise_based_server_call",
-    "registered_method_lookup_in_transport": "registered_method_lookup_in_transport",
-    "promise_based_inproc_transport": "event_engine_client,event_engine_listener,promise_based_client_call,promise_based_inproc_transport,promise_based_server_call,registered_method_lookup_in_transport",
+    "promise_based_inproc_transport": "event_engine_client,event_engine_listener,promise_based_client_call,promise_based_inproc_transport,promise_based_server_call",
    "rstpit": "rstpit",
    "schedule_cancellation_over_write": "schedule_cancellation_over_write",
    "server_privacy": "server_privacy",
@ -96,9 +95,6 @@ EXPERIMENTS = {
            "event_engine_listener_test": [
                "event_engine_listener",
            ],
-            "surface_registered_method_lookup": [
-                "registered_method_lookup_in_transport",
-            ],
        },
    },
    "ios": {
@ -134,9 +130,6 @@ EXPERIMENTS = {
            ],
        },
        "on": {
-            "surface_registered_method_lookup": [
-                "registered_method_lookup_in_transport",
-            ],
        },
    },
    "posix": {
@ -200,9 +193,6 @@ EXPERIMENTS = {
            "resolver_component_tests_runner_invoker": [
                "event_engine_dns",
            ],
-            "surface_registered_method_lookup": [
-                "registered_method_lookup_in_transport",
-            ],
            "xds_end2end_test": [
                "work_serializer_dispatch",
            ],
--- a/src/core/lib/experiments/experiments.cc
+++ b/src/core/lib/experiments/experiments.cc
@ -96,18 +96,12 @@ const char* const additional_constraints_chaotic_good = "{}";
 const uint8_t required_experiments_chaotic_good[] = {
    static_cast<uint8_t>(grpc_core::kExperimentIdPromiseBasedClientCall),
    static_cast<uint8_t>(grpc_core::kExperimentIdPromiseBasedServerCall)};
-const char* const description_registered_method_lookup_in_transport =
-    "Change registered method's lookup point to transport";
-const char* const additional_constraints_registered_method_lookup_in_transport =
-    "{}";
 const char* const description_promise_based_inproc_transport =
    "Use promises for the in-process transport.";
 const char* const additional_constraints_promise_based_inproc_transport = "{}";
 const uint8_t required_experiments_promise_based_inproc_transport[] = {
    static_cast<uint8_t>(grpc_core::kExperimentIdPromiseBasedClientCall),
-    static_cast<uint8_t>(grpc_core::kExperimentIdPromiseBasedServerCall),
-    static_cast<uint8_t>(
-        grpc_core::kExperimentIdRegisteredMethodLookupInTransport)};
+    static_cast<uint8_t>(grpc_core::kExperimentIdPromiseBasedServerCall)};
 const char* const description_rstpit =
    "On RST_STREAM on a server, reduce MAX_CONCURRENT_STREAMS for a short "
    "duration";
@ -201,14 +195,10 @@ const ExperimentMetadata g_experiment_metadata[] = {
    {"chaotic_good", description_chaotic_good,
     additional_constraints_chaotic_good, required_experiments_chaotic_good, 2,
     false, true},
-    {"registered_method_lookup_in_transport",
-     description_registered_method_lookup_in_transport,
-     additional_constraints_registered_method_lookup_in_transport, nullptr, 0,
-     true, true},
    {"promise_based_inproc_transport",
     description_promise_based_inproc_transport,
     additional_constraints_promise_based_inproc_transport,
-     required_experiments_promise_based_inproc_transport, 3, false, false},
+     required_experiments_promise_based_inproc_transport, 2, false, false},
    {"rstpit", description_rstpit, additional_constraints_rstpit, nullptr, 0,
     false, true},
    {"schedule_cancellation_over_write",
@ -316,18 +306,12 @@ const char* const additional_constraints_chaotic_good = "{}";
 const uint8_t required_experiments_chaotic_good[] = {
    static_cast<uint8_t>(grpc_core::kExperimentIdPromiseBasedClientCall),
    static_cast<uint8_t>(grpc_core::kExperimentIdPromiseBasedServerCall)};
-const char* const description_registered_method_lookup_in_transport =
-    "Change registered method's lookup point to transport";
-const char* const additional_constraints_registered_method_lookup_in_transport =
-    "{}";
 const char* const description_promise_based_inproc_transport =
    "Use promises for the in-process transport.";
 const char* const additional_constraints_promise_based_inproc_transport = "{}";
 const uint8_t required_experiments_promise_based_inproc_transport[] = {
    static_cast<uint8_t>(grpc_core::kExperimentIdPromiseBasedClientCall),
-    static_cast<uint8_t>(grpc_core::kExperimentIdPromiseBasedServerCall),
-    static_cast<uint8_t>(
-        grpc_core::kExperimentIdRegisteredMethodLookupInTransport)};
+    static_cast<uint8_t>(grpc_core::kExperimentIdPromiseBasedServerCall)};
 const char* const description_rstpit =
    "On RST_STREAM on a server, reduce MAX_CONCURRENT_STREAMS for a short "
    "duration";
@ -421,14 +405,10 @@ const ExperimentMetadata g_experiment_metadata[] = {
    {"chaotic_good", description_chaotic_good,
     additional_constraints_chaotic_good, required_experiments_chaotic_good, 2,
     false, true},
-    {"registered_method_lookup_in_transport",
-     description_registered_method_lookup_in_transport,
-     additional_constraints_registered_method_lookup_in_transport, nullptr, 0,
-     true, true},
    {"promise_based_inproc_transport",
     description_promise_based_inproc_transport,
     additional_constraints_promise_based_inproc_transport,
-     required_experiments_promise_based_inproc_transport, 3, false, false},
+     required_experiments_promise_based_inproc_transport, 2, false, false},
    {"rstpit", description_rstpit, additional_constraints_rstpit, nullptr, 0,
     false, true},
    {"schedule_cancellation_over_write",
@ -536,18 +516,12 @@ const char* const additional_constraints_chaotic_good = "{}";
 const uint8_t required_experiments_chaotic_good[] = {
    static_cast<uint8_t>(grpc_core::kExperimentIdPromiseBasedClientCall),
    static_cast<uint8_t>(grpc_core::kExperimentIdPromiseBasedServerCall)};
-const char* const description_registered_method_lookup_in_transport =
-    "Change registered method's lookup point to transport";
-const char* const additional_constraints_registered_method_lookup_in_transport =
-    "{}";
 const char* const description_promise_based_inproc_transport =
    "Use promises for the in-process transport.";
 const char* const additional_constraints_promise_based_inproc_transport = "{}";
 const uint8_t required_experiments_promise_based_inproc_transport[] = {
    static_cast<uint8_t>(grpc_core::kExperimentIdPromiseBasedClientCall),
-    static_cast<uint8_t>(grpc_core::kExperimentIdPromiseBasedServerCall),
-    static_cast<uint8_t>(
-        grpc_core::kExperimentIdRegisteredMethodLookupInTransport)};
+    static_cast<uint8_t>(grpc_core::kExperimentIdPromiseBasedServerCall)};
 const char* const description_rstpit =
    "On RST_STREAM on a server, reduce MAX_CONCURRENT_STREAMS for a short "
    "duration";
@ -641,14 +615,10 @@ const ExperimentMetadata g_experiment_metadata[] = {
    {"chaotic_good", description_chaotic_good,
     additional_constraints_chaotic_good, required_experiments_chaotic_good, 2,
     false, true},
-    {"registered_method_lookup_in_transport",
-     description_registered_method_lookup_in_transport,
-     additional_constraints_registered_method_lookup_in_transport, nullptr, 0,
-     true, true},
    {"promise_based_inproc_transport",
     description_promise_based_inproc_transport,
     additional_constraints_promise_based_inproc_transport,
-     required_experiments_promise_based_inproc_transport, 3, false, false},
+     required_experiments_promise_based_inproc_transport, 2, false, false},
    {"rstpit", description_rstpit, additional_constraints_rstpit, nullptr, 0,
     false, true},
    {"schedule_cancellation_over_write",
--- a/src/core/lib/experiments/experiments.h
+++ b/src/core/lib/experiments/experiments.h
@ -87,8 +87,6 @@ inline bool IsPendingQueueCapEnabled() { return true; }
 inline bool IsPromiseBasedClientCallEnabled() { return false; }
 inline bool IsPromiseBasedServerCallEnabled() { return false; }
 inline bool IsChaoticGoodEnabled() { return false; }
-#define GRPC_EXPERIMENT_IS_INCLUDED_REGISTERED_METHOD_LOOKUP_IN_TRANSPORT
-inline bool IsRegisteredMethodLookupInTransportEnabled() { return true; }
 inline bool IsPromiseBasedInprocTransportEnabled() { return false; }
 inline bool IsRstpitEnabled() { return false; }
 inline bool IsScheduleCancellationOverWriteEnabled() { return false; }
@ -135,8 +133,6 @@ inline bool IsPendingQueueCapEnabled() { return true; }
 inline bool IsPromiseBasedClientCallEnabled() { return false; }
 inline bool IsPromiseBasedServerCallEnabled() { return false; }
 inline bool IsChaoticGoodEnabled() { return false; }
-#define GRPC_EXPERIMENT_IS_INCLUDED_REGISTERED_METHOD_LOOKUP_IN_TRANSPORT
-inline bool IsRegisteredMethodLookupInTransportEnabled() { return true; }
 inline bool IsPromiseBasedInprocTransportEnabled() { return false; }
 inline bool IsRstpitEnabled() { return false; }
 inline bool IsScheduleCancellationOverWriteEnabled() { return false; }
@ -184,8 +180,6 @@ inline bool IsPendingQueueCapEnabled() { return true; }
 inline bool IsPromiseBasedClientCallEnabled() { return false; }
 inline bool IsPromiseBasedServerCallEnabled() { return false; }
 inline bool IsChaoticGoodEnabled() { return false; }
-#define GRPC_EXPERIMENT_IS_INCLUDED_REGISTERED_METHOD_LOOKUP_IN_TRANSPORT
-inline bool IsRegisteredMethodLookupInTransportEnabled() { return true; }
 inline bool IsPromiseBasedInprocTransportEnabled() { return false; }
 inline bool IsRstpitEnabled() { return false; }
 inline bool IsScheduleCancellationOverWriteEnabled() { return false; }
@ -222,7 +216,6 @@ enum ExperimentIds {
  kExperimentIdPromiseBasedClientCall,
  kExperimentIdPromiseBasedServerCall,
  kExperimentIdChaoticGood,
-  kExperimentIdRegisteredMethodLookupInTransport,
  kExperimentIdPromiseBasedInprocTransport,
  kExperimentIdRstpit,
  kExperimentIdScheduleCancellationOverWrite,
@ -309,10 +302,6 @@ inline bool IsPromiseBasedServerCallEnabled() {
 inline bool IsChaoticGoodEnabled() {
  return IsExperimentEnabled(kExperimentIdChaoticGood);
 }
-#define GRPC_EXPERIMENT_IS_INCLUDED_REGISTERED_METHOD_LOOKUP_IN_TRANSPORT
-inline bool IsRegisteredMethodLookupInTransportEnabled() {
-  return IsExperimentEnabled(kExperimentIdRegisteredMethodLookupInTransport);
-}
 #define GRPC_EXPERIMENT_IS_INCLUDED_PROMISE_BASED_INPROC_TRANSPORT
 inline bool IsPromiseBasedInprocTransportEnabled() {
  return IsExperimentEnabled(kExperimentIdPromiseBasedInprocTransport);
--- a/src/core/lib/experiments/experiments.yaml
+++ b/src/core/lib/experiments/experiments.yaml
@ -165,7 +165,7 @@
  owner: ctiller@google.com
  test_tags: []
  allow_in_fuzzing_config: false # experiment currently crashes if enabled
-  requires: [promise_based_client_call, promise_based_server_call, registered_method_lookup_in_transport]
+  requires: [promise_based_client_call, promise_based_server_call]
 - name: promise_based_server_call
  description:
    If set, use the new gRPC promise based call code when it's appropriate
@ -173,12 +173,6 @@
  expiry: 2024/06/14
  owner: ctiller@google.com
  test_tags: ["core_end2end_test", "cpp_end2end_test", "xds_end2end_test", "logging_test"]
- name: registered_method_lookup_in_transport
-  description:
-    Change registered method's lookup point to transport
-  expiry: 2024/03/31
-  owner: yashkt@google.com
-  test_tags: ["surface_registered_method_lookup"]
 - name: rstpit
  description:
    On RST_STREAM on a server, reduce MAX_CONCURRENT_STREAMS for a short duration
--- a/src/core/lib/experiments/rollouts.yaml
+++ b/src/core/lib/experiments/rollouts.yaml
@ -96,8 +96,6 @@
    posix: false
 - name: promise_based_server_call
  default: false
- name: registered_method_lookup_in_transport
-  default: true
 - name: rstpit
  default: false
 - name: schedule_cancellation_over_write
--- a/src/core/lib/surface/server.cc
+++ b/src/core/lib/surface/server.cc
@ -1330,13 +1330,10 @@ void Server::ChannelData::InitTransport(RefCountedPtr<Server> server,
    ++accept_stream_types;
    op->set_accept_stream = true;
    op->set_accept_stream_fn = AcceptStream;
-    if (IsRegisteredMethodLookupInTransportEnabled()) {
-      op->set_registered_method_matcher_fn = [](void* arg,
-                                                ClientMetadata* metadata) {
-        static_cast<ChannelData*>(arg)->SetRegisteredMethodOnMetadata(
-            *metadata);
-      };
-    }
+    op->set_registered_method_matcher_fn = [](void* arg,
+                                              ClientMetadata* metadata) {
+      static_cast<ChannelData*>(arg)->SetRegisteredMethodOnMetadata(*metadata);
+    };
    op->set_accept_stream_user_data = this;
  }
  if (transport->server_transport() != nullptr) {
@ -1527,15 +1524,9 @@ ArenaPromise<ServerMetadataHandle> Server::ChannelData::MakeCallPromise(
  }
  // Find request matcher.
  RequestMatcherInterface* matcher;
-  RegisteredMethod* rm = nullptr;
-  if (IsRegisteredMethodLookupInTransportEnabled()) {
-    rm = static_cast<RegisteredMethod*>(
-        call_args.client_initial_metadata->get(GrpcRegisteredMethod())
-            .value_or(nullptr));
-  } else {
-    rm = chand->GetRegisteredMethod(host_ptr->as_string_view(),
-                                    path_ptr->as_string_view());
-  }
+  RegisteredMethod* rm = static_cast<RegisteredMethod*>(
+      call_args.client_initial_metadata->get(GrpcRegisteredMethod())
+          .value_or(nullptr));
  ArenaPromise<absl::StatusOr<NextResult<MessageHandle>>>
      maybe_read_first_message([] { return NextResult<MessageHandle>(); });
  if (rm != nullptr) {
@ -1760,7 +1751,6 @@ void Server::CallData::KillZombie() {

 // If this changes, change MakeCallPromise too.
 void Server::CallData::StartNewRpc(grpc_call_element* elem) {
-  auto* chand = static_cast<ChannelData*>(elem->channel_data);
  if (server_->ShutdownCalled()) {
    state_.store(CallState::ZOMBIED, std::memory_order_relaxed);
    KillZombie();
@ -1771,15 +1761,8 @@ void Server::CallData::StartNewRpc(grpc_call_element* elem) {
  grpc_server_register_method_payload_handling payload_handling =
      GRPC_SRM_PAYLOAD_NONE;
  if (path_.has_value() && host_.has_value()) {
-    RegisteredMethod* rm;
-    if (IsRegisteredMethodLookupInTransportEnabled()) {
-      rm = static_cast<RegisteredMethod*>(
-          recv_initial_metadata_->get(GrpcRegisteredMethod())
-              .value_or(nullptr));
-    } else {
-      rm = chand->GetRegisteredMethod(host_->as_string_view(),
-                                      path_->as_string_view());
-    }
+    RegisteredMethod* rm = static_cast<RegisteredMethod*>(
+        recv_initial_metadata_->get(GrpcRegisteredMethod()).value_or(nullptr));
    if (rm != nullptr) {
      matcher_ = rm->matcher.get();
      payload_handling = rm->payload_handling;
--- a/test/cpp/end2end/BUILD
+++ b/test/cpp/end2end/BUILD
@ -364,7 +364,6 @@ grpc_cc_test(
    tags = [
        "cpp_end2end_test",
        "no_test_ios",
-        "surface_registered_method_lookup",
    ],
    deps = [
        ":end2end_test_lib",