From 03bf69960024930edf6160f46b1f00f5d636e25c Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Tue, 18 Jan 2022 15:44:18 +0100 Subject: [PATCH] Revert "Eliminate slice interning (#28363)" (#28598) This reverts commit 6703186b7a18f4c5cf71eb3eaef47e514c946f08. --- BUILD | 2 + CMakeLists.txt | 30 ++ Makefile | 2 + build_autogenerated.yaml | 23 ++ config.m4 | 1 + config.w32 | 1 + gRPC-C++.podspec | 2 + gRPC-Core.podspec | 3 + grpc.def | 4 + grpc.gemspec | 2 + grpc.gyp | 2 + include/grpc/impl/codegen/slice.h | 5 +- include/grpc/slice.h | 11 + package.xml | 2 + .../binder/transport/binder_transport.cc | 2 +- .../chttp2/transport/hpack_parser.cc | 66 ++++- .../ext/transport/chttp2/transport/parsing.cc | 2 +- .../cronet/transport/cronet_transport.cc | 7 +- src/core/ext/xds/xds_api.cc | 2 +- src/core/ext/xds/xds_server_config_fetcher.cc | 2 +- src/core/lib/compression/compression.cc | 2 +- .../lib/compression/compression_internal.cc | 2 +- src/core/lib/event_engine/memory_allocator.cc | 22 +- src/core/lib/iomgr/error.cc | 1 + .../security/authorization/evaluate_args.cc | 2 +- .../file_external_account_credentials.cc | 1 + src/core/lib/slice/slice.cc | 245 +++++++++++----- src/core/lib/slice/slice.h | 43 +-- src/core/lib/slice/slice_intern.cc | 269 ++++++++++++++++++ src/core/lib/slice/slice_internal.h | 42 ++- src/core/lib/slice/slice_refcount.cc | 16 -- src/core/lib/slice/slice_refcount.h | 74 ++++- src/core/lib/slice/slice_refcount_base.h | 136 +++++++-- src/core/lib/slice/slice_utils.h | 200 +++++++++++++ src/core/lib/surface/call.cc | 2 +- src/core/lib/surface/init.cc | 2 + src/core/lib/surface/server.cc | 24 +- src/core/lib/surface/server.h | 4 +- src/cpp/ext/filters/census/client_filter.cc | 6 +- src/cpp/ext/filters/census/server_filter.cc | 2 +- src/python/grpcio/grpc_core_dependencies.py | 1 + src/ruby/ext/grpc/rb_grpc_imports.generated.c | 8 + src/ruby/ext/grpc/rb_grpc_imports.generated.h | 12 + test/core/end2end/fuzzers/api_fuzzer.cc | 5 + test/core/end2end/fuzzers/api_fuzzer.proto | 6 +- test/core/slice/BUILD | 12 + test/core/slice/slice_intern_test.cc | 67 +++++ test/core/slice/slice_test.cc | 7 +- .../core/surface/public_headers_must_be_c89.c | 4 + .../transport/chttp2/hpack_encoder_test.cc | 9 +- test/core/util/evaluate_args_test_util.h | 11 +- test/cpp/end2end/channelz_service_test.cc | 2 +- tools/doxygen/Doxyfile.c++.internal | 2 + tools/doxygen/Doxyfile.core.internal | 2 + tools/run_tests/generated/tests.json | 24 ++ 55 files changed, 1232 insertions(+), 206 deletions(-) create mode 100644 src/core/lib/slice/slice_intern.cc create mode 100644 src/core/lib/slice/slice_utils.h create mode 100644 test/core/slice/slice_intern_test.cc diff --git a/BUILD b/BUILD index 072de8454b1..023b1cede4b 100644 --- a/BUILD +++ b/BUILD @@ -1573,6 +1573,7 @@ grpc_cc_library( hdrs = [ "src/core/lib/slice/slice_refcount.h", "src/core/lib/slice/slice_refcount_base.h", + "src/core/lib/slice/slice_utils.h", ], public_hdrs = [ "include/grpc/slice.h", @@ -1886,6 +1887,7 @@ grpc_cc_library( "src/core/lib/slice/percent_encoding.cc", "src/core/lib/slice/slice_api.cc", "src/core/lib/slice/slice_buffer.cc", + "src/core/lib/slice/slice_intern.cc", "src/core/lib/slice/slice_split.cc", "src/core/lib/surface/api_trace.cc", "src/core/lib/surface/builtins.cc", diff --git a/CMakeLists.txt b/CMakeLists.txt index 693e06ba9a6..d5f00c92555 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -735,6 +735,7 @@ if(gRPC_BUILD_TESTS) endif() add_dependencies(buildtests_c server_test) add_dependencies(buildtests_c slice_buffer_test) + add_dependencies(buildtests_c slice_intern_test) add_dependencies(buildtests_c slice_split_test) add_dependencies(buildtests_c slice_string_helpers_test) add_dependencies(buildtests_c sockaddr_resolver_test) @@ -2081,6 +2082,7 @@ add_library(grpc src/core/lib/slice/slice.cc src/core/lib/slice/slice_api.cc src/core/lib/slice/slice_buffer.cc + src/core/lib/slice/slice_intern.cc src/core/lib/slice/slice_refcount.cc src/core/lib/slice/slice_split.cc src/core/lib/slice/slice_string_helpers.cc @@ -2665,6 +2667,7 @@ add_library(grpc_unsecure src/core/lib/slice/slice.cc src/core/lib/slice/slice_api.cc src/core/lib/slice/slice_buffer.cc + src/core/lib/slice/slice_intern.cc src/core/lib/slice/slice_refcount.cc src/core/lib/slice/slice_split.cc src/core/lib/slice/slice_string_helpers.cc @@ -6703,6 +6706,33 @@ target_link_libraries(slice_buffer_test ) +endif() +if(gRPC_BUILD_TESTS) + +add_executable(slice_intern_test + test/core/slice/slice_intern_test.cc +) + +target_include_directories(slice_intern_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} +) + +target_link_libraries(slice_intern_test + ${_gRPC_ALLTARGETS_LIBRARIES} + grpc_test_util +) + + endif() if(gRPC_BUILD_TESTS) diff --git a/Makefile b/Makefile index 46b9da7cf0d..52e7383c83d 100644 --- a/Makefile +++ b/Makefile @@ -1566,6 +1566,7 @@ LIBGRPC_SRC = \ src/core/lib/slice/slice.cc \ src/core/lib/slice/slice_api.cc \ src/core/lib/slice/slice_buffer.cc \ + src/core/lib/slice/slice_intern.cc \ src/core/lib/slice/slice_refcount.cc \ src/core/lib/slice/slice_split.cc \ src/core/lib/slice/slice_string_helpers.cc \ @@ -1997,6 +1998,7 @@ LIBGRPC_UNSECURE_SRC = \ src/core/lib/slice/slice.cc \ src/core/lib/slice/slice_api.cc \ src/core/lib/slice/slice_buffer.cc \ + src/core/lib/slice/slice_intern.cc \ src/core/lib/slice/slice_refcount.cc \ src/core/lib/slice/slice_split.cc \ src/core/lib/slice/slice_string_helpers.cc \ diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 079e2e84515..70f07694113 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -932,6 +932,7 @@ libs: - src/core/lib/slice/slice_refcount_base.h - src/core/lib/slice/slice_split.h - src/core/lib/slice/slice_string_helpers.h + - src/core/lib/slice/slice_utils.h - src/core/lib/surface/api_trace.h - src/core/lib/surface/builtins.h - src/core/lib/surface/call.h @@ -1532,6 +1533,7 @@ libs: - src/core/lib/slice/slice.cc - src/core/lib/slice/slice_api.cc - src/core/lib/slice/slice_buffer.cc + - src/core/lib/slice/slice_intern.cc - src/core/lib/slice/slice_refcount.cc - src/core/lib/slice/slice_split.cc - src/core/lib/slice/slice_string_helpers.cc @@ -1987,6 +1989,7 @@ libs: - src/core/lib/slice/slice_refcount_base.h - src/core/lib/slice/slice_split.h - src/core/lib/slice/slice_string_helpers.h + - src/core/lib/slice/slice_utils.h - src/core/lib/surface/api_trace.h - src/core/lib/surface/builtins.h - src/core/lib/surface/call.h @@ -2263,6 +2266,7 @@ libs: - src/core/lib/slice/slice.cc - src/core/lib/slice/slice_api.cc - src/core/lib/slice/slice_buffer.cc + - src/core/lib/slice/slice_intern.cc - src/core/lib/slice/slice_refcount.cc - src/core/lib/slice/slice_split.cc - src/core/lib/slice/slice_string_helpers.cc @@ -3764,6 +3768,7 @@ targets: - src/core/lib/slice/slice_refcount.h - src/core/lib/slice/slice_refcount_base.h - src/core/lib/slice/slice_string_helpers.h + - src/core/lib/slice/slice_utils.h src: - src/core/lib/debug/trace.cc - src/core/lib/event_engine/memory_allocator.cc @@ -4014,6 +4019,15 @@ targets: deps: - grpc_test_util uses_polling: false +- name: slice_intern_test + build: test + language: c + headers: [] + src: + - test/core/slice/slice_intern_test.cc + deps: + - grpc_test_util + uses_polling: false - name: slice_split_test build: test language: c @@ -4036,6 +4050,7 @@ targets: - src/core/lib/slice/slice_refcount.h - src/core/lib/slice/slice_refcount_base.h - src/core/lib/slice/slice_string_helpers.h + - src/core/lib/slice/slice_utils.h src: - src/core/lib/debug/trace.cc - src/core/lib/slice/slice.cc @@ -4513,6 +4528,7 @@ targets: - src/core/lib/slice/slice_refcount.h - src/core/lib/slice/slice_refcount_base.h - src/core/lib/slice/slice_string_helpers.h + - src/core/lib/slice/slice_utils.h src: - src/core/lib/debug/trace.cc - src/core/lib/event_engine/memory_allocator.cc @@ -5005,6 +5021,7 @@ targets: - src/core/lib/slice/slice_refcount.h - src/core/lib/slice/slice_refcount_base.h - src/core/lib/slice/slice_string_helpers.h + - src/core/lib/slice/slice_utils.h src: - src/core/lib/debug/trace.cc - src/core/lib/event_engine/memory_allocator.cc @@ -5637,6 +5654,7 @@ targets: - src/core/lib/slice/slice_refcount.h - src/core/lib/slice/slice_refcount_base.h - src/core/lib/slice/slice_string_helpers.h + - src/core/lib/slice/slice_utils.h src: - src/core/lib/debug/trace.cc - src/core/lib/iomgr/combiner.cc @@ -5846,6 +5864,7 @@ targets: - src/core/lib/slice/slice_refcount.h - src/core/lib/slice/slice_refcount_base.h - src/core/lib/slice/slice_string_helpers.h + - src/core/lib/slice/slice_utils.h - test/core/promise/test_wakeup_schedulers.h src: - src/core/lib/debug/trace.cc @@ -6610,6 +6629,7 @@ targets: - src/core/lib/slice/slice_refcount.h - src/core/lib/slice/slice_refcount_base.h - src/core/lib/slice/slice_string_helpers.h + - src/core/lib/slice/slice_utils.h - test/core/resource_quota/call_checker.h src: - src/core/lib/debug/trace.cc @@ -6915,6 +6935,7 @@ targets: - src/core/lib/slice/slice_refcount.h - src/core/lib/slice/slice_refcount_base.h - src/core/lib/slice/slice_string_helpers.h + - src/core/lib/slice/slice_utils.h - test/core/promise/test_wakeup_schedulers.h src: - src/core/lib/debug/trace.cc @@ -7267,6 +7288,7 @@ targets: - src/core/lib/slice/slice_refcount.h - src/core/lib/slice/slice_refcount_base.h - src/core/lib/slice/slice_string_helpers.h + - src/core/lib/slice/slice_utils.h src: - src/core/lib/debug/trace.cc - src/core/lib/event_engine/memory_allocator.cc @@ -7703,6 +7725,7 @@ targets: - src/core/lib/slice/slice_refcount.h - src/core/lib/slice/slice_refcount_base.h - src/core/lib/slice/slice_string_helpers.h + - src/core/lib/slice/slice_utils.h - test/core/util/build.h src: - src/core/lib/debug/trace.cc diff --git a/config.m4 b/config.m4 index fde499331f5..e15fa572fe1 100644 --- a/config.m4 +++ b/config.m4 @@ -628,6 +628,7 @@ if test "$PHP_GRPC" != "no"; then src/core/lib/slice/slice.cc \ src/core/lib/slice/slice_api.cc \ src/core/lib/slice/slice_buffer.cc \ + src/core/lib/slice/slice_intern.cc \ src/core/lib/slice/slice_refcount.cc \ src/core/lib/slice/slice_split.cc \ src/core/lib/slice/slice_string_helpers.cc \ diff --git a/config.w32 b/config.w32 index 7a7b2bae78e..0c88a5e6fe8 100644 --- a/config.w32 +++ b/config.w32 @@ -594,6 +594,7 @@ if (PHP_GRPC != "no") { "src\\core\\lib\\slice\\slice.cc " + "src\\core\\lib\\slice\\slice_api.cc " + "src\\core\\lib\\slice\\slice_buffer.cc " + + "src\\core\\lib\\slice\\slice_intern.cc " + "src\\core\\lib\\slice\\slice_refcount.cc " + "src\\core\\lib\\slice\\slice_split.cc " + "src\\core\\lib\\slice\\slice_string_helpers.cc " + diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index 2c5a88da891..91a954408b3 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -805,6 +805,7 @@ Pod::Spec.new do |s| 'src/core/lib/slice/slice_refcount_base.h', 'src/core/lib/slice/slice_split.h', 'src/core/lib/slice/slice_string_helpers.h', + 'src/core/lib/slice/slice_utils.h', 'src/core/lib/surface/api_trace.h', 'src/core/lib/surface/builtins.h', 'src/core/lib/surface/call.h', @@ -1537,6 +1538,7 @@ Pod::Spec.new do |s| 'src/core/lib/slice/slice_refcount_base.h', 'src/core/lib/slice/slice_split.h', 'src/core/lib/slice/slice_string_helpers.h', + 'src/core/lib/slice/slice_utils.h', 'src/core/lib/surface/api_trace.h', 'src/core/lib/surface/builtins.h', 'src/core/lib/surface/call.h', diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index cd7e15ff575..4c2980404a2 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -1327,6 +1327,7 @@ Pod::Spec.new do |s| 'src/core/lib/slice/slice.h', 'src/core/lib/slice/slice_api.cc', 'src/core/lib/slice/slice_buffer.cc', + 'src/core/lib/slice/slice_intern.cc', 'src/core/lib/slice/slice_internal.h', 'src/core/lib/slice/slice_refcount.cc', 'src/core/lib/slice/slice_refcount.h', @@ -1335,6 +1336,7 @@ Pod::Spec.new do |s| 'src/core/lib/slice/slice_split.h', 'src/core/lib/slice/slice_string_helpers.cc', 'src/core/lib/slice/slice_string_helpers.h', + 'src/core/lib/slice/slice_utils.h', 'src/core/lib/surface/api_trace.cc', 'src/core/lib/surface/api_trace.h', 'src/core/lib/surface/builtins.cc', @@ -2078,6 +2080,7 @@ Pod::Spec.new do |s| 'src/core/lib/slice/slice_refcount_base.h', 'src/core/lib/slice/slice_split.h', 'src/core/lib/slice/slice_string_helpers.h', + 'src/core/lib/slice/slice_utils.h', 'src/core/lib/surface/api_trace.h', 'src/core/lib/surface/builtins.h', 'src/core/lib/surface/call.h', diff --git a/grpc.def b/grpc.def index a6959b94fb3..58979d814b9 100644 --- a/grpc.def +++ b/grpc.def @@ -189,6 +189,7 @@ EXPORTS grpc_slice_new_with_len grpc_slice_malloc grpc_slice_malloc_large + grpc_slice_intern grpc_slice_from_copied_string grpc_slice_from_copied_buffer grpc_slice_from_static_string @@ -199,6 +200,8 @@ EXPORTS grpc_slice_split_tail_maybe_ref grpc_slice_split_head grpc_empty_slice + grpc_slice_default_hash_impl + grpc_slice_default_eq_impl grpc_slice_eq grpc_slice_cmp grpc_slice_str_cmp @@ -206,6 +209,7 @@ EXPORTS grpc_slice_rchr grpc_slice_chr grpc_slice_slice + grpc_slice_hash grpc_slice_is_equivalent grpc_slice_dup grpc_slice_to_c_string diff --git a/grpc.gemspec b/grpc.gemspec index ec09d055143..154971f7d8e 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -1246,6 +1246,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/slice/slice.h ) s.files += %w( src/core/lib/slice/slice_api.cc ) s.files += %w( src/core/lib/slice/slice_buffer.cc ) + s.files += %w( src/core/lib/slice/slice_intern.cc ) s.files += %w( src/core/lib/slice/slice_internal.h ) s.files += %w( src/core/lib/slice/slice_refcount.cc ) s.files += %w( src/core/lib/slice/slice_refcount.h ) @@ -1254,6 +1255,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/slice/slice_split.h ) s.files += %w( src/core/lib/slice/slice_string_helpers.cc ) s.files += %w( src/core/lib/slice/slice_string_helpers.h ) + s.files += %w( src/core/lib/slice/slice_utils.h ) s.files += %w( src/core/lib/surface/api_trace.cc ) s.files += %w( src/core/lib/surface/api_trace.h ) s.files += %w( src/core/lib/surface/builtins.cc ) diff --git a/grpc.gyp b/grpc.gyp index bd9440dc29d..abd8ed7d8e2 100644 --- a/grpc.gyp +++ b/grpc.gyp @@ -1014,6 +1014,7 @@ 'src/core/lib/slice/slice.cc', 'src/core/lib/slice/slice_api.cc', 'src/core/lib/slice/slice_buffer.cc', + 'src/core/lib/slice/slice_intern.cc', 'src/core/lib/slice/slice_refcount.cc', 'src/core/lib/slice/slice_split.cc', 'src/core/lib/slice/slice_string_helpers.cc', @@ -1417,6 +1418,7 @@ 'src/core/lib/slice/slice.cc', 'src/core/lib/slice/slice_api.cc', 'src/core/lib/slice/slice_buffer.cc', + 'src/core/lib/slice/slice_intern.cc', 'src/core/lib/slice/slice_refcount.cc', 'src/core/lib/slice/slice_split.cc', 'src/core/lib/slice/slice_string_helpers.cc', diff --git a/include/grpc/impl/codegen/slice.h b/include/grpc/impl/codegen/slice.h index 130e5efffd2..4412058104e 100644 --- a/include/grpc/impl/codegen/slice.h +++ b/include/grpc/impl/codegen/slice.h @@ -58,10 +58,7 @@ struct grpc_slice_refcount; Multiple grpc_slice values may share a ref count. If the slice does not have a refcount, it represents an inlined small piece - of data that is copied by value. - - As a special case, a slice can be given refcount == uintptr_t(1), meaning - that the slice represents external data that is not refcounted. */ + of data that is copied by value. */ struct grpc_slice { struct grpc_slice_refcount* refcount; union grpc_slice_data { diff --git a/include/grpc/slice.h b/include/grpc/slice.h index 15978fe54c5..65d20878831 100644 --- a/include/grpc/slice.h +++ b/include/grpc/slice.h @@ -69,6 +69,12 @@ GPRAPI grpc_slice grpc_slice_malloc_large(size_t length); #define GRPC_SLICE_MALLOC(len) grpc_slice_malloc(len) +/** Intern a slice: + + The return value for two invocations of this function with the same sequence + of bytes is a slice which points to the same memory. */ +GPRAPI grpc_slice grpc_slice_intern(grpc_slice slice); + /** Create a slice by copying a string. Does not preserve null terminators. Equivalent to: @@ -123,6 +129,9 @@ GPRAPI grpc_slice grpc_slice_split_head(grpc_slice* s, size_t split); GPRAPI grpc_slice grpc_empty_slice(void); +GPRAPI uint32_t grpc_slice_default_hash_impl(grpc_slice s); +GPRAPI int grpc_slice_default_eq_impl(grpc_slice a, grpc_slice b); + GPRAPI int grpc_slice_eq(grpc_slice a, grpc_slice b); /** Returns <0 if a < b, ==0 if a == b, >0 if a > b @@ -142,6 +151,8 @@ GPRAPI int grpc_slice_chr(grpc_slice s, char c); if it's not found */ GPRAPI int grpc_slice_slice(grpc_slice haystack, grpc_slice needle); +GPRAPI uint32_t grpc_slice_hash(grpc_slice s); + /** Do two slices point at the same memory, with the same length If a or b is inlined, actually compares data */ GPRAPI int grpc_slice_is_equivalent(grpc_slice a, grpc_slice b); diff --git a/package.xml b/package.xml index 8da5915d5b1..9090ab25a1a 100644 --- a/package.xml +++ b/package.xml @@ -1226,6 +1226,7 @@ + @@ -1234,6 +1235,7 @@ + diff --git a/src/core/ext/transport/binder/transport/binder_transport.cc b/src/core/ext/transport/binder/transport/binder_transport.cc index 3206bec8e7f..964d6bef881 100644 --- a/src/core/ext/transport/binder/transport/binder_transport.cc +++ b/src/core/ext/transport/binder/transport/binder_transport.cc @@ -36,7 +36,7 @@ #include "src/core/ext/transport/binder/wire_format/wire_reader_impl.h" #include "src/core/ext/transport/binder/wire_format/wire_writer.h" #include "src/core/lib/iomgr/exec_ctx.h" -#include "src/core/lib/slice/slice_internal.h" +#include "src/core/lib/slice/slice_utils.h" #include "src/core/lib/transport/byte_stream.h" #include "src/core/lib/transport/error_utils.h" #include "src/core/lib/transport/metadata_batch.h" diff --git a/src/core/ext/transport/chttp2/transport/hpack_parser.cc b/src/core/ext/transport/chttp2/transport/hpack_parser.cc index cb17759bba5..18abbdacde7 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_parser.cc +++ b/src/core/ext/transport/chttp2/transport/hpack_parser.cc @@ -647,6 +647,17 @@ class HPackParser::Input { // Helper to parse a string and turn it into a slice with appropriate memory // management characteristics class HPackParser::String { + public: + // Helper to specify a string should be internalized + struct Intern {}; + // Helper to specify a string should be externalized + struct Extern {}; + + private: + // Forward declare take functions... we'll need them in the public interface + Slice Take(Extern); + Slice Take(Intern); + public: String(const String&) = delete; String& operator=(const String&) = delete; @@ -660,7 +671,11 @@ class HPackParser::String { } // Take the value and leave this empty - Slice Take(); + // Use Intern/Extern to choose memory management + template + auto Take() -> decltype(this->Take(T())) { + return Take(T()); + } // Return a reference to the value as a string view absl::string_view string_view() const { @@ -947,11 +962,13 @@ class HPackParser::Parser { case 1: switch (cur & 0xf) { case 0: // literal key - return FinishHeaderOmitFromTable(ParseLiteralKey()); + return FinishHeaderOmitFromTable(ParseLiteralKey()); case 0xf: // varint encoded key index - return FinishHeaderOmitFromTable(ParseVarIdxKey(0xf)); + return FinishHeaderOmitFromTable( + ParseVarIdxKey(0xf)); default: // inline encoded key index - return FinishHeaderOmitFromTable(ParseIdxKey(cur & 0xf)); + return FinishHeaderOmitFromTable( + ParseIdxKey(cur & 0xf)); } // Update max table size. // First byte format: 001xxxxx @@ -978,20 +995,23 @@ class HPackParser::Parser { case 4: if (cur == 0x40) { // literal key - return FinishHeaderAndAddToTable(ParseLiteralKey()); + return FinishHeaderAndAddToTable(ParseLiteralKey()); } ABSL_FALLTHROUGH_INTENDED; case 5: case 6: // inline encoded key index - return FinishHeaderAndAddToTable(ParseIdxKey(cur & 0x3f)); + return FinishHeaderAndAddToTable( + ParseIdxKey(cur & 0x3f)); case 7: if (cur == 0x7f) { // varint encoded key index - return FinishHeaderAndAddToTable(ParseVarIdxKey(0x3f)); + return FinishHeaderAndAddToTable( + ParseVarIdxKey(0x3f)); } else { // inline encoded key index - return FinishHeaderAndAddToTable(ParseIdxKey(cur & 0x3f)); + return FinishHeaderAndAddToTable( + ParseIdxKey(cur & 0x3f)); } // Indexed Header Field Representation // First byte format: 1xxxxxxx @@ -1093,6 +1113,7 @@ class HPackParser::Parser { } // Parse a string encoded key and a string encoded value + template absl::optional ParseLiteralKey() { auto key = String::Parse(input_); if (!key.has_value()) return {}; @@ -1101,7 +1122,7 @@ class HPackParser::Parser { return {}; } auto key_string = key->string_view(); - auto value_slice = value->Take(); + auto value_slice = value->Take(); const auto transport_size = key_string.size() + value_slice.size() + hpack_constants::kEntryOverhead; return grpc_metadata_batch::Parse( @@ -1112,6 +1133,7 @@ class HPackParser::Parser { } // Parse an index encoded key and a string encoded value + template absl::optional ParseIdxKey(uint32_t index) { const auto* elem = table_->Lookup(index); if (GPR_UNLIKELY(elem == nullptr)) { @@ -1120,17 +1142,19 @@ class HPackParser::Parser { } auto value = ParseValueString(elem->is_binary_header()); if (GPR_UNLIKELY(!value.has_value())) return {}; - return elem->WithNewValue( - value->Take(), [=](absl::string_view error, const Slice& value) { - ReportMetadataParseError(elem->key(), error, value.as_string_view()); - }); + return elem->WithNewValue(value->Take(), + [=](absl::string_view error, const Slice& value) { + ReportMetadataParseError( + elem->key(), error, value.as_string_view()); + }); } // Parse a varint index encoded key and a string encoded value + template absl::optional ParseVarIdxKey(uint32_t offset) { auto index = input_->ParseVarint(offset); if (GPR_UNLIKELY(!index.has_value())) return {}; - return ParseIdxKey(*index); + return ParseIdxKey(*index); } // Parse a string, figuring out if it's binary or not by the key name. @@ -1226,7 +1250,7 @@ class HPackParser::Parser { const LogInfo log_info_; }; -Slice HPackParser::String::Take() { +Slice HPackParser::String::Take(Extern) { if (auto* p = absl::get_if(&value_)) { return p->Copy(); } else if (auto* p = absl::get_if>(&value_)) { @@ -1237,6 +1261,18 @@ Slice HPackParser::String::Take() { GPR_UNREACHABLE_CODE(return Slice()); } +Slice HPackParser::String::Take(Intern) { + ManagedMemorySlice m; + if (auto* p = absl::get_if(&value_)) { + m = ManagedMemorySlice(&p->c_slice()); + } else if (auto* p = absl::get_if>(&value_)) { + m = ManagedMemorySlice(reinterpret_cast(p->data()), p->size()); + } else if (auto* p = absl::get_if>(&value_)) { + m = ManagedMemorySlice(reinterpret_cast(p->data()), p->size()); + } + return Slice(m); +} + /* PUBLIC INTERFACE */ HPackParser::HPackParser() = default; diff --git a/src/core/ext/transport/chttp2/transport/parsing.cc b/src/core/ext/transport/chttp2/transport/parsing.cc index d05f27b98c5..63dbd8e5824 100644 --- a/src/core/ext/transport/chttp2/transport/parsing.cc +++ b/src/core/ext/transport/chttp2/transport/parsing.cc @@ -28,8 +28,8 @@ #include "src/core/ext/transport/chttp2/transport/internal.h" #include "src/core/lib/profiling/timers.h" -#include "src/core/lib/slice/slice_internal.h" #include "src/core/lib/slice/slice_string_helpers.h" +#include "src/core/lib/slice/slice_utils.h" #include "src/core/lib/transport/http2_errors.h" #include "src/core/lib/transport/status_conversion.h" #include "src/core/lib/transport/timeout_encoding.h" diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.cc b/src/core/ext/transport/cronet/transport/cronet_transport.cc index f25d8e0f5a7..e82217e0d44 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.cc +++ b/src/core/ext/transport/cronet/transport/cronet_transport.cc @@ -410,10 +410,11 @@ static void convert_cronet_array_to_metadata( grpc_slice value; if (absl::EndsWith(header_array->headers[i].key, "-bin")) { value = grpc_slice_from_static_string(header_array->headers[i].value); - value = grpc_chttp2_base64_decode_with_length( - value, grpc_chttp2_base64_infer_length_after_decode(value)); + value = grpc_slice_intern(grpc_chttp2_base64_decode_with_length( + value, grpc_chttp2_base64_infer_length_after_decode(value))); } else { - value = grpc_slice_from_static_string(header_array->headers[i].value); + value = grpc_slice_intern( + grpc_slice_from_static_string(header_array->headers[i].value)); } mds->Append(header_array->headers[i].key, grpc_core::Slice(value), [&](absl::string_view error, const grpc_core::Slice& value) { diff --git a/src/core/ext/xds/xds_api.cc b/src/core/ext/xds/xds_api.cc index d9cc5b8828a..6f885e18b4e 100644 --- a/src/core/ext/xds/xds_api.cc +++ b/src/core/ext/xds/xds_api.cc @@ -57,7 +57,7 @@ #include "src/core/lib/iomgr/error.h" #include "src/core/lib/iomgr/sockaddr.h" #include "src/core/lib/iomgr/socket_utils.h" -#include "src/core/lib/slice/slice_internal.h" +#include "src/core/lib/slice/slice_utils.h" #include "src/core/lib/uri/uri_parser.h" namespace grpc_core { diff --git a/src/core/ext/xds/xds_server_config_fetcher.cc b/src/core/ext/xds/xds_server_config_fetcher.cc index e78c35e4a24..699ba1d2549 100644 --- a/src/core/ext/xds/xds_server_config_fetcher.cc +++ b/src/core/ext/xds/xds_server_config_fetcher.cc @@ -37,7 +37,7 @@ #include "src/core/lib/iomgr/sockaddr.h" #include "src/core/lib/iomgr/socket_utils.h" #include "src/core/lib/security/credentials/xds/xds_credentials.h" -#include "src/core/lib/slice/slice_internal.h" +#include "src/core/lib/slice/slice_utils.h" #include "src/core/lib/surface/api_trace.h" #include "src/core/lib/surface/server.h" #include "src/core/lib/transport/error_utils.h" diff --git a/src/core/lib/compression/compression.cc b/src/core/lib/compression/compression.cc index ce39d7e8343..ba86dbd48ac 100644 --- a/src/core/lib/compression/compression.cc +++ b/src/core/lib/compression/compression.cc @@ -25,7 +25,7 @@ #include "src/core/lib/compression/compression_internal.h" #include "src/core/lib/gpr/useful.h" -#include "src/core/lib/slice/slice_internal.h" +#include "src/core/lib/slice/slice_utils.h" #include "src/core/lib/surface/api_trace.h" int grpc_compression_algorithm_is_message(grpc_compression_algorithm) { diff --git a/src/core/lib/compression/compression_internal.cc b/src/core/lib/compression/compression_internal.cc index 01dd6c75218..3ca36af31b8 100644 --- a/src/core/lib/compression/compression_internal.cc +++ b/src/core/lib/compression/compression_internal.cc @@ -33,7 +33,7 @@ #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/gpr/useful.h" -#include "src/core/lib/slice/slice_internal.h" +#include "src/core/lib/slice/slice_utils.h" #include "src/core/lib/surface/api_trace.h" namespace grpc_core { diff --git a/src/core/lib/event_engine/memory_allocator.cc b/src/core/lib/event_engine/memory_allocator.cc index afb056ecd7b..8b6994e76d8 100644 --- a/src/core/lib/event_engine/memory_allocator.cc +++ b/src/core/lib/event_engine/memory_allocator.cc @@ -26,24 +26,28 @@ namespace { // Reference count for a slice allocated by MemoryAllocator::MakeSlice. // Takes care of releasing memory back when the slice is destroyed. -class SliceRefCount : public grpc_slice_refcount { +class SliceRefCount { public: + static void Destroy(void* p) { + auto* rc = static_cast(p); + rc->~SliceRefCount(); + gpr_free(rc); + } SliceRefCount(std::shared_ptr allocator, size_t size) - : grpc_slice_refcount(Destroy), + : base_(grpc_slice_refcount::Type::REGULAR, &refs_, Destroy, this, + &base_), allocator_(std::move(allocator)), size_(size) { // Nothing to do here. } ~SliceRefCount() { allocator_->Release(size_); } - private: - static void Destroy(grpc_slice_refcount* p) { - auto* rc = static_cast(p); - rc->~SliceRefCount(); - gpr_free(rc); - } + grpc_slice_refcount* base_refcount() { return &base_; } + private: + grpc_slice_refcount base_; + std::atomic refs_{1}; std::shared_ptr allocator_; size_t size_; }; @@ -55,7 +59,7 @@ grpc_slice MemoryAllocator::MakeSlice(MemoryRequest request) { void* p = gpr_malloc(size); new (p) SliceRefCount(allocator_, size); grpc_slice slice; - slice.refcount = static_cast(p); + slice.refcount = static_cast(p)->base_refcount(); slice.data.refcounted.bytes = static_cast(p) + sizeof(SliceRefCount); slice.data.refcounted.length = size - sizeof(SliceRefCount); diff --git a/src/core/lib/iomgr/error.cc b/src/core/lib/iomgr/error.cc index a442f88f62e..f5147c62aac 100644 --- a/src/core/lib/iomgr/error.cc +++ b/src/core/lib/iomgr/error.cc @@ -35,6 +35,7 @@ #include "src/core/lib/gpr/useful.h" #include "src/core/lib/iomgr/error_internal.h" #include "src/core/lib/slice/slice_internal.h" +#include "src/core/lib/slice/slice_utils.h" grpc_core::DebugOnlyTraceFlag grpc_trace_error_refcount(false, "error_refcount"); diff --git a/src/core/lib/security/authorization/evaluate_args.cc b/src/core/lib/security/authorization/evaluate_args.cc index 1571d8be55a..8d84f45c513 100644 --- a/src/core/lib/security/authorization/evaluate_args.cc +++ b/src/core/lib/security/authorization/evaluate_args.cc @@ -22,7 +22,7 @@ #include "src/core/lib/address_utils/sockaddr_utils.h" #include "src/core/lib/gprpp/host_port.h" #include "src/core/lib/security/credentials/tls/tls_utils.h" -#include "src/core/lib/slice/slice_internal.h" +#include "src/core/lib/slice/slice_utils.h" namespace grpc_core { diff --git a/src/core/lib/security/credentials/external/file_external_account_credentials.cc b/src/core/lib/security/credentials/external/file_external_account_credentials.cc index a959e8c871f..d596d29414c 100644 --- a/src/core/lib/security/credentials/external/file_external_account_credentials.cc +++ b/src/core/lib/security/credentials/external/file_external_account_credentials.cc @@ -21,6 +21,7 @@ #include "src/core/lib/iomgr/load_file.h" #include "src/core/lib/slice/slice_internal.h" +#include "src/core/lib/slice/slice_utils.h" namespace grpc_core { diff --git a/src/core/lib/slice/slice.cc b/src/core/lib/slice/slice.cc index ce556a25702..e0c4cb038a0 100644 --- a/src/core/lib/slice/slice.cc +++ b/src/core/lib/slice/slice.cc @@ -18,8 +18,6 @@ #include -#include "src/core/lib/slice/slice.h" - #include #include @@ -29,7 +27,6 @@ #include "src/core/lib/gprpp/memory.h" #include "src/core/lib/gprpp/ref_counted.h" #include "src/core/lib/slice/slice_internal.h" -#include "src/core/lib/slice/slice_refcount_base.h" char* grpc_slice_to_c_string(grpc_slice slice) { char* out = static_cast(gpr_malloc(GRPC_SLICE_LENGTH(slice) + 1)); @@ -38,9 +35,7 @@ char* grpc_slice_to_c_string(grpc_slice slice) { return out; } -grpc_slice grpc_empty_slice(void) { - return grpc_core::slice_detail::EmptySlice(); -} +grpc_slice grpc_empty_slice(void) { return grpc_core::UnmanagedMemorySlice(); } grpc_slice grpc_slice_copy(grpc_slice s) { grpc_slice out = GRPC_SLICE_MALLOC(GRPC_SLICE_LENGTH(s)); @@ -51,21 +46,30 @@ grpc_slice grpc_slice_copy(grpc_slice s) { namespace grpc_core { +/* grpc_slice_from_static_string support structure - a refcount that does + nothing */ +grpc_slice_refcount kNoopRefcount(grpc_slice_refcount::Type::NOP); +static_assert(std::is_trivially_destructible::value, + "kNoopRefcount must be trivially destructible."); + /* grpc_slice_new support structures - we create a refcount object extended with the user provided data pointer & destroy function */ -class NewSliceRefcount : public grpc_slice_refcount { +class NewSliceRefcount { public: + static void Destroy(void* arg) { delete static_cast(arg); } + NewSliceRefcount(void (*destroy)(void*), void* user_data) - : grpc_slice_refcount(Destroy), + : base_(grpc_slice_refcount::Type::REGULAR, &refs_, Destroy, this, + &base_), user_destroy_(destroy), user_data_(user_data) {} ~NewSliceRefcount() { user_destroy_(user_data_); } - private: - static void Destroy(grpc_slice_refcount* arg) { - delete static_cast(arg); - } + grpc_slice_refcount* base_refcount() { return &base_; } + private: + grpc_slice_refcount base_; + std::atomic refs_{1}; void (*user_destroy_)(void*); void* user_data_; }; @@ -73,8 +77,7 @@ class NewSliceRefcount : public grpc_slice_refcount { } // namespace grpc_core size_t grpc_slice_memory_usage(grpc_slice s) { - if (s.refcount == nullptr || - s.refcount == grpc_slice_refcount::NoopRefcount()) { + if (s.refcount == nullptr || s.refcount == &grpc_core::kNoopRefcount) { return 0; } else { return s.data.refcounted.length; @@ -82,18 +85,19 @@ size_t grpc_slice_memory_usage(grpc_slice s) { } grpc_slice grpc_slice_from_static_buffer(const void* s, size_t len) { - return grpc_core::StaticSlice::FromStaticBuffer(s, len).TakeCSlice(); + return grpc_core::ExternallyManagedSlice(s, len); } grpc_slice grpc_slice_from_static_string(const char* s) { - return grpc_core::StaticSlice::FromStaticString(s).TakeCSlice(); + return grpc_core::ExternallyManagedSlice(s, strlen(s)); } grpc_slice grpc_slice_new_with_user_data(void* p, size_t len, void (*destroy)(void*), void* user_data) { grpc_slice slice; - slice.refcount = new grpc_core::NewSliceRefcount(destroy, user_data); + slice.refcount = + (new grpc_core::NewSliceRefcount(destroy, user_data))->base_refcount(); slice.data.refcounted.bytes = static_cast(p); slice.data.refcounted.length = len; return slice; @@ -107,51 +111,68 @@ grpc_slice grpc_slice_new(void* p, size_t len, void (*destroy)(void*)) { namespace grpc_core { /* grpc_slice_new_with_len support structures - we create a refcount object extended with the user provided data pointer & destroy function */ -class NewWithLenSliceRefcount : public grpc_slice_refcount { +class NewWithLenSliceRefcount { public: + static void Destroy(void* arg) { + delete static_cast(arg); + } + NewWithLenSliceRefcount(void (*destroy)(void*, size_t), void* user_data, size_t user_length) - : grpc_slice_refcount(Destroy), + : base_(grpc_slice_refcount::Type::REGULAR, &refs_, Destroy, this, + &base_), user_data_(user_data), user_length_(user_length), user_destroy_(destroy) {} ~NewWithLenSliceRefcount() { user_destroy_(user_data_, user_length_); } - private: - static void Destroy(grpc_slice_refcount* arg) { - delete static_cast(arg); - } + grpc_slice_refcount* base_refcount() { return &base_; } + private: + grpc_slice_refcount base_; + std::atomic refs_{1}; void* user_data_; size_t user_length_; void (*user_destroy_)(void*, size_t); }; /** grpc_slice_from_moved_(string|buffer) ref count .*/ -class MovedStringSliceRefCount : public grpc_slice_refcount { +class MovedStringSliceRefCount { public: explicit MovedStringSliceRefCount(UniquePtr&& str) - : grpc_slice_refcount(Destroy), str_(std::move(str)) {} + : base_(grpc_slice_refcount::Type::REGULAR, &refs_, Destroy, this, + &base_), + str_(std::move(str)) {} + + grpc_slice_refcount* base_refcount() { return &base_; } private: - static void Destroy(grpc_slice_refcount* arg) { + static void Destroy(void* arg) { delete static_cast(arg); } + grpc_slice_refcount base_; + std::atomic refs_{1}; UniquePtr str_; }; // grpc_slice_from_cpp_string() ref count. -class MovedCppStringSliceRefCount : public grpc_slice_refcount { +class MovedCppStringSliceRefCount { public: explicit MovedCppStringSliceRefCount(std::string&& str) - : grpc_slice_refcount(Destroy), str_(std::move(str)) {} + : base_(grpc_slice_refcount::Type::REGULAR, &refs_, Destroy, this, + &base_), + str_(std::move(str)) {} + + grpc_slice_refcount* base_refcount() { return &base_; } private: - static void Destroy(grpc_slice_refcount* arg) { + static void Destroy(void* arg) { delete static_cast(arg); } + grpc_slice_refcount base_; + std::atomic refs_{1}; std::string str_; }; @@ -160,17 +181,44 @@ class MovedCppStringSliceRefCount : public grpc_slice_refcount { grpc_slice grpc_slice_new_with_len(void* p, size_t len, void (*destroy)(void*, size_t)) { grpc_slice slice; - slice.refcount = new grpc_core::NewWithLenSliceRefcount(destroy, p, len); + slice.refcount = (new grpc_core::NewWithLenSliceRefcount(destroy, p, len)) + ->base_refcount(); slice.data.refcounted.bytes = static_cast(p); slice.data.refcounted.length = len; return slice; } -grpc_slice grpc_slice_from_copied_buffer(const char* source, size_t len) { - if (len == 0) return grpc_empty_slice(); - grpc_slice out = grpc_slice_malloc(len); - memcpy(GRPC_SLICE_START_PTR(out), source, len); - return out; +grpc_core::UnmanagedMemorySlice::UnmanagedMemorySlice(const char* source, + size_t length) { + if (length <= sizeof(data.inlined.bytes)) { + refcount = nullptr; + data.inlined.length = static_cast(length); + } else { + HeapInit(length); + } + if (length > 0) { + memcpy(GRPC_SLICE_START_PTR(*this), source, length); + } +} + +grpc_core::UnmanagedMemorySlice::UnmanagedMemorySlice(const char* source) + : grpc_core::UnmanagedMemorySlice::UnmanagedMemorySlice(source, + strlen(source)) {} + +grpc_slice grpc_slice_from_copied_buffer(const char* source, size_t length) { + grpc_slice slice; + if (length <= sizeof(slice.data.inlined.bytes)) { + slice.refcount = nullptr; + slice.data.inlined.length = length; + } else { + // Create a ref-counted slice. + slice = grpc_core::UnmanagedMemorySlice( + length, grpc_core::UnmanagedMemorySlice::ForceHeapAllocation()); + } + if (length > 0) { + memcpy(GRPC_SLICE_START_PTR(slice), source, length); + } + return slice; } grpc_slice grpc_slice_from_copied_string(const char* source) { @@ -186,7 +234,8 @@ grpc_slice grpc_slice_from_moved_buffer(grpc_core::UniquePtr p, slice.data.inlined.length = len; memcpy(GRPC_SLICE_START_PTR(slice), ptr, len); } else { - slice.refcount = new grpc_core::MovedStringSliceRefCount(std::move(p)); + slice.refcount = (new grpc_core::MovedStringSliceRefCount(std::move(p))) + ->base_refcount(); slice.data.refcounted.bytes = ptr; slice.data.refcounted.length = len; } @@ -208,44 +257,94 @@ grpc_slice grpc_slice_from_cpp_string(std::string str) { slice.data.refcounted.bytes = reinterpret_cast(const_cast(str.data())); slice.data.refcounted.length = str.size(); - slice.refcount = new grpc_core::MovedCppStringSliceRefCount(std::move(str)); + slice.refcount = + (new grpc_core::MovedCppStringSliceRefCount(std::move(str))) + ->base_refcount(); } return slice; } +namespace { + +class MallocRefCount { + public: + static void Destroy(void* arg) { + MallocRefCount* r = static_cast(arg); + r->~MallocRefCount(); + gpr_free(r); + } + + MallocRefCount() + : base_(grpc_slice_refcount::Type::REGULAR, &refs_, Destroy, this, + &base_) {} + ~MallocRefCount() = default; + + grpc_slice_refcount* base_refcount() { return &base_; } + + private: + grpc_slice_refcount base_; + std::atomic refs_{1}; +}; + +} // namespace + grpc_slice grpc_slice_malloc_large(size_t length) { - grpc_slice slice; - uint8_t* memory = new uint8_t[sizeof(grpc_slice_refcount) + length]; - slice.refcount = new (memory) grpc_slice_refcount( - [](grpc_slice_refcount* p) { delete[] reinterpret_cast(p); }); - slice.data.refcounted.bytes = memory + sizeof(grpc_slice_refcount); - slice.data.refcounted.length = length; - return slice; + return grpc_core::UnmanagedMemorySlice( + length, grpc_core::UnmanagedMemorySlice::ForceHeapAllocation()); +} + +void grpc_core::UnmanagedMemorySlice::HeapInit(size_t length) { + /* Memory layout used by the slice created here: + + +-----------+----------------------------------------------------------+ + | refcount | bytes | + +-----------+----------------------------------------------------------+ + + refcount is a malloc_refcount + bytes is an array of bytes of the requested length + Both parts are placed in the same allocation returned from gpr_malloc */ + auto* rc = + static_cast(gpr_malloc(sizeof(MallocRefCount) + length)); + + /* Initial refcount on rc is 1 - and it's up to the caller to release + this reference. */ + new (rc) MallocRefCount(); + + /* Build up the slice to be returned. */ + /* The slices refcount points back to the allocated block. */ + refcount = rc->base_refcount(); + /* The data bytes are placed immediately after the refcount struct */ + data.refcounted.bytes = reinterpret_cast(rc + 1); + /* And the length of the block is set to the requested length */ + data.refcounted.length = length; } grpc_slice grpc_slice_malloc(size_t length) { - if (length <= GRPC_SLICE_INLINED_SIZE) { - grpc_slice slice; - slice.refcount = nullptr; - slice.data.inlined.length = length; - return slice; + return grpc_core::UnmanagedMemorySlice(length); +} + +grpc_core::UnmanagedMemorySlice::UnmanagedMemorySlice(size_t length) { + if (length > sizeof(data.inlined.bytes)) { + HeapInit(length); } else { - return grpc_slice_malloc_large(length); + /* small slice: just inline the data */ + refcount = nullptr; + data.inlined.length = static_cast(length); } } -static grpc_slice sub_no_ref(const grpc_slice& source, size_t begin, - size_t end) { - grpc_slice subset; +template +static Slice sub_no_ref(const Slice& source, size_t begin, size_t end) { + Slice subset; GPR_ASSERT(end >= begin); - if (source.refcount != nullptr) { + if (source.refcount) { /* Enforce preconditions */ GPR_ASSERT(source.data.refcounted.length >= end); /* Build the result */ - subset.refcount = source.refcount; + subset.refcount = source.refcount->sub_refcount(); /* Point into the source array */ subset.data.refcounted.bytes = source.data.refcounted.bytes + begin; subset.data.refcounted.length = end - begin; @@ -264,6 +363,11 @@ grpc_slice grpc_slice_sub_no_ref(grpc_slice source, size_t begin, size_t end) { return sub_no_ref(source, begin, end); } +grpc_core::UnmanagedMemorySlice grpc_slice_sub_no_ref( + const grpc_core::UnmanagedMemorySlice& source, size_t begin, size_t end) { + return sub_no_ref(source, begin, end); +} + grpc_slice grpc_slice_sub(grpc_slice source, size_t begin, size_t end) { grpc_slice subset; @@ -293,12 +397,6 @@ grpc_slice grpc_slice_split_tail_maybe_ref(grpc_slice* source, size_t split, memcpy(tail.data.inlined.bytes, source->data.inlined.bytes + split, tail.data.inlined.length); source->data.inlined.length = static_cast(split); - } else if (source->refcount == grpc_slice_refcount::NoopRefcount()) { - /* refcount == NoopRefcount(), so we can just split in-place */ - tail.refcount = grpc_slice_refcount::NoopRefcount(); - tail.data.refcounted.bytes = source->data.refcounted.bytes + split; - tail.data.refcounted.length = source->data.refcounted.length - split; - source->data.refcounted.length = split; } else { size_t tail_length = source->data.refcounted.length - split; GPR_ASSERT(source->data.refcounted.length >= split); @@ -309,18 +407,21 @@ grpc_slice grpc_slice_split_tail_maybe_ref(grpc_slice* source, size_t split, tail.data.inlined.length = static_cast(tail_length); memcpy(tail.data.inlined.bytes, source->data.refcounted.bytes + split, tail_length); + source->refcount = source->refcount->sub_refcount(); } else { /* Build the result */ switch (ref_whom) { case GRPC_SLICE_REF_TAIL: - tail.refcount = source->refcount; - source->refcount = grpc_slice_refcount::NoopRefcount(); + tail.refcount = source->refcount->sub_refcount(); + source->refcount = &grpc_core::kNoopRefcount; break; case GRPC_SLICE_REF_HEAD: - tail.refcount = grpc_slice_refcount::NoopRefcount(); + tail.refcount = &grpc_core::kNoopRefcount; + source->refcount = source->refcount->sub_refcount(); break; case GRPC_SLICE_REF_BOTH: - tail.refcount = source->refcount; + tail.refcount = source->refcount->sub_refcount(); + source->refcount = source->refcount->sub_refcount(); /* Bump the refcount */ tail.refcount->Ref(); break; @@ -358,18 +459,20 @@ grpc_slice grpc_slice_split_head(grpc_slice* source, size_t split) { head.refcount = nullptr; head.data.inlined.length = static_cast(split); memcpy(head.data.inlined.bytes, source->data.refcounted.bytes, split); + source->refcount = source->refcount->sub_refcount(); source->data.refcounted.bytes += split; source->data.refcounted.length -= split; } else { GPR_ASSERT(source->data.refcounted.length >= split); /* Build the result */ - head.refcount = source->refcount; + head.refcount = source->refcount->sub_refcount(); /* Bump the refcount */ head.refcount->Ref(); /* Point into the source array */ head.data.refcounted.bytes = source->data.refcounted.bytes; head.data.refcounted.length = split; + source->refcount = source->refcount->sub_refcount(); source->data.refcounted.bytes += split; source->data.refcounted.length -= split; } @@ -377,13 +480,21 @@ grpc_slice grpc_slice_split_head(grpc_slice* source, size_t split) { return head; } -int grpc_slice_eq(grpc_slice a, grpc_slice b) { +int grpc_slice_default_eq_impl(grpc_slice a, grpc_slice b) { if (GRPC_SLICE_LENGTH(a) != GRPC_SLICE_LENGTH(b)) return false; if (GRPC_SLICE_LENGTH(a) == 0) return true; return 0 == memcmp(GRPC_SLICE_START_PTR(a), GRPC_SLICE_START_PTR(b), GRPC_SLICE_LENGTH(a)); } +int grpc_slice_eq(grpc_slice a, grpc_slice b) { + if (a.refcount && b.refcount && + a.refcount->GetType() == b.refcount->GetType()) { + return a.refcount->Eq(a, b); + } + return grpc_slice_default_eq_impl(a, b); +} + int grpc_slice_differs_refcounted(const grpc_slice& a, const grpc_slice& b_not_inline) { size_t a_len; diff --git a/src/core/lib/slice/slice.h b/src/core/lib/slice/slice.h index c4c9284bec6..357ea708ccc 100644 --- a/src/core/lib/slice/slice.h +++ b/src/core/lib/slice/slice.h @@ -25,8 +25,6 @@ #include "src/core/lib/gpr/string.h" #include "src/core/lib/slice/slice_internal.h" -#include "src/core/lib/slice/slice_refcount.h" -#include "src/core/lib/slice/slice_refcount_base.h" // Herein lies grpc_core::Slice and its team of thin wrappers around grpc_slice. // They aim to keep you safe by providing strong guarantees around lifetime and @@ -105,8 +103,6 @@ class BaseSlice { return grpc_slice_is_equivalent(slice_, other.slice_); } - uint32_t Hash() const { return grpc_slice_hash_internal(slice_); } - protected: BaseSlice() : slice_(EmptySlice()) {} explicit BaseSlice(const grpc_slice& slice) : slice_(slice) {} @@ -166,7 +162,7 @@ inline bool operator!=(const grpc_slice& a, const BaseSlice& b) { template struct CopyConstructors { static Out FromCopiedString(const char* s) { - return FromCopiedBuffer(s, strlen(s)); + return Out(grpc_slice_from_copied_string(s)); } static Out FromCopiedString(absl::string_view s) { return FromCopiedBuffer(s.data(), s.size()); @@ -175,7 +171,7 @@ struct CopyConstructors { return Out(grpc_slice_from_cpp_string(std::move(s))); } static Out FromCopiedBuffer(const char* p, size_t len) { - return Out(grpc_slice_from_copied_buffer(p, len)); + return Out(UnmanagedMemorySlice(p, len)); } template @@ -194,20 +190,11 @@ struct CopyConstructors { template struct StaticConstructors { static Out FromStaticString(const char* s) { - return FromStaticBuffer(s, strlen(s)); + return Out(grpc_slice_from_static_string(s)); } static Out FromStaticString(absl::string_view s) { - return FromStaticBuffer(s.data(), s.size()); - } - - static Out FromStaticBuffer(const void* s, size_t len) { - grpc_slice slice; - slice.refcount = grpc_slice_refcount::NoopRefcount(); - slice.data.refcounted.bytes = - const_cast(static_cast(s)); - slice.data.refcounted.length = len; - return Out(slice); + return Out(ExternallyManagedSlice(s.data(), s.size())); } }; @@ -219,8 +206,11 @@ class StaticSlice : public slice_detail::BaseSlice, StaticSlice() = default; explicit StaticSlice(const grpc_slice& slice) : slice_detail::BaseSlice(slice) { - GPR_DEBUG_ASSERT(slice.refcount == grpc_slice_refcount::NoopRefcount()); + GPR_DEBUG_ASSERT(slice.refcount->GetType() == + grpc_slice_refcount::Type::NOP); } + explicit StaticSlice(const StaticMetadataSlice& slice) + : slice_detail::BaseSlice(slice) {} StaticSlice(const StaticSlice& other) : slice_detail::BaseSlice(other.c_slice()) {} @@ -242,7 +232,8 @@ class MutableSlice : public slice_detail::BaseSlice, MutableSlice() = default; explicit MutableSlice(const grpc_slice& slice) : slice_detail::BaseSlice(slice) { - GPR_DEBUG_ASSERT(slice.refcount == nullptr || slice.refcount->IsUnique()); + GPR_DEBUG_ASSERT(slice.refcount == nullptr || + slice.refcount->IsRegularUnique()); } ~MutableSlice() { grpc_slice_unref_internal(c_slice()); } @@ -306,7 +297,7 @@ class Slice : public slice_detail::BaseSlice, if (c_slice().refcount == nullptr) { return Slice(c_slice()); } - if (c_slice().refcount == grpc_slice_refcount::NoopRefcount()) { + if (c_slice().refcount->GetType() == grpc_slice_refcount::Type::NOP) { return Slice(grpc_slice_copy(c_slice())); } return Slice(TakeCSlice()); @@ -318,7 +309,7 @@ class Slice : public slice_detail::BaseSlice, if (c_slice().refcount == nullptr) { return Slice(c_slice()); } - if (c_slice().refcount == grpc_slice_refcount::NoopRefcount()) { + if (c_slice().refcount->GetType() == grpc_slice_refcount::Type::NOP) { return Slice(grpc_slice_copy(c_slice())); } return Slice(grpc_slice_ref_internal(c_slice())); @@ -336,8 +327,8 @@ class Slice : public slice_detail::BaseSlice, if (c_slice().refcount == nullptr) { return MutableSlice(c_slice()); } - if (c_slice().refcount != grpc_slice_refcount::NoopRefcount() && - c_slice().refcount->IsUnique()) { + if (c_slice().refcount->GetType() == grpc_slice_refcount::Type::REGULAR && + c_slice().refcount->IsRegularUnique()) { return MutableSlice(TakeCSlice()); } return MutableSlice(grpc_slice_copy(c_slice())); @@ -368,15 +359,11 @@ class Slice : public slice_detail::BaseSlice, const uint8_t* begin, const uint8_t* end) { grpc_slice out; out.refcount = r; - if (r != grpc_slice_refcount::NoopRefcount()) r->Ref(); + r->Ref(); out.data.refcounted.bytes = const_cast(begin); out.data.refcounted.length = end - begin; return Slice(out); } - - static Slice FromExternalString(absl::string_view str) { - return FromStaticString(str); - } }; } // namespace grpc_core diff --git a/src/core/lib/slice/slice_intern.cc b/src/core/lib/slice/slice_intern.cc new file mode 100644 index 00000000000..2ab13d1458b --- /dev/null +++ b/src/core/lib/slice/slice_intern.cc @@ -0,0 +1,269 @@ +/* + * + * Copyright 2016 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 +#include + +#include +#include + +#include "src/core/lib/gpr/murmur_hash.h" +#include "src/core/lib/gprpp/sync.h" +#include "src/core/lib/iomgr/iomgr_internal.h" /* for iomgr_abort_on_leaks() */ +#include "src/core/lib/profiling/timers.h" +#include "src/core/lib/slice/slice_internal.h" +#include "src/core/lib/slice/slice_string_helpers.h" +#include "src/core/lib/slice/slice_utils.h" + +#define LOG2_SHARD_COUNT 5 +#define SHARD_COUNT (1 << LOG2_SHARD_COUNT) +#define INITIAL_SHARD_CAPACITY 8 + +#define TABLE_IDX(hash, capacity) (((hash) >> LOG2_SHARD_COUNT) % (capacity)) +#define SHARD_IDX(hash) ((hash) & ((1 << LOG2_SHARD_COUNT) - 1)) + +using grpc_core::InternedSliceRefcount; + +typedef struct slice_shard { + grpc_core::Mutex mu; + InternedSliceRefcount** strs; + size_t count; + size_t capacity; +} slice_shard; + +static slice_shard* g_shards; + +namespace grpc_core { + +/* hash seed: decided at initialization time */ +uint32_t g_hash_seed; +static bool g_forced_hash_seed = false; + +InternedSliceRefcount::~InternedSliceRefcount() { + slice_shard* shard = &g_shards[SHARD_IDX(this->hash)]; + MutexLock lock(&shard->mu); + InternedSliceRefcount** prev_next; + InternedSliceRefcount* cur; + for (prev_next = &shard->strs[TABLE_IDX(this->hash, shard->capacity)], + cur = *prev_next; + cur != this; prev_next = &cur->bucket_next, cur = cur->bucket_next) { + } + *prev_next = cur->bucket_next; + shard->count--; +} + +} // namespace grpc_core + +static void grow_shard(slice_shard* shard) { + GPR_TIMER_SCOPE("grow_strtab", 0); + + size_t capacity = shard->capacity * 2; + size_t i; + InternedSliceRefcount** strtab; + InternedSliceRefcount *s, *next; + + strtab = static_cast( + gpr_zalloc(sizeof(InternedSliceRefcount*) * capacity)); + + for (i = 0; i < shard->capacity; i++) { + for (s = shard->strs[i]; s; s = next) { + size_t idx = TABLE_IDX(s->hash, capacity); + next = s->bucket_next; + s->bucket_next = strtab[idx]; + strtab[idx] = s; + } + } + gpr_free(shard->strs); + shard->strs = strtab; + shard->capacity = capacity; +} + +grpc_core::InternedSlice::InternedSlice(InternedSliceRefcount* s) { + refcount = &s->base; + data.refcounted.bytes = reinterpret_cast(s + 1); + data.refcounted.length = s->length; +} + +uint32_t grpc_slice_default_hash_impl(grpc_slice s) { + return gpr_murmur_hash3(GRPC_SLICE_START_PTR(s), GRPC_SLICE_LENGTH(s), + grpc_core::g_hash_seed); +} + +uint32_t grpc_slice_hash(grpc_slice s) { return grpc_slice_hash_internal(s); } + +grpc_slice grpc_slice_intern(grpc_slice slice) { + /* TODO(arjunroy): At present, this is capable of returning either a static or + an interned slice. This yields weirdness like the constructor for + ManagedMemorySlice instantiating itself as an instance of a derived type + (StaticMetadataSlice or InternedSlice). Should reexamine. */ + return grpc_core::ManagedMemorySlice(&slice); +} + +// Helper methods to enable us to select appropriately overloaded slice methods +// whether we're dealing with a slice, or a buffer with length, when interning +// strings. Helpers for FindOrCreateInternedSlice(). +static const char* GetBuffer(const std::pair& buflen) { + return buflen.first; +} +static size_t GetLength(const std::pair& buflen) { + return buflen.second; +} +static const void* GetBuffer(const grpc_slice& slice) { + return GRPC_SLICE_START_PTR(slice); +} +static size_t GetLength(const grpc_slice& slice) { + return GRPC_SLICE_LENGTH(slice); +} + +// Creates an interned slice for a string that does not currently exist in the +// intern table. SliceArgs is either a const grpc_slice& or a const +// pair&. Hash is the pre-computed hash value. We must +// already hold the shard lock. Helper for FindOrCreateInternedSlice(). +// +// Returns: a newly interned slice. +template +static InternedSliceRefcount* InternNewStringLocked(slice_shard* shard, + size_t shard_idx, + uint32_t hash, + const SliceArgs& args) { + /* string data goes after the internal_string header */ + size_t len = GetLength(args); + const void* buffer = GetBuffer(args); + InternedSliceRefcount* s = + static_cast(gpr_malloc(sizeof(*s) + len)); + new (s) grpc_core::InternedSliceRefcount(len, hash, shard->strs[shard_idx]); + // TODO(arjunroy): Investigate why hpack tried to intern the nullptr string. + // https://github.com/grpc/grpc/pull/20110#issuecomment-526729282 + if (len > 0) { + memcpy(reinterpret_cast(s + 1), buffer, len); + } + shard->strs[shard_idx] = s; + shard->count++; + if (shard->count > shard->capacity * 2) { + grow_shard(shard); + } + return s; +} + +// Attempt to see if the provided slice or string matches an existing interned +// slice. SliceArgs... is either a const grpc_slice& or a string and length. In +// either case, hash is the pre-computed hash value. We must already hold the +// shard lock. Helper for FindOrCreateInternedSlice(). +// +// Returns: a pre-existing matching static slice, or null. +template +static InternedSliceRefcount* MatchInternedSliceLocked(uint32_t hash, + size_t idx, + const SliceArgs& args) { + InternedSliceRefcount* s; + slice_shard* shard = &g_shards[SHARD_IDX(hash)]; + /* search for an existing string */ + for (s = shard->strs[idx]; s; s = s->bucket_next) { + if (s->hash == hash && grpc_core::InternedSlice(s) == args) { + if (grpc_core::IncrementIfNonzero(&s->refcnt)) { + return s; + } + } + } + return nullptr; +} + +// Attempt to see if the provided slice or string matches an existing interned +// slice, and failing that, create an interned slice with its contents. Returns +// either the existing matching interned slice or the newly created one. +// SliceArgs is either a const grpc_slice& or const pair&. +// In either case, hash is the pre-computed hash value. We do not hold the +// shard lock here, but do take it. +// +// Returns: an interned slice, either pre-existing/matched or newly created. +template +static InternedSliceRefcount* FindOrCreateInternedSlice(uint32_t hash, + const SliceArgs& args) { + slice_shard* shard = &g_shards[SHARD_IDX(hash)]; + grpc_core::MutexLock lock(&shard->mu); + const size_t idx = TABLE_IDX(hash, shard->capacity); + InternedSliceRefcount* s = MatchInternedSliceLocked(hash, idx, args); + if (s == nullptr) { + s = InternNewStringLocked(shard, idx, hash, args); + } + return s; +} + +grpc_core::ManagedMemorySlice::ManagedMemorySlice(const char* string) + : grpc_core::ManagedMemorySlice::ManagedMemorySlice(string, + strlen(string)) {} + +grpc_core::ManagedMemorySlice::ManagedMemorySlice(const char* buf, size_t len) { + GPR_TIMER_SCOPE("grpc_slice_intern", 0); + const uint32_t hash = gpr_murmur_hash3(buf, len, g_hash_seed); + *this = grpc_core::InternedSlice(FindOrCreateInternedSlice( + hash, std::pair(buf, len))); +} + +grpc_core::ManagedMemorySlice::ManagedMemorySlice(const grpc_slice* slice_ptr) { + GPR_TIMER_SCOPE("grpc_slice_intern", 0); + const grpc_slice& slice = *slice_ptr; + const uint32_t hash = grpc_slice_hash_internal(slice); + *this = grpc_core::InternedSlice(FindOrCreateInternedSlice(hash, slice)); +} + +void grpc_test_only_set_slice_hash_seed(uint32_t seed) { + grpc_core::g_hash_seed = seed; + grpc_core::g_forced_hash_seed = true; +} + +void grpc_slice_intern_init(void) { + if (!grpc_core::g_forced_hash_seed) { + grpc_core::g_hash_seed = + static_cast(gpr_now(GPR_CLOCK_REALTIME).tv_nsec); + } + g_shards = new slice_shard[SHARD_COUNT]; + for (size_t i = 0; i < SHARD_COUNT; i++) { + slice_shard* shard = &g_shards[i]; + shard->count = 0; + shard->capacity = INITIAL_SHARD_CAPACITY; + shard->strs = static_cast( + gpr_zalloc(sizeof(*shard->strs) * shard->capacity)); + } +} + +void grpc_slice_intern_shutdown(void) { + for (size_t i = 0; i < SHARD_COUNT; i++) { + slice_shard* shard = &g_shards[i]; + /* TODO(ctiller): GPR_ASSERT(shard->count == 0); */ + if (shard->count != 0) { + gpr_log(GPR_DEBUG, "WARNING: %" PRIuPTR " metadata strings were leaked", + shard->count); + for (size_t j = 0; j < shard->capacity; j++) { + for (InternedSliceRefcount* s = shard->strs[j]; s; s = s->bucket_next) { + char* text = grpc_dump_slice(grpc_core::InternedSlice(s), + GPR_DUMP_HEX | GPR_DUMP_ASCII); + gpr_log(GPR_DEBUG, "LEAKED: %s", text); + gpr_free(text); + } + } + if (grpc_iomgr_abort_on_leaks()) { + abort(); + } + } + gpr_free(shard->strs); + } + delete[] g_shards; +} diff --git a/src/core/lib/slice/slice_internal.h b/src/core/lib/slice/slice_internal.h index d4f22bbfec0..ae664edc6ce 100644 --- a/src/core/lib/slice/slice_internal.h +++ b/src/core/lib/slice/slice_internal.h @@ -23,8 +23,6 @@ #include -#include "absl/strings/string_view.h" - #include #include #include @@ -33,6 +31,7 @@ #include "src/core/lib/gprpp/memory.h" #include "src/core/lib/gprpp/ref_counted.h" #include "src/core/lib/slice/slice_refcount.h" +#include "src/core/lib/slice/slice_utils.h" void grpc_slice_buffer_reset_and_unref_internal(grpc_slice_buffer* sb); void grpc_slice_buffer_partial_unref_internal(grpc_slice_buffer* sb, @@ -53,6 +52,21 @@ void grpc_slice_buffer_remove_first(grpc_slice_buffer* sb); void grpc_slice_buffer_sub_first(grpc_slice_buffer* sb, size_t begin, size_t end); +/* Check if a slice is interned */ +bool grpc_slice_is_interned(const grpc_slice& slice); +inline bool grpc_slice_is_interned(const grpc_slice& slice) { + return (slice.refcount && + (slice.refcount->GetType() == grpc_slice_refcount::Type::INTERNED)); +} + +inline bool grpc_slice_static_interned_equal(const grpc_slice& a, + const grpc_slice& b) { + GPR_DEBUG_ASSERT(grpc_slice_is_interned(a) && grpc_slice_is_interned(b)); + return a.refcount == b.refcount; +} + +void grpc_slice_intern_init(void); +void grpc_slice_intern_shutdown(void); void grpc_test_only_set_slice_hash_seed(uint32_t seed); // if slice matches a static slice, returns the static slice // otherwise returns the passed in slice (without reffing it) @@ -63,11 +77,21 @@ grpc_slice grpc_slice_maybe_static_intern(grpc_slice slice, uint32_t grpc_static_slice_hash(grpc_slice s); int grpc_static_slice_eq(grpc_slice a, grpc_slice b); -inline uint32_t grpc_slice_hash_internal(const grpc_slice& s) { +inline uint32_t grpc_slice_hash_refcounted(const grpc_slice& s) { + GPR_DEBUG_ASSERT(s.refcount != nullptr); + return s.refcount->Hash(s); +} + +inline uint32_t grpc_slice_default_hash_internal(const grpc_slice& s) { return gpr_murmur_hash3(GRPC_SLICE_START_PTR(s), GRPC_SLICE_LENGTH(s), grpc_core::g_hash_seed); } +inline uint32_t grpc_slice_hash_internal(const grpc_slice& s) { + return s.refcount == nullptr ? grpc_slice_default_hash_internal(s) + : grpc_slice_hash_refcounted(s); +} + grpc_slice grpc_slice_from_moved_buffer(grpc_core::UniquePtr p, size_t len); grpc_slice grpc_slice_from_moved_string(grpc_core::UniquePtr p); @@ -78,6 +102,9 @@ grpc_slice grpc_slice_from_cpp_string(std::string str); // 0. All other slices will return the size of the allocated chars. size_t grpc_slice_memory_usage(grpc_slice s); +grpc_core::UnmanagedMemorySlice grpc_slice_sub_no_ref( + const grpc_core::UnmanagedMemorySlice& source, size_t begin, size_t end); + namespace grpc_core { struct SliceHash { @@ -86,15 +113,6 @@ struct SliceHash { } }; -extern uint32_t g_hash_seed; - -// Converts grpc_slice to absl::string_view. -inline absl::string_view StringViewFromSlice(const grpc_slice& slice) { - return absl::string_view( - reinterpret_cast(GRPC_SLICE_START_PTR(slice)), - GRPC_SLICE_LENGTH(slice)); -} - } // namespace grpc_core inline bool operator==(const grpc_slice& s1, const grpc_slice& s2) { diff --git a/src/core/lib/slice/slice_refcount.cc b/src/core/lib/slice/slice_refcount.cc index c3c0226a80e..10de62b72e7 100644 --- a/src/core/lib/slice/slice_refcount.cc +++ b/src/core/lib/slice/slice_refcount.cc @@ -15,19 +15,3 @@ #include #include "src/core/lib/slice/slice_refcount.h" - -#include - -namespace grpc_core { - -uint32_t g_hash_seed = []() { - std::random_device rd; - std::uniform_int_distribution dist; - return dist(rd); -}(); - -} // namespace grpc_core - -void grpc_test_only_set_slice_hash_seed(uint32_t seed) { - grpc_core::g_hash_seed = seed; -} diff --git a/src/core/lib/slice/slice_refcount.h b/src/core/lib/slice/slice_refcount.h index c3a90ca6f2a..a2f5f918ca2 100644 --- a/src/core/lib/slice/slice_refcount.h +++ b/src/core/lib/slice/slice_refcount.h @@ -27,18 +27,88 @@ namespace grpc_core { extern uint32_t g_hash_seed; +extern grpc_slice_refcount kNoopRefcount; + +// TODO(ctiller): when this is removed, remove the std::atomic* in +// grpc_slice_refcount and just put it there directly. +struct InternedSliceRefcount { + static void Destroy(void* arg) { + auto* rc = static_cast(arg); + rc->~InternedSliceRefcount(); + gpr_free(rc); + } + + InternedSliceRefcount(size_t length, uint32_t hash, + InternedSliceRefcount* bucket_next) + : base(grpc_slice_refcount::Type::INTERNED, &refcnt, Destroy, this, &sub), + sub(grpc_slice_refcount::Type::REGULAR, &refcnt, Destroy, this, &sub), + length(length), + hash(hash), + bucket_next(bucket_next) {} + + ~InternedSliceRefcount(); + + grpc_slice_refcount base; + grpc_slice_refcount sub; + const size_t length; + std::atomic refcnt{1}; + const uint32_t hash; + InternedSliceRefcount* bucket_next; +}; } // namespace grpc_core +inline size_t grpc_refcounted_slice_length(const grpc_slice& slice) { + GPR_DEBUG_ASSERT(slice.refcount != nullptr); + return slice.data.refcounted.length; +} + +inline const uint8_t* grpc_refcounted_slice_data(const grpc_slice& slice) { + GPR_DEBUG_ASSERT(slice.refcount != nullptr); + return slice.data.refcounted.bytes; +} + +inline int grpc_slice_refcount::Eq(const grpc_slice& a, const grpc_slice& b) { + GPR_DEBUG_ASSERT(a.refcount != nullptr); + GPR_DEBUG_ASSERT(a.refcount == this); + switch (ref_type_) { + case Type::INTERNED: + return a.refcount == b.refcount; + case Type::NOP: + case Type::REGULAR: + break; + } + if (grpc_refcounted_slice_length(a) != GRPC_SLICE_LENGTH(b)) return false; + if (grpc_refcounted_slice_length(a) == 0) return true; + return 0 == memcmp(grpc_refcounted_slice_data(a), GRPC_SLICE_START_PTR(b), + grpc_refcounted_slice_length(a)); +} + +inline uint32_t grpc_slice_refcount::Hash(const grpc_slice& slice) { + GPR_DEBUG_ASSERT(slice.refcount != nullptr); + GPR_DEBUG_ASSERT(slice.refcount == this); + switch (ref_type_) { + case Type::INTERNED: + return reinterpret_cast(slice.refcount) + ->hash; + case Type::NOP: + case Type::REGULAR: + break; + } + return gpr_murmur_hash3(grpc_refcounted_slice_data(slice), + grpc_refcounted_slice_length(slice), + grpc_core::g_hash_seed); +} + inline const grpc_slice& grpc_slice_ref_internal(const grpc_slice& slice) { - if (reinterpret_cast(slice.refcount) > 1) { + if (slice.refcount) { slice.refcount->Ref(); } return slice; } inline void grpc_slice_unref_internal(const grpc_slice& slice) { - if (reinterpret_cast(slice.refcount) > 1) { + if (slice.refcount) { slice.refcount->Unref(); } } diff --git a/src/core/lib/slice/slice_refcount_base.h b/src/core/lib/slice/slice_refcount_base.h index 79aaf9c1f04..5d81fcd8fc8 100644 --- a/src/core/lib/slice/slice_refcount_base.h +++ b/src/core/lib/slice/slice_refcount_base.h @@ -23,39 +23,143 @@ #include // grpc_slice_refcount : A reference count for grpc_slice. +// +// Non-inlined grpc_slice objects are refcounted. Historically this was +// implemented via grpc_slice_refcount, a C-style polymorphic class using a +// manually managed vtable of operations. Subclasses would define their own +// vtable; the 'virtual' methods (ref, unref, equals and hash) would simply call +// the function pointers in the vtable as necessary. +// +// Unfortunately, this leads to some inefficiencies in the generated code that +// can be improved upon. For example, equality checking for interned slices is a +// simple equality check on the refcount pointer. With the vtable approach, this +// would translate to roughly the following (high-level) instructions: +// +// grpc_slice_equals(slice1, slice2): +// load vtable->eq -> eq_func +// call eq_func(slice1, slice2) +// +// interned_slice_equals(slice1, slice2) +// load slice1.ref -> r1 +// load slice2.ref -> r2 +// cmp r1, r2 -> retval +// ret retval +// +// This leads to a function call for a function defined in another translation +// unit, which imposes memory barriers, which reduces the compiler's ability to +// optimize (in addition to the added overhead of call/ret). Additionally, it +// may be harder to reason about branch prediction when we're jumping to +// essentially arbitrarily provided function pointers. +// +// In addition, it is arguable that while virtualization was helpful for +// Equals()/Hash() methods, that it was fundamentally unnecessary for +// Ref()/Unref(). +// +// Instead, grpc_slice_refcount provides the same functionality as the C-style +// virtual class, but in a de-virtualized manner - Eq(), Hash(), Ref() and +// Unref() are provided within this header file. Fastpaths for Eq()/Hash() +// (interned and static metadata slices), as well as the Ref() operation, can +// all be inlined without any memory barriers. +// +// It does this by: +// 1. Using grpc_core::RefCount<> (header-only) for Ref/Unref. Two special cases +// need support: No-op ref/unref (eg. static metadata slices) and stream +// slice references (where all the slices share the streamref). This is in +// addition to the normal case of '1 slice, 1 ref'. +// To support these cases, we explicitly track a nullable pointer to the +// underlying RefCount<>. No-op ref/unref is used by checking the pointer for +// null, and doing nothing if it is. Both stream slice refs and 'normal' +// slices use the same path for Ref/Unref (by targeting the non-null +// pointer). +// +// 2. introducing the notion of grpc_slice_refcount::Type. This describes if a +// slice ref is used by a static metadata slice, an interned slice, or other +// slices. We switch on the slice ref type in order to provide fastpaths for +// Equals() and Hash(). +// +// In total, this saves us roughly 1-2% latency for unary calls, with smaller +// calls benefitting. The effect is present, but not as useful, for larger calls +// where the cost of sending the data dominates. +// TODO(arjunroy): Investigate if this can be removed with strongly typed +// grpc_slices. struct grpc_slice_refcount { public: - typedef void (*DestroyerFn)(grpc_slice_refcount*); - - static grpc_slice_refcount* NoopRefcount() { - return reinterpret_cast(1); - } + enum class Type { + INTERNED, // Refcount for an interned slice. + NOP, // No-Op + REGULAR // Refcount for non-static-metadata, non-interned slices. + }; + typedef void (*DestroyerFn)(void*); grpc_slice_refcount() = default; + explicit grpc_slice_refcount(Type t) : ref_type_(t) {} + + explicit grpc_slice_refcount(grpc_slice_refcount* sub) : sub_refcount_(sub) {} // Regular constructor for grpc_slice_refcount. // // Parameters: - // 1. DestroyerFn destroyer_fn - // Called when the refcount goes to 0, with 'this' as parameter. - explicit grpc_slice_refcount(DestroyerFn destroyer_fn) - : destroyer_fn_(destroyer_fn) {} + // 1. grpc_slice_refcount::Type type + // Whether we are the refcount for a static + // metadata slice, an interned slice, or any other kind of slice. + // + // 2. std::atomic* ref + // The pointer to the actual underlying grpc_core::RefCount. + // TODO(ctiller): remove the pointer indirection and just put the refcount on + // this object once we remove interning. + // + // 3. DestroyerFn destroyer_fn + // Called when the refcount goes to 0, with destroyer_arg as parameter. + // + // 4. void* destroyer_arg + // Argument for the virtualized destructor. + // + // 5. grpc_slice_refcount* sub + // Argument used for interned slices. + grpc_slice_refcount(grpc_slice_refcount::Type type, std::atomic* ref, + DestroyerFn destroyer_fn, void* destroyer_arg, + grpc_slice_refcount* sub) + : ref_(ref), + ref_type_(type), + sub_refcount_(sub), + dest_fn_(destroyer_fn), + destroy_fn_arg_(destroyer_arg) {} + // Initializer for static refcounts. + grpc_slice_refcount(grpc_slice_refcount* sub, Type type) + : ref_type_(type), sub_refcount_(sub) {} - void Ref() { ref_.fetch_add(1, std::memory_order_relaxed); } + Type GetType() const { return ref_type_; } + + int Eq(const grpc_slice& a, const grpc_slice& b); + + uint32_t Hash(const grpc_slice& slice); + void Ref() { + if (ref_ == nullptr) return; + ref_->fetch_add(1, std::memory_order_relaxed); + } void Unref() { - if (ref_.fetch_sub(1, std::memory_order_acq_rel) == 1) { - destroyer_fn_(this); + if (ref_ == nullptr) return; + if (ref_->fetch_sub(1, std::memory_order_acq_rel) == 1) { + dest_fn_(destroy_fn_arg_); } } - // Is this the only instance? + // Only for type REGULAR, is this the only instance? // For this to be useful the caller needs to ensure that if this is the only // instance, no other instance could be created during this call. - bool IsUnique() const { return ref_.load(std::memory_order_relaxed) == 1; } + bool IsRegularUnique() { + GPR_DEBUG_ASSERT(ref_type_ == Type::REGULAR); + return ref_->load(std::memory_order_relaxed) == 1; + } + + grpc_slice_refcount* sub_refcount() const { return sub_refcount_; } private: - std::atomic ref_{1}; - DestroyerFn destroyer_fn_ = nullptr; + std::atomic* ref_ = nullptr; + const Type ref_type_ = Type::REGULAR; + grpc_slice_refcount* sub_refcount_ = this; + DestroyerFn dest_fn_ = nullptr; + void* destroy_fn_arg_ = nullptr; }; #endif // GRPC_CORE_LIB_SLICE_SLICE_REFCOUNT_BASE_H diff --git a/src/core/lib/slice/slice_utils.h b/src/core/lib/slice/slice_utils.h new file mode 100644 index 00000000000..d9a4253c1aa --- /dev/null +++ b/src/core/lib/slice/slice_utils.h @@ -0,0 +1,200 @@ +/* + * + * Copyright 2019 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_SLICE_SLICE_UTILS_H +#define GRPC_CORE_LIB_SLICE_SLICE_UTILS_H + +#include + +#include + +#include "absl/strings/string_view.h" + +#include + +#include "src/core/lib/gpr/murmur_hash.h" + +namespace grpc_core { +extern uint32_t g_hash_seed; +} // namespace grpc_core + +// When we compare two slices, and we know the latter is not inlined, we can +// short circuit our comparison operator. We specifically use differs() +// semantics instead of equals() semantics due to more favourable code +// generation when using differs(). Specifically, we may use the output of +// grpc_slice_differs_refcounted for control flow. If we use differs() +// semantics, we end with a tailcall to memcmp(). If we use equals() semantics, +// we need to invert the result that memcmp provides us, which costs several +// instructions to do so. If we're using the result for control flow (i.e. +// branching based on the output) then we're just performing the extra +// operations to invert the result pointlessly. Concretely, we save 6 ops on +// x86-64/clang with differs(). +int grpc_slice_differs_refcounted(const grpc_slice& a, + const grpc_slice& b_not_inline); + +// When we compare two slices, and we *know* that one of them is static or +// interned, we can short circuit our slice equality function. The second slice +// here must be static or interned; slice a can be any slice, inlined or not. +inline bool grpc_slice_eq_static_interned(const grpc_slice& a, + const grpc_slice& b_static_interned) { + if (a.refcount == b_static_interned.refcount) { + return true; + } + return !grpc_slice_differs_refcounted(a, b_static_interned); +} + +// TODO(arjunroy): These type declarations ought to be in +// src/core/lib/slice/slice_internal.h instead; they are here due to a circular +// header depedency between slice_internal.h and +// src/core/lib/transport/metadata.h. We need to fix this circular reference and +// when we do, move these type declarations. +// +// Internal slice type declarations. +// Externally, a grpc_slice is a grpc_slice is a grpc_slice. +// Internally, we may have heap allocated slices, static slices, interned +// slices, and inlined slices. If we know the specific type of slice +// we're dealing with, we can save cycles (e.g. fast-paths when we know we don't +// need to take a reference on a slice). Rather than introducing new methods +// ad-hoc in these cases, we rely on type-system backed overloads to keep +// internal APIs clean. +// +// For each overload, the definition and layout of the underlying slice does not +// change; this is purely type-system information. +namespace grpc_core { + +// There are two main types of slices: those that have their memory +// managed by the slice library and those that do not. +// +// The following types of slices are not managed: +// - inlined slices (i.e., refcount is null) +// - slices that have a custom refcount type (i.e., not STATIC or INTERNED) +// - slices where the memory is managed by some external agent. The slice is not +// ref-counted by grpc, and the programmer is responsible for ensuring the +// data is valid for the duration of the period that grpc may access it. +// +// The following types of slices are managed: +// - static metadata slices (i.e., refcount type is STATIC) +// - interned slices (i.e., refcount type is INTERNED) +// +// This categorization is reflected in the following hierarchy: +// +// - grpc_slice +// > - UnmanagedMemorySlice +// > - ExternallyManagedSlice +// - ManagedMemorySlice +// > - InternedSlice +// - StaticMetadataSlice +// +struct ManagedMemorySlice : public grpc_slice { + ManagedMemorySlice() { + refcount = nullptr; + data.refcounted.bytes = nullptr; + data.refcounted.length = 0; + } + explicit ManagedMemorySlice(const char* string); + ManagedMemorySlice(const char* buf, size_t len); + explicit ManagedMemorySlice(const grpc_slice* slice); + bool operator==(const grpc_slice& other) const { + if (refcount == other.refcount) { + return true; + } + return !grpc_slice_differs_refcounted(other, *this); + } + bool operator!=(const grpc_slice& other) const { return !(*this == other); } + bool operator==(std::pair buflen) const { + return data.refcounted.length == buflen.second && buflen.first != nullptr && + memcmp(buflen.first, data.refcounted.bytes, buflen.second) == 0; + } +}; +struct UnmanagedMemorySlice : public grpc_slice { + // TODO(arjunroy): Can we use a default=false param instead of this enum? + enum class ForceHeapAllocation {}; + UnmanagedMemorySlice() { + refcount = nullptr; + data.inlined.length = 0; + } + explicit UnmanagedMemorySlice(const char* source); + UnmanagedMemorySlice(const char* source, size_t length); + // The first constructor creates a slice that may be heap allocated, or + // inlined in the slice structure if length is small enough + // (< GRPC_SLICE_INLINED_SIZE). The second constructor forces heap alloc. + explicit UnmanagedMemorySlice(size_t length); + explicit UnmanagedMemorySlice(size_t length, const ForceHeapAllocation&) { + HeapInit(length); + } + + private: + void HeapInit(size_t length); +}; + +extern grpc_slice_refcount kNoopRefcount; + +struct ExternallyManagedSlice : public UnmanagedMemorySlice { + ExternallyManagedSlice() + : ExternallyManagedSlice(&kNoopRefcount, 0, nullptr) {} + explicit ExternallyManagedSlice(const char* s) + : ExternallyManagedSlice(s, strlen(s)) {} + ExternallyManagedSlice(const void* s, size_t len) + : ExternallyManagedSlice( + &kNoopRefcount, len, + reinterpret_cast(const_cast(s))) {} + ExternallyManagedSlice(grpc_slice_refcount* ref, size_t length, + uint8_t* bytes) { + refcount = ref; + data.refcounted.length = length; + data.refcounted.bytes = bytes; + } + bool operator==(const grpc_slice& other) const { + return data.refcounted.length == GRPC_SLICE_LENGTH(other) && + memcmp(data.refcounted.bytes, GRPC_SLICE_START_PTR(other), + data.refcounted.length) == 0; + } + bool operator!=(const grpc_slice& other) const { return !(*this == other); } + uint32_t Hash() { + return gpr_murmur_hash3(data.refcounted.bytes, data.refcounted.length, + g_hash_seed); + } +}; + +struct StaticMetadataSlice : public ManagedMemorySlice { + StaticMetadataSlice(grpc_slice_refcount* ref, size_t length, + const uint8_t* bytes) { + refcount = ref; + data.refcounted.length = length; + // NB: grpc_slice may or may not point to a static slice, but we are + // definitely pointing to static data here. Since we are not changing + // the underlying C-type, we need a const_cast here. + data.refcounted.bytes = const_cast(bytes); + } +}; + +struct InternedSliceRefcount; +struct InternedSlice : public ManagedMemorySlice { + explicit InternedSlice(InternedSliceRefcount* s); +}; + +// Converts grpc_slice to absl::string_view. +inline absl::string_view StringViewFromSlice(const grpc_slice& slice) { + return absl::string_view( + reinterpret_cast(GRPC_SLICE_START_PTR(slice)), + GRPC_SLICE_LENGTH(slice)); +} + +} // namespace grpc_core + +#endif /* GRPC_CORE_LIB_SLICE_SLICE_UTILS_H */ diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index 06b517fab4c..52e5a161f90 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -50,9 +50,9 @@ #include "src/core/lib/iomgr/timer.h" #include "src/core/lib/profiling/timers.h" #include "src/core/lib/resource_quota/arena.h" -#include "src/core/lib/slice/slice_internal.h" #include "src/core/lib/slice/slice_split.h" #include "src/core/lib/slice/slice_string_helpers.h" +#include "src/core/lib/slice/slice_utils.h" #include "src/core/lib/surface/api_trace.h" #include "src/core/lib/surface/call_test_only.h" #include "src/core/lib/surface/channel.h" diff --git a/src/core/lib/surface/init.cc b/src/core/lib/surface/init.cc index 191ef4f0610..9c0a588f880 100644 --- a/src/core/lib/surface/init.cc +++ b/src/core/lib/surface/init.cc @@ -104,6 +104,7 @@ void grpc_init(void) { grpc_core::Fork::GlobalInit(); grpc_fork_handlers_auto_register(); grpc_stats_init(); + grpc_slice_intern_init(); grpc_core::channelz::ChannelzRegistry::Init(); grpc_security_pre_init(); grpc_core::ApplicationCallbackExecCtx::GlobalInit(); @@ -139,6 +140,7 @@ void grpc_shutdown_internal_locked(void) grpc_iomgr_shutdown(); gpr_timers_global_destroy(); grpc_tracer_shutdown(); + grpc_slice_intern_shutdown(); grpc_core::channelz::ChannelzRegistry::Shutdown(); grpc_stats_shutdown(); grpc_core::Fork::GlobalShutdown(); diff --git a/src/core/lib/surface/server.cc b/src/core/lib/surface/server.cc index da38f98900a..9c4c669e413 100644 --- a/src/core/lib/surface/server.cc +++ b/src/core/lib/surface/server.cc @@ -982,7 +982,19 @@ class Server::ChannelData::ConnectivityWatcher // Server::ChannelData::~ChannelData() { - registered_methods_.reset(); + if (registered_methods_ != nullptr) { + for (const ChannelRegisteredMethod& crm : *registered_methods_) { + grpc_slice_unref_internal(crm.method); + GPR_DEBUG_ASSERT(crm.method.refcount == &kNoopRefcount || + crm.method.refcount == nullptr); + if (crm.has_host) { + grpc_slice_unref_internal(crm.host); + GPR_DEBUG_ASSERT(crm.host.refcount == &kNoopRefcount || + crm.host.refcount == nullptr); + } + } + registered_methods_.reset(); + } if (server_ != nullptr) { if (server_->channelz_node_ != nullptr && channelz_socket_uuid_ != 0) { server_->channelz_node_->RemoveChildSocket(channelz_socket_uuid_); @@ -1015,11 +1027,11 @@ void Server::ChannelData::InitTransport(RefCountedPtr server, registered_methods_ = absl::make_unique>(slots); for (std::unique_ptr& rm : server_->registered_methods_) { - Slice host; - Slice method = Slice::FromExternalString(rm->method); + ExternallyManagedSlice host; + ExternallyManagedSlice method(rm->method.c_str()); const bool has_host = !rm->host.empty(); if (has_host) { - host = Slice::FromExternalString(rm->host.c_str()); + host = ExternallyManagedSlice(rm->host.c_str()); } uint32_t hash = MixHash32(has_host ? host.Hash() : 0, method.Hash()); uint32_t probes = 0; @@ -1034,9 +1046,9 @@ void Server::ChannelData::InitTransport(RefCountedPtr server, crm->flags = rm->flags; crm->has_host = has_host; if (has_host) { - crm->host = std::move(host); + crm->host = host; } - crm->method = std::move(method); + crm->method = method; } GPR_ASSERT(slots <= UINT32_MAX); registered_method_max_probes_ = max_probes; diff --git a/src/core/lib/surface/server.h b/src/core/lib/surface/server.h index 276d917fc06..4e83c3ef45c 100644 --- a/src/core/lib/surface/server.h +++ b/src/core/lib/surface/server.h @@ -176,8 +176,8 @@ class Server : public InternallyRefCounted, RegisteredMethod* server_registered_method = nullptr; uint32_t flags; bool has_host; - Slice method; - Slice host; + ExternallyManagedSlice method; + ExternallyManagedSlice host; }; class RequestMatcherInterface; diff --git a/src/cpp/ext/filters/census/client_filter.cc b/src/cpp/ext/filters/census/client_filter.cc index 16438b3f3cd..e1e26d19f72 100644 --- a/src/cpp/ext/filters/census/client_filter.cc +++ b/src/cpp/ext/filters/census/client_filter.cc @@ -100,9 +100,9 @@ void OpenCensusCallTracer::OpenCensusCallAttemptTracer:: size_t tracing_len = TraceContextSerialize(context_.Context(), tracing_buf, kMaxTraceContextLen); if (tracing_len > 0) { - send_initial_metadata->Set( - grpc_core::GrpcTraceBinMetadata(), - grpc_core::Slice::FromCopiedBuffer(tracing_buf, tracing_len)); + send_initial_metadata->Set(grpc_core::GrpcTraceBinMetadata(), + grpc_core::Slice(grpc_core::UnmanagedMemorySlice( + tracing_buf, tracing_len))); } grpc_slice tags = grpc_empty_slice(); // TODO(unknown): Add in tagging serialization. diff --git a/src/cpp/ext/filters/census/server_filter.cc b/src/cpp/ext/filters/census/server_filter.cc index 011cd420755..2653c552952 100644 --- a/src/cpp/ext/filters/census/server_filter.cc +++ b/src/cpp/ext/filters/census/server_filter.cc @@ -128,7 +128,7 @@ void CensusServerCallData::StartTransportStreamOpBatch( if (len > 0) { op->send_trailing_metadata()->batch()->Set( grpc_core::GrpcServerStatsBinMetadata(), - grpc_core::Slice::FromCopiedBuffer(stats_buf_, len)); + grpc_core::Slice(grpc_core::UnmanagedMemorySlice(stats_buf_, len))); } } // Call next op. diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py index cec32a17134..70ad5dc6c4f 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -603,6 +603,7 @@ CORE_SOURCE_FILES = [ 'src/core/lib/slice/slice.cc', 'src/core/lib/slice/slice_api.cc', 'src/core/lib/slice/slice_buffer.cc', + 'src/core/lib/slice/slice_intern.cc', 'src/core/lib/slice/slice_refcount.cc', 'src/core/lib/slice/slice_split.cc', 'src/core/lib/slice/slice_string_helpers.cc', diff --git a/src/ruby/ext/grpc/rb_grpc_imports.generated.c b/src/ruby/ext/grpc/rb_grpc_imports.generated.c index 45b475aaea0..1f2e3748e74 100644 --- a/src/ruby/ext/grpc/rb_grpc_imports.generated.c +++ b/src/ruby/ext/grpc/rb_grpc_imports.generated.c @@ -212,6 +212,7 @@ grpc_slice_new_with_user_data_type grpc_slice_new_with_user_data_import; grpc_slice_new_with_len_type grpc_slice_new_with_len_import; grpc_slice_malloc_type grpc_slice_malloc_import; grpc_slice_malloc_large_type grpc_slice_malloc_large_import; +grpc_slice_intern_type grpc_slice_intern_import; grpc_slice_from_copied_string_type grpc_slice_from_copied_string_import; grpc_slice_from_copied_buffer_type grpc_slice_from_copied_buffer_import; grpc_slice_from_static_string_type grpc_slice_from_static_string_import; @@ -222,6 +223,8 @@ grpc_slice_split_tail_type grpc_slice_split_tail_import; grpc_slice_split_tail_maybe_ref_type grpc_slice_split_tail_maybe_ref_import; grpc_slice_split_head_type grpc_slice_split_head_import; grpc_empty_slice_type grpc_empty_slice_import; +grpc_slice_default_hash_impl_type grpc_slice_default_hash_impl_import; +grpc_slice_default_eq_impl_type grpc_slice_default_eq_impl_import; grpc_slice_eq_type grpc_slice_eq_import; grpc_slice_cmp_type grpc_slice_cmp_import; grpc_slice_str_cmp_type grpc_slice_str_cmp_import; @@ -229,6 +232,7 @@ grpc_slice_buf_start_eq_type grpc_slice_buf_start_eq_import; grpc_slice_rchr_type grpc_slice_rchr_import; grpc_slice_chr_type grpc_slice_chr_import; grpc_slice_slice_type grpc_slice_slice_import; +grpc_slice_hash_type grpc_slice_hash_import; grpc_slice_is_equivalent_type grpc_slice_is_equivalent_import; grpc_slice_dup_type grpc_slice_dup_import; grpc_slice_to_c_string_type grpc_slice_to_c_string_import; @@ -497,6 +501,7 @@ void grpc_rb_load_imports(HMODULE library) { grpc_slice_new_with_len_import = (grpc_slice_new_with_len_type) GetProcAddress(library, "grpc_slice_new_with_len"); grpc_slice_malloc_import = (grpc_slice_malloc_type) GetProcAddress(library, "grpc_slice_malloc"); grpc_slice_malloc_large_import = (grpc_slice_malloc_large_type) GetProcAddress(library, "grpc_slice_malloc_large"); + grpc_slice_intern_import = (grpc_slice_intern_type) GetProcAddress(library, "grpc_slice_intern"); grpc_slice_from_copied_string_import = (grpc_slice_from_copied_string_type) GetProcAddress(library, "grpc_slice_from_copied_string"); grpc_slice_from_copied_buffer_import = (grpc_slice_from_copied_buffer_type) GetProcAddress(library, "grpc_slice_from_copied_buffer"); grpc_slice_from_static_string_import = (grpc_slice_from_static_string_type) GetProcAddress(library, "grpc_slice_from_static_string"); @@ -507,6 +512,8 @@ void grpc_rb_load_imports(HMODULE library) { grpc_slice_split_tail_maybe_ref_import = (grpc_slice_split_tail_maybe_ref_type) GetProcAddress(library, "grpc_slice_split_tail_maybe_ref"); grpc_slice_split_head_import = (grpc_slice_split_head_type) GetProcAddress(library, "grpc_slice_split_head"); grpc_empty_slice_import = (grpc_empty_slice_type) GetProcAddress(library, "grpc_empty_slice"); + grpc_slice_default_hash_impl_import = (grpc_slice_default_hash_impl_type) GetProcAddress(library, "grpc_slice_default_hash_impl"); + grpc_slice_default_eq_impl_import = (grpc_slice_default_eq_impl_type) GetProcAddress(library, "grpc_slice_default_eq_impl"); grpc_slice_eq_import = (grpc_slice_eq_type) GetProcAddress(library, "grpc_slice_eq"); grpc_slice_cmp_import = (grpc_slice_cmp_type) GetProcAddress(library, "grpc_slice_cmp"); grpc_slice_str_cmp_import = (grpc_slice_str_cmp_type) GetProcAddress(library, "grpc_slice_str_cmp"); @@ -514,6 +521,7 @@ void grpc_rb_load_imports(HMODULE library) { grpc_slice_rchr_import = (grpc_slice_rchr_type) GetProcAddress(library, "grpc_slice_rchr"); grpc_slice_chr_import = (grpc_slice_chr_type) GetProcAddress(library, "grpc_slice_chr"); grpc_slice_slice_import = (grpc_slice_slice_type) GetProcAddress(library, "grpc_slice_slice"); + grpc_slice_hash_import = (grpc_slice_hash_type) GetProcAddress(library, "grpc_slice_hash"); grpc_slice_is_equivalent_import = (grpc_slice_is_equivalent_type) GetProcAddress(library, "grpc_slice_is_equivalent"); grpc_slice_dup_import = (grpc_slice_dup_type) GetProcAddress(library, "grpc_slice_dup"); grpc_slice_to_c_string_import = (grpc_slice_to_c_string_type) GetProcAddress(library, "grpc_slice_to_c_string"); diff --git a/src/ruby/ext/grpc/rb_grpc_imports.generated.h b/src/ruby/ext/grpc/rb_grpc_imports.generated.h index 02619469444..ba60c8aff35 100644 --- a/src/ruby/ext/grpc/rb_grpc_imports.generated.h +++ b/src/ruby/ext/grpc/rb_grpc_imports.generated.h @@ -611,6 +611,9 @@ extern grpc_slice_malloc_type grpc_slice_malloc_import; typedef grpc_slice(*grpc_slice_malloc_large_type)(size_t length); extern grpc_slice_malloc_large_type grpc_slice_malloc_large_import; #define grpc_slice_malloc_large grpc_slice_malloc_large_import +typedef grpc_slice(*grpc_slice_intern_type)(grpc_slice slice); +extern grpc_slice_intern_type grpc_slice_intern_import; +#define grpc_slice_intern grpc_slice_intern_import typedef grpc_slice(*grpc_slice_from_copied_string_type)(const char* source); extern grpc_slice_from_copied_string_type grpc_slice_from_copied_string_import; #define grpc_slice_from_copied_string grpc_slice_from_copied_string_import @@ -641,6 +644,12 @@ extern grpc_slice_split_head_type grpc_slice_split_head_import; typedef grpc_slice(*grpc_empty_slice_type)(void); extern grpc_empty_slice_type grpc_empty_slice_import; #define grpc_empty_slice grpc_empty_slice_import +typedef uint32_t(*grpc_slice_default_hash_impl_type)(grpc_slice s); +extern grpc_slice_default_hash_impl_type grpc_slice_default_hash_impl_import; +#define grpc_slice_default_hash_impl grpc_slice_default_hash_impl_import +typedef int(*grpc_slice_default_eq_impl_type)(grpc_slice a, grpc_slice b); +extern grpc_slice_default_eq_impl_type grpc_slice_default_eq_impl_import; +#define grpc_slice_default_eq_impl grpc_slice_default_eq_impl_import typedef int(*grpc_slice_eq_type)(grpc_slice a, grpc_slice b); extern grpc_slice_eq_type grpc_slice_eq_import; #define grpc_slice_eq grpc_slice_eq_import @@ -662,6 +671,9 @@ extern grpc_slice_chr_type grpc_slice_chr_import; typedef int(*grpc_slice_slice_type)(grpc_slice haystack, grpc_slice needle); extern grpc_slice_slice_type grpc_slice_slice_import; #define grpc_slice_slice grpc_slice_slice_import +typedef uint32_t(*grpc_slice_hash_type)(grpc_slice s); +extern grpc_slice_hash_type grpc_slice_hash_import; +#define grpc_slice_hash grpc_slice_hash_import typedef int(*grpc_slice_is_equivalent_type)(grpc_slice a, grpc_slice b); extern grpc_slice_is_equivalent_type grpc_slice_is_equivalent_import; #define grpc_slice_is_equivalent grpc_slice_is_equivalent_import diff --git a/test/core/end2end/fuzzers/api_fuzzer.cc b/test/core/end2end/fuzzers/api_fuzzer.cc index e1c357596f1..2d83fdda98d 100644 --- a/test/core/end2end/fuzzers/api_fuzzer.cc +++ b/test/core/end2end/fuzzers/api_fuzzer.cc @@ -381,6 +381,11 @@ class Call : public std::enable_shared_from_this { template grpc_slice ReadSlice(const T& s) { grpc_slice slice = grpc_slice_from_cpp_string(s.value()); + if (s.intern()) { + auto interned_slice = grpc_slice_intern(slice); + grpc_slice_unref(slice); + slice = interned_slice; + } unref_slices_.push_back(slice); return slice; } diff --git a/test/core/end2end/fuzzers/api_fuzzer.proto b/test/core/end2end/fuzzers/api_fuzzer.proto index eab3551cb01..af867c3a931 100644 --- a/test/core/end2end/fuzzers/api_fuzzer.proto +++ b/test/core/end2end/fuzzers/api_fuzzer.proto @@ -19,15 +19,13 @@ package api_fuzzer; message Empty {} message ByteSlice { - reserved 2; - reserved "intern"; bytes value = 1; + bool intern = 2; } message StringSlice { - reserved 2; - reserved "intern"; string value = 1; + bool intern = 2; } message ResourceQuota {} diff --git a/test/core/slice/BUILD b/test/core/slice/BUILD index a51b697deed..192443a4a29 100644 --- a/test/core/slice/BUILD +++ b/test/core/slice/BUILD @@ -95,6 +95,18 @@ grpc_cc_test( ], ) +grpc_cc_test( + name = "slice_intern_test", + srcs = ["slice_intern_test.cc"], + language = "C++", + uses_polling = False, + deps = [ + "//:gpr", + "//:grpc", + "//test/core/util:grpc_test_util", + ], +) + grpc_cc_test( name = "slice_string_helpers_test", srcs = ["slice_string_helpers_test.cc"], diff --git a/test/core/slice/slice_intern_test.cc b/test/core/slice/slice_intern_test.cc new file mode 100644 index 00000000000..47e79c62d96 --- /dev/null +++ b/test/core/slice/slice_intern_test.cc @@ -0,0 +1,67 @@ +/* + * + * Copyright 2015 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 +#include + +#include +#include +#include +#include + +#include "src/core/lib/gprpp/memory.h" +#include "src/core/lib/slice/slice_internal.h" +#include "test/core/util/test_config.h" + +#define LOG_TEST_NAME(x) gpr_log(GPR_INFO, "%s", x); + +static void test_slice_interning(void) { + LOG_TEST_NAME("test_slice_interning"); + + grpc_init(); + grpc_slice src1 = grpc_slice_from_copied_string("hello123456789123456789"); + grpc_slice src2 = grpc_slice_from_copied_string("hello123456789123456789"); + + // Explicitly checking that the slices are at different addresses prevents + // failure with windows opt 64bit build. + // See https://github.com/grpc/grpc/issues/20519 + GPR_ASSERT(&src1 != &src2); + GPR_ASSERT(GRPC_SLICE_START_PTR(src1) != GRPC_SLICE_START_PTR(src2)); + + grpc_slice interned1 = grpc_slice_intern(src1); + grpc_slice interned2 = grpc_slice_intern(src2); + GPR_ASSERT(GRPC_SLICE_START_PTR(interned1) == + GRPC_SLICE_START_PTR(interned2)); + GPR_ASSERT(GRPC_SLICE_START_PTR(interned1) != GRPC_SLICE_START_PTR(src1)); + GPR_ASSERT(GRPC_SLICE_START_PTR(interned2) != GRPC_SLICE_START_PTR(src2)); + grpc_slice_unref(src1); + grpc_slice_unref(src2); + grpc_slice_unref(interned1); + grpc_slice_unref(interned2); + grpc_shutdown(); +} + +int main(int argc, char** argv) { + grpc::testing::TestEnvironment env(argc, argv); + grpc_init(); + test_slice_interning(); + grpc_shutdown(); + return 0; +} diff --git a/test/core/slice/slice_test.cc b/test/core/slice/slice_test.cc index 1bf2de65d12..1f5ea505054 100644 --- a/test/core/slice/slice_test.cc +++ b/test/core/slice/slice_test.cc @@ -358,7 +358,8 @@ size_t SumSlice(const Slice& slice) { TEST(SliceTest, ExternalAsOwned) { auto external_string = absl::make_unique(RandomString(1024)); - Slice slice = Slice::FromExternalString(*external_string); + Slice slice(ExternallyManagedSlice(external_string->data(), + external_string->length())); const auto initial_sum = SumSlice(slice); Slice owned = slice.AsOwned(); EXPECT_EQ(initial_sum, SumSlice(owned)); @@ -374,7 +375,9 @@ TEST(SliceTest, ExternalAsOwned) { TEST(SliceTest, ExternalTakeOwned) { std::unique_ptr external_string( new std::string(RandomString(1024))); - SumSlice(Slice::FromExternalString(*external_string).TakeOwned()); + SumSlice(Slice(ExternallyManagedSlice(external_string->data(), + external_string->length())) + .TakeOwned()); } TEST(SliceTest, StaticSlice) { diff --git a/test/core/surface/public_headers_must_be_c89.c b/test/core/surface/public_headers_must_be_c89.c index 35af6c516e7..96ebc19c460 100644 --- a/test/core/surface/public_headers_must_be_c89.c +++ b/test/core/surface/public_headers_must_be_c89.c @@ -256,6 +256,7 @@ int main(int argc, char **argv) { printf("%lx", (unsigned long) grpc_slice_new_with_len); printf("%lx", (unsigned long) grpc_slice_malloc); printf("%lx", (unsigned long) grpc_slice_malloc_large); + printf("%lx", (unsigned long) grpc_slice_intern); printf("%lx", (unsigned long) grpc_slice_from_copied_string); printf("%lx", (unsigned long) grpc_slice_from_copied_buffer); printf("%lx", (unsigned long) grpc_slice_from_static_string); @@ -266,6 +267,8 @@ int main(int argc, char **argv) { printf("%lx", (unsigned long) grpc_slice_split_tail_maybe_ref); printf("%lx", (unsigned long) grpc_slice_split_head); printf("%lx", (unsigned long) grpc_empty_slice); + printf("%lx", (unsigned long) grpc_slice_default_hash_impl); + printf("%lx", (unsigned long) grpc_slice_default_eq_impl); printf("%lx", (unsigned long) grpc_slice_eq); printf("%lx", (unsigned long) grpc_slice_cmp); printf("%lx", (unsigned long) grpc_slice_str_cmp); @@ -273,6 +276,7 @@ int main(int argc, char **argv) { printf("%lx", (unsigned long) grpc_slice_rchr); printf("%lx", (unsigned long) grpc_slice_chr); printf("%lx", (unsigned long) grpc_slice_slice); + printf("%lx", (unsigned long) grpc_slice_hash); printf("%lx", (unsigned long) grpc_slice_is_equivalent); printf("%lx", (unsigned long) grpc_slice_dup); printf("%lx", (unsigned long) grpc_slice_to_c_string); diff --git a/test/core/transport/chttp2/hpack_encoder_test.cc b/test/core/transport/chttp2/hpack_encoder_test.cc index c49c4372326..9d11f41a7bf 100644 --- a/test/core/transport/chttp2/hpack_encoder_test.cc +++ b/test/core/transport/chttp2/hpack_encoder_test.cc @@ -50,6 +50,7 @@ int g_failure = 0; typedef struct { bool eof; bool use_true_binary_metadata; + bool only_intern_key; } verify_params; /* verify that the output frames that are generated by encoding the stream @@ -168,8 +169,11 @@ static void verify(const verify_params params, const char* expected, for (i = 0; i < nheaders; i++) { char* key = va_arg(l, char*); char* value = va_arg(l, char*); - b.Append(key, grpc_core::Slice::FromStaticString(value), - CrashOnAppendError); + grpc_slice value_slice = grpc_slice_from_static_string(value); + if (!params.only_intern_key) { + value_slice = grpc_slice_intern(value_slice); + } + b.Append(key, grpc_core::Slice(value_slice), CrashOnAppendError); } va_end(l); @@ -208,6 +212,7 @@ static void test_basic_headers() { verify_params params = { false, false, + false, }; verify(params, "000005 0104 deadbeef 00 0161 0161", 1, "a", "a"); verify(params, "00000a 0104 deadbeef 00 0161 0161 00 0162 0163", 2, "a", "a", diff --git a/test/core/util/evaluate_args_test_util.h b/test/core/util/evaluate_args_test_util.h index 6beadf56860..27f5b445f63 100644 --- a/test/core/util/evaluate_args_test_util.h +++ b/test/core/util/evaluate_args_test_util.h @@ -33,11 +33,12 @@ class EvaluateArgsTestUtil { ~EvaluateArgsTestUtil() { delete channel_args_; } void AddPairToMetadata(const char* key, const char* value) { - metadata_.Append(key, Slice::FromStaticString(value), - [](absl::string_view, const Slice&) { - // We should never ever see an error here. - abort(); - }); + metadata_.Append( + key, Slice(grpc_slice_intern(grpc_slice_from_static_string(value))), + [](absl::string_view, const Slice&) { + // We should never ever see an error here. + abort(); + }); } void SetLocalEndpoint(absl::string_view local_uri) { diff --git a/test/cpp/end2end/channelz_service_test.cc b/test/cpp/end2end/channelz_service_test.cc index 027a08c9bfe..1f9423565bf 100644 --- a/test/cpp/end2end/channelz_service_test.cc +++ b/test/cpp/end2end/channelz_service_test.cc @@ -38,7 +38,7 @@ #include "src/core/lib/iomgr/load_file.h" #include "src/core/lib/security/credentials/tls/grpc_tls_certificate_provider.h" #include "src/core/lib/security/security_connector/ssl_utils.h" -#include "src/core/lib/slice/slice_internal.h" +#include "src/core/lib/slice/slice_utils.h" #include "src/cpp/client/secure_credentials.h" #include "src/proto/grpc/channelz/channelz.grpc.pb.h" #include "src/proto/grpc/testing/echo.grpc.pb.h" diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index 083a42cda0d..c55f47006c7 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -2225,6 +2225,7 @@ src/core/lib/slice/slice.cc \ src/core/lib/slice/slice.h \ src/core/lib/slice/slice_api.cc \ src/core/lib/slice/slice_buffer.cc \ +src/core/lib/slice/slice_intern.cc \ src/core/lib/slice/slice_internal.h \ src/core/lib/slice/slice_refcount.cc \ src/core/lib/slice/slice_refcount.h \ @@ -2233,6 +2234,7 @@ src/core/lib/slice/slice_split.cc \ src/core/lib/slice/slice_split.h \ src/core/lib/slice/slice_string_helpers.cc \ src/core/lib/slice/slice_string_helpers.h \ +src/core/lib/slice/slice_utils.h \ src/core/lib/surface/api_trace.cc \ src/core/lib/surface/api_trace.h \ src/core/lib/surface/builtins.cc \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index ee5f20c23f7..8ac97d3a429 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -2024,6 +2024,7 @@ src/core/lib/slice/slice.cc \ src/core/lib/slice/slice.h \ src/core/lib/slice/slice_api.cc \ src/core/lib/slice/slice_buffer.cc \ +src/core/lib/slice/slice_intern.cc \ src/core/lib/slice/slice_internal.h \ src/core/lib/slice/slice_refcount.cc \ src/core/lib/slice/slice_refcount.h \ @@ -2032,6 +2033,7 @@ src/core/lib/slice/slice_split.cc \ src/core/lib/slice/slice_split.h \ src/core/lib/slice/slice_string_helpers.cc \ src/core/lib/slice/slice_string_helpers.h \ +src/core/lib/slice/slice_utils.h \ src/core/lib/surface/README.md \ src/core/lib/surface/api_trace.cc \ src/core/lib/surface/api_trace.h \ diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index 3c6f6115693..b03d7fa57fc 100644 --- a/tools/run_tests/generated/tests.json +++ b/tools/run_tests/generated/tests.json @@ -2151,6 +2151,30 @@ ], "uses_polling": false }, + { + "args": [], + "benchmark": false, + "ci_platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "gtest": false, + "language": "c", + "name": "slice_intern_test", + "platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "uses_polling": false + }, { "args": [], "benchmark": false,