From 0724a90fd8dba3688d7a583559aa43a6093fc9bc Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Thu, 20 Oct 2022 18:15:09 -0700 Subject: [PATCH] Revert "Revert "[arena] pool allocator (#31298)" (#31414)" (#31415) This reverts commit 2c11e56433c47829388c93f80468b5816148ac3f. --- BUILD | 7 +- CMakeLists.txt | 38 --- Makefile | 2 - build_autogenerated.yaml | 14 -- config.m4 | 1 - config.w32 | 1 - gRPC-C++.podspec | 2 - gRPC-Core.podspec | 3 - grpc.gemspec | 2 - grpc.gyp | 2 - package.xml | 2 - .../channel_idle/channel_idle_filter.h | 1 - .../fault_injection/fault_injection_filter.h | 1 - .../filters/http/client/http_client_filter.cc | 4 +- .../filters/http/client/http_client_filter.h | 1 - .../filters/http/client_authority_filter.cc | 2 +- .../filters/http/client_authority_filter.h | 1 - .../filters/http/server/http_server_filter.cc | 20 +- .../filters/http/server/http_server_filter.h | 1 - .../server_load_reporting_filter.cc | 2 +- .../server_load_reporting_filter.h | 1 - .../server_config_selector_filter.cc | 5 +- src/core/lib/channel/channel_stack.h | 1 - src/core/lib/channel/connected_channel.cc | 10 +- src/core/lib/channel/promise_based_filter.h | 10 +- src/core/lib/promise/call_push_pull.h | 4 +- src/core/lib/promise/detail/status.h | 23 ++ src/core/lib/promise/try_seq.h | 4 +- src/core/lib/resource_quota/arena.cc | 19 ++ src/core/lib/resource_quota/arena.h | 89 +++++++ .../authorization/grpc_server_authz_filter.cc | 7 +- .../authorization/grpc_server_authz_filter.h | 1 - .../security/credentials/call_creds_util.h | 2 +- .../composite/composite_credentials.cc | 2 +- .../composite/composite_credentials.h | 2 +- .../credentials/fake/fake_credentials.cc | 2 +- .../credentials/fake/fake_credentials.h | 2 +- .../credentials/iam/iam_credentials.cc | 2 +- .../credentials/iam/iam_credentials.h | 2 +- .../credentials/jwt/jwt_credentials.cc | 2 +- .../credentials/jwt/jwt_credentials.h | 2 +- .../credentials/oauth2/oauth2_credentials.cc | 1 - .../credentials/oauth2/oauth2_credentials.h | 2 +- .../credentials/plugin/plugin_credentials.cc | 2 +- .../credentials/plugin/plugin_credentials.h | 2 +- .../security/transport/client_auth_filter.cc | 2 +- src/core/lib/surface/call.cc | 22 +- src/core/lib/surface/call_trace.cc | 1 - src/core/lib/surface/lame_client.cc | 3 +- src/core/lib/surface/lame_client.h | 1 - src/core/lib/transport/call_fragments.cc | 45 ---- src/core/lib/transport/call_fragments.h | 232 ------------------ src/core/lib/transport/transport.cc | 16 ++ src/core/lib/transport/transport.h | 57 ++++- src/core/lib/transport/transport_impl.h | 1 - src/python/grpcio/grpc_core_dependencies.py | 1 - test/core/filters/client_auth_filter_test.cc | 17 +- .../filters/client_authority_filter_test.cc | 14 +- test/core/filters/filter_fuzzer.cc | 9 +- test/core/resource_quota/arena_test.cc | 45 +++- test/core/security/credentials_test.cc | 19 +- test/core/security/oauth2_utils.cc | 5 +- test/core/transport/BUILD | 15 -- test/core/transport/call_fragments_test.cc | 96 -------- tools/doxygen/Doxyfile.c++.internal | 2 - tools/doxygen/Doxyfile.core.internal | 2 - tools/run_tests/generated/tests.json | 24 -- 67 files changed, 334 insertions(+), 601 deletions(-) delete mode 100644 src/core/lib/transport/call_fragments.cc delete mode 100644 src/core/lib/transport/call_fragments.h delete mode 100644 test/core/transport/call_fragments_test.cc diff --git a/BUILD b/BUILD index bd6af7bffdc..c9ae8316450 100644 --- a/BUILD +++ b/BUILD @@ -2045,6 +2045,10 @@ grpc_cc_library( hdrs = [ "//src/core:lib/resource_quota/arena.h", ], + external_deps = [ + "absl/meta:type_traits", + "absl/utility", + ], deps = [ "construct_destruct", "context", @@ -3299,7 +3303,6 @@ grpc_cc_library( "//src/core:lib/surface/server.cc", "//src/core:lib/surface/validate_metadata.cc", "//src/core:lib/surface/version.cc", - "//src/core:lib/transport/call_fragments.cc", "//src/core:lib/transport/connectivity_state.cc", "//src/core:lib/transport/error_utils.cc", "//src/core:lib/transport/metadata_batch.cc", @@ -3389,7 +3392,6 @@ grpc_cc_library( "//src/core:lib/surface/validate_metadata.h", "//src/core:lib/transport/connectivity_state.h", "//src/core:lib/transport/metadata_batch.h", - "//src/core:lib/transport/call_fragments.h", "//src/core:lib/transport/parsed_metadata.h", "//src/core:lib/transport/status_conversion.h", "//src/core:lib/transport/timeout_encoding.h", @@ -3483,6 +3485,7 @@ grpc_cc_library( "poll", "pollset_set", "promise", + "promise_status", "ref_counted", "ref_counted_ptr", "resolved_address", diff --git a/CMakeLists.txt b/CMakeLists.txt index 874fe03dd12..e4cf1a944cd 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -847,7 +847,6 @@ if(gRPC_BUILD_TESTS) add_dependencies(buildtests_cxx byte_buffer_test) add_dependencies(buildtests_cxx c_slice_buffer_test) add_dependencies(buildtests_cxx call_finalization_test) - add_dependencies(buildtests_cxx call_fragments_test) add_dependencies(buildtests_cxx call_push_pull_test) add_dependencies(buildtests_cxx cancel_ares_query_test) add_dependencies(buildtests_cxx cel_authorization_engine_test) @@ -2324,7 +2323,6 @@ add_library(grpc src/core/lib/surface/validate_metadata.cc src/core/lib/surface/version.cc src/core/lib/transport/bdp_estimator.cc - src/core/lib/transport/call_fragments.cc src/core/lib/transport/connectivity_state.cc src/core/lib/transport/error_utils.cc src/core/lib/transport/handshaker.cc @@ -2901,7 +2899,6 @@ add_library(grpc_unsecure src/core/lib/surface/validate_metadata.cc src/core/lib/surface/version.cc src/core/lib/transport/bdp_estimator.cc - src/core/lib/transport/call_fragments.cc src/core/lib/transport/connectivity_state.cc src/core/lib/transport/error_utils.cc src/core/lib/transport/handshaker.cc @@ -6730,41 +6727,6 @@ target_link_libraries(call_finalization_test ) -endif() -if(gRPC_BUILD_TESTS) - -add_executable(call_fragments_test - test/core/transport/call_fragments_test.cc - third_party/googletest/googletest/src/gtest-all.cc - third_party/googletest/googlemock/src/gmock-all.cc -) - -target_include_directories(call_fragments_test - PRIVATE - ${CMAKE_CURRENT_SOURCE_DIR} - ${CMAKE_CURRENT_SOURCE_DIR}/include - ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR} - ${_gRPC_RE2_INCLUDE_DIR} - ${_gRPC_SSL_INCLUDE_DIR} - ${_gRPC_UPB_GENERATED_DIR} - ${_gRPC_UPB_GRPC_GENERATED_DIR} - ${_gRPC_UPB_INCLUDE_DIR} - ${_gRPC_XXHASH_INCLUDE_DIR} - ${_gRPC_ZLIB_INCLUDE_DIR} - third_party/googletest/googletest/include - third_party/googletest/googletest - third_party/googletest/googlemock/include - third_party/googletest/googlemock - ${_gRPC_PROTO_GENS_DIR} -) - -target_link_libraries(call_fragments_test - ${_gRPC_PROTOBUF_LIBRARIES} - ${_gRPC_ALLTARGETS_LIBRARIES} - grpc_test_util -) - - endif() if(gRPC_BUILD_TESTS) diff --git a/Makefile b/Makefile index 7cc622bd4b1..3cf69edab53 100644 --- a/Makefile +++ b/Makefile @@ -1610,7 +1610,6 @@ LIBGRPC_SRC = \ src/core/lib/surface/validate_metadata.cc \ src/core/lib/surface/version.cc \ src/core/lib/transport/bdp_estimator.cc \ - src/core/lib/transport/call_fragments.cc \ src/core/lib/transport/connectivity_state.cc \ src/core/lib/transport/error_utils.cc \ src/core/lib/transport/handshaker.cc \ @@ -2050,7 +2049,6 @@ LIBGRPC_UNSECURE_SRC = \ src/core/lib/surface/validate_metadata.cc \ src/core/lib/surface/version.cc \ src/core/lib/transport/bdp_estimator.cc \ - src/core/lib/transport/call_fragments.cc \ src/core/lib/transport/connectivity_state.cc \ src/core/lib/transport/error_utils.cc \ src/core/lib/transport/handshaker.cc \ diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 90805131fb9..a9d04aa9d92 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -980,7 +980,6 @@ libs: - src/core/lib/surface/server.h - src/core/lib/surface/validate_metadata.h - src/core/lib/transport/bdp_estimator.h - - src/core/lib/transport/call_fragments.h - src/core/lib/transport/connectivity_state.h - src/core/lib/transport/error_utils.h - src/core/lib/transport/handshaker.h @@ -1680,7 +1679,6 @@ libs: - src/core/lib/surface/validate_metadata.cc - src/core/lib/surface/version.cc - src/core/lib/transport/bdp_estimator.cc - - src/core/lib/transport/call_fragments.cc - src/core/lib/transport/connectivity_state.cc - src/core/lib/transport/error_utils.cc - src/core/lib/transport/handshaker.cc @@ -2170,7 +2168,6 @@ libs: - src/core/lib/surface/server.h - src/core/lib/surface/validate_metadata.h - src/core/lib/transport/bdp_estimator.h - - src/core/lib/transport/call_fragments.h - src/core/lib/transport/connectivity_state.h - src/core/lib/transport/error_utils.h - src/core/lib/transport/handshaker.h @@ -2503,7 +2500,6 @@ libs: - src/core/lib/surface/validate_metadata.cc - src/core/lib/surface/version.cc - src/core/lib/transport/bdp_estimator.cc - - src/core/lib/transport/call_fragments.cc - src/core/lib/transport/connectivity_state.cc - src/core/lib/transport/error_utils.cc - src/core/lib/transport/handshaker.cc @@ -4507,16 +4503,6 @@ targets: - test/core/channel/call_finalization_test.cc deps: - grpc_test_util -- name: call_fragments_test - gtest: true - build: test - language: c++ - headers: - - test/core/promise/test_context.h - src: - - test/core/transport/call_fragments_test.cc - deps: - - grpc_test_util - name: call_push_pull_test gtest: true build: test diff --git a/config.m4 b/config.m4 index 7f12b605ede..a174c65f06d 100644 --- a/config.m4 +++ b/config.m4 @@ -734,7 +734,6 @@ if test "$PHP_GRPC" != "no"; then src/core/lib/surface/validate_metadata.cc \ src/core/lib/surface/version.cc \ src/core/lib/transport/bdp_estimator.cc \ - src/core/lib/transport/call_fragments.cc \ src/core/lib/transport/connectivity_state.cc \ src/core/lib/transport/error_utils.cc \ src/core/lib/transport/handshaker.cc \ diff --git a/config.w32 b/config.w32 index ba89250f7b0..761adf04f16 100644 --- a/config.w32 +++ b/config.w32 @@ -700,7 +700,6 @@ if (PHP_GRPC != "no") { "src\\core\\lib\\surface\\validate_metadata.cc " + "src\\core\\lib\\surface\\version.cc " + "src\\core\\lib\\transport\\bdp_estimator.cc " + - "src\\core\\lib\\transport\\call_fragments.cc " + "src\\core\\lib\\transport\\connectivity_state.cc " + "src\\core\\lib\\transport\\error_utils.cc " + "src\\core\\lib\\transport\\handshaker.cc " + diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index 0eea4068f75..4096819a511 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -954,7 +954,6 @@ Pod::Spec.new do |s| 'src/core/lib/surface/server.h', 'src/core/lib/surface/validate_metadata.h', 'src/core/lib/transport/bdp_estimator.h', - 'src/core/lib/transport/call_fragments.h', 'src/core/lib/transport/connectivity_state.h', 'src/core/lib/transport/error_utils.h', 'src/core/lib/transport/handshaker.h', @@ -1831,7 +1830,6 @@ Pod::Spec.new do |s| 'src/core/lib/surface/server.h', 'src/core/lib/surface/validate_metadata.h', 'src/core/lib/transport/bdp_estimator.h', - 'src/core/lib/transport/call_fragments.h', 'src/core/lib/transport/connectivity_state.h', 'src/core/lib/transport/error_utils.h', 'src/core/lib/transport/handshaker.h', diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index c4e0f0ec73d..8e81682d138 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -1585,8 +1585,6 @@ Pod::Spec.new do |s| 'src/core/lib/surface/version.cc', 'src/core/lib/transport/bdp_estimator.cc', 'src/core/lib/transport/bdp_estimator.h', - 'src/core/lib/transport/call_fragments.cc', - 'src/core/lib/transport/call_fragments.h', 'src/core/lib/transport/connectivity_state.cc', 'src/core/lib/transport/connectivity_state.h', 'src/core/lib/transport/error_utils.cc', @@ -2469,7 +2467,6 @@ Pod::Spec.new do |s| 'src/core/lib/surface/server.h', 'src/core/lib/surface/validate_metadata.h', 'src/core/lib/transport/bdp_estimator.h', - 'src/core/lib/transport/call_fragments.h', 'src/core/lib/transport/connectivity_state.h', 'src/core/lib/transport/error_utils.h', 'src/core/lib/transport/handshaker.h', diff --git a/grpc.gemspec b/grpc.gemspec index e7a90e90daf..389d6772292 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -1496,8 +1496,6 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/surface/version.cc ) s.files += %w( src/core/lib/transport/bdp_estimator.cc ) s.files += %w( src/core/lib/transport/bdp_estimator.h ) - s.files += %w( src/core/lib/transport/call_fragments.cc ) - s.files += %w( src/core/lib/transport/call_fragments.h ) s.files += %w( src/core/lib/transport/connectivity_state.cc ) s.files += %w( src/core/lib/transport/connectivity_state.h ) s.files += %w( src/core/lib/transport/error_utils.cc ) diff --git a/grpc.gyp b/grpc.gyp index 4825c701b58..dfaeba22175 100644 --- a/grpc.gyp +++ b/grpc.gyp @@ -1024,7 +1024,6 @@ 'src/core/lib/surface/validate_metadata.cc', 'src/core/lib/surface/version.cc', 'src/core/lib/transport/bdp_estimator.cc', - 'src/core/lib/transport/call_fragments.cc', 'src/core/lib/transport/connectivity_state.cc', 'src/core/lib/transport/error_utils.cc', 'src/core/lib/transport/handshaker.cc', @@ -1443,7 +1442,6 @@ 'src/core/lib/surface/validate_metadata.cc', 'src/core/lib/surface/version.cc', 'src/core/lib/transport/bdp_estimator.cc', - 'src/core/lib/transport/call_fragments.cc', 'src/core/lib/transport/connectivity_state.cc', 'src/core/lib/transport/error_utils.cc', 'src/core/lib/transport/handshaker.cc', diff --git a/package.xml b/package.xml index 09d9606b75e..d8acf73610e 100644 --- a/package.xml +++ b/package.xml @@ -1478,8 +1478,6 @@ - - diff --git a/src/core/ext/filters/channel_idle/channel_idle_filter.h b/src/core/ext/filters/channel_idle/channel_idle_filter.h index 696c92b2f20..bc49a2a0174 100644 --- a/src/core/ext/filters/channel_idle/channel_idle_filter.h +++ b/src/core/ext/filters/channel_idle/channel_idle_filter.h @@ -35,7 +35,6 @@ #include "src/core/lib/gprpp/time.h" #include "src/core/lib/promise/activity.h" #include "src/core/lib/promise/arena_promise.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/connectivity_state.h" #include "src/core/lib/transport/transport.h" diff --git a/src/core/ext/filters/fault_injection/fault_injection_filter.h b/src/core/ext/filters/fault_injection/fault_injection_filter.h index a96d35715c2..611c3d7f45b 100644 --- a/src/core/ext/filters/fault_injection/fault_injection_filter.h +++ b/src/core/ext/filters/fault_injection/fault_injection_filter.h @@ -32,7 +32,6 @@ #include "src/core/lib/channel/promise_based_filter.h" #include "src/core/lib/gprpp/sync.h" #include "src/core/lib/promise/arena_promise.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/transport.h" // Channel arg key for enabling parsing fault injection via method config. diff --git a/src/core/ext/filters/http/client/http_client_filter.cc b/src/core/ext/filters/http/client/http_client_filter.cc index bb3ea724222..83ca6db4703 100644 --- a/src/core/ext/filters/http/client/http_client_filter.cc +++ b/src/core/ext/filters/http/client/http_client_filter.cc @@ -21,6 +21,7 @@ #include #include +#include #include #include #include @@ -45,7 +46,6 @@ #include "src/core/lib/promise/seq.h" #include "src/core/lib/resource_quota/arena.h" #include "src/core/lib/slice/percent_encoding.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/status_conversion.h" #include "src/core/lib/transport/transport_fwd.h" #include "src/core/lib/transport/transport_impl.h" @@ -126,7 +126,7 @@ ArenaPromise HttpClientFilter::MakeCallPromise( Seq(next_promise_factory(std::move(call_args)), [](ServerMetadataHandle md) -> ServerMetadataHandle { auto r = CheckServerMetadata(md.get()); - if (!r.ok()) return ServerMetadataHandle(r); + if (!r.ok()) return ServerMetadataFromStatus(r); return md; }), []() { return absl::OkStatus(); }, diff --git a/src/core/ext/filters/http/client/http_client_filter.h b/src/core/ext/filters/http/client/http_client_filter.h index 7d33c9dc64d..5a62d01053a 100644 --- a/src/core/ext/filters/http/client/http_client_filter.h +++ b/src/core/ext/filters/http/client/http_client_filter.h @@ -27,7 +27,6 @@ #include "src/core/lib/channel/promise_based_filter.h" #include "src/core/lib/promise/arena_promise.h" #include "src/core/lib/slice/slice.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/metadata_batch.h" #include "src/core/lib/transport/transport.h" diff --git a/src/core/ext/filters/http/client_authority_filter.cc b/src/core/ext/filters/http/client_authority_filter.cc index 67d1de3458e..33d5518928f 100644 --- a/src/core/ext/filters/http/client_authority_filter.cc +++ b/src/core/ext/filters/http/client_authority_filter.cc @@ -23,6 +23,7 @@ #include #include +#include #include "absl/status/status.h" #include "absl/strings/string_view.h" @@ -35,7 +36,6 @@ #include "src/core/lib/config/core_configuration.h" #include "src/core/lib/surface/channel_init.h" #include "src/core/lib/surface/channel_stack_type.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/metadata_batch.h" namespace grpc_core { diff --git a/src/core/ext/filters/http/client_authority_filter.h b/src/core/ext/filters/http/client_authority_filter.h index 1eed0ffa220..4f52e52f88a 100644 --- a/src/core/ext/filters/http/client_authority_filter.h +++ b/src/core/ext/filters/http/client_authority_filter.h @@ -30,7 +30,6 @@ #include "src/core/lib/channel/promise_based_filter.h" #include "src/core/lib/promise/arena_promise.h" #include "src/core/lib/slice/slice.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/transport.h" namespace grpc_core { diff --git a/src/core/ext/filters/http/server/http_server_filter.cc b/src/core/ext/filters/http/server/http_server_filter.cc index eb7ef5d3e97..f6314d61592 100644 --- a/src/core/ext/filters/http/server/http_server_filter.cc +++ b/src/core/ext/filters/http/server/http_server_filter.cc @@ -21,6 +21,7 @@ #include "src/core/ext/filters/http/server/http_server_filter.h" #include +#include #include #include "absl/base/attributes.h" @@ -40,7 +41,6 @@ #include "src/core/lib/resource_quota/arena.h" #include "src/core/lib/slice/percent_encoding.h" #include "src/core/lib/slice/slice.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/metadata_batch.h" namespace grpc_core { @@ -75,11 +75,11 @@ ArenaPromise HttpServerFilter::MakeCallPromise( case HttpMethodMetadata::kInvalid: case HttpMethodMetadata::kGet: return Immediate( - ServerMetadataHandle(absl::UnknownError("Bad method header"))); + ServerMetadataFromStatus(absl::UnknownError("Bad method header"))); } } else { return Immediate( - ServerMetadataHandle(absl::UnknownError("Missing :method header"))); + ServerMetadataFromStatus(absl::UnknownError("Missing :method header"))); } auto te = md->Take(TeMetadata()); @@ -87,21 +87,21 @@ ArenaPromise HttpServerFilter::MakeCallPromise( // Do nothing, ok. } else if (!te.has_value()) { return Immediate( - ServerMetadataHandle(absl::UnknownError("Missing :te header"))); + ServerMetadataFromStatus(absl::UnknownError("Missing :te header"))); } else { return Immediate( - ServerMetadataHandle(absl::UnknownError("Bad :te header"))); + ServerMetadataFromStatus(absl::UnknownError("Bad :te header"))); } auto scheme = md->Take(HttpSchemeMetadata()); if (scheme.has_value()) { if (*scheme == HttpSchemeMetadata::kInvalid) { return Immediate( - ServerMetadataHandle(absl::UnknownError("Bad :scheme header"))); + ServerMetadataFromStatus(absl::UnknownError("Bad :scheme header"))); } } else { return Immediate( - ServerMetadataHandle(absl::UnknownError("Missing :scheme header"))); + ServerMetadataFromStatus(absl::UnknownError("Missing :scheme header"))); } md->Remove(ContentTypeMetadata()); @@ -109,7 +109,7 @@ ArenaPromise HttpServerFilter::MakeCallPromise( Slice* path_slice = md->get_pointer(HttpPathMetadata()); if (path_slice == nullptr) { return Immediate( - ServerMetadataHandle(absl::UnknownError("Missing :path header"))); + ServerMetadataFromStatus(absl::UnknownError("Missing :path header"))); } if (md->get_pointer(HttpAuthorityMetadata()) == nullptr) { @@ -120,8 +120,8 @@ ArenaPromise HttpServerFilter::MakeCallPromise( } if (md->get_pointer(HttpAuthorityMetadata()) == nullptr) { - return Immediate( - ServerMetadataHandle(absl::UnknownError("Missing :authority header"))); + return Immediate(ServerMetadataFromStatus( + absl::UnknownError("Missing :authority header"))); } if (!surface_user_agent_) { diff --git a/src/core/ext/filters/http/server/http_server_filter.h b/src/core/ext/filters/http/server/http_server_filter.h index f9d1a3081fb..b4fc7ecc496 100644 --- a/src/core/ext/filters/http/server/http_server_filter.h +++ b/src/core/ext/filters/http/server/http_server_filter.h @@ -27,7 +27,6 @@ #include "src/core/lib/channel/channel_fwd.h" #include "src/core/lib/channel/promise_based_filter.h" #include "src/core/lib/promise/arena_promise.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/transport.h" namespace grpc_core { diff --git a/src/core/ext/filters/load_reporting/server_load_reporting_filter.cc b/src/core/ext/filters/load_reporting/server_load_reporting_filter.cc index 50e2af1e144..0d7285e2b37 100644 --- a/src/core/ext/filters/load_reporting/server_load_reporting_filter.cc +++ b/src/core/ext/filters/load_reporting/server_load_reporting_filter.cc @@ -25,6 +25,7 @@ #include #include +#include #include #include @@ -63,7 +64,6 @@ #include "src/core/lib/slice/slice.h" #include "src/core/lib/surface/channel_init.h" #include "src/core/lib/surface/channel_stack_type.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/metadata_batch.h" #include "src/core/lib/uri/uri_parser.h" #include "src/cpp/server/load_reporter/constants.h" diff --git a/src/core/ext/filters/load_reporting/server_load_reporting_filter.h b/src/core/ext/filters/load_reporting/server_load_reporting_filter.h index e63184c54b0..f786bfe0b9b 100644 --- a/src/core/ext/filters/load_reporting/server_load_reporting_filter.h +++ b/src/core/ext/filters/load_reporting/server_load_reporting_filter.h @@ -30,7 +30,6 @@ #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/channel/promise_based_filter.h" #include "src/core/lib/promise/arena_promise.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/transport.h" namespace grpc_core { diff --git a/src/core/ext/filters/server_config_selector/server_config_selector_filter.cc b/src/core/ext/filters/server_config_selector/server_config_selector_filter.cc index cd45c3ee2a3..bed20e7a984 100644 --- a/src/core/ext/filters/server_config_selector/server_config_selector_filter.cc +++ b/src/core/ext/filters/server_config_selector/server_config_selector_filter.cc @@ -40,7 +40,6 @@ #include "src/core/lib/promise/promise.h" #include "src/core/lib/resource_quota/arena.h" #include "src/core/lib/service_config/service_config_call_data.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/transport.h" namespace grpc_core { @@ -133,11 +132,11 @@ ServerConfigSelectorFilter::~ServerConfigSelectorFilter() { ArenaPromise ServerConfigSelectorFilter::MakeCallPromise( CallArgs call_args, NextPromiseFactory next_promise_factory) { auto sel = config_selector(); - if (!sel.ok()) return Immediate(ServerMetadataHandle(sel.status())); + if (!sel.ok()) return Immediate(ServerMetadataFromStatus(sel.status())); auto call_config = sel.value()->GetCallConfig(call_args.client_initial_metadata.get()); if (!call_config.error.ok()) { - auto r = Immediate(ServerMetadataHandle( + auto r = Immediate(ServerMetadataFromStatus( absl::UnavailableError(StatusToString(call_config.error)))); return std::move(r); } diff --git a/src/core/lib/channel/channel_stack.h b/src/core/lib/channel/channel_stack.h index 1167772502e..6d19e126c12 100644 --- a/src/core/lib/channel/channel_stack.h +++ b/src/core/lib/channel/channel_stack.h @@ -70,7 +70,6 @@ #include "src/core/lib/iomgr/polling_entity.h" #include "src/core/lib/promise/arena_promise.h" #include "src/core/lib/resource_quota/arena.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/transport.h" struct grpc_channel_element_args { diff --git a/src/core/lib/channel/connected_channel.cc b/src/core/lib/channel/connected_channel.cc index e1b374096c9..02a1ecd8692 100644 --- a/src/core/lib/channel/connected_channel.cc +++ b/src/core/lib/channel/connected_channel.cc @@ -67,7 +67,6 @@ #include "src/core/lib/surface/call.h" #include "src/core/lib/surface/call_trace.h" #include "src/core/lib/surface/channel_stack_type.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/metadata_batch.h" #include "src/core/lib/transport/transport.h" #include "src/core/lib/transport/transport_fwd.h" @@ -372,7 +371,7 @@ class ClientStream : public Orphanable { batch_payload_.send_initial_metadata.peer_string = GetContext()->peer_string_atm_ptr(); server_initial_metadata_ = - GetContext()->MakeServerMetadata(); + GetContext()->MakePooled(GetContext()); batch_payload_.recv_initial_metadata.recv_initial_metadata = server_initial_metadata_.get(); batch_payload_.recv_initial_metadata.recv_initial_metadata_ready = @@ -381,7 +380,7 @@ class ClientStream : public Orphanable { nullptr; batch_payload_.recv_initial_metadata.peer_string = nullptr; server_trailing_metadata_ = - GetContext()->MakeClientMetadata(); + GetContext()->MakePooled(GetContext()); batch_payload_.recv_trailing_metadata.recv_trailing_metadata = server_trailing_metadata_.get(); batch_payload_.recv_trailing_metadata.collect_stats = @@ -421,7 +420,8 @@ class ClientStream : public Orphanable { } else { GPR_ASSERT(!absl::holds_alternative(send_message_state_)); client_trailing_metadata_ = - GetContext()->MakeClientMetadata(); + GetContext()->MakePooled( + GetContext()); send_message_state_ = Closed{}; send_message_.send_trailing_metadata = true; batch_payload_.send_trailing_metadata.send_trailing_metadata = @@ -446,7 +446,7 @@ class ClientStream : public Orphanable { pending->payload->Length()); } recv_message_state_ = server_to_client_messages_->Push( - GetContext()->MakeMessage( + GetContext()->MakePooled( std::move(*pending->payload), pending->flags)); } else { if (grpc_call_trace.enabled()) { diff --git a/src/core/lib/channel/promise_based_filter.h b/src/core/lib/channel/promise_based_filter.h index 2cabc081bfb..910849cac30 100644 --- a/src/core/lib/channel/promise_based_filter.h +++ b/src/core/lib/channel/promise_based_filter.h @@ -60,7 +60,6 @@ #include "src/core/lib/promise/latch.h" #include "src/core/lib/promise/poll.h" #include "src/core/lib/resource_quota/arena.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/error_utils.h" #include "src/core/lib/transport/metadata_batch.h" #include "src/core/lib/transport/transport.h" @@ -253,14 +252,15 @@ class BaseCallData : public Activity, private Wakeable { grpc_transport_stream_op_batch* batch_; }; - static FragmentHandle WrapMetadata( + static Arena::PoolPtr WrapMetadata( grpc_metadata_batch* p) { - return FragmentHandle(p, false); + return Arena::PoolPtr(p, + Arena::PooledDeleter(nullptr)); } static grpc_metadata_batch* UnwrapMetadata( - FragmentHandle p) { - return p.Unwrap(); + Arena::PoolPtr p) { + return p.release(); } Arena* arena() { return arena_; } diff --git a/src/core/lib/promise/call_push_pull.h b/src/core/lib/promise/call_push_pull.h index 9f39d7ebb42..c5672a00412 100644 --- a/src/core/lib/promise/call_push_pull.h +++ b/src/core/lib/promise/call_push_pull.h @@ -79,7 +79,7 @@ class CallPushPull { if (IsStatusOk(*status)) { done_.set(kDonePush); } else { - return Result(std::move(*status)); + return StatusCast(std::move(*status)); } } } @@ -97,7 +97,7 @@ class CallPushPull { if (IsStatusOk(*status)) { done_.set(kDonePull); } else { - return Result(std::move(*status)); + return StatusCast(std::move(*status)); } } } diff --git a/src/core/lib/promise/detail/status.h b/src/core/lib/promise/detail/status.h index a56c6fe08c0..6b71feb3e07 100644 --- a/src/core/lib/promise/detail/status.h +++ b/src/core/lib/promise/detail/status.h @@ -45,6 +45,29 @@ inline absl::Status IntoStatus(absl::Status* status) { // can participate in TrySeq as result types that affect control flow. inline bool IsStatusOk(const absl::Status& status) { return status.ok(); } +template +struct StatusCastImpl; + +template +struct StatusCastImpl { + static To Cast(To&& t) { return std::move(t); } +}; + +template +struct StatusCastImpl, absl::Status> { + static absl::StatusOr Cast(absl::Status&& t) { return std::move(t); } +}; + +template +struct StatusCastImpl, const absl::Status&> { + static absl::StatusOr Cast(const absl::Status& t) { return t; } +}; + +template +To StatusCast(From&& from) { + return StatusCastImpl::Cast(std::forward(from)); +} + } // namespace grpc_core #endif // GRPC_CORE_LIB_PROMISE_DETAIL_STATUS_H diff --git a/src/core/lib/promise/try_seq.h b/src/core/lib/promise/try_seq.h index 2f206bd516d..32e657627f2 100644 --- a/src/core/lib/promise/try_seq.h +++ b/src/core/lib/promise/try_seq.h @@ -69,7 +69,7 @@ struct TrySeqTraitsWithSfinae> { template static Poll CheckResultAndRunNext(absl::StatusOr prior, RunNext run_next) { - if (!prior.ok()) return Result(prior.status()); + if (!prior.ok()) return StatusCast(prior.status()); return run_next(std::move(prior)); } }; @@ -105,7 +105,7 @@ struct TrySeqTraitsWithSfinae { template static Poll CheckResultAndRunNext(absl::Status prior, RunNext run_next) { - if (!prior.ok()) return Result(std::move(prior)); + if (!prior.ok()) return StatusCast(std::move(prior)); return run_next(std::move(prior)); } }; diff --git a/src/core/lib/resource_quota/arena.cc b/src/core/lib/resource_quota/arena.cc index 6d6def4ef24..4b7de4071ad 100644 --- a/src/core/lib/resource_quota/arena.cc +++ b/src/core/lib/resource_quota/arena.cc @@ -116,4 +116,23 @@ void Arena::ManagedNewObject::Link(std::atomic* head) { } } +void* Arena::AllocPooled(size_t alloc_size, std::atomic* head) { + FreePoolNode* p = head->load(std::memory_order_acquire); + while (p != nullptr) { + if (head->compare_exchange_weak(p, p->next, std::memory_order_acq_rel, + std::memory_order_relaxed)) { + return p; + } + } + return Alloc(alloc_size); +} + +void Arena::FreePooled(void* p, std::atomic* head) { + FreePoolNode* node = static_cast(p); + node->next = head->load(std::memory_order_acquire); + while (!head->compare_exchange_weak( + node->next, node, std::memory_order_acq_rel, std::memory_order_relaxed)) { + } +} + } // namespace grpc_core diff --git a/src/core/lib/resource_quota/arena.h b/src/core/lib/resource_quota/arena.h index 39038a6b80b..fb3d97a65a4 100644 --- a/src/core/lib/resource_quota/arena.h +++ b/src/core/lib/resource_quota/arena.h @@ -31,8 +31,12 @@ #include #include +#include #include +#include "absl/meta/type_traits.h" +#include "absl/utility/utility.h" + #include #include "src/core/lib/gpr/alloc.h" @@ -42,7 +46,46 @@ namespace grpc_core { +namespace arena_detail { + +template +struct PoolIndexForSize; + +template +struct PoolIndexForSize< + absl::enable_if_t, kIndex, + kObjectSize, kSmallestRemainingBucket, kBucketSizes...> { + static constexpr size_t kPool = kIndex; + static constexpr size_t kSize = kSmallestRemainingBucket; +}; + +template +struct PoolIndexForSize< + absl::enable_if_t<(kObjectSize > kSmallestRemainingBucket)>, kIndex, + kObjectSize, kSmallestRemainingBucket, kBucketSizes...> + : public PoolIndexForSize { +}; + +template +constexpr size_t PoolFromObjectSize( + absl::integer_sequence) { + return PoolIndexForSize::kPool; +} + +template +constexpr size_t AllocationSizeFromObjectSize( + absl::integer_sequence) { + return PoolIndexForSize::kSize; +} + +} // namespace arena_detail + class Arena { + using PoolSizes = absl::integer_sequence; + public: // Create an arena, with \a initial_size bytes in the first allocated buffer. static Arena* Create(size_t initial_size, MemoryAllocator* memory_allocator); @@ -88,6 +131,37 @@ class Arena { return &p->t; } + class PooledDeleter { + public: + explicit PooledDeleter(Arena* arena) : arena_(arena) {} + PooledDeleter() = default; + template + void operator()(T* p) { + // TODO(ctiller): promise based filter hijacks ownership of some pointers + // to make them appear as PoolPtr without really transferring ownership, + // by setting the arena to nullptr. + // This is a transitional hack and should be removed once promise based + // filter is removed. + if (arena_ != nullptr) arena_->DeletePooled(p); + } + + private: + Arena* arena_; + }; + + template + using PoolPtr = std::unique_ptr; + + template + PoolPtr MakePooled(Args&&... args) { + return PoolPtr( + new (AllocPooled( + arena_detail::AllocationSizeFromObjectSize(PoolSizes()), + &pools_[arena_detail::PoolFromObjectSize(PoolSizes())])) + T(std::forward(args)...), + PooledDeleter(this)); + } + private: struct Zone { Zone* prev; @@ -128,6 +202,20 @@ class Arena { void* AllocZone(size_t size); + template + void DeletePooled(T* p) { + p->~T(); + FreePooled( + p, &pools_[arena_detail::PoolFromObjectSize(PoolSizes())]); + } + + struct FreePoolNode { + FreePoolNode* next; + }; + + void* AllocPooled(size_t alloc_size, std::atomic* head); + static void FreePooled(void* p, std::atomic* head); + // Keep track of the total used size. We use this in our call sizing // hysteresis. std::atomic total_used_{0}; @@ -140,6 +228,7 @@ class Arena { // last zone; the zone list is reverse-walked during arena destruction only. std::atomic last_zone_{nullptr}; std::atomic managed_new_head_{nullptr}; + std::atomic pools_[PoolSizes::size()]{}; // The backing memory quota MemoryAllocator* const memory_allocator_; }; diff --git a/src/core/lib/security/authorization/grpc_server_authz_filter.cc b/src/core/lib/security/authorization/grpc_server_authz_filter.cc index 4ee98da9a1a..3f4d9b832a2 100644 --- a/src/core/lib/security/authorization/grpc_server_authz_filter.cc +++ b/src/core/lib/security/authorization/grpc_server_authz_filter.cc @@ -17,6 +17,7 @@ #include "src/core/lib/security/authorization/grpc_server_authz_filter.h" #include +#include #include #include @@ -32,7 +33,6 @@ #include "src/core/lib/promise/promise.h" #include "src/core/lib/security/authorization/authorization_engine.h" #include "src/core/lib/security/authorization/evaluate_args.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/transport.h" namespace grpc_core { @@ -108,8 +108,9 @@ bool GrpcServerAuthzFilter::IsAuthorized( ArenaPromise GrpcServerAuthzFilter::MakeCallPromise( CallArgs call_args, NextPromiseFactory next_promise_factory) { if (!IsAuthorized(call_args.client_initial_metadata)) { - return ArenaPromise(Immediate(ServerMetadataHandle( - absl::PermissionDeniedError("Unauthorized RPC request rejected.")))); + return ArenaPromise( + Immediate(ServerMetadataFromStatus(absl::PermissionDeniedError( + "Unauthorized RPC request rejected.")))); } return next_promise_factory(std::move(call_args)); } diff --git a/src/core/lib/security/authorization/grpc_server_authz_filter.h b/src/core/lib/security/authorization/grpc_server_authz_filter.h index d210df59ccb..b324513ec90 100644 --- a/src/core/lib/security/authorization/grpc_server_authz_filter.h +++ b/src/core/lib/security/authorization/grpc_server_authz_filter.h @@ -30,7 +30,6 @@ #include "src/core/lib/security/authorization/authorization_policy_provider.h" #include "src/core/lib/security/authorization/evaluate_args.h" #include "src/core/lib/security/context/security_context.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/transport.h" namespace grpc_core { diff --git a/src/core/lib/security/credentials/call_creds_util.h b/src/core/lib/security/credentials/call_creds_util.h index cb66c1db069..75b6e83c5f6 100644 --- a/src/core/lib/security/credentials/call_creds_util.h +++ b/src/core/lib/security/credentials/call_creds_util.h @@ -24,7 +24,7 @@ #include #include "src/core/lib/security/credentials/credentials.h" -#include "src/core/lib/transport/call_fragments.h" +#include "src/core/lib/transport/transport.h" namespace grpc_core { diff --git a/src/core/lib/security/credentials/composite/composite_credentials.cc b/src/core/lib/security/credentials/composite/composite_credentials.cc index abb2412f960..32d3685e109 100644 --- a/src/core/lib/security/credentials/composite/composite_credentials.cc +++ b/src/core/lib/security/credentials/composite/composite_credentials.cc @@ -21,6 +21,7 @@ #include "src/core/lib/security/credentials/composite/composite_credentials.h" #include +#include #include #include "absl/strings/str_cat.h" @@ -33,7 +34,6 @@ #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/promise/try_seq.h" #include "src/core/lib/surface/api_trace.h" -#include "src/core/lib/transport/call_fragments.h" // // grpc_composite_channel_credentials diff --git a/src/core/lib/security/credentials/composite/composite_credentials.h b/src/core/lib/security/credentials/composite/composite_credentials.h index ad432816948..045216e9a4a 100644 --- a/src/core/lib/security/credentials/composite/composite_credentials.h +++ b/src/core/lib/security/credentials/composite/composite_credentials.h @@ -39,7 +39,7 @@ #include "src/core/lib/promise/arena_promise.h" #include "src/core/lib/security/credentials/credentials.h" #include "src/core/lib/security/security_connector/security_connector.h" -#include "src/core/lib/transport/call_fragments.h" +#include "src/core/lib/transport/transport.h" /* -- Composite channel credentials. -- */ diff --git a/src/core/lib/security/credentials/fake/fake_credentials.cc b/src/core/lib/security/credentials/fake/fake_credentials.cc index 760a296934d..64c181e588d 100644 --- a/src/core/lib/security/credentials/fake/fake_credentials.cc +++ b/src/core/lib/security/credentials/fake/fake_credentials.cc @@ -22,6 +22,7 @@ #include +#include #include #include "absl/strings/string_view.h" @@ -31,7 +32,6 @@ #include "src/core/lib/promise/promise.h" #include "src/core/lib/security/security_connector/fake/fake_security_connector.h" #include "src/core/lib/security/security_connector/security_connector.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/metadata_batch.h" /* -- Fake transport security credentials. -- */ diff --git a/src/core/lib/security/credentials/fake/fake_credentials.h b/src/core/lib/security/credentials/fake/fake_credentials.h index 4226e0aec3a..4e8e8556a74 100644 --- a/src/core/lib/security/credentials/fake/fake_credentials.h +++ b/src/core/lib/security/credentials/fake/fake_credentials.h @@ -35,7 +35,7 @@ #include "src/core/lib/promise/arena_promise.h" #include "src/core/lib/security/credentials/credentials.h" #include "src/core/lib/slice/slice.h" -#include "src/core/lib/transport/call_fragments.h" +#include "src/core/lib/transport/transport.h" #define GRPC_ARG_FAKE_SECURITY_EXPECTED_TARGETS \ "grpc.fake_security.expected_targets" diff --git a/src/core/lib/security/credentials/iam/iam_credentials.cc b/src/core/lib/security/credentials/iam/iam_credentials.cc index 0138f618331..0d73c390eba 100644 --- a/src/core/lib/security/credentials/iam/iam_credentials.cc +++ b/src/core/lib/security/credentials/iam/iam_credentials.cc @@ -22,6 +22,7 @@ #include +#include #include #include "absl/strings/str_format.h" @@ -34,7 +35,6 @@ #include "src/core/lib/iomgr/exec_ctx.h" #include "src/core/lib/promise/promise.h" #include "src/core/lib/surface/api_trace.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/metadata_batch.h" grpc_core::ArenaPromise> diff --git a/src/core/lib/security/credentials/iam/iam_credentials.h b/src/core/lib/security/credentials/iam/iam_credentials.h index e01a86ffeb1..f0c83af549e 100644 --- a/src/core/lib/security/credentials/iam/iam_credentials.h +++ b/src/core/lib/security/credentials/iam/iam_credentials.h @@ -33,7 +33,7 @@ #include "src/core/lib/promise/arena_promise.h" #include "src/core/lib/security/credentials/credentials.h" #include "src/core/lib/slice/slice.h" -#include "src/core/lib/transport/call_fragments.h" +#include "src/core/lib/transport/transport.h" class grpc_google_iam_credentials : public grpc_call_credentials { public: diff --git a/src/core/lib/security/credentials/jwt/jwt_credentials.cc b/src/core/lib/security/credentials/jwt/jwt_credentials.cc index 798fe8cc7b0..f3e518c3d9a 100644 --- a/src/core/lib/security/credentials/jwt/jwt_credentials.cc +++ b/src/core/lib/security/credentials/jwt/jwt_credentials.cc @@ -24,6 +24,7 @@ #include #include +#include #include #include @@ -42,7 +43,6 @@ #include "src/core/lib/promise/promise.h" #include "src/core/lib/security/credentials/call_creds_util.h" #include "src/core/lib/surface/api_trace.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/metadata_batch.h" #include "src/core/lib/uri/uri_parser.h" diff --git a/src/core/lib/security/credentials/jwt/jwt_credentials.h b/src/core/lib/security/credentials/jwt/jwt_credentials.h index 4c51d892f14..226a95a6577 100644 --- a/src/core/lib/security/credentials/jwt/jwt_credentials.h +++ b/src/core/lib/security/credentials/jwt/jwt_credentials.h @@ -43,7 +43,7 @@ #include "src/core/lib/security/credentials/credentials.h" #include "src/core/lib/security/credentials/jwt/json_token.h" #include "src/core/lib/slice/slice.h" -#include "src/core/lib/transport/call_fragments.h" +#include "src/core/lib/transport/transport.h" class grpc_service_account_jwt_access_credentials : public grpc_call_credentials { diff --git a/src/core/lib/security/credentials/oauth2/oauth2_credentials.cc b/src/core/lib/security/credentials/oauth2/oauth2_credentials.cc index 1b28bbd800d..678c3b2c0c2 100644 --- a/src/core/lib/security/credentials/oauth2/oauth2_credentials.cc +++ b/src/core/lib/security/credentials/oauth2/oauth2_credentials.cc @@ -58,7 +58,6 @@ #include "src/core/lib/promise/promise.h" #include "src/core/lib/security/util/json_util.h" #include "src/core/lib/surface/api_trace.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/error_utils.h" #include "src/core/lib/transport/metadata_batch.h" #include "src/core/lib/uri/uri_parser.h" diff --git a/src/core/lib/security/credentials/oauth2/oauth2_credentials.h b/src/core/lib/security/credentials/oauth2/oauth2_credentials.h index 33d20048a51..5ecfd80d89b 100644 --- a/src/core/lib/security/credentials/oauth2/oauth2_credentials.h +++ b/src/core/lib/security/credentials/oauth2/oauth2_credentials.h @@ -48,7 +48,7 @@ #include "src/core/lib/promise/arena_promise.h" #include "src/core/lib/security/credentials/credentials.h" #include "src/core/lib/slice/slice.h" -#include "src/core/lib/transport/call_fragments.h" +#include "src/core/lib/transport/transport.h" #include "src/core/lib/uri/uri_parser.h" // Constants. diff --git a/src/core/lib/security/credentials/plugin/plugin_credentials.cc b/src/core/lib/security/credentials/plugin/plugin_credentials.cc index c5b32ab50c8..c575f8c09f6 100644 --- a/src/core/lib/security/credentials/plugin/plugin_credentials.cc +++ b/src/core/lib/security/credentials/plugin/plugin_credentials.cc @@ -21,6 +21,7 @@ #include "src/core/lib/security/credentials/plugin/plugin_credentials.h" #include +#include #include "absl/status/status.h" #include "absl/strings/str_cat.h" @@ -37,7 +38,6 @@ #include "src/core/lib/slice/slice_internal.h" #include "src/core/lib/surface/api_trace.h" #include "src/core/lib/surface/validate_metadata.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/metadata_batch.h" grpc_core::TraceFlag grpc_plugin_credentials_trace(false, "plugin_credentials"); diff --git a/src/core/lib/security/credentials/plugin/plugin_credentials.h b/src/core/lib/security/credentials/plugin/plugin_credentials.h index 07148d042a6..a8cd4f5baa2 100644 --- a/src/core/lib/security/credentials/plugin/plugin_credentials.h +++ b/src/core/lib/security/credentials/plugin/plugin_credentials.h @@ -46,7 +46,7 @@ #include "src/core/lib/security/credentials/call_creds_util.h" #include "src/core/lib/security/credentials/credentials.h" #include "src/core/lib/slice/slice.h" -#include "src/core/lib/transport/call_fragments.h" +#include "src/core/lib/transport/transport.h" extern grpc_core::TraceFlag grpc_plugin_credentials_trace; diff --git a/src/core/lib/security/transport/client_auth_filter.cc b/src/core/lib/security/transport/client_auth_filter.cc index 223dc3364a8..6bde7e8709f 100644 --- a/src/core/lib/security/transport/client_auth_filter.cc +++ b/src/core/lib/security/transport/client_auth_filter.cc @@ -21,6 +21,7 @@ #include #include +#include #include // IWYU pragma: keep #include @@ -52,7 +53,6 @@ #include "src/core/lib/security/security_connector/security_connector.h" #include "src/core/lib/security/transport/auth_filters.h" #include "src/core/lib/slice/slice.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/metadata_batch.h" #include "src/core/lib/transport/transport.h" diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index e428f939427..e196a94d2f6 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -94,7 +95,6 @@ #include "src/core/lib/surface/completion_queue.h" #include "src/core/lib/surface/server.h" #include "src/core/lib/surface/validate_metadata.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/error_utils.h" #include "src/core/lib/transport/metadata_batch.h" #include "src/core/lib/transport/transport.h" @@ -1945,17 +1945,14 @@ class PromiseBasedCall : public Call, public Activity, public Wakeable { public promise_detail::Context, public promise_detail::Context, public promise_detail::Context, - public promise_detail::Context, - public promise_detail::Context { + public promise_detail::Context { public: explicit ScopedContext(PromiseBasedCall* call) : ScopedActivity(call), promise_detail::Context(call->arena()), promise_detail::Context(call->context_), promise_detail::Context(&call->call_context_), - promise_detail::Context(&call->finalization_), - promise_detail::Context( - &call->fragment_allocator_) {} + promise_detail::Context(&call->finalization_) {} }; class Completion { @@ -2178,7 +2175,6 @@ class PromiseBasedCall : public Call, public Activity, public Wakeable { /* Contexts for various subsystems (security, tracing, ...). */ grpc_call_context_element context_[GRPC_CONTEXT_COUNT] = {}; grpc_completion_queue* cq_ ABSL_GUARDED_BY(mu_); - FragmentAllocator fragment_allocator_ ABSL_GUARDED_BY(mu_); NonOwningWakable* non_owning_wakeable_ ABSL_GUARDED_BY(mu_) = nullptr; CompletionInfo completion_info_[6]; grpc_call_stats final_stats_{}; @@ -2372,7 +2368,7 @@ class ClientPromiseBasedCall final : public PromiseBasedCall { global_stats().IncrementClientCallsCreated(); ScopedContext context(this); send_initial_metadata_ = - GetContext()->MakeClientMetadata(); + GetContext()->MakePooled(GetContext()); send_initial_metadata_->Set(HttpPathMetadata(), std::move(*args->path)); if (args->authority.has_value()) { send_initial_metadata_->Set(HttpAuthorityMetadata(), @@ -2401,7 +2397,7 @@ class ClientPromiseBasedCall final : public PromiseBasedCall { void Orphan() override { MutexLock lock(mu()); ScopedContext ctx(this); - if (!completed_) Finish(ServerMetadataHandle(absl::CancelledError())); + if (!completed_) Finish(ServerMetadataFromStatus(absl::CancelledError())); } bool is_trailers_only() const override { MutexLock lock(mu()); @@ -2485,7 +2481,7 @@ void ClientPromiseBasedCall::StartPromise( void ClientPromiseBasedCall::CancelWithError(grpc_error_handle error) { MutexLock lock(mu()); ScopedContext context(this); - Finish(ServerMetadataHandle(grpc_error_to_absl_status(error))); + Finish(ServerMetadataFromStatus(grpc_error_to_absl_status(error))); } grpc_call_error ClientPromiseBasedCall::ValidateBatch(const grpc_op* ops, @@ -2568,8 +2564,8 @@ void ClientPromiseBasedCall::CommitBatch(const grpc_op* ops, size_t nops, &op.data.send_message.send_message->data.raw.slice_buffer, send.c_slice_buffer()); outstanding_send_.emplace(client_to_server_messages_.sender.Push( - GetContext()->MakeMessage(std::move(send), - op.flags))); + GetContext()->MakePooled(std::move(send), + op.flags))); } else { FailCompletion(completion); } @@ -2681,7 +2677,7 @@ void ClientPromiseBasedCall::UpdateOnce() { outstanding_send_.reset(); if (!*result) { FailCompletion(send_message_completion_); - Finish(ServerMetadataHandle(absl::Status( + Finish(ServerMetadataFromStatus(absl::Status( absl::StatusCode::kInternal, "Failed to send message to server"))); } } diff --git a/src/core/lib/surface/call_trace.cc b/src/core/lib/surface/call_trace.cc index e234c2d7004..a8a8d5690a5 100644 --- a/src/core/lib/surface/call_trace.cc +++ b/src/core/lib/surface/call_trace.cc @@ -35,7 +35,6 @@ #include "src/core/lib/iomgr/closure.h" #include "src/core/lib/promise/activity.h" #include "src/core/lib/promise/arena_promise.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/metadata_batch.h" #include "src/core/lib/transport/transport.h" diff --git a/src/core/lib/surface/lame_client.cc b/src/core/lib/surface/lame_client.cc index f078f85d601..8a29582c02b 100644 --- a/src/core/lib/surface/lame_client.cc +++ b/src/core/lib/surface/lame_client.cc @@ -46,7 +46,6 @@ #include "src/core/lib/surface/api_trace.h" #include "src/core/lib/surface/channel.h" #include "src/core/lib/surface/channel_stack_type.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/connectivity_state.h" #include "src/core/lib/transport/transport.h" @@ -78,7 +77,7 @@ ArenaPromise LameClientFilter::MakeCallPromise( // TODO(ctiller): remove if check once promise_based_filter is removed (Close // is still needed) if (args.incoming_messages != nullptr) args.incoming_messages->Close(); - return Immediate(ServerMetadataHandle(error_)); + return Immediate(ServerMetadataFromStatus(error_)); } bool LameClientFilter::GetChannelInfo(const grpc_channel_info*) { return true; } diff --git a/src/core/lib/surface/lame_client.h b/src/core/lib/surface/lame_client.h index 8f048866f5f..ef5a8d6247a 100644 --- a/src/core/lib/surface/lame_client.h +++ b/src/core/lib/surface/lame_client.h @@ -35,7 +35,6 @@ #include "src/core/lib/gprpp/sync.h" #include "src/core/lib/iomgr/error.h" #include "src/core/lib/promise/arena_promise.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/connectivity_state.h" #include "src/core/lib/transport/transport.h" diff --git a/src/core/lib/transport/call_fragments.cc b/src/core/lib/transport/call_fragments.cc deleted file mode 100644 index 1fb6941294d..00000000000 --- a/src/core/lib/transport/call_fragments.cc +++ /dev/null @@ -1,45 +0,0 @@ -// Copyright 2022 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_fragments.h" - -namespace grpc_core { - -FragmentAllocator::Node* FragmentAllocator::AllocateNode() { - if (free_list_ != nullptr) { - Node* node = free_list_; - free_list_ = free_list_->next_free; - return node; - } - return static_cast(GetContext()->Alloc(sizeof(Node))); -} - -void FragmentAllocator::FreeNode(Node* node) { - node->next_free = free_list_; - free_list_ = node; -} - -void FragmentAllocator::Delete(grpc_metadata_batch* p) { - p->~grpc_metadata_batch(); - FreeNode(reinterpret_cast(p)); -} - -void FragmentAllocator::Delete(Message* m) { - m->~Message(); - FreeNode(reinterpret_cast(m)); -} - -} // namespace grpc_core diff --git a/src/core/lib/transport/call_fragments.h b/src/core/lib/transport/call_fragments.h deleted file mode 100644 index 47dfa1d749b..00000000000 --- a/src/core/lib/transport/call_fragments.h +++ /dev/null @@ -1,232 +0,0 @@ -// Copyright 2022 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_CORE_LIB_TRANSPORT_CALL_FRAGMENTS_H -#define GRPC_CORE_LIB_TRANSPORT_CALL_FRAGMENTS_H - -#include - -#include - -#include -#include - -#include "absl/status/status.h" -#include "absl/types/optional.h" - -#include - -#include "src/core/lib/promise/context.h" -#include "src/core/lib/resource_quota/arena.h" -#include "src/core/lib/slice/slice.h" -#include "src/core/lib/slice/slice_buffer.h" -#include "src/core/lib/transport/metadata_batch.h" - -namespace grpc_core { - -// TODO(ctiller): eliminate once MetadataHandle is constructable directly. -namespace promise_filter_detail { -class BaseCallData; -} // namespace promise_filter_detail - -class FragmentAllocator; - -// Small owned "handle" type to ensure one accessor at a time to metadata. -// The focus here is to get promises to use the syntax we'd like - we'll -// probably substitute some other smart pointer later. -template -class FragmentHandle { - public: - FragmentHandle() = default; - - FragmentHandle(const FragmentHandle&) = delete; - FragmentHandle& operator=(const FragmentHandle&) = delete; - - FragmentHandle(FragmentHandle&& other) noexcept - : handle_(other.handle_), - allocated_by_allocator_(other.allocated_by_allocator_) { - other.handle_ = nullptr; - other.allocated_by_allocator_ = false; - } - FragmentHandle& operator=(FragmentHandle&& other) noexcept { - DestroyHandle(); - handle_ = other.handle_; - allocated_by_allocator_ = other.allocated_by_allocator_; - other.handle_ = nullptr; - other.allocated_by_allocator_ = false; - return *this; - } - - explicit FragmentHandle(const absl::Status& status); - - ~FragmentHandle() { DestroyHandle(); } - - T* operator->() const { return handle_; } - bool has_value() const { return handle_ != nullptr; } - T* get() const { return handle_; } - void reset() { *this = FragmentHandle(); } - - static FragmentHandle TestOnlyWrap(T* p) { return FragmentHandle(p, false); } - - private: - // We restrict access to construction from a pointer to limit the number of - // cases that need dealing with as this code evolves. - friend class promise_filter_detail::BaseCallData; - friend class FragmentAllocator; - - explicit FragmentHandle(T* handle, bool allocated_by_allocator) - : handle_(handle), allocated_by_allocator_(allocated_by_allocator) {} - - void DestroyHandle(); - - T* Unwrap() { - T* result = handle_; - handle_ = nullptr; - return result; - } - - T* handle_ = nullptr; - // TODO(ctiller): remove this once promise_based_filter goes away. - // This bit determines whether the pointer is allocated by a metadata - // allocator or some other system. If it's held by a metadata allocator, we'll - // release it back when we're done with it. - bool allocated_by_allocator_ = false; -}; - -// Server metadata type -// TODO(ctiller): This should be a bespoke instance of MetadataMap<> -using ServerMetadata = grpc_metadata_batch; -using ServerMetadataHandle = FragmentHandle; - -// Client initial metadata type -// TODO(ctiller): This should be a bespoke instance of MetadataMap<> -using ClientMetadata = grpc_metadata_batch; -using ClientMetadataHandle = FragmentHandle; - -class Message { - public: - Message() = default; - ~Message() = default; - Message(SliceBuffer payload, uint32_t flags) - : payload_(std::move(payload)), flags_(flags) {} - Message(const Message&) = delete; - Message& operator=(const Message&) = delete; - - uint32_t flags() const { return flags_; } - SliceBuffer* payload() { return &payload_; } - const SliceBuffer* payload() const { return &payload_; } - - private: - SliceBuffer payload_; - uint32_t flags_ = 0; -}; - -using MessageHandle = FragmentHandle; - -// Ok/not-ok check for trailing metadata, so that it can be used as result types -// for TrySeq. -inline bool IsStatusOk(const ServerMetadataHandle& m) { - return m->get(GrpcStatusMetadata()).value_or(GRPC_STATUS_UNKNOWN) == - GRPC_STATUS_OK; -} - -// Within a call arena we need metadata at least four times - (client,server) x -// (initial,trailing), and possibly more for early returning promises. -// Since we often don't need these *simultaneously*, we can save memory by -// allocating/releasing them. -// We'd still like the memory to be part of the arena though, so this type -// creates a small free list of metadata objects and a central (call context) -// based place to create/destroy them. -class FragmentAllocator { - public: - FragmentAllocator() = default; - ~FragmentAllocator() = default; - FragmentAllocator(const FragmentAllocator&) = delete; - FragmentAllocator& operator=(const FragmentAllocator&) = delete; - - ClientMetadataHandle MakeClientMetadata() { - auto* node = AllocateNode(); - // TODO(ctiller): once we finish the promise transition, have metadata map - // know about arena contexts and allocate directly from there. - // (we could do so before, but there's enough places where we don't have a - // promise context up that it's too much whackamole) - new (&node->batch) ClientMetadata(GetContext()); - return ClientMetadataHandle(&node->batch, true); - } - - ServerMetadataHandle MakeServerMetadata() { return MakeClientMetadata(); } - - template - MessageHandle MakeMessage(Args&&... args) { - auto* node = AllocateNode(); - new (&node->message) Message(std::forward(args)...); - return MessageHandle(&node->message, true); - } - - private: - union Node { - Node* next_free; - grpc_metadata_batch batch; - Message message; - }; - - template - friend class FragmentHandle; - - void Delete(grpc_metadata_batch* p); - void Delete(Message* m); - - Node* AllocateNode(); - void FreeNode(Node* node); - - Node* free_list_ = nullptr; -}; - -template <> -struct ContextType {}; - -template -FragmentHandle::FragmentHandle(const absl::Status& status) { - // TODO(ctiller): currently we guarantee that MetadataAllocator is only - // present for promise based calls, and if we're using promise_based_filter - // it's not present. If we're in a promise based call, the correct thing is to - // use the metadata allocator to track the memory we need. If we're not, we - // need to do the hacky thing promise_based_filter does. - // This all goes away when promise_based_filter goes away, and this code will - // just assume there's an allocator present and move forward. - if (HasContext()) { - handle_ = nullptr; - allocated_by_allocator_ = false; - *this = GetContext()->MakeServerMetadata(); - } else { - handle_ = GetContext()->New(GetContext()); - allocated_by_allocator_ = false; - } - handle_->Set(GrpcStatusMetadata(), - static_cast(status.code())); - if (status.ok()) return; - handle_->Set(GrpcMessageMetadata(), - Slice::FromCopiedString(status.message())); -} - -template -void FragmentHandle::DestroyHandle() { - if (allocated_by_allocator_) { - GetContext()->Delete(handle_); - } -} - -} // namespace grpc_core - -#endif // GRPC_CORE_LIB_TRANSPORT_CALL_FRAGMENTS_H diff --git a/src/core/lib/transport/transport.cc b/src/core/lib/transport/transport.cc index eebbd6529c0..05b2f62b97f 100644 --- a/src/core/lib/transport/transport.cc +++ b/src/core/lib/transport/transport.cc @@ -32,6 +32,8 @@ #include "src/core/lib/event_engine/default_event_engine.h" #include "src/core/lib/gpr/alloc.h" #include "src/core/lib/iomgr/exec_ctx.h" +#include "src/core/lib/promise/context.h" +#include "src/core/lib/slice/slice.h" #include "src/core/lib/transport/transport_impl.h" grpc_core::DebugOnlyTraceFlag grpc_trace_stream_refcount(false, @@ -267,3 +269,17 @@ grpc_transport_stream_op_batch* grpc_make_transport_stream_op( op->op.on_complete = &op->outer_on_complete; return &op->op; } + +namespace grpc_core { + +ServerMetadataHandle ServerMetadataFromStatus(const absl::Status& status) { + auto hdl = + GetContext()->MakePooled(GetContext()); + hdl->Set(GrpcStatusMetadata(), static_cast(status.code())); + if (!status.ok()) { + hdl->Set(GrpcMessageMetadata(), Slice::FromCopiedString(status.message())); + } + return hdl; +} + +} // namespace grpc_core diff --git a/src/core/lib/transport/transport.h b/src/core/lib/transport/transport.h index 1164e4a22b2..f2d3d70f44f 100644 --- a/src/core/lib/transport/transport.h +++ b/src/core/lib/transport/transport.h @@ -27,7 +27,9 @@ #include #include +#include +#include "absl/status/status.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" @@ -49,11 +51,11 @@ #include "src/core/lib/iomgr/iomgr_fwd.h" #include "src/core/lib/iomgr/polling_entity.h" #include "src/core/lib/promise/arena_promise.h" +#include "src/core/lib/promise/detail/status.h" #include "src/core/lib/promise/latch.h" #include "src/core/lib/promise/pipe.h" #include "src/core/lib/resource_quota/arena.h" #include "src/core/lib/slice/slice_buffer.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/connectivity_state.h" #include "src/core/lib/transport/metadata_batch.h" #include "src/core/lib/transport/transport_fwd.h" @@ -81,6 +83,59 @@ struct grpc_transport_stream_op_batch_payload; namespace grpc_core { +// Server metadata type +// TODO(ctiller): This should be a bespoke instance of MetadataMap<> +using ServerMetadata = grpc_metadata_batch; +using ServerMetadataHandle = Arena::PoolPtr; + +// Client initial metadata type +// TODO(ctiller): This should be a bespoke instance of MetadataMap<> +using ClientMetadata = grpc_metadata_batch; +using ClientMetadataHandle = Arena::PoolPtr; + +class Message { + public: + Message() = default; + ~Message() = default; + Message(SliceBuffer payload, uint32_t flags) + : payload_(std::move(payload)), flags_(flags) {} + Message(const Message&) = delete; + Message& operator=(const Message&) = delete; + + uint32_t flags() const { return flags_; } + SliceBuffer* payload() { return &payload_; } + const SliceBuffer* payload() const { return &payload_; } + + private: + SliceBuffer payload_; + uint32_t flags_ = 0; +}; + +using MessageHandle = Arena::PoolPtr; + +// Ok/not-ok check for trailing metadata, so that it can be used as result types +// for TrySeq. +inline bool IsStatusOk(const ServerMetadataHandle& m) { + return m->get(GrpcStatusMetadata()).value_or(GRPC_STATUS_UNKNOWN) == + GRPC_STATUS_OK; +} + +ServerMetadataHandle ServerMetadataFromStatus(const absl::Status& status); + +template <> +struct StatusCastImpl { + static ServerMetadataHandle Cast(const absl::Status& m) { + return ServerMetadataFromStatus(m); + } +}; + +template <> +struct StatusCastImpl { + static ServerMetadataHandle Cast(const absl::Status& m) { + return ServerMetadataFromStatus(m); + } +}; + struct CallArgs { // Initial metadata from the client to the server. // During promise setup this can be manipulated by filters (and then diff --git a/src/core/lib/transport/transport_impl.h b/src/core/lib/transport/transport_impl.h index 44e7c46ab86..fd586af7523 100644 --- a/src/core/lib/transport/transport_impl.h +++ b/src/core/lib/transport/transport_impl.h @@ -30,7 +30,6 @@ #include "src/core/lib/iomgr/iomgr_fwd.h" #include "src/core/lib/promise/arena_promise.h" #include "src/core/lib/resource_quota/arena.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/transport.h" #include "src/core/lib/transport/transport_fwd.h" diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py index b5fd68e4b87..208c545d1f6 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -709,7 +709,6 @@ CORE_SOURCE_FILES = [ 'src/core/lib/surface/validate_metadata.cc', 'src/core/lib/surface/version.cc', 'src/core/lib/transport/bdp_estimator.cc', - 'src/core/lib/transport/call_fragments.cc', 'src/core/lib/transport/connectivity_state.cc', 'src/core/lib/transport/error_utils.cc', 'src/core/lib/transport/handshaker.cc', diff --git a/test/core/filters/client_auth_filter_test.cc b/test/core/filters/client_auth_filter_test.cc index 6bfcb5a563e..54cbddb012f 100644 --- a/test/core/filters/client_auth_filter_test.cc +++ b/test/core/filters/client_auth_filter_test.cc @@ -49,7 +49,6 @@ #include "src/core/lib/security/security_connector/security_connector.h" #include "src/core/lib/security/transport/auth_filters.h" #include "src/core/lib/slice/slice.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/metadata_batch.h" #include "src/core/lib/transport/transport.h" #include "test/core/promise/test_context.h" @@ -154,13 +153,14 @@ TEST_F(ClientAuthFilterTest, CallCredsFails) { TestContext context(arena_.get()); TestContext promise_call_context(call_context_); auto promise = filter->MakeCallPromise( - CallArgs{ClientMetadataHandle::TestOnlyWrap(&initial_metadata_batch_), + CallArgs{ClientMetadataHandle(&initial_metadata_batch_, + Arena::PooledDeleter(nullptr)), nullptr, nullptr, nullptr}, [&](CallArgs /*call_args*/) { return ArenaPromise( [&]() -> Poll { - return ServerMetadataHandle::TestOnlyWrap( - &trailing_metadata_batch_); + return ServerMetadataHandle(&trailing_metadata_batch_, + Arena::PooledDeleter(nullptr)); }); }); auto result = promise(); @@ -174,7 +174,6 @@ TEST_F(ClientAuthFilterTest, CallCredsFails) { (*server_metadata)->get_pointer(GrpcMessageMetadata()); ASSERT_TRUE(message_md != nullptr); EXPECT_EQ(message_md->as_string_view(), "access denied"); - (*server_metadata)->~ServerMetadata(); } TEST_F(ClientAuthFilterTest, RewritesInvalidStatusFromCallCreds) { @@ -184,13 +183,14 @@ TEST_F(ClientAuthFilterTest, RewritesInvalidStatusFromCallCreds) { TestContext context(arena_.get()); TestContext promise_call_context(call_context_); auto promise = filter->MakeCallPromise( - CallArgs{ClientMetadataHandle::TestOnlyWrap(&initial_metadata_batch_), + CallArgs{ClientMetadataHandle(&initial_metadata_batch_, + Arena::PooledDeleter(nullptr)), nullptr, nullptr, nullptr}, [&](CallArgs /*call_args*/) { return ArenaPromise( [&]() -> Poll { - return ServerMetadataHandle::TestOnlyWrap( - &trailing_metadata_batch_); + return ServerMetadataHandle(&trailing_metadata_batch_, + Arena::PooledDeleter(nullptr)); }); }); auto result = promise(); @@ -206,7 +206,6 @@ TEST_F(ClientAuthFilterTest, RewritesInvalidStatusFromCallCreds) { EXPECT_EQ(message_md->as_string_view(), "Illegal status code from call credentials; original status: " "ABORTED: nope"); - (*server_metadata)->~ServerMetadata(); } } // namespace diff --git a/test/core/filters/client_authority_filter_test.cc b/test/core/filters/client_authority_filter_test.cc index fe21c37271a..b771a777c8e 100644 --- a/test/core/filters/client_authority_filter_test.cc +++ b/test/core/filters/client_authority_filter_test.cc @@ -71,7 +71,8 @@ TEST(ClientAuthorityFilterTest, PromiseCompletesImmediatelyAndSetsAuthority) { // TODO(ctiller): use Activity here, once it's ready. TestContext context(arena.get()); auto promise = filter.MakeCallPromise( - CallArgs{ClientMetadataHandle::TestOnlyWrap(&initial_metadata_batch), + CallArgs{ClientMetadataHandle(&initial_metadata_batch, + Arena::PooledDeleter(nullptr)), nullptr, nullptr, nullptr}, [&](CallArgs call_args) { EXPECT_EQ(call_args.client_initial_metadata @@ -81,8 +82,8 @@ TEST(ClientAuthorityFilterTest, PromiseCompletesImmediatelyAndSetsAuthority) { seen = true; return ArenaPromise( [&]() -> Poll { - return ServerMetadataHandle::TestOnlyWrap( - &trailing_metadata_batch); + return ServerMetadataHandle(&trailing_metadata_batch, + Arena::PooledDeleter(nullptr)); }); }); auto result = promise(); @@ -103,7 +104,8 @@ TEST(ClientAuthorityFilterTest, // TODO(ctiller): use Activity here, once it's ready. TestContext context(arena.get()); auto promise = filter.MakeCallPromise( - CallArgs{ClientMetadataHandle::TestOnlyWrap(&initial_metadata_batch), + CallArgs{ClientMetadataHandle(&initial_metadata_batch, + Arena::PooledDeleter(nullptr)), nullptr, nullptr, nullptr}, [&](CallArgs call_args) { EXPECT_EQ(call_args.client_initial_metadata @@ -113,8 +115,8 @@ TEST(ClientAuthorityFilterTest, seen = true; return ArenaPromise( [&]() -> Poll { - return ServerMetadataHandle::TestOnlyWrap( - &trailing_metadata_batch); + return ServerMetadataHandle(&trailing_metadata_batch, + Arena::PooledDeleter(nullptr)); }); }); auto result = promise(); diff --git a/test/core/filters/filter_fuzzer.cc b/test/core/filters/filter_fuzzer.cc index cf381c5eed3..6b075578199 100644 --- a/test/core/filters/filter_fuzzer.cc +++ b/test/core/filters/filter_fuzzer.cc @@ -79,7 +79,6 @@ #include "src/core/lib/security/transport/auth_filters.h" #include "src/core/lib/slice/slice.h" #include "src/core/lib/surface/channel_stack_type.h" -#include "src/core/lib/transport/call_fragments.h" #include "src/core/lib/transport/handshaker.h" #include "src/core/lib/transport/metadata_batch.h" #include "src/core/lib/transport/transport.h" @@ -586,7 +585,7 @@ class MainLoop { }; template - absl::optional> LoadMetadata( + absl::optional> LoadMetadata( const filter_fuzzer::Metadata& metadata, std::unique_ptr* out) { if (*out != nullptr) return absl::nullopt; *out = std::make_unique(arena_.get()); @@ -594,7 +593,7 @@ class MainLoop { (*out)->Append(md.key(), Slice::FromCopiedString(md.value()), [](absl::string_view, const Slice&) {}); } - return FragmentHandle::TestOnlyWrap(out->get()); + return Arena::PoolPtr(out->get(), Arena::PooledDeleter(nullptr)); } void Step() { @@ -608,8 +607,8 @@ class MainLoop { Poll CheckCompletion() { if (server_trailing_metadata_ != nullptr) { - return ServerMetadataHandle::TestOnlyWrap( - server_trailing_metadata_.get()); + return ServerMetadataHandle(server_trailing_metadata_.get(), + Arena::PooledDeleter(nullptr)); } server_trailing_metadata_waker_ = MakeOwningWaker(); return Pending{}; diff --git a/test/core/resource_quota/arena_test.cc b/test/core/resource_quota/arena_test.cc index 6cddd6ba82d..5495add760a 100644 --- a/test/core/resource_quota/arena_test.cc +++ b/test/core/resource_quota/arena_test.cc @@ -39,12 +39,12 @@ #include "src/core/lib/resource_quota/resource_quota.h" #include "test/core/util/test_config.h" -using grpc_core::Arena; -using grpc_core::ExecCtx; +namespace grpc_core { -static auto* g_memory_allocator = new grpc_core::MemoryAllocator( - grpc_core::ResourceQuota::Default()->memory_quota()->CreateMemoryAllocator( - "test")); +namespace { +auto* g_memory_allocator = new MemoryAllocator( + ResourceQuota::Default()->memory_quota()->CreateMemoryAllocator("test")); +} TEST(ArenaTest, NoOp) { ExecCtx exec_ctx; @@ -116,10 +116,10 @@ TEST(ArenaTest, ConcurrentAlloc) { gpr_event_init(&args.ev_start); args.arena = Arena::Create(1024, g_memory_allocator); - grpc_core::Thread thds[CONCURRENT_TEST_THREADS]; + Thread thds[CONCURRENT_TEST_THREADS]; for (int i = 0; i < CONCURRENT_TEST_THREADS; i++) { - thds[i] = grpc_core::Thread( + thds[i] = Thread( "grpc_concurrent_test", [](void* arg) { concurrent_test_args* a = static_cast(arg); @@ -146,10 +146,10 @@ TEST(ArenaTest, ConcurrentManagedNew) { gpr_event_init(&args.ev_start); args.arena = Arena::Create(1024, g_memory_allocator); - grpc_core::Thread thds[CONCURRENT_TEST_THREADS]; + Thread thds[CONCURRENT_TEST_THREADS]; for (int i = 0; i < CONCURRENT_TEST_THREADS; i++) { - thds[i] = grpc_core::Thread( + thds[i] = Thread( "grpc_concurrent_test", [](void* arg) { concurrent_test_args* a = static_cast(arg); @@ -172,6 +172,33 @@ TEST(ArenaTest, ConcurrentManagedNew) { args.arena->Destroy(); } +TEST(ArenaTest, PooledObjectsArePooled) { + struct TestObj { + char a[100]; + }; + + auto arena = MakeScopedArena(1024, g_memory_allocator); + auto obj = arena->MakePooled(); + void* p = obj.get(); + obj.reset(); + obj = arena->MakePooled(); + EXPECT_EQ(p, obj.get()); +} + +TEST(ArenaTest, CreateManyObjects) { + struct TestObj { + char a[100]; + }; + auto arena = MakeScopedArena(1024, g_memory_allocator); + std::vector> objs; + objs.reserve(1000); + for (int i = 0; i < 1000; i++) { + objs.emplace_back(arena->MakePooled()); + } +} + +} // namespace grpc_core + int main(int argc, char* argv[]) { grpc::testing::TestEnvironment give_me_a_name(&argc, argv); ::testing::InitGoogleTest(&argc, argv); diff --git a/test/core/security/credentials_test.cc b/test/core/security/credentials_test.cc index a51c215c8e9..527a89e1543 100644 --- a/test/core/security/credentials_test.cc +++ b/test/core/security/credentials_test.cc @@ -463,15 +463,16 @@ class RequestMetadataState : public RefCounted { md_.Set(HttpPathMetadata(), Slice::FromStaticString(path)); activity_ = MakeActivity( [this, creds] { - return Seq(creds->GetRequestMetadata( - ClientMetadataHandle::TestOnlyWrap(&md_), - &get_request_metadata_args_), - [this](absl::StatusOr metadata) { - if (metadata.ok()) { - GPR_ASSERT(metadata->get() == &md_); - } - return metadata.status(); - }); + return Seq( + creds->GetRequestMetadata( + ClientMetadataHandle(&md_, Arena::PooledDeleter(nullptr)), + &get_request_metadata_args_), + [this](absl::StatusOr metadata) { + if (metadata.ok()) { + GPR_ASSERT(metadata->get() == &md_); + } + return metadata.status(); + }); }, ExecCtxWakeupScheduler(), [self](absl::Status status) mutable { diff --git a/test/core/security/oauth2_utils.cc b/test/core/security/oauth2_utils.cc index d496d45cce4..1a990aa3597 100644 --- a/test/core/security/oauth2_utils.cc +++ b/test/core/security/oauth2_utils.cc @@ -59,8 +59,9 @@ char* grpc_test_fetch_oauth2_token_with_credentials( [creds, &initial_metadata, &get_request_metadata_args]() { return grpc_core::Map( creds->GetRequestMetadata( - grpc_core::ClientMetadataHandle::TestOnlyWrap( - &initial_metadata), + grpc_core::ClientMetadataHandle( + &initial_metadata, + grpc_core::Arena::PooledDeleter(nullptr)), &get_request_metadata_args), [](const absl::StatusOr& s) { return s.status(); diff --git a/test/core/transport/BUILD b/test/core/transport/BUILD index 1550a2472ee..142e22b2c83 100644 --- a/test/core/transport/BUILD +++ b/test/core/transport/BUILD @@ -65,21 +65,6 @@ grpc_cc_test( ], ) -grpc_cc_test( - name = "call_fragments_test", - srcs = ["call_fragments_test.cc"], - external_deps = [ - "gtest", - ], - language = "C++", - deps = [ - "//:gpr", - "//:grpc", - "//test/core/promise:test_context", - "//test/core/util:grpc_test_util", - ], -) - grpc_cc_test( name = "metadata_map_test", srcs = ["metadata_map_test.cc"], diff --git a/test/core/transport/call_fragments_test.cc b/test/core/transport/call_fragments_test.cc deleted file mode 100644 index d38e3a466ba..00000000000 --- a/test/core/transport/call_fragments_test.cc +++ /dev/null @@ -1,96 +0,0 @@ -// -// Copyright 2021 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 "src/core/lib/transport/call_fragments.h" - -#include -#include -#include - -#include "gmock/gmock.h" -#include "gtest/gtest.h" - -#include - -#include "src/core/lib/gprpp/ref_counted_ptr.h" -#include "src/core/lib/resource_quota/memory_quota.h" -#include "src/core/lib/resource_quota/resource_quota.h" -#include "test/core/promise/test_context.h" -#include "test/core/util/test_config.h" - -using testing::Each; - -namespace grpc_core { -namespace testing { - -class CallFragmentsTest : public ::testing::Test { - protected: - CallFragmentsTest() {} - ~CallFragmentsTest() override {} - - private: - MemoryAllocator memory_allocator_ = - ResourceQuota::Default()->memory_quota()->CreateMemoryAllocator("test"); - ScopedArenaPtr arena_ = MakeScopedArena(4096, &memory_allocator_); - FragmentAllocator fragment_allocator_; - - TestContext arena_context_{arena_.get()}; - TestContext fragment_allocator_context_{ - &fragment_allocator_}; -}; - -// Ensure test fixture can init/destroy successfully. -TEST_F(CallFragmentsTest, Nothing) {} - -// Ensure we can create/destroy some client metadata. -TEST_F(CallFragmentsTest, ClientMetadata) { - GetContext()->MakeClientMetadata(); -} - -// Ensure we can create/destroy some server metadata. -TEST_F(CallFragmentsTest, ServerMetadata) { - GetContext()->MakeServerMetadata(); -} - -// Ensure repeated allocation/deallocations reuse memory. -TEST_F(CallFragmentsTest, RepeatedAllocationsReuseMemory) { - void* p = GetContext()->MakeClientMetadata().get(); - void* q = GetContext()->MakeClientMetadata().get(); - EXPECT_EQ(p, q); -} - -// Ensure repeated allocation reinitializes. -TEST_F(CallFragmentsTest, RepeatedAllocationsReinitialize) { - std::vector addresses; - for (int i = 0; i < 4; i++) { - ClientMetadataHandle metadata = - GetContext()->MakeClientMetadata(); - EXPECT_EQ(metadata->get_pointer(HttpPathMetadata()), nullptr); - metadata->Set(HttpPathMetadata(), Slice::FromCopiedString("/")); - EXPECT_EQ(metadata->get_pointer(HttpPathMetadata())->as_string_view(), "/"); - addresses.push_back(metadata.get()); - } - EXPECT_THAT(addresses, Each(addresses[0])); -} - -} // namespace testing -} // namespace grpc_core - -int main(int argc, char** argv) { - testing::InitGoogleTest(&argc, argv); - grpc::testing::TestEnvironment env(&argc, argv); - return RUN_ALL_TESTS(); -}; diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index d31ecd6dde7..d9798161d26 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -2481,8 +2481,6 @@ src/core/lib/surface/validate_metadata.h \ src/core/lib/surface/version.cc \ src/core/lib/transport/bdp_estimator.cc \ src/core/lib/transport/bdp_estimator.h \ -src/core/lib/transport/call_fragments.cc \ -src/core/lib/transport/call_fragments.h \ src/core/lib/transport/connectivity_state.cc \ src/core/lib/transport/connectivity_state.h \ src/core/lib/transport/error_utils.cc \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index 0878596f609..f78238d8063 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -2274,8 +2274,6 @@ src/core/lib/surface/version.cc \ src/core/lib/transport/README.md \ src/core/lib/transport/bdp_estimator.cc \ src/core/lib/transport/bdp_estimator.h \ -src/core/lib/transport/call_fragments.cc \ -src/core/lib/transport/call_fragments.h \ src/core/lib/transport/connectivity_state.cc \ src/core/lib/transport/connectivity_state.h \ src/core/lib/transport/error_utils.cc \ diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index ed23fb8d39d..fc286d3d5c7 100644 --- a/tools/run_tests/generated/tests.json +++ b/tools/run_tests/generated/tests.json @@ -1415,30 +1415,6 @@ ], "uses_polling": true }, - { - "args": [], - "benchmark": false, - "ci_platforms": [ - "linux", - "mac", - "posix", - "windows" - ], - "cpu_cost": 1.0, - "exclude_configs": [], - "exclude_iomgrs": [], - "flaky": false, - "gtest": true, - "language": "c++", - "name": "call_fragments_test", - "platforms": [ - "linux", - "mac", - "posix", - "windows" - ], - "uses_polling": true - }, { "args": [], "benchmark": false,