From 59004b493104293ca9725c55f4af44498755b8d9 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Tue, 30 Jan 2024 15:58:45 -0800 Subject: [PATCH] [channel] Separate call size estimation out of `grpc_core::Channel` (#35732) Closes #35732 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35732 from ctiller:chan 0d7a29fc772587f08a5d5f626acd0d0ce25e3aef PiperOrigin-RevId: 602865976 --- BUILD | 1 + CMakeLists.txt | 3 ++ Makefile | 2 + Package.swift | 2 + build_autogenerated.yaml | 6 +++ config.m4 | 1 + config.w32 | 1 + gRPC-C++.podspec | 2 + gRPC-Core.podspec | 3 ++ grpc.gemspec | 2 + grpc.gyp | 3 ++ package.xml | 2 + src/core/BUILD | 11 ++++ src/core/lib/surface/channel.cc | 22 +------- src/core/lib/surface/channel.h | 21 ++------ src/core/lib/transport/call_size_estimator.cc | 41 +++++++++++++++ src/core/lib/transport/call_size_estimator.h | 52 +++++++++++++++++++ src/python/grpcio/grpc_core_dependencies.py | 1 + tools/doxygen/Doxyfile.c++.internal | 2 + tools/doxygen/Doxyfile.core.internal | 2 + 20 files changed, 144 insertions(+), 36 deletions(-) create mode 100644 src/core/lib/transport/call_size_estimator.cc create mode 100644 src/core/lib/transport/call_size_estimator.h diff --git a/BUILD b/BUILD index 7c2afe0697a..12f5560f78d 100644 --- a/BUILD +++ b/BUILD @@ -1533,6 +1533,7 @@ grpc_cc_library( "//src/core:bitset", "//src/core:call_filters", "//src/core:call_final_info", + "//src/core:call_size_estimator", "//src/core:call_spine", "//src/core:cancel_callback", "//src/core:channel_args", diff --git a/CMakeLists.txt b/CMakeLists.txt index 539fb5cb73e..76ed926128c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -2528,6 +2528,7 @@ add_library(grpc src/core/lib/transport/bdp_estimator.cc src/core/lib/transport/call_filters.cc src/core/lib/transport/call_final_info.cc + src/core/lib/transport/call_size_estimator.cc src/core/lib/transport/call_spine.cc src/core/lib/transport/connectivity_state.cc src/core/lib/transport/error_utils.cc @@ -3244,6 +3245,7 @@ add_library(grpc_unsecure src/core/lib/transport/bdp_estimator.cc src/core/lib/transport/call_filters.cc src/core/lib/transport/call_final_info.cc + src/core/lib/transport/call_size_estimator.cc src/core/lib/transport/call_spine.cc src/core/lib/transport/connectivity_state.cc src/core/lib/transport/error_utils.cc @@ -5332,6 +5334,7 @@ add_library(grpc_authorization_provider src/core/lib/transport/batch_builder.cc src/core/lib/transport/call_filters.cc src/core/lib/transport/call_final_info.cc + src/core/lib/transport/call_size_estimator.cc src/core/lib/transport/call_spine.cc src/core/lib/transport/connectivity_state.cc src/core/lib/transport/error_utils.cc diff --git a/Makefile b/Makefile index b857b96f504..ae698c94429 100644 --- a/Makefile +++ b/Makefile @@ -1710,6 +1710,7 @@ LIBGRPC_SRC = \ src/core/lib/transport/bdp_estimator.cc \ src/core/lib/transport/call_filters.cc \ src/core/lib/transport/call_final_info.cc \ + src/core/lib/transport/call_size_estimator.cc \ src/core/lib/transport/call_spine.cc \ src/core/lib/transport/connectivity_state.cc \ src/core/lib/transport/error_utils.cc \ @@ -2260,6 +2261,7 @@ LIBGRPC_UNSECURE_SRC = \ src/core/lib/transport/bdp_estimator.cc \ src/core/lib/transport/call_filters.cc \ src/core/lib/transport/call_final_info.cc \ + src/core/lib/transport/call_size_estimator.cc \ src/core/lib/transport/call_spine.cc \ src/core/lib/transport/connectivity_state.cc \ src/core/lib/transport/error_utils.cc \ diff --git a/Package.swift b/Package.swift index 5bbde1b5430..01210a2c043 100644 --- a/Package.swift +++ b/Package.swift @@ -1903,6 +1903,8 @@ let package = Package( "src/core/lib/transport/call_filters.h", "src/core/lib/transport/call_final_info.cc", "src/core/lib/transport/call_final_info.h", + "src/core/lib/transport/call_size_estimator.cc", + "src/core/lib/transport/call_size_estimator.h", "src/core/lib/transport/call_spine.cc", "src/core/lib/transport/call_spine.h", "src/core/lib/transport/connectivity_state.cc", diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 634658fae83..2d90e5b657b 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -1179,6 +1179,7 @@ libs: - src/core/lib/transport/bdp_estimator.h - src/core/lib/transport/call_filters.h - src/core/lib/transport/call_final_info.h + - src/core/lib/transport/call_size_estimator.h - src/core/lib/transport/call_spine.h - src/core/lib/transport/connectivity_state.h - src/core/lib/transport/custom_metadata.h @@ -1983,6 +1984,7 @@ libs: - src/core/lib/transport/bdp_estimator.cc - src/core/lib/transport/call_filters.cc - src/core/lib/transport/call_final_info.cc + - src/core/lib/transport/call_size_estimator.cc - src/core/lib/transport/call_spine.cc - src/core/lib/transport/connectivity_state.cc - src/core/lib/transport/error_utils.cc @@ -2630,6 +2632,7 @@ libs: - src/core/lib/transport/bdp_estimator.h - src/core/lib/transport/call_filters.h - src/core/lib/transport/call_final_info.h + - src/core/lib/transport/call_size_estimator.h - src/core/lib/transport/call_spine.h - src/core/lib/transport/connectivity_state.h - src/core/lib/transport/custom_metadata.h @@ -3050,6 +3053,7 @@ libs: - src/core/lib/transport/bdp_estimator.cc - src/core/lib/transport/call_filters.cc - src/core/lib/transport/call_final_info.cc + - src/core/lib/transport/call_size_estimator.cc - src/core/lib/transport/call_spine.cc - src/core/lib/transport/connectivity_state.cc - src/core/lib/transport/error_utils.cc @@ -4673,6 +4677,7 @@ libs: - src/core/lib/transport/batch_builder.h - src/core/lib/transport/call_filters.h - src/core/lib/transport/call_final_info.h + - src/core/lib/transport/call_size_estimator.h - src/core/lib/transport/call_spine.h - src/core/lib/transport/connectivity_state.h - src/core/lib/transport/custom_metadata.h @@ -4970,6 +4975,7 @@ libs: - src/core/lib/transport/batch_builder.cc - src/core/lib/transport/call_filters.cc - src/core/lib/transport/call_final_info.cc + - src/core/lib/transport/call_size_estimator.cc - src/core/lib/transport/call_spine.cc - src/core/lib/transport/connectivity_state.cc - src/core/lib/transport/error_utils.cc diff --git a/config.m4 b/config.m4 index 89713d1a315..5d66de065ca 100644 --- a/config.m4 +++ b/config.m4 @@ -838,6 +838,7 @@ if test "$PHP_GRPC" != "no"; then src/core/lib/transport/bdp_estimator.cc \ src/core/lib/transport/call_filters.cc \ src/core/lib/transport/call_final_info.cc \ + src/core/lib/transport/call_size_estimator.cc \ src/core/lib/transport/call_spine.cc \ src/core/lib/transport/connectivity_state.cc \ src/core/lib/transport/error_utils.cc \ diff --git a/config.w32 b/config.w32 index 0b6600482bd..e62812a47c5 100644 --- a/config.w32 +++ b/config.w32 @@ -803,6 +803,7 @@ if (PHP_GRPC != "no") { "src\\core\\lib\\transport\\bdp_estimator.cc " + "src\\core\\lib\\transport\\call_filters.cc " + "src\\core\\lib\\transport\\call_final_info.cc " + + "src\\core\\lib\\transport\\call_size_estimator.cc " + "src\\core\\lib\\transport\\call_spine.cc " + "src\\core\\lib\\transport\\connectivity_state.cc " + "src\\core\\lib\\transport\\error_utils.cc " + diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index cb91126225b..b8e1e878059 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -1283,6 +1283,7 @@ Pod::Spec.new do |s| 'src/core/lib/transport/bdp_estimator.h', 'src/core/lib/transport/call_filters.h', 'src/core/lib/transport/call_final_info.h', + 'src/core/lib/transport/call_size_estimator.h', 'src/core/lib/transport/call_spine.h', 'src/core/lib/transport/connectivity_state.h', 'src/core/lib/transport/custom_metadata.h', @@ -2538,6 +2539,7 @@ Pod::Spec.new do |s| 'src/core/lib/transport/bdp_estimator.h', 'src/core/lib/transport/call_filters.h', 'src/core/lib/transport/call_final_info.h', + 'src/core/lib/transport/call_size_estimator.h', 'src/core/lib/transport/call_spine.h', 'src/core/lib/transport/connectivity_state.h', 'src/core/lib/transport/custom_metadata.h', diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index 8c5a7ffec9d..5c5ed2dadc5 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -2012,6 +2012,8 @@ Pod::Spec.new do |s| 'src/core/lib/transport/call_filters.h', 'src/core/lib/transport/call_final_info.cc', 'src/core/lib/transport/call_final_info.h', + 'src/core/lib/transport/call_size_estimator.cc', + 'src/core/lib/transport/call_size_estimator.h', 'src/core/lib/transport/call_spine.cc', 'src/core/lib/transport/call_spine.h', 'src/core/lib/transport/connectivity_state.cc', @@ -3316,6 +3318,7 @@ Pod::Spec.new do |s| 'src/core/lib/transport/bdp_estimator.h', 'src/core/lib/transport/call_filters.h', 'src/core/lib/transport/call_final_info.h', + 'src/core/lib/transport/call_size_estimator.h', 'src/core/lib/transport/call_spine.h', 'src/core/lib/transport/connectivity_state.h', 'src/core/lib/transport/custom_metadata.h', diff --git a/grpc.gemspec b/grpc.gemspec index dd2024bb35e..5364162c61e 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -1905,6 +1905,8 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/transport/call_filters.h ) s.files += %w( src/core/lib/transport/call_final_info.cc ) s.files += %w( src/core/lib/transport/call_final_info.h ) + s.files += %w( src/core/lib/transport/call_size_estimator.cc ) + s.files += %w( src/core/lib/transport/call_size_estimator.h ) s.files += %w( src/core/lib/transport/call_spine.cc ) s.files += %w( src/core/lib/transport/call_spine.h ) s.files += %w( src/core/lib/transport/connectivity_state.cc ) diff --git a/grpc.gyp b/grpc.gyp index 6a86620aeee..dde4c4b0bc8 100644 --- a/grpc.gyp +++ b/grpc.gyp @@ -1024,6 +1024,7 @@ 'src/core/lib/transport/bdp_estimator.cc', 'src/core/lib/transport/call_filters.cc', 'src/core/lib/transport/call_final_info.cc', + 'src/core/lib/transport/call_size_estimator.cc', 'src/core/lib/transport/call_spine.cc', 'src/core/lib/transport/connectivity_state.cc', 'src/core/lib/transport/error_utils.cc', @@ -1514,6 +1515,7 @@ 'src/core/lib/transport/bdp_estimator.cc', 'src/core/lib/transport/call_filters.cc', 'src/core/lib/transport/call_final_info.cc', + 'src/core/lib/transport/call_size_estimator.cc', 'src/core/lib/transport/call_spine.cc', 'src/core/lib/transport/connectivity_state.cc', 'src/core/lib/transport/error_utils.cc', @@ -2284,6 +2286,7 @@ 'src/core/lib/transport/batch_builder.cc', 'src/core/lib/transport/call_filters.cc', 'src/core/lib/transport/call_final_info.cc', + 'src/core/lib/transport/call_size_estimator.cc', 'src/core/lib/transport/call_spine.cc', 'src/core/lib/transport/connectivity_state.cc', 'src/core/lib/transport/error_utils.cc', diff --git a/package.xml b/package.xml index 75bfae51bd7..781e4ddd7e8 100644 --- a/package.xml +++ b/package.xml @@ -1887,6 +1887,8 @@ + + diff --git a/src/core/BUILD b/src/core/BUILD index 5ff1b119925..127c4110910 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -6868,6 +6868,17 @@ grpc_cc_library( ], ) +grpc_cc_library( + name = "call_size_estimator", + srcs = [ + "lib/transport/call_size_estimator.cc", + ], + hdrs = [ + "lib/transport/call_size_estimator.h", + ], + deps = ["//:gpr_platform"], +) + grpc_cc_library( name = "compression_internal", srcs = [ diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index 17d36b50e9a..f43b19a19c0 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -71,8 +71,8 @@ Channel::Channel(bool is_client, bool is_promising, std::string target, : is_client_(is_client), is_promising_(is_promising), compression_options_(compression_options), - call_size_estimate_(channel_stack->call_stack_size + - grpc_call_get_initial_size_estimate()), + call_size_estimator_(channel_stack->call_stack_size + + grpc_call_get_initial_size_estimate()), channelz_node_(channel_args.GetObjectRef()), allocator_(channel_args.GetObject() ->memory_quota() @@ -230,24 +230,6 @@ absl::StatusOr> Channel::Create( return CreateWithBuilder(&builder); } -void Channel::UpdateCallSizeEstimate(size_t size) { - size_t cur = call_size_estimate_.load(std::memory_order_relaxed); - if (cur < size) { - // size grew: update estimate - call_size_estimate_.compare_exchange_weak( - cur, size, std::memory_order_relaxed, std::memory_order_relaxed); - // if we lose: never mind, something else will likely update soon enough - } else if (cur == size) { - // no change: holding pattern - } else if (cur > 0) { - // size shrank: decrease estimate - call_size_estimate_.compare_exchange_weak( - cur, std::min(cur - 1, (255 * cur + size) / 256), - std::memory_order_relaxed, std::memory_order_relaxed); - // if we lose: never mind, something else will likely update soon enough - } -} - } // namespace grpc_core char* grpc_channel_get_target(grpc_channel* channel) { diff --git a/src/core/lib/surface/channel.h b/src/core/lib/surface/channel.h index f6e4f7ed932..502eceb3feb 100644 --- a/src/core/lib/surface/channel.h +++ b/src/core/lib/surface/channel.h @@ -55,6 +55,7 @@ #include "src/core/lib/resource_quota/memory_quota.h" #include "src/core/lib/slice/slice.h" #include "src/core/lib/surface/channel_stack_type.h" +#include "src/core/lib/transport/call_size_estimator.h" #include "src/core/lib/transport/transport.h" /// The same as grpc_channel_destroy, but doesn't create an ExecCtx, and so @@ -80,9 +81,6 @@ grpc_channel_stack* grpc_channel_get_channel_stack(grpc_channel* channel); grpc_core::channelz::ChannelNode* grpc_channel_get_channelz_node( grpc_channel* channel); -size_t grpc_channel_get_call_size_estimate(grpc_channel* channel); -void grpc_channel_update_call_size_estimate(grpc_channel* channel, size_t size); - namespace grpc_core { struct RegisteredCall { @@ -124,20 +122,11 @@ class Channel : public RefCounted, channelz::ChannelNode* channelz_node() const { return channelz_node_.get(); } - size_t CallSizeEstimate() { - // We round up our current estimate to the NEXT value of kRoundUpSize. - // This ensures: - // 1. a consistent size allocation when our estimate is drifting slowly - // (which is common) - which tends to help most allocators reuse memory - // 2. a small amount of allowed growth over the estimate without hitting - // the arena size doubling case, reducing overall memory usage - static constexpr size_t kRoundUpSize = 256; - return (call_size_estimate_.load(std::memory_order_relaxed) + - 2 * kRoundUpSize) & - ~(kRoundUpSize - 1); + size_t CallSizeEstimate() { return call_size_estimator_.CallSizeEstimate(); } + void UpdateCallSizeEstimate(size_t size) { + call_size_estimator_.UpdateCallSizeEstimate(size); } - void UpdateCallSizeEstimate(size_t size); absl::string_view target() const { return target_; } MemoryAllocator* allocator() { return &allocator_; } bool is_client() const { return is_client_; } @@ -162,7 +151,7 @@ class Channel : public RefCounted, const bool is_client_; const bool is_promising_; const grpc_compression_options compression_options_; - std::atomic call_size_estimate_; + CallSizeEstimator call_size_estimator_; CallRegistrationTable registration_table_; RefCountedPtr channelz_node_; MemoryAllocator allocator_; diff --git a/src/core/lib/transport/call_size_estimator.cc b/src/core/lib/transport/call_size_estimator.cc new file mode 100644 index 00000000000..88fb02528a3 --- /dev/null +++ b/src/core/lib/transport/call_size_estimator.cc @@ -0,0 +1,41 @@ +// Copyright 2024 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. + +#include + +#include "src/core/lib/transport/call_size_estimator.h" + +#include + +namespace grpc_core { + +void CallSizeEstimator::UpdateCallSizeEstimate(size_t size) { + size_t cur = call_size_estimate_.load(std::memory_order_relaxed); + if (cur < size) { + // size grew: update estimate + call_size_estimate_.compare_exchange_weak( + cur, size, std::memory_order_relaxed, std::memory_order_relaxed); + // if we lose: never mind, something else will likely update soon enough + } else if (cur == size) { + // no change: holding pattern + } else if (cur > 0) { + // size shrank: decrease estimate + call_size_estimate_.compare_exchange_weak( + cur, std::min(cur - 1, (255 * cur + size) / 256), + std::memory_order_relaxed, std::memory_order_relaxed); + // if we lose: never mind, something else will likely update soon enough + } +} + +} // namespace grpc_core diff --git a/src/core/lib/transport/call_size_estimator.h b/src/core/lib/transport/call_size_estimator.h new file mode 100644 index 00000000000..ffffcca531e --- /dev/null +++ b/src/core/lib/transport/call_size_estimator.h @@ -0,0 +1,52 @@ +// Copyright 2024 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. + +#ifndef GRPC_SRC_CORE_LIB_TRANSPORT_CALL_SIZE_ESTIMATOR_H +#define GRPC_SRC_CORE_LIB_TRANSPORT_CALL_SIZE_ESTIMATOR_H + +#include + +#include + +#include + +namespace grpc_core { + +class CallSizeEstimator { + public: + explicit CallSizeEstimator(size_t initial_estimate) + : call_size_estimate_(initial_estimate) {} + + size_t CallSizeEstimate() { + // We round up our current estimate to the NEXT value of kRoundUpSize. + // This ensures: + // 1. a consistent size allocation when our estimate is drifting slowly + // (which is common) - which tends to help most allocators reuse memory + // 2. a small amount of allowed growth over the estimate without hitting + // the arena size doubling case, reducing overall memory usage + static constexpr size_t kRoundUpSize = 256; + return (call_size_estimate_.load(std::memory_order_relaxed) + + 2 * kRoundUpSize) & + ~(kRoundUpSize - 1); + } + + void UpdateCallSizeEstimate(size_t size); + + private: + std::atomic call_size_estimate_; +}; + +} // namespace grpc_core + +#endif // GRPC_SRC_CORE_LIB_TRANSPORT_CALL_SIZE_ESTIMATOR_H diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py index 7c4a43be9ca..562bc38c523 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -812,6 +812,7 @@ CORE_SOURCE_FILES = [ 'src/core/lib/transport/bdp_estimator.cc', 'src/core/lib/transport/call_filters.cc', 'src/core/lib/transport/call_final_info.cc', + 'src/core/lib/transport/call_size_estimator.cc', 'src/core/lib/transport/call_spine.cc', 'src/core/lib/transport/connectivity_state.cc', 'src/core/lib/transport/error_utils.cc', diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index 320cfe8c255..13d4cb43767 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -2904,6 +2904,8 @@ src/core/lib/transport/call_filters.cc \ src/core/lib/transport/call_filters.h \ src/core/lib/transport/call_final_info.cc \ src/core/lib/transport/call_final_info.h \ +src/core/lib/transport/call_size_estimator.cc \ +src/core/lib/transport/call_size_estimator.h \ src/core/lib/transport/call_spine.cc \ src/core/lib/transport/call_spine.h \ src/core/lib/transport/connectivity_state.cc \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index 4cc0d41398b..7433bc02717 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -2685,6 +2685,8 @@ src/core/lib/transport/call_filters.cc \ src/core/lib/transport/call_filters.h \ src/core/lib/transport/call_final_info.cc \ src/core/lib/transport/call_final_info.h \ +src/core/lib/transport/call_size_estimator.cc \ +src/core/lib/transport/call_size_estimator.h \ src/core/lib/transport/call_spine.cc \ src/core/lib/transport/call_spine.h \ src/core/lib/transport/connectivity_state.cc \