From 05af32503c2fa4ad8c966ebe5b227b159d76331e Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 31 May 2024 17:00:48 -0700 Subject: [PATCH] [context] Move ServiceConfigCallData to arena based context (#36779) Closes #36779 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36779 from ctiller:ctx5 be0be931da10c39a08978e88a6e570270d008804 PiperOrigin-RevId: 639195392 --- src/core/client_channel/client_channel.cc | 2 +- .../client_channel/client_channel_filter.cc | 20 +++++++--------- .../client_channel/client_channel_internal.h | 10 +++++--- src/core/client_channel/retry_filter.cc | 7 ++---- src/core/client_channel/retry_filter.h | 3 +-- .../retry_filter_legacy_call_data.cc | 15 ++++++------ .../fault_injection/fault_injection_filter.cc | 5 +--- .../message_compress/compression_filter.cc | 3 +-- .../message_size/message_size_filter.cc | 10 +++----- .../message_size/message_size_filter.h | 3 +-- src/core/ext/filters/rbac/rbac_filter.cc | 5 +--- .../stateful_session_filter.cc | 5 +--- src/core/lib/channel/context.h | 9 -------- src/core/lib/resource_quota/arena.h | 4 ++++ src/core/resolver/xds/xds_resolver.cc | 5 +--- .../server/server_config_selector_filter.cc | 3 +-- .../service_config/service_config_call_data.h | 23 +++++++++++-------- .../service_config_channel_arg_filter.cc | 3 +-- 18 files changed, 54 insertions(+), 81 deletions(-) diff --git a/src/core/client_channel/client_channel.cc b/src/core/client_channel/client_channel.cc index 5395efce7b9..75e261f34c1 100644 --- a/src/core/client_channel/client_channel.cc +++ b/src/core/client_channel/client_channel.cc @@ -1350,7 +1350,7 @@ absl::Status ClientChannel::ApplyServiceConfigToCall( // below us in the stack, and it will be cleaned up when the call ends. auto* service_config_call_data = GetContext()->New( - GetContext(), GetContext()); + GetContext()); // Use the ConfigSelector to determine the config for the call. absl::Status call_config_status = config_selector.GetCallConfig( {&client_initial_metadata, GetContext(), diff --git a/src/core/client_channel/client_channel_filter.cc b/src/core/client_channel/client_channel_filter.cc index 9f935dc1b59..cfdda558f3e 100644 --- a/src/core/client_channel/client_channel_filter.cc +++ b/src/core/client_channel/client_channel_filter.cc @@ -444,10 +444,9 @@ const grpc_channel_filter ClientChannelFilter::kFilterVtableWithoutPromises = { namespace { -ClientChannelServiceConfigCallData* GetServiceConfigCallData( - grpc_call_context_element* context) { - return static_cast( - context[GRPC_CONTEXT_SERVICE_CONFIG_CALL_DATA].value); +ClientChannelServiceConfigCallData* GetServiceConfigCallData(Arena* arena) { + return DownCast( + arena->GetContext()); } class DynamicTerminationFilter final { @@ -482,7 +481,7 @@ class DynamicTerminationFilter final { std::move(call_args), []() { auto* service_config_call_data = - GetServiceConfigCallData(GetContext()); + GetServiceConfigCallData(GetContext()); service_config_call_data->Commit(); }, /*is_transparent_retry=*/false); @@ -535,8 +534,7 @@ class DynamicTerminationFilter::CallData final { calld->call_context_, calld->path_, /*start_time=*/0, calld->deadline_, calld->arena_, calld->call_combiner_}; - auto* service_config_call_data = - GetServiceConfigCallData(calld->call_context_); + auto* service_config_call_data = GetServiceConfigCallData(calld->arena_); calld->lb_call_ = client_channel->CreateLoadBalancedCall( args, pollent, nullptr, [service_config_call_data]() { service_config_call_data->Commit(); }, @@ -2066,7 +2064,7 @@ grpc_error_handle ClientChannelFilter::CallData::ApplyServiceConfigToCallLocked( // itself in the call context, so that it can be accessed by filters // below us in the stack, and it will be cleaned up when the call ends. auto* service_config_call_data = - arena()->New(arena(), call_context()); + arena()->New(arena()); // Use the ConfigSelector to determine the config for the call. absl::Status call_config_status = (*config_selector) @@ -2546,8 +2544,7 @@ void ClientChannelFilter::FilterBasedCallData:: void* arg, grpc_error_handle error) { auto* calld = static_cast(arg); auto* chand = calld->chand(); - auto* service_config_call_data = - GetServiceConfigCallData(calld->call_context()); + auto* service_config_call_data = GetServiceConfigCallData(calld->arena()); if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_call_trace)) { gpr_log(GPR_INFO, "chand=%p calld=%p: got recv_trailing_metadata_ready: error=%s " @@ -2667,8 +2664,7 @@ class ClientChannelFilter::LoadBalancedCall::Metadata final ServiceConfigCallData::CallAttributeInterface* ClientChannelFilter::LoadBalancedCall::LbCallState::GetCallAttribute( UniqueTypeName type) const { - auto* service_config_call_data = - GetServiceConfigCallData(lb_call_->call_context_); + auto* service_config_call_data = GetServiceConfigCallData(lb_call_->arena_); return service_config_call_data->GetCallAttribute(type); } diff --git a/src/core/client_channel/client_channel_internal.h b/src/core/client_channel/client_channel_internal.h index b4d204834ab..ccc9dfef8c9 100644 --- a/src/core/client_channel/client_channel_internal.h +++ b/src/core/client_channel/client_channel_internal.h @@ -58,9 +58,8 @@ class ClientChannelLbCallState : public LoadBalancingPolicy::CallState { // Internal type for ServiceConfigCallData. Handles call commits. class ClientChannelServiceConfigCallData final : public ServiceConfigCallData { public: - ClientChannelServiceConfigCallData(Arena* arena, - grpc_call_context_element* call_context) - : ServiceConfigCallData(arena, call_context) {} + explicit ClientChannelServiceConfigCallData(Arena* arena) + : ServiceConfigCallData(arena) {} void SetOnCommit(absl::AnyInvocable on_commit) { CHECK(on_commit_ == nullptr); @@ -76,6 +75,11 @@ class ClientChannelServiceConfigCallData final : public ServiceConfigCallData { absl::AnyInvocable on_commit_; }; +template <> +struct ContextSubclass { + using Base = ServiceConfigCallData; +}; + class SubchannelInterfaceWithCallDestination : public SubchannelInterface { public: using SubchannelInterface::SubchannelInterface; diff --git a/src/core/client_channel/retry_filter.cc b/src/core/client_channel/retry_filter.cc index fae1ae427fb..1833ffa80d6 100644 --- a/src/core/client_channel/retry_filter.cc +++ b/src/core/client_channel/retry_filter.cc @@ -130,11 +130,8 @@ RetryFilter::RetryFilter(const ChannelArgs& args, grpc_error_handle* error) server_name, config->max_milli_tokens(), config->milli_token_ratio()); } -const RetryMethodConfig* RetryFilter::GetRetryPolicy( - const grpc_call_context_element* context) { - if (context == nullptr) return nullptr; - auto* svc_cfg_call_data = static_cast( - context[GRPC_CONTEXT_SERVICE_CONFIG_CALL_DATA].value); +const RetryMethodConfig* RetryFilter::GetRetryPolicy(Arena* arena) { + auto* svc_cfg_call_data = arena->GetContext(); if (svc_cfg_call_data == nullptr) return nullptr; return static_cast( svc_cfg_call_data->GetMethodParsedConfig(service_config_parser_index_)); diff --git a/src/core/client_channel/retry_filter.h b/src/core/client_channel/retry_filter.h index 0da23f845b2..dc4968bd483 100644 --- a/src/core/client_channel/retry_filter.h +++ b/src/core/client_channel/retry_filter.h @@ -66,8 +66,7 @@ class RetryFilter final { // any even moderately compelling reason to do so. static double BackoffJitter() { return 0.2; } - const internal::RetryMethodConfig* GetRetryPolicy( - const grpc_call_context_element* context); + const internal::RetryMethodConfig* GetRetryPolicy(Arena* arena); RefCountedPtr retry_throttle_data() const { return retry_throttle_data_; diff --git a/src/core/client_channel/retry_filter_legacy_call_data.cc b/src/core/client_channel/retry_filter_legacy_call_data.cc index 8f1d48d48ae..d4feda31cd4 100644 --- a/src/core/client_channel/retry_filter_legacy_call_data.cc +++ b/src/core/client_channel/retry_filter_legacy_call_data.cc @@ -137,9 +137,8 @@ RetryFilter::LegacyCallData::CallAttempt::CallAttempt( lb_call_committed_ = true; if (calld_->retry_committed_) { auto* service_config_call_data = - static_cast( - calld_->call_context_[GRPC_CONTEXT_SERVICE_CONFIG_CALL_DATA] - .value); + DownCast( + calld_->arena_->GetContext()); service_config_call_data->Commit(); } }, @@ -1545,7 +1544,7 @@ RetryFilter::LegacyCallData::LegacyCallData(RetryFilter* chand, const grpc_call_element_args& args) : chand_(chand), retry_throttle_data_(chand->retry_throttle_data()), - retry_policy_(chand->GetRetryPolicy(args.context)), + retry_policy_(chand->GetRetryPolicy(args.arena)), retry_backoff_( BackOff::Options() .set_initial_backoff(retry_policy_ == nullptr @@ -1685,8 +1684,8 @@ void RetryFilter::LegacyCallData::StartTransportStreamOpBatch( } PendingBatchClear(pending); auto* service_config_call_data = - static_cast( - call_context_[GRPC_CONTEXT_SERVICE_CONFIG_CALL_DATA].value); + DownCast( + arena_->GetContext()); committed_call_ = CreateLoadBalancedCall( [service_config_call_data]() { service_config_call_data->Commit(); }, /*is_transparent_retry=*/false); @@ -1976,8 +1975,8 @@ void RetryFilter::LegacyCallData::RetryCommit(CallAttempt* call_attempt) { // problem anymore. if (call_attempt->lb_call_committed()) { auto* service_config_call_data = - static_cast( - call_context_[GRPC_CONTEXT_SERVICE_CONFIG_CALL_DATA].value); + DownCast( + arena_->GetContext()); service_config_call_data->Commit(); } // Free cached send ops. diff --git a/src/core/ext/filters/fault_injection/fault_injection_filter.cc b/src/core/ext/filters/fault_injection/fault_injection_filter.cc index ae64f83ddc8..93ed9478ac2 100644 --- a/src/core/ext/filters/fault_injection/fault_injection_filter.cc +++ b/src/core/ext/filters/fault_injection/fault_injection_filter.cc @@ -167,10 +167,7 @@ FaultInjectionFilter::MakeInjectionDecision( const ClientMetadata& initial_metadata) { // Fetch the fault injection policy from the service config, based on the // relative index for which policy should this CallData use. - auto* service_config_call_data = static_cast( - GetContext< - grpc_call_context_element>()[GRPC_CONTEXT_SERVICE_CONFIG_CALL_DATA] - .value); + auto* service_config_call_data = GetContext(); auto* method_params = static_cast( service_config_call_data->GetMethodParsedConfig( service_config_parser_index_)); diff --git a/src/core/ext/filters/http/message_compress/compression_filter.cc b/src/core/ext/filters/http/message_compress/compression_filter.cc index 96272570aa4..263371fda4f 100644 --- a/src/core/ext/filters/http/message_compress/compression_filter.cc +++ b/src/core/ext/filters/http/message_compress/compression_filter.cc @@ -232,8 +232,7 @@ ChannelCompression::DecompressArgs ChannelCompression::HandleIncomingMetadata( auto max_recv_message_length = max_recv_size_; const MessageSizeParsedConfig* limits = MessageSizeParsedConfig::GetFromCallContext( - GetContext(), - message_size_service_config_parser_index_); + GetContext(), message_size_service_config_parser_index_); if (limits != nullptr && limits->max_recv_size().has_value() && (!max_recv_message_length.has_value() || *limits->max_recv_size() < *max_recv_message_length)) { diff --git a/src/core/ext/filters/message_size/message_size_filter.cc b/src/core/ext/filters/message_size/message_size_filter.cc index 2c73c63a370..bf79c4dae93 100644 --- a/src/core/ext/filters/message_size/message_size_filter.cc +++ b/src/core/ext/filters/message_size/message_size_filter.cc @@ -64,11 +64,8 @@ const NoInterceptor ServerMessageSizeFilter::Call::OnFinalize; // const MessageSizeParsedConfig* MessageSizeParsedConfig::GetFromCallContext( - const grpc_call_context_element* context, - size_t service_config_parser_index) { - if (context == nullptr) return nullptr; - auto* svc_cfg_call_data = static_cast( - context[GRPC_CONTEXT_SERVICE_CONFIG_CALL_DATA].value); + Arena* arena, size_t service_config_parser_index) { + auto* svc_cfg_call_data = arena->GetContext(); if (svc_cfg_call_data == nullptr) return nullptr; return static_cast( svc_cfg_call_data->GetMethodParsedConfig(service_config_parser_index)); @@ -188,8 +185,7 @@ ClientMessageSizeFilter::Call::Call(ClientMessageSizeFilter* filter) // size to the receive limit. const MessageSizeParsedConfig* config_from_call_context = MessageSizeParsedConfig::GetFromCallContext( - GetContext(), - filter->service_config_parser_index_); + GetContext(), filter->service_config_parser_index_); if (config_from_call_context != nullptr) { absl::optional max_send_size = limits_.max_send_size(); absl::optional max_recv_size = limits_.max_recv_size(); diff --git a/src/core/ext/filters/message_size/message_size_filter.h b/src/core/ext/filters/message_size/message_size_filter.h index 200f6880883..e6b83900304 100644 --- a/src/core/ext/filters/message_size/message_size_filter.h +++ b/src/core/ext/filters/message_size/message_size_filter.h @@ -55,8 +55,7 @@ class MessageSizeParsedConfig : public ServiceConfigParser::ParsedConfig { : max_send_size_(max_send_size), max_recv_size_(max_recv_size) {} static const MessageSizeParsedConfig* GetFromCallContext( - const grpc_call_context_element* context, - size_t service_config_parser_index); + Arena* arena, size_t service_config_parser_index); static MessageSizeParsedConfig GetFromChannelArgs(const ChannelArgs& args); diff --git a/src/core/ext/filters/rbac/rbac_filter.cc b/src/core/ext/filters/rbac/rbac_filter.cc index c89c6962e19..199669b448d 100644 --- a/src/core/ext/filters/rbac/rbac_filter.cc +++ b/src/core/ext/filters/rbac/rbac_filter.cc @@ -53,10 +53,7 @@ const NoInterceptor RbacFilter::Call::OnFinalize; absl::Status RbacFilter::Call::OnClientInitialMetadata(ClientMetadata& md, RbacFilter* filter) { // Fetch and apply the rbac policy from the service config. - auto* service_config_call_data = static_cast( - GetContext< - grpc_call_context_element>()[GRPC_CONTEXT_SERVICE_CONFIG_CALL_DATA] - .value); + auto* service_config_call_data = GetContext(); auto* method_params = static_cast( service_config_call_data->GetMethodParsedConfig( filter->service_config_parser_index_)); diff --git a/src/core/ext/filters/stateful_session/stateful_session_filter.cc b/src/core/ext/filters/stateful_session/stateful_session_filter.cc index f383f3fb9ed..2648ca95249 100644 --- a/src/core/ext/filters/stateful_session/stateful_session_filter.cc +++ b/src/core/ext/filters/stateful_session/stateful_session_filter.cc @@ -225,10 +225,7 @@ bool IsConfiguredPath(absl::string_view configured_path, void StatefulSessionFilter::Call::OnClientInitialMetadata( ClientMetadata& md, StatefulSessionFilter* filter) { // Get config. - auto* service_config_call_data = static_cast( - GetContext< - grpc_call_context_element>()[GRPC_CONTEXT_SERVICE_CONFIG_CALL_DATA] - .value); + auto* service_config_call_data = GetContext(); CHECK_NE(service_config_call_data, nullptr); auto* method_params = static_cast( service_config_call_data->GetMethodParsedConfig( diff --git a/src/core/lib/channel/context.h b/src/core/lib/channel/context.h index f31f6afb938..fbe64803a33 100644 --- a/src/core/lib/channel/context.h +++ b/src/core/lib/channel/context.h @@ -29,9 +29,6 @@ /// This enum represents the indexes into the array, where each index /// contains a different type of value. typedef enum { - /// Holds a pointer to ServiceConfigCallData associated with this call. - GRPC_CONTEXT_SERVICE_CONFIG_CALL_DATA, - GRPC_CONTEXT_COUNT } grpc_context_index; @@ -54,12 +51,6 @@ namespace promise_detail { template struct OldStyleContext; -template <> -struct OldStyleContext { - static constexpr grpc_context_index kIndex = - GRPC_CONTEXT_SERVICE_CONFIG_CALL_DATA; -}; - template class Context::kIndex)>> { public: diff --git a/src/core/lib/resource_quota/arena.h b/src/core/lib/resource_quota/arena.h index 0b92e4b5b74..916dbec4548 100644 --- a/src/core/lib/resource_quota/arena.h +++ b/src/core/lib/resource_quota/arena.h @@ -266,6 +266,10 @@ class Arena final : public RefCounted` and `SetContext<>` functions + // for modern promise-based code -- however legacy filter stack based code + // often needs to access these directly. template T* GetContext() { return static_cast( diff --git a/src/core/resolver/xds/xds_resolver.cc b/src/core/resolver/xds/xds_resolver.cc index 6e51ab1d856..c61d219a8d0 100644 --- a/src/core/resolver/xds/xds_resolver.cc +++ b/src/core/resolver/xds/xds_resolver.cc @@ -854,10 +854,7 @@ const grpc_channel_filter XdsResolver::ClusterSelectionFilter::kFilter = void XdsResolver::ClusterSelectionFilter::Call::OnClientInitialMetadata( ClientMetadata&) { auto* service_config_call_data = - static_cast( - GetContext() - [GRPC_CONTEXT_SERVICE_CONFIG_CALL_DATA] - .value); + GetContext(); CHECK_NE(service_config_call_data, nullptr); auto* route_state_attribute = static_cast( service_config_call_data->GetCallAttribute()); diff --git a/src/core/server/server_config_selector_filter.cc b/src/core/server/server_config_selector_filter.cc index b4acec5329e..7fc886544ed 100644 --- a/src/core/server/server_config_selector_filter.cc +++ b/src/core/server/server_config_selector_filter.cc @@ -149,8 +149,7 @@ absl::Status ServerConfigSelectorFilter::Call::OnClientInitialMetadata( return absl::UnavailableError(StatusToString(call_config.status())); } auto* service_config_call_data = - GetContext()->New( - GetContext(), GetContext()); + GetContext()->New(GetContext()); service_config_call_data->SetServiceConfig( std::move(call_config->service_config), call_config->method_configs); return absl::OkStatus(); diff --git a/src/core/service_config/service_config_call_data.h b/src/core/service_config/service_config_call_data.h index a422893c6e2..73e9f33f19e 100644 --- a/src/core/service_config/service_config_call_data.h +++ b/src/core/service_config/service_config_call_data.h @@ -49,11 +49,7 @@ class ServiceConfigCallData { virtual UniqueTypeName type() const = 0; }; - ServiceConfigCallData(Arena* arena, grpc_call_context_element* call_context) - : call_attributes_(arena) { - call_context[GRPC_CONTEXT_SERVICE_CONFIG_CALL_DATA].value = this; - call_context[GRPC_CONTEXT_SERVICE_CONFIG_CALL_DATA].destroy = Destroy; - } + explicit ServiceConfigCallData(Arena* arena); virtual ~ServiceConfigCallData() = default; @@ -101,16 +97,23 @@ class ServiceConfigCallData { } private: - static void Destroy(void* ptr) { - auto* self = static_cast(ptr); - self->~ServiceConfigCallData(); - } - RefCountedPtr service_config_; const ServiceConfigParser::ParsedConfigVector* method_configs_ = nullptr; ChunkedVector call_attributes_; }; +template <> +struct ArenaContextType { + static void Destroy(ServiceConfigCallData* ptr) { + ptr->~ServiceConfigCallData(); + } +}; + +inline ServiceConfigCallData::ServiceConfigCallData(Arena* arena) + : call_attributes_(arena) { + arena->SetContext(this); +} + } // namespace grpc_core #endif // GRPC_SRC_CORE_SERVICE_CONFIG_SERVICE_CONFIG_CALL_DATA_H diff --git a/src/core/service_config/service_config_channel_arg_filter.cc b/src/core/service_config/service_config_channel_arg_filter.cc index 9afa99018fc..7470fd5db2c 100644 --- a/src/core/service_config/service_config_channel_arg_filter.cc +++ b/src/core/service_config/service_config_channel_arg_filter.cc @@ -113,8 +113,7 @@ void ServiceConfigChannelArgFilter::Call::OnClientInitialMetadata( md.get_pointer(HttpPathMetadata())->c_slice()); } auto* arena = GetContext(); - auto* service_config_call_data = arena->New( - arena, GetContext()); + auto* service_config_call_data = arena->New(arena); service_config_call_data->SetServiceConfig(filter->service_config_, method_configs); }