From c0e2fa60f9b9b389d5d52eef6c991c9c846ca486 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 21 Aug 2023 14:23:02 -0700 Subject: [PATCH] [xDS] avoid creating duplicate hierarchical path attribute values (#34106) --- BUILD | 1 + CMakeLists.txt | 5 + Makefile | 2 + Package.swift | 2 + build_autogenerated.yaml | 10 ++ 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 | 20 +++ .../lb_policy/address_filtering.cc | 15 +- .../lb_policy/address_filtering.h | 12 +- .../lb_policy/priority/priority.cc | 7 +- .../weighted_target/weighted_target.cc | 7 +- .../lb_policy/xds/xds_cluster_resolver.cc | 11 +- src/core/lib/channel/channel_args.cc | 64 ++++---- src/core/lib/channel/channel_args.h | 96 +----------- .../lib/compression/compression_internal.cc | 1 + src/core/lib/gprpp/ref_counted_string.cc | 44 ++++++ src/core/lib/gprpp/ref_counted_string.h | 146 ++++++++++++++++++ src/python/grpcio/grpc_core_dependencies.py | 1 + tools/doxygen/Doxyfile.c++.internal | 2 + tools/doxygen/Doxyfile.core.internal | 2 + 26 files changed, 319 insertions(+), 143 deletions(-) create mode 100644 src/core/lib/gprpp/ref_counted_string.cc create mode 100644 src/core/lib/gprpp/ref_counted_string.h diff --git a/BUILD b/BUILD index 99d1be772b7..71485338ba2 100644 --- a/BUILD +++ b/BUILD @@ -1585,6 +1585,7 @@ grpc_cc_library( "//src/core:promise_status", "//src/core:race", "//src/core:ref_counted", + "//src/core:ref_counted_string", "//src/core:resolved_address", "//src/core:resource_quota", "//src/core:resource_quota_trace", diff --git a/CMakeLists.txt b/CMakeLists.txt index 5ba2aee3f17..e73ed867506 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -2196,6 +2196,7 @@ add_library(grpc src/core/lib/experiments/experiments.cc src/core/lib/gprpp/load_file.cc src/core/lib/gprpp/per_cpu.cc + src/core/lib/gprpp/ref_counted_string.cc src/core/lib/gprpp/status_helper.cc src/core/lib/gprpp/time.cc src/core/lib/gprpp/time_averaged_stats.cc @@ -2901,6 +2902,7 @@ add_library(grpc_unsecure src/core/lib/experiments/experiments.cc src/core/lib/gprpp/load_file.cc src/core/lib/gprpp/per_cpu.cc + src/core/lib/gprpp/ref_counted_string.cc src/core/lib/gprpp/status_helper.cc src/core/lib/gprpp/time.cc src/core/lib/gprpp/time_averaged_stats.cc @@ -4798,6 +4800,7 @@ add_library(grpc_authorization_provider src/core/lib/experiments/experiments.cc src/core/lib/gprpp/load_file.cc src/core/lib/gprpp/per_cpu.cc + src/core/lib/gprpp/ref_counted_string.cc src/core/lib/gprpp/status_helper.cc src/core/lib/gprpp/time.cc src/core/lib/gprpp/time_averaged_stats.cc @@ -10510,6 +10513,7 @@ if(gRPC_BUILD_TESTS) add_executable(endpoint_config_test src/core/lib/channel/channel_args.cc src/core/lib/event_engine/channel_args_endpoint_config.cc + src/core/lib/gprpp/ref_counted_string.cc src/core/lib/gprpp/time.cc src/core/lib/surface/channel_stack_type.cc test/core/event_engine/endpoint_config_test.cc @@ -12091,6 +12095,7 @@ add_executable(frame_test src/core/lib/experiments/experiments.cc src/core/lib/gprpp/load_file.cc src/core/lib/gprpp/per_cpu.cc + src/core/lib/gprpp/ref_counted_string.cc src/core/lib/gprpp/status_helper.cc src/core/lib/gprpp/time.cc src/core/lib/gprpp/time_averaged_stats.cc diff --git a/Makefile b/Makefile index c7f498c57de..d0d49b15a14 100644 --- a/Makefile +++ b/Makefile @@ -1479,6 +1479,7 @@ LIBGRPC_SRC = \ src/core/lib/experiments/experiments.cc \ src/core/lib/gprpp/load_file.cc \ src/core/lib/gprpp/per_cpu.cc \ + src/core/lib/gprpp/ref_counted_string.cc \ src/core/lib/gprpp/status_helper.cc \ src/core/lib/gprpp/time.cc \ src/core/lib/gprpp/time_averaged_stats.cc \ @@ -2045,6 +2046,7 @@ LIBGRPC_UNSECURE_SRC = \ src/core/lib/experiments/experiments.cc \ src/core/lib/gprpp/load_file.cc \ src/core/lib/gprpp/per_cpu.cc \ + src/core/lib/gprpp/ref_counted_string.cc \ src/core/lib/gprpp/status_helper.cc \ src/core/lib/gprpp/time.cc \ src/core/lib/gprpp/time_averaged_stats.cc \ diff --git a/Package.swift b/Package.swift index 17c4cedb05b..8ab107515be 100644 --- a/Package.swift +++ b/Package.swift @@ -1239,6 +1239,8 @@ let package = Package( "src/core/lib/gprpp/posix/thd.cc", "src/core/lib/gprpp/ref_counted.h", "src/core/lib/gprpp/ref_counted_ptr.h", + "src/core/lib/gprpp/ref_counted_string.cc", + "src/core/lib/gprpp/ref_counted_string.h", "src/core/lib/gprpp/single_set_ptr.h", "src/core/lib/gprpp/sorted_pack.h", "src/core/lib/gprpp/stat.h", diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 7e640d1fc07..73c018b8fcd 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -756,6 +756,7 @@ libs: - src/core/lib/gprpp/per_cpu.h - src/core/lib/gprpp/ref_counted.h - src/core/lib/gprpp/ref_counted_ptr.h + - src/core/lib/gprpp/ref_counted_string.h - src/core/lib/gprpp/single_set_ptr.h - src/core/lib/gprpp/sorted_pack.h - src/core/lib/gprpp/status_helper.h @@ -1550,6 +1551,7 @@ libs: - src/core/lib/experiments/experiments.cc - src/core/lib/gprpp/load_file.cc - src/core/lib/gprpp/per_cpu.cc + - src/core/lib/gprpp/ref_counted_string.cc - src/core/lib/gprpp/status_helper.cc - src/core/lib/gprpp/time.cc - src/core/lib/gprpp/time_averaged_stats.cc @@ -2161,6 +2163,7 @@ libs: - src/core/lib/gprpp/per_cpu.h - src/core/lib/gprpp/ref_counted.h - src/core/lib/gprpp/ref_counted_ptr.h + - src/core/lib/gprpp/ref_counted_string.h - src/core/lib/gprpp/single_set_ptr.h - src/core/lib/gprpp/sorted_pack.h - src/core/lib/gprpp/status_helper.h @@ -2562,6 +2565,7 @@ libs: - src/core/lib/experiments/experiments.cc - src/core/lib/gprpp/load_file.cc - src/core/lib/gprpp/per_cpu.cc + - src/core/lib/gprpp/ref_counted_string.cc - src/core/lib/gprpp/status_helper.cc - src/core/lib/gprpp/time.cc - src/core/lib/gprpp/time_averaged_stats.cc @@ -4125,6 +4129,7 @@ libs: - src/core/lib/gprpp/per_cpu.h - src/core/lib/gprpp/ref_counted.h - src/core/lib/gprpp/ref_counted_ptr.h + - src/core/lib/gprpp/ref_counted_string.h - src/core/lib/gprpp/sorted_pack.h - src/core/lib/gprpp/status_helper.h - src/core/lib/gprpp/table.h @@ -4406,6 +4411,7 @@ libs: - src/core/lib/experiments/experiments.cc - src/core/lib/gprpp/load_file.cc - src/core/lib/gprpp/per_cpu.cc + - src/core/lib/gprpp/ref_counted_string.cc - src/core/lib/gprpp/status_helper.cc - src/core/lib/gprpp/time.cc - src/core/lib/gprpp/time_averaged_stats.cc @@ -7644,11 +7650,13 @@ targets: - src/core/lib/gprpp/orphanable.h - src/core/lib/gprpp/ref_counted.h - src/core/lib/gprpp/ref_counted_ptr.h + - src/core/lib/gprpp/ref_counted_string.h - src/core/lib/gprpp/time.h - src/core/lib/surface/channel_stack_type.h src: - src/core/lib/channel/channel_args.cc - src/core/lib/event_engine/channel_args_endpoint_config.cc + - src/core/lib/gprpp/ref_counted_string.cc - src/core/lib/gprpp/time.cc - src/core/lib/surface/channel_stack_type.cc - test/core/event_engine/endpoint_config_test.cc @@ -8770,6 +8778,7 @@ targets: - src/core/lib/gprpp/per_cpu.h - src/core/lib/gprpp/ref_counted.h - src/core/lib/gprpp/ref_counted_ptr.h + - src/core/lib/gprpp/ref_counted_string.h - src/core/lib/gprpp/sorted_pack.h - src/core/lib/gprpp/status_helper.h - src/core/lib/gprpp/table.h @@ -9033,6 +9042,7 @@ targets: - src/core/lib/experiments/experiments.cc - src/core/lib/gprpp/load_file.cc - src/core/lib/gprpp/per_cpu.cc + - src/core/lib/gprpp/ref_counted_string.cc - src/core/lib/gprpp/status_helper.cc - src/core/lib/gprpp/time.cc - src/core/lib/gprpp/time_averaged_stats.cc diff --git a/config.m4 b/config.m4 index 52d064461ba..584c7fdab45 100644 --- a/config.m4 +++ b/config.m4 @@ -606,6 +606,7 @@ if test "$PHP_GRPC" != "no"; then src/core/lib/gprpp/posix/env.cc \ src/core/lib/gprpp/posix/stat.cc \ src/core/lib/gprpp/posix/thd.cc \ + src/core/lib/gprpp/ref_counted_string.cc \ src/core/lib/gprpp/status_helper.cc \ src/core/lib/gprpp/strerror.cc \ src/core/lib/gprpp/tchar.cc \ diff --git a/config.w32 b/config.w32 index 192db270f13..bc39c417066 100644 --- a/config.w32 +++ b/config.w32 @@ -571,6 +571,7 @@ if (PHP_GRPC != "no") { "src\\core\\lib\\gprpp\\posix\\env.cc " + "src\\core\\lib\\gprpp\\posix\\stat.cc " + "src\\core\\lib\\gprpp\\posix\\thd.cc " + + "src\\core\\lib\\gprpp\\ref_counted_string.cc " + "src\\core\\lib\\gprpp\\status_helper.cc " + "src\\core\\lib\\gprpp\\strerror.cc " + "src\\core\\lib\\gprpp\\tchar.cc " + diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index 74e72fcab62..f95f144545c 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -844,6 +844,7 @@ Pod::Spec.new do |s| 'src/core/lib/gprpp/per_cpu.h', 'src/core/lib/gprpp/ref_counted.h', 'src/core/lib/gprpp/ref_counted_ptr.h', + 'src/core/lib/gprpp/ref_counted_string.h', 'src/core/lib/gprpp/single_set_ptr.h', 'src/core/lib/gprpp/sorted_pack.h', 'src/core/lib/gprpp/stat.h', @@ -1900,6 +1901,7 @@ Pod::Spec.new do |s| 'src/core/lib/gprpp/per_cpu.h', 'src/core/lib/gprpp/ref_counted.h', 'src/core/lib/gprpp/ref_counted_ptr.h', + 'src/core/lib/gprpp/ref_counted_string.h', 'src/core/lib/gprpp/single_set_ptr.h', 'src/core/lib/gprpp/sorted_pack.h', 'src/core/lib/gprpp/stat.h', diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index 940658b9920..dcd7f2bda34 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -1340,6 +1340,8 @@ Pod::Spec.new do |s| 'src/core/lib/gprpp/posix/thd.cc', 'src/core/lib/gprpp/ref_counted.h', 'src/core/lib/gprpp/ref_counted_ptr.h', + 'src/core/lib/gprpp/ref_counted_string.cc', + 'src/core/lib/gprpp/ref_counted_string.h', 'src/core/lib/gprpp/single_set_ptr.h', 'src/core/lib/gprpp/sorted_pack.h', 'src/core/lib/gprpp/stat.h', @@ -2647,6 +2649,7 @@ Pod::Spec.new do |s| 'src/core/lib/gprpp/per_cpu.h', 'src/core/lib/gprpp/ref_counted.h', 'src/core/lib/gprpp/ref_counted_ptr.h', + 'src/core/lib/gprpp/ref_counted_string.h', 'src/core/lib/gprpp/single_set_ptr.h', 'src/core/lib/gprpp/sorted_pack.h', 'src/core/lib/gprpp/stat.h', diff --git a/grpc.gemspec b/grpc.gemspec index 4ffcc56765e..944dba89cee 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -1245,6 +1245,8 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/gprpp/posix/thd.cc ) s.files += %w( src/core/lib/gprpp/ref_counted.h ) s.files += %w( src/core/lib/gprpp/ref_counted_ptr.h ) + s.files += %w( src/core/lib/gprpp/ref_counted_string.cc ) + s.files += %w( src/core/lib/gprpp/ref_counted_string.h ) s.files += %w( src/core/lib/gprpp/single_set_ptr.h ) s.files += %w( src/core/lib/gprpp/sorted_pack.h ) s.files += %w( src/core/lib/gprpp/stat.h ) diff --git a/grpc.gyp b/grpc.gyp index 6f78dddb3b0..6b74971bec3 100644 --- a/grpc.gyp +++ b/grpc.gyp @@ -795,6 +795,7 @@ 'src/core/lib/experiments/experiments.cc', 'src/core/lib/gprpp/load_file.cc', 'src/core/lib/gprpp/per_cpu.cc', + 'src/core/lib/gprpp/ref_counted_string.cc', 'src/core/lib/gprpp/status_helper.cc', 'src/core/lib/gprpp/time.cc', 'src/core/lib/gprpp/time_averaged_stats.cc', @@ -1301,6 +1302,7 @@ 'src/core/lib/experiments/experiments.cc', 'src/core/lib/gprpp/load_file.cc', 'src/core/lib/gprpp/per_cpu.cc', + 'src/core/lib/gprpp/ref_counted_string.cc', 'src/core/lib/gprpp/status_helper.cc', 'src/core/lib/gprpp/time.cc', 'src/core/lib/gprpp/time_averaged_stats.cc', @@ -2027,6 +2029,7 @@ 'src/core/lib/experiments/experiments.cc', 'src/core/lib/gprpp/load_file.cc', 'src/core/lib/gprpp/per_cpu.cc', + 'src/core/lib/gprpp/ref_counted_string.cc', 'src/core/lib/gprpp/status_helper.cc', 'src/core/lib/gprpp/time.cc', 'src/core/lib/gprpp/time_averaged_stats.cc', diff --git a/package.xml b/package.xml index e96864b2a0d..d0788e58548 100644 --- a/package.xml +++ b/package.xml @@ -1227,6 +1227,8 @@ + + diff --git a/src/core/BUILD b/src/core/BUILD index cba9d591d70..44458014cc0 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -960,6 +960,23 @@ grpc_cc_library( ], ) +grpc_cc_library( + name = "ref_counted_string", + srcs = [ + "lib/gprpp/ref_counted_string.cc", + ], + hdrs = [ + "lib/gprpp/ref_counted_string.h", + ], + external_deps = ["absl/strings"], + language = "c++", + deps = [ + "ref_counted", + "//:gpr", + "//:ref_counted_ptr", + ], +) + grpc_cc_library( name = "uuid_v4", srcs = ["lib/gprpp/uuid_v4.cc"], @@ -2680,6 +2697,7 @@ grpc_cc_library( "channel_stack_type", "dual_ref_counted", "ref_counted", + "ref_counted_string", "time", "useful", "//:channel_arg_names", @@ -4499,6 +4517,7 @@ grpc_cc_library( "lb_policy_factory", "lb_policy_registry", "pollset_set", + "ref_counted_string", "validation_errors", "//:channel_arg_names", "//:config", @@ -4653,6 +4672,7 @@ grpc_cc_library( deps = [ "channel_args", "ref_counted", + "ref_counted_string", "//:gpr_platform", "//:ref_counted_ptr", "//:server_address", diff --git a/src/core/ext/filters/client_channel/lb_policy/address_filtering.cc b/src/core/ext/filters/client_channel/lb_policy/address_filtering.cc index 0ddaaf0f523..8dcc5f8d5c4 100644 --- a/src/core/ext/filters/client_channel/lb_policy/address_filtering.cc +++ b/src/core/ext/filters/client_channel/lb_policy/address_filtering.cc @@ -36,7 +36,7 @@ int HierarchicalPathArg::ChannelArgsCompare(const HierarchicalPathArg* a, const HierarchicalPathArg* b) { for (size_t i = 0; i < a->path_.size(); ++i) { if (b->path_.size() == i) return 1; - int r = a->path_[i].compare(b->path_[i]); + int r = a->path_[i].as_string_view().compare(b->path_[i].as_string_view()); if (r != 0) return r; } if (b->path_.size() > a->path_.size()) return -1; @@ -47,19 +47,24 @@ absl::StatusOr MakeHierarchicalAddressMap( const absl::StatusOr& addresses) { if (!addresses.ok()) return addresses.status(); HierarchicalAddressMap result; + RefCountedPtr remaining_path_attr; for (const ServerAddress& address : *addresses) { const auto* path_arg = address.args().GetObject(); if (path_arg == nullptr) continue; - const std::vector& path = path_arg->path(); + const std::vector& path = path_arg->path(); auto it = path.begin(); if (it == path.end()) continue; ServerAddressList& target_list = result[*it]; ChannelArgs args = address.args(); ++it; if (it != path.end()) { - std::vector remaining_path(it, path.end()); - args = args.SetObject( - MakeRefCounted(std::move(remaining_path))); + std::vector remaining_path(it, path.end()); + if (remaining_path_attr == nullptr || + remaining_path_attr->path() != remaining_path) { + remaining_path_attr = + MakeRefCounted(std::move(remaining_path)); + } + args = args.SetObject(remaining_path_attr); } target_list.emplace_back(address.address(), args); } diff --git a/src/core/ext/filters/client_channel/lb_policy/address_filtering.h b/src/core/ext/filters/client_channel/lb_policy/address_filtering.h index 2e4aa2017d4..b17c13bbb8c 100644 --- a/src/core/ext/filters/client_channel/lb_policy/address_filtering.h +++ b/src/core/ext/filters/client_channel/lb_policy/address_filtering.h @@ -20,7 +20,6 @@ #include #include -#include #include #include @@ -28,6 +27,7 @@ #include "absl/strings/string_view.h" #include "src/core/lib/gprpp/ref_counted.h" +#include "src/core/lib/gprpp/ref_counted_string.h" #include "src/core/lib/resolver/server_address.h" // The resolver returns a flat list of addresses. When a hierarchy of @@ -88,7 +88,7 @@ namespace grpc_core { // to be associated with the address. class HierarchicalPathArg : public RefCounted { public: - explicit HierarchicalPathArg(std::vector path) + explicit HierarchicalPathArg(std::vector path) : path_(std::move(path)) {} // Channel arg traits methods. @@ -96,15 +96,17 @@ class HierarchicalPathArg : public RefCounted { static int ChannelArgsCompare(const HierarchicalPathArg* a, const HierarchicalPathArg* b); - const std::vector& path() const { return path_; } + const std::vector& path() const { return path_; } private: - std::vector path_; + std::vector path_; }; // A map from the next path element to the addresses that fall under // that path element. -using HierarchicalAddressMap = std::map; +using HierarchicalAddressMap = + std::map; // Splits up the addresses into a separate list for each child. absl::StatusOr MakeHierarchicalAddressMap( diff --git a/src/core/ext/filters/client_channel/lb_policy/priority/priority.cc b/src/core/ext/filters/client_channel/lb_policy/priority/priority.cc index fe22f3b9263..716b5f41e88 100644 --- a/src/core/ext/filters/client_channel/lb_policy/priority/priority.cc +++ b/src/core/ext/filters/client_channel/lb_policy/priority/priority.cc @@ -681,7 +681,12 @@ absl::Status PriorityLb::ChildPriority::UpdateLocked( UpdateArgs update_args; update_args.config = std::move(config); if (priority_policy_->addresses_.ok()) { - update_args.addresses = (*priority_policy_->addresses_)[name_]; + auto it = priority_policy_->addresses_->find(name_); + if (it == priority_policy_->addresses_->end()) { + update_args.addresses.emplace(); + } else { + update_args.addresses = it->second; + } } else { update_args.addresses = priority_policy_->addresses_.status(); } diff --git a/src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc b/src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc index 334de35027e..b3ccbbf9e8f 100644 --- a/src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc +++ b/src/core/ext/filters/client_channel/lb_policy/weighted_target/weighted_target.cc @@ -339,7 +339,12 @@ absl::Status WeightedTargetLb::UpdateLocked(UpdateArgs args) { } absl::StatusOr addresses; if (address_map.ok()) { - addresses = std::move((*address_map)[name]); + auto it = address_map->find(name); + if (it == address_map->end()) { + addresses.emplace(); + } else { + addresses = std::move(it->second); + } } else { addresses = address_map.status(); } diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc index ca487ad5dc5..ca92241b84d 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc @@ -56,6 +56,7 @@ #include "src/core/lib/gprpp/debug_location.h" #include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" +#include "src/core/lib/gprpp/ref_counted_string.h" #include "src/core/lib/gprpp/validation_errors.h" #include "src/core/lib/gprpp/work_serializer.h" #include "src/core/lib/iomgr/pollset_set.h" @@ -764,8 +765,11 @@ ServerAddressList XdsClusterResolverLb::CreateChildPolicyAddressesLocked() { for (const auto& p : priority_entry.localities) { const auto& locality_name = p.first; const auto& locality = p.second; - std::vector hierarchical_path = { - priority_child_name, locality_name->AsHumanReadableString()}; + std::vector hierarchical_path = { + RefCountedStringValue(priority_child_name), + RefCountedStringValue(locality_name->AsHumanReadableString())}; + auto hierarchical_path_attr = + MakeRefCounted(std::move(hierarchical_path)); for (const auto& endpoint : locality.endpoints) { uint32_t endpoint_weight = locality.lb_weight * @@ -773,8 +777,7 @@ ServerAddressList XdsClusterResolverLb::CreateChildPolicyAddressesLocked() { addresses.emplace_back( endpoint.address(), endpoint.args() - .SetObject( - MakeRefCounted(hierarchical_path)) + .SetObject(hierarchical_path_attr) .Set(GRPC_ARG_ADDRESS_WEIGHT, endpoint_weight) .SetObject(locality_name->Ref()) .Set(GRPC_ARG_XDS_LOCALITY_WEIGHT, locality.lb_weight)); diff --git a/src/core/lib/channel/channel_args.cc b/src/core/lib/channel/channel_args.cc index 4496abad4b9..3d8c04c3456 100644 --- a/src/core/lib/channel/channel_args.cc +++ b/src/core/lib/channel/channel_args.cc @@ -28,7 +28,6 @@ #include #include #include -#include #include #include @@ -46,20 +45,6 @@ namespace grpc_core { -RefCountedPtr RcString::Make(absl::string_view src) { - void* p = gpr_malloc(sizeof(Header) + src.length() + 1); - return RefCountedPtr(new (p) RcString(src)); -} - -RcString::RcString(absl::string_view src) : header_{{}, src.length()} { - memcpy(payload_, src.data(), header_.length); - // Null terminate because we frequently need to convert to char* still to go - // back and forth to the old c-style api. - payload_[header_.length] = 0; -} - -void RcString::Destroy() { gpr_free(this); } - const grpc_arg_pointer_vtable ChannelArgs::Value::int_vtable_{ // copy [](void* p) { return p; }, @@ -74,13 +59,15 @@ const grpc_arg_pointer_vtable ChannelArgs::Value::int_vtable_{ const grpc_arg_pointer_vtable ChannelArgs::Value::string_vtable_{ // copy - [](void* p) -> void* { return static_cast(p)->Ref().release(); }, + [](void* p) -> void* { + return static_cast(p)->Ref().release(); + }, // destroy - [](void* p) { static_cast(p)->Unref(); }, + [](void* p) { static_cast(p)->Unref(); }, // cmp [](void* p1, void* p2) -> int { - return QsortCompare(static_cast(p1)->as_string_view(), - static_cast(p2)->as_string_view()); + return QsortCompare(static_cast(p1)->as_string_view(), + static_cast(p2)->as_string_view()); }, }; @@ -139,7 +126,7 @@ bool ChannelArgs::WantMinimalStack() const { return GetBool(GRPC_ARG_MINIMAL_STACK).value_or(false); } -ChannelArgs::ChannelArgs(AVL args) +ChannelArgs::ChannelArgs(AVL args) : args_(std::move(args)) {} ChannelArgs ChannelArgs::Set(grpc_arg arg) const { @@ -175,8 +162,8 @@ grpc_arg ChannelArgs::Value::MakeCArg(const char* name) const { } if (rep_.c_vtable() == &string_vtable_) { return grpc_channel_arg_string_create( - c_name, - const_cast(static_cast(rep_.c_pointer())->c_str())); + c_name, const_cast( + static_cast(rep_.c_pointer())->c_str())); } return grpc_channel_arg_pointer_create(c_name, rep_.c_pointer(), rep_.c_vtable()); @@ -184,9 +171,10 @@ grpc_arg ChannelArgs::Value::MakeCArg(const char* name) const { ChannelArgs::CPtr ChannelArgs::ToC() const { std::vector c_args; - args_.ForEach([&c_args](const RcStringValue& key, const Value& value) { - c_args.push_back(value.MakeCArg(key.c_str())); - }); + args_.ForEach( + [&c_args](const RefCountedStringValue& key, const Value& value) { + c_args.push_back(value.MakeCArg(key.c_str())); + }); return CPtr(static_cast( grpc_channel_args_copy_and_add(nullptr, c_args.data(), c_args.size()))); } @@ -203,7 +191,7 @@ ChannelArgs ChannelArgs::Set(absl::string_view name, Value value) const { if (const auto* p = args_.Lookup(name)) { if (*p == value) return *this; // already have this value for this key } - return ChannelArgs(args_.Add(RcStringValue(name), std::move(value))); + return ChannelArgs(args_.Add(RefCountedStringValue(name), std::move(value))); } ChannelArgs ChannelArgs::Set(absl::string_view name, @@ -227,7 +215,7 @@ ChannelArgs ChannelArgs::Remove(absl::string_view name) const { ChannelArgs ChannelArgs::RemoveAllKeysWithPrefix( absl::string_view prefix) const { auto args = args_; - args_.ForEach([&](const RcStringValue& key, const Value&) { + args_.ForEach([&](const RefCountedStringValue& key, const Value&) { if (absl::StartsWith(key.as_string_view(), prefix)) args = args.Remove(key); }); return ChannelArgs(std::move(args)); @@ -299,17 +287,18 @@ std::string ChannelArgs::Value::ToString() const { } if (rep_.c_vtable() == &string_vtable_) { return std::string( - static_cast(rep_.c_pointer())->as_string_view()); + static_cast(rep_.c_pointer())->as_string_view()); } return absl::StrFormat("%p", rep_.c_pointer()); } std::string ChannelArgs::ToString() const { std::vector arg_strings; - args_.ForEach([&arg_strings](const RcStringValue& key, const Value& value) { - arg_strings.push_back( - absl::StrCat(key.as_string_view(), "=", value.ToString())); - }); + args_.ForEach( + [&arg_strings](const RefCountedStringValue& key, const Value& value) { + arg_strings.push_back( + absl::StrCat(key.as_string_view(), "=", value.ToString())); + }); return absl::StrCat("{", absl::StrJoin(arg_strings, ", "), "}"); } @@ -317,14 +306,15 @@ ChannelArgs ChannelArgs::UnionWith(ChannelArgs other) const { if (args_.Empty()) return other; if (other.args_.Empty()) return *this; if (args_.Height() <= other.args_.Height()) { - args_.ForEach([&other](const RcStringValue& key, const Value& value) { - other.args_ = other.args_.Add(key, value); - }); + args_.ForEach( + [&other](const RefCountedStringValue& key, const Value& value) { + other.args_ = other.args_.Add(key, value); + }); return other; } else { auto result = *this; other.args_.ForEach( - [&result](const RcStringValue& key, const Value& value) { + [&result](const RefCountedStringValue& key, const Value& value) { if (result.args_.Lookup(key) == nullptr) { result.args_ = result.args_.Add(key, value); } @@ -335,7 +325,7 @@ ChannelArgs ChannelArgs::UnionWith(ChannelArgs other) const { ChannelArgs ChannelArgs::FuzzingReferenceUnionWith(ChannelArgs other) const { // DO NOT OPTIMIZE THIS!! - args_.ForEach([&other](const RcStringValue& key, const Value& value) { + args_.ForEach([&other](const RefCountedStringValue& key, const Value& value) { other.args_ = other.args_.Add(key, value); }); return other; diff --git a/src/core/lib/channel/channel_args.h b/src/core/lib/channel/channel_args.h index 676ca6066f8..38bb070213c 100644 --- a/src/core/lib/channel/channel_args.h +++ b/src/core/lib/channel/channel_args.h @@ -44,6 +44,7 @@ #include "src/core/lib/gprpp/dual_ref_counted.h" #include "src/core/lib/gprpp/ref_counted.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" +#include "src/core/lib/gprpp/ref_counted_string.h" #include "src/core/lib/gprpp/time.h" #include "src/core/lib/surface/channel_stack_type.h" @@ -220,91 +221,6 @@ struct GetObjectImpl::value, void>> { }; }; -// Immutable reference counted string -class RcString { - public: - static RefCountedPtr Make(absl::string_view src); - - RefCountedPtr Ref() { - IncrementRefCount(); - return RefCountedPtr(this); - } - void IncrementRefCount() { header_.rc.Ref(); } - void Unref() { - if (header_.rc.Unref()) Destroy(); - } - - absl::string_view as_string_view() const { - return absl::string_view(payload_, header_.length); - } - - char* c_str() { return payload_; } - - private: - explicit RcString(absl::string_view src); - void Destroy(); - - struct Header { - RefCount rc; - size_t length; - }; - Header header_; - char payload_[]; -}; - -// Wrapper around RefCountedPtr to give value semantics, especially to -// overloaded operators. -class RcStringValue { - public: - RcStringValue() : str_{} {} - explicit RcStringValue(absl::string_view str) : str_(RcString::Make(str)) {} - - absl::string_view as_string_view() const { - return str_ == nullptr ? absl::string_view() : str_->as_string_view(); - } - - const char* c_str() const { return str_ == nullptr ? "" : str_->c_str(); } - - private: - RefCountedPtr str_; -}; - -inline bool operator==(const RcStringValue& lhs, absl::string_view rhs) { - return lhs.as_string_view() == rhs; -} - -inline bool operator==(absl::string_view lhs, const RcStringValue& rhs) { - return lhs == rhs.as_string_view(); -} - -inline bool operator==(const RcStringValue& lhs, const RcStringValue& rhs) { - return lhs.as_string_view() == rhs.as_string_view(); -} - -inline bool operator<(const RcStringValue& lhs, absl::string_view rhs) { - return lhs.as_string_view() < rhs; -} - -inline bool operator<(absl::string_view lhs, const RcStringValue& rhs) { - return lhs < rhs.as_string_view(); -} - -inline bool operator<(const RcStringValue& lhs, const RcStringValue& rhs) { - return lhs.as_string_view() < rhs.as_string_view(); -} - -inline bool operator>(const RcStringValue& lhs, absl::string_view rhs) { - return lhs.as_string_view() > rhs; -} - -inline bool operator>(absl::string_view lhs, const RcStringValue& rhs) { - return lhs > rhs.as_string_view(); -} - -inline bool operator>(const RcStringValue& lhs, const RcStringValue& rhs) { - return lhs.as_string_view() > rhs.as_string_view(); -} - // Provide the canonical name for a type's channel arg key template struct ChannelArgNameTraits { @@ -370,16 +286,16 @@ class ChannelArgs { public: explicit Value(int n) : rep_(reinterpret_cast(n), &int_vtable_) {} explicit Value(std::string s) - : rep_(RcString::Make(s).release(), &string_vtable_) {} + : rep_(RefCountedString::Make(s).release(), &string_vtable_) {} explicit Value(Pointer p) : rep_(std::move(p)) {} absl::optional GetIfInt() const { if (rep_.c_vtable() != &int_vtable_) return absl::nullopt; return reinterpret_cast(rep_.c_pointer()); } - RefCountedPtr GetIfString() const { + RefCountedPtr GetIfString() const { if (rep_.c_vtable() != &string_vtable_) return nullptr; - return static_cast(rep_.c_pointer())->Ref(); + return static_cast(rep_.c_pointer())->Ref(); } const Pointer* GetIfPointer() const { if (rep_.c_vtable() == &int_vtable_) return nullptr; @@ -558,12 +474,12 @@ class ChannelArgs { std::string ToString() const; private: - explicit ChannelArgs(AVL args); + explicit ChannelArgs(AVL args); GRPC_MUST_USE_RESULT ChannelArgs Set(absl::string_view name, Value value) const; - AVL args_; + AVL args_; }; std::ostream& operator<<(std::ostream& out, const ChannelArgs& args); diff --git a/src/core/lib/compression/compression_internal.cc b/src/core/lib/compression/compression_internal.cc index 793e8969d87..6b18915b668 100644 --- a/src/core/lib/compression/compression_internal.cc +++ b/src/core/lib/compression/compression_internal.cc @@ -33,6 +33,7 @@ #include "src/core/lib/debug/trace.h" #include "src/core/lib/gprpp/crash.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" +#include "src/core/lib/gprpp/ref_counted_string.h" #include "src/core/lib/surface/api_trace.h" namespace grpc_core { diff --git a/src/core/lib/gprpp/ref_counted_string.cc b/src/core/lib/gprpp/ref_counted_string.cc new file mode 100644 index 00000000000..c5a3d4d6f67 --- /dev/null +++ b/src/core/lib/gprpp/ref_counted_string.cc @@ -0,0 +1,44 @@ +// +// Copyright 2023 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/gprpp/ref_counted_string.h" + +#include + +#include + +#include + +namespace grpc_core { + +RefCountedPtr RefCountedString::Make(absl::string_view src) { + void* p = gpr_malloc(sizeof(Header) + src.length() + 1); + return RefCountedPtr(new (p) RefCountedString(src)); +} + +RefCountedString::RefCountedString(absl::string_view src) + : header_{{}, src.length()} { + memcpy(payload_, src.data(), header_.length); + // Null terminate because we frequently need to convert to char* still to go + // back and forth to the old c-style api. + payload_[header_.length] = 0; +} + +void RefCountedString::Destroy() { gpr_free(this); } + +} // namespace grpc_core diff --git a/src/core/lib/gprpp/ref_counted_string.h b/src/core/lib/gprpp/ref_counted_string.h new file mode 100644 index 00000000000..8bcdb87eea9 --- /dev/null +++ b/src/core/lib/gprpp/ref_counted_string.h @@ -0,0 +1,146 @@ +// +// Copyright 2023 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_GPRPP_REF_COUNTED_STRING_H +#define GRPC_SRC_CORE_LIB_GPRPP_REF_COUNTED_STRING_H + +#include + +#include + +#include "absl/strings/string_view.h" + +#include "src/core/lib/gprpp/ref_counted.h" +#include "src/core/lib/gprpp/ref_counted_ptr.h" + +namespace grpc_core { + +// An immutable reference counted string. +class RefCountedString { + public: + static RefCountedPtr Make(absl::string_view src); + + // Not copyable. + RefCountedString(const RefCountedString&) = delete; + RefCountedString& operator=(const RefCountedString&) = delete; + + // Provide the same interface as RefCounted<>. + // We reimplement this instead of inheritting to make pointer math + // easier in Make(). + RefCountedPtr Ref() { + IncrementRefCount(); + return RefCountedPtr(this); + } + void Unref() { + if (header_.rc.Unref()) Destroy(); + } + + absl::string_view as_string_view() const { + return absl::string_view(payload_, header_.length); + } + + char* c_str() { return payload_; } + + private: + // Allow RefCountedPtr<> to access IncrementRefCount(). + template + friend class RefCountedPtr; + + explicit RefCountedString(absl::string_view src); + void IncrementRefCount() { header_.rc.Ref(); } + void Destroy(); + + struct Header { + RefCount rc; + size_t length; + }; + Header header_; + char payload_[]; +}; + +// Wrapper around RefCountedPtr to give value semantics, +// especially to overloaded operators. +class RefCountedStringValue { + public: + RefCountedStringValue() : str_{} {} + explicit RefCountedStringValue(absl::string_view str) + : str_(RefCountedString::Make(str)) {} + + absl::string_view as_string_view() const { + return str_ == nullptr ? absl::string_view() : str_->as_string_view(); + } + + const char* c_str() const { return str_ == nullptr ? "" : str_->c_str(); } + + private: + RefCountedPtr str_; +}; + +inline bool operator==(const RefCountedStringValue& lhs, + absl::string_view rhs) { + return lhs.as_string_view() == rhs; +} +inline bool operator==(absl::string_view lhs, + const RefCountedStringValue& rhs) { + return lhs == rhs.as_string_view(); +} +inline bool operator==(const RefCountedStringValue& lhs, + const RefCountedStringValue& rhs) { + return lhs.as_string_view() == rhs.as_string_view(); +} + +inline bool operator<(const RefCountedStringValue& lhs, absl::string_view rhs) { + return lhs.as_string_view() < rhs; +} +inline bool operator<(absl::string_view lhs, const RefCountedStringValue& rhs) { + return lhs < rhs.as_string_view(); +} +inline bool operator<(const RefCountedStringValue& lhs, + const RefCountedStringValue& rhs) { + return lhs.as_string_view() < rhs.as_string_view(); +} + +inline bool operator>(const RefCountedStringValue& lhs, absl::string_view rhs) { + return lhs.as_string_view() > rhs; +} +inline bool operator>(absl::string_view lhs, const RefCountedStringValue& rhs) { + return lhs > rhs.as_string_view(); +} +inline bool operator>(const RefCountedStringValue& lhs, + const RefCountedStringValue& rhs) { + return lhs.as_string_view() > rhs.as_string_view(); +} + +// A sorting functor to support heterogeneous lookups in sorted containers. +struct RefCountedStringValueLessThan { + using is_transparent = void; + bool operator()(const RefCountedStringValue& lhs, + const RefCountedStringValue& rhs) const { + return lhs < rhs; + } + bool operator()(absl::string_view lhs, + const RefCountedStringValue& rhs) const { + return lhs < rhs; + } + bool operator()(const RefCountedStringValue& lhs, + absl::string_view rhs) const { + return lhs < rhs; + } +}; + +} // namespace grpc_core + +#endif // GRPC_SRC_CORE_LIB_GPRPP_REF_COUNTED_STRING_H diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py index 935f005225e..f69c0e01f1e 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -580,6 +580,7 @@ CORE_SOURCE_FILES = [ 'src/core/lib/gprpp/posix/env.cc', 'src/core/lib/gprpp/posix/stat.cc', 'src/core/lib/gprpp/posix/thd.cc', + 'src/core/lib/gprpp/ref_counted_string.cc', 'src/core/lib/gprpp/status_helper.cc', 'src/core/lib/gprpp/strerror.cc', 'src/core/lib/gprpp/tchar.cc', diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index e08e77e05a4..4a54b5bf9a4 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -2242,6 +2242,8 @@ src/core/lib/gprpp/posix/stat.cc \ src/core/lib/gprpp/posix/thd.cc \ src/core/lib/gprpp/ref_counted.h \ src/core/lib/gprpp/ref_counted_ptr.h \ +src/core/lib/gprpp/ref_counted_string.cc \ +src/core/lib/gprpp/ref_counted_string.h \ src/core/lib/gprpp/single_set_ptr.h \ src/core/lib/gprpp/sorted_pack.h \ src/core/lib/gprpp/stat.h \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index 166a2822a07..d30d0792f44 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -2022,6 +2022,8 @@ src/core/lib/gprpp/posix/stat.cc \ src/core/lib/gprpp/posix/thd.cc \ src/core/lib/gprpp/ref_counted.h \ src/core/lib/gprpp/ref_counted_ptr.h \ +src/core/lib/gprpp/ref_counted_string.cc \ +src/core/lib/gprpp/ref_counted_string.h \ src/core/lib/gprpp/single_set_ptr.h \ src/core/lib/gprpp/sorted_pack.h \ src/core/lib/gprpp/stat.h \