diff --git a/BUILD b/BUILD index 9e4a4c70b87..5a3aa72fb4d 100644 --- a/BUILD +++ b/BUILD @@ -4569,11 +4569,13 @@ grpc_cc_library( "gpr_platform", "grpc_trace", "hpack_parse_result", + "stats", "//src/core:hpack_constants", "//src/core:metadata_batch", "//src/core:no_destruct", "//src/core:parsed_metadata", "//src/core:slice", + "//src/core:unique_ptr_with_bitset", ], ) diff --git a/CMakeLists.txt b/CMakeLists.txt index ef4fe428f55..da48ca9f836 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1527,6 +1527,7 @@ if(gRPC_BUILD_TESTS) add_dependencies(buildtests_cxx try_join_test) add_dependencies(buildtests_cxx try_seq_metadata_test) add_dependencies(buildtests_cxx try_seq_test) + add_dependencies(buildtests_cxx unique_ptr_with_bitset_test) add_dependencies(buildtests_cxx unique_type_name_test) add_dependencies(buildtests_cxx unknown_frame_bad_client_test) add_dependencies(buildtests_cxx uri_parser_test) @@ -32484,6 +32485,40 @@ target_link_libraries(try_seq_test ) +endif() +if(gRPC_BUILD_TESTS) + +add_executable(unique_ptr_with_bitset_test + test/core/util/unique_ptr_with_bitset_test.cc +) +target_compile_features(unique_ptr_with_bitset_test PUBLIC cxx_std_14) +target_include_directories(unique_ptr_with_bitset_test + PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR} + ${CMAKE_CURRENT_SOURCE_DIR}/include + ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR} + ${_gRPC_RE2_INCLUDE_DIR} + ${_gRPC_SSL_INCLUDE_DIR} + ${_gRPC_UPB_GENERATED_DIR} + ${_gRPC_UPB_GRPC_GENERATED_DIR} + ${_gRPC_UPB_INCLUDE_DIR} + ${_gRPC_XXHASH_INCLUDE_DIR} + ${_gRPC_ZLIB_INCLUDE_DIR} + third_party/googletest/googletest/include + third_party/googletest/googletest + third_party/googletest/googlemock/include + third_party/googletest/googlemock + ${_gRPC_PROTO_GENS_DIR} +) + +target_link_libraries(unique_ptr_with_bitset_test + ${_gRPC_ALLTARGETS_LIBRARIES} + gtest + absl::check + absl::bits +) + + endif() if(gRPC_BUILD_TESTS) diff --git a/Package.swift b/Package.swift index 793720ceda8..1e3548bc567 100644 --- a/Package.swift +++ b/Package.swift @@ -1950,6 +1950,7 @@ let package = Package( "src/core/util/time_precise.cc", "src/core/util/time_precise.h", "src/core/util/tmpfile.h", + "src/core/util/unique_ptr_with_bitset.h", "src/core/util/upb_utils.h", "src/core/util/useful.h", "src/core/util/windows/cpu.cc", diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index 269a8ab875e..b95f89cafd6 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -1219,6 +1219,7 @@ libs: - src/core/util/latent_see.h - src/core/util/ring_buffer.h - src/core/util/spinlock.h + - src/core/util/unique_ptr_with_bitset.h - src/core/util/upb_utils.h - src/core/xds/grpc/certificate_provider_store.h - src/core/xds/grpc/file_watcher_certificate_provider_factory.h @@ -2705,6 +2706,7 @@ libs: - src/core/util/latent_see.h - src/core/util/ring_buffer.h - src/core/util/spinlock.h + - src/core/util/unique_ptr_with_bitset.h - src/core/util/upb_utils.h - third_party/upb/upb/generated_code_support.h src: @@ -20491,6 +20493,19 @@ targets: - absl/status:statusor - gpr uses_polling: false +- name: unique_ptr_with_bitset_test + gtest: true + build: test + language: c++ + headers: + - src/core/util/unique_ptr_with_bitset.h + src: + - test/core/util/unique_ptr_with_bitset_test.cc + deps: + - gtest + - absl/log:check + - absl/numeric:bits + uses_polling: false - name: unique_type_name_test gtest: true build: test diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index 7e66f9de672..fe2176500b4 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -1326,6 +1326,7 @@ Pod::Spec.new do |s| 'src/core/util/string.h', 'src/core/util/time_precise.h', 'src/core/util/tmpfile.h', + 'src/core/util/unique_ptr_with_bitset.h', 'src/core/util/upb_utils.h', 'src/core/util/useful.h', 'src/core/xds/grpc/certificate_provider_store.h', @@ -2609,6 +2610,7 @@ Pod::Spec.new do |s| 'src/core/util/string.h', 'src/core/util/time_precise.h', 'src/core/util/tmpfile.h', + 'src/core/util/unique_ptr_with_bitset.h', 'src/core/util/upb_utils.h', 'src/core/util/useful.h', 'src/core/xds/grpc/certificate_provider_store.h', diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index db3d4fabede..10002b9dfe6 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -2066,6 +2066,7 @@ Pod::Spec.new do |s| 'src/core/util/time_precise.cc', 'src/core/util/time_precise.h', 'src/core/util/tmpfile.h', + 'src/core/util/unique_ptr_with_bitset.h', 'src/core/util/upb_utils.h', 'src/core/util/useful.h', 'src/core/util/windows/cpu.cc', @@ -3389,6 +3390,7 @@ Pod::Spec.new do |s| 'src/core/util/string.h', 'src/core/util/time_precise.h', 'src/core/util/tmpfile.h', + 'src/core/util/unique_ptr_with_bitset.h', 'src/core/util/upb_utils.h', 'src/core/util/useful.h', 'src/core/xds/grpc/certificate_provider_store.h', diff --git a/grpc.gemspec b/grpc.gemspec index 6d0344c4613..2fb9d03a2d4 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -1952,6 +1952,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/util/time_precise.cc ) s.files += %w( src/core/util/time_precise.h ) s.files += %w( src/core/util/tmpfile.h ) + s.files += %w( src/core/util/unique_ptr_with_bitset.h ) s.files += %w( src/core/util/upb_utils.h ) s.files += %w( src/core/util/useful.h ) s.files += %w( src/core/util/windows/cpu.cc ) diff --git a/package.xml b/package.xml index 484d0f6a26b..7b3a1bbdb60 100644 --- a/package.xml +++ b/package.xml @@ -1934,6 +1934,7 @@ + diff --git a/src/core/BUILD b/src/core/BUILD index 2e69723908a..e762447cfbc 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -289,6 +289,17 @@ grpc_cc_library( deps = ["//:gpr_platform"], ) +grpc_cc_library( + name = "unique_ptr_with_bitset", + hdrs = ["util/unique_ptr_with_bitset.h"], + external_deps = [ + "absl/log:check", + "absl/numeric:bits", + ], + language = "c++", + deps = ["//:gpr_platform"], +) + grpc_cc_library( name = "examine_stack", srcs = [ diff --git a/src/core/ext/transport/chttp2/transport/hpack_parser.cc b/src/core/ext/transport/chttp2/transport/hpack_parser.cc index 15b108f627a..04c64dd5df9 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_parser.cc +++ b/src/core/ext/transport/chttp2/transport/hpack_parser.cc @@ -713,7 +713,7 @@ class HPackParser::Parser { LOG(INFO) << "HTTP:" << log_info_.stream_id << ":" << type << ":" << (log_info_.is_client ? "CLI" : "SVR") << ": " << memento.md.DebugString() - << (memento.parse_status == nullptr + << (memento.parse_status.get() == nullptr ? "" : absl::StrCat( " (parse error: ", @@ -724,7 +724,7 @@ class HPackParser::Parser { void EmitHeader(const HPackTable::Memento& md) { // Pass up to the transport state_.frame_length += md.md.transport_size(); - if (md.parse_status != nullptr) { + if (md.parse_status.get() != nullptr) { // Reject any requests with invalid metadata. input_->SetErrorAndContinueParsing(*md.parse_status); } @@ -974,7 +974,7 @@ class HPackParser::Parser { } else { const auto* memento = absl::get(state_.key); key_string = memento->md.key(); - if (state_.field_error.ok() && memento->parse_status != nullptr) { + if (state_.field_error.ok() && memento->parse_status.get() != nullptr) { input_->SetErrorAndContinueParsing(*memento->parse_status); } } diff --git a/src/core/ext/transport/chttp2/transport/hpack_parser_table.cc b/src/core/ext/transport/chttp2/transport/hpack_parser_table.cc index f27f7a1019d..7a0b7bd8498 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_parser_table.cc +++ b/src/core/ext/transport/chttp2/transport/hpack_parser_table.cc @@ -37,6 +37,7 @@ #include "src/core/ext/transport/chttp2/transport/hpack_parse_result.h" #include "src/core/lib/debug/trace.h" #include "src/core/lib/slice/slice.h" +#include "src/core/telemetry/stats.h" namespace grpc_core { @@ -47,6 +48,10 @@ void HPackTable::MementoRingBuffer::Put(Memento m) { return entries_.push_back(std::move(m)); } size_t index = (first_entry_ + num_entries_) % max_entries_; + if (timestamp_index_ == kNoTimestamp) { + timestamp_index_ = index; + timestamp_ = Timestamp::Now(); + } entries_[index] = std::move(m); ++num_entries_; } @@ -54,12 +59,31 @@ void HPackTable::MementoRingBuffer::Put(Memento m) { auto HPackTable::MementoRingBuffer::PopOne() -> Memento { CHECK_GT(num_entries_, 0u); size_t index = first_entry_ % max_entries_; + if (index == timestamp_index_) { + global_stats().IncrementHttp2HpackEntryLifetime( + (Timestamp::Now() - timestamp_).millis()); + timestamp_index_ = kNoTimestamp; + } ++first_entry_; --num_entries_; - return std::move(entries_[index]); + auto& entry = entries_[index]; + if (!entry.parse_status.TestBit(Memento::kUsedBit)) { + global_stats().IncrementHttp2HpackMisses(); + } + return std::move(entry); } -auto HPackTable::MementoRingBuffer::Lookup(uint32_t index) const +auto HPackTable::MementoRingBuffer::Lookup(uint32_t index) -> const Memento* { + if (index >= num_entries_) return nullptr; + uint32_t offset = (num_entries_ - 1u - index + first_entry_) % max_entries_; + auto& entry = entries_[offset]; + const bool was_used = entry.parse_status.TestBit(Memento::kUsedBit); + entry.parse_status.SetBit(Memento::kUsedBit); + if (!was_used) global_stats().IncrementHttp2HpackHits(); + return &entry; +} + +auto HPackTable::MementoRingBuffer::Peek(uint32_t index) const -> const Memento* { if (index >= num_entries_) return nullptr; uint32_t offset = (num_entries_ - 1u - index + first_entry_) % max_entries_; @@ -79,14 +103,22 @@ void HPackTable::MementoRingBuffer::Rebuild(uint32_t max_entries) { entries_.swap(entries); } -void HPackTable::MementoRingBuffer::ForEach( - absl::FunctionRef f) const { +template +void HPackTable::MementoRingBuffer::ForEach(F f) const { uint32_t index = 0; - while (auto* m = Lookup(index++)) { + while (auto* m = Peek(index++)) { f(index, *m); } } +HPackTable::MementoRingBuffer::~MementoRingBuffer() { + ForEach([](uint32_t, const Memento& m) { + if (!m.parse_status.TestBit(Memento::kUsedBit)) { + global_stats().IncrementHttp2HpackMisses(); + } + }); +} + // Evict one element from the table void HPackTable::EvictOne() { auto first_entry = entries_.PopOne(); diff --git a/src/core/ext/transport/chttp2/transport/hpack_parser_table.h b/src/core/ext/transport/chttp2/transport/hpack_parser_table.h index d126b3eeb43..06e0aeb7f42 100644 --- a/src/core/ext/transport/chttp2/transport/hpack_parser_table.h +++ b/src/core/ext/transport/chttp2/transport/hpack_parser_table.h @@ -21,6 +21,8 @@ #include +#include +#include #include #include #include @@ -34,6 +36,7 @@ #include "src/core/lib/gprpp/no_destruct.h" #include "src/core/lib/transport/metadata_batch.h" #include "src/core/lib/transport/parsed_metadata.h" +#include "src/core/util/unique_ptr_with_bitset.h" namespace grpc_core { @@ -54,11 +57,14 @@ class HPackTable { struct Memento { ParsedMetadata md; - std::unique_ptr parse_status; + // Alongside parse_status we store one bit indicating whether this memento + // has been looked up (and therefore consumed) or not. + UniquePtrWithBitset parse_status; + static const int kUsedBit = 0; }; // Lookup, but don't ref. - const Memento* Lookup(uint32_t index) const { + const Memento* Lookup(uint32_t index) { // Static table comes first, just return an entry from it. // NB: This imposes the constraint that the first // GRPC_CHTTP2_LAST_STATIC_ENTRY entries in the core static metadata table @@ -97,6 +103,14 @@ class HPackTable { class MementoRingBuffer { public: + MementoRingBuffer() {} + ~MementoRingBuffer(); + + MementoRingBuffer(const MementoRingBuffer&) = delete; + MementoRingBuffer& operator=(const MementoRingBuffer&) = delete; + MementoRingBuffer(MementoRingBuffer&&) = default; + MementoRingBuffer& operator=(MementoRingBuffer&&) = default; + // Rebuild this buffer with a new max_entries_ size. void Rebuild(uint32_t max_entries); @@ -109,10 +123,11 @@ class HPackTable { Memento PopOne(); // Lookup the entry at index, or return nullptr if none exists. - const Memento* Lookup(uint32_t index) const; + const Memento* Lookup(uint32_t index); + const Memento* Peek(uint32_t index) const; - void ForEach(absl::FunctionRef - f) const; + template + void ForEach(F f) const; uint32_t max_entries() const { return max_entries_; } uint32_t num_entries() const { return num_entries_; } @@ -126,11 +141,17 @@ class HPackTable { // Maximum number of entries we could possibly fit in the table, given // defined overheads. uint32_t max_entries_ = hpack_constants::kInitialTableEntries; + // Which index holds a timestamp (or kNoTimestamp if none do). + static constexpr uint32_t kNoTimestamp = + std::numeric_limits::max(); + uint32_t timestamp_index_ = kNoTimestamp; + // The timestamp associated with timestamp_entry_. + Timestamp timestamp_; std::vector entries_; }; - const Memento* LookupDynamic(uint32_t index) const { + const Memento* LookupDynamic(uint32_t index) { // Not static - find the value in the list of valid entries const uint32_t tbl_index = index - (hpack_constants::kLastStaticEntry + 1); return entries_.Lookup(tbl_index); diff --git a/src/core/telemetry/stats_data.cc b/src/core/telemetry/stats_data.cc index 4a317b7fcf0..b39702f4762 100644 --- a/src/core/telemetry/stats_data.cc +++ b/src/core/telemetry/stats_data.cc @@ -106,6 +106,20 @@ Histogram_10000_20 operator-(const Histogram_10000_20& left, } return result; } +void HistogramCollector_1800000_40::Collect( + Histogram_1800000_40* result) const { + for (int i = 0; i < 40; i++) { + result->buckets_[i] += buckets_[i].load(std::memory_order_relaxed); + } +} +Histogram_1800000_40 operator-(const Histogram_1800000_40& left, + const Histogram_1800000_40& right) { + Histogram_1800000_40 result; + for (int i = 0; i < 40; i++) { + result.buckets_[i] = left.buckets_[i] - right.buckets_[i]; + } + return result; +} const absl::string_view GlobalStats::counter_name[static_cast(Counter::COUNT)] = { "client_calls_created", @@ -123,6 +137,8 @@ const absl::string_view "http2_writes_begun", "http2_transport_stalls", "http2_stream_stalls", + "http2_hpack_hits", + "http2_hpack_misses", "cq_pluck_creates", "cq_next_creates", "cq_callback_creates", @@ -161,6 +177,8 @@ const absl::string_view GlobalStats::counter_doc[static_cast( "control window", "Number of times sending was completely stalled by the stream flow control " "window", + "Number of HPACK cache hits", + "Number of HPACK cache misses (entries added but never used)", "Number of completion queues created for cq_pluck (indicates sync api " "usage)", "Number of completion queues created for cq_next (indicates cq async api " @@ -192,6 +210,7 @@ const absl::string_view "tcp_read_offer_iov_size", "http2_send_message_size", "http2_metadata_size", + "http2_hpack_entry_lifetime", "wrr_subchannel_list_size", "wrr_subchannel_ready_size", "work_serializer_run_time_ms", @@ -223,6 +242,7 @@ const absl::string_view GlobalStats::histogram_doc[static_cast( "Number of byte segments offered to each syscall_read", "Size of messages received by HTTP2 transport", "Number of bytes consumed by metadata, according to HPACK accounting rules", + "Lifetime of HPACK entries in the cache (in milliseconds)", "Number of subchannels in a subchannel list at picker creation time", "Number of READY subchannels in a subchannel list at picker creation time", "Number of milliseconds work serializers run for", @@ -278,6 +298,15 @@ const int kStatsTable10[21] = {0, 1, 2, 4, 7, 12, 19, const uint8_t kStatsTable11[23] = {3, 3, 4, 5, 5, 6, 7, 8, 9, 9, 10, 11, 12, 12, 13, 14, 15, 15, 16, 17, 18, 18, 19}; +const int kStatsTable12[41] = { + 0, 1, 2, 3, 5, 8, 12, 18, 26, + 37, 53, 76, 108, 153, 217, 308, 436, 617, + 873, 1235, 1748, 2473, 3499, 4950, 7003, 9907, 14015, + 19825, 28044, 39670, 56116, 79379, 112286, 158835, 224680, 317821, + 449574, 635945, 899575, 1272492, 1800000}; +const uint8_t kStatsTable13[37] = { + 4, 5, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, + 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39}; } // namespace int Histogram_100000_20::BucketFor(int value) { if (value < 3) { @@ -405,6 +434,29 @@ int Histogram_10000_20::BucketFor(int value) { } } } +int Histogram_1800000_40::BucketFor(int value) { + if (value < 4) { + if (value < 0) { + return 0; + } else { + return value; + } + } else { + if (value < 1048577) { + DblUint val; + val.dbl = value; + const int bucket = + kStatsTable13[((val.uint - 4616189618054758400ull) >> 51)]; + return bucket - (value < kStatsTable12[bucket]); + } else { + if (value < 1272492) { + return 38; + } else { + return 39; + } + } + } +} GlobalStats::GlobalStats() : client_calls_created{0}, server_calls_created{0}, @@ -421,6 +473,8 @@ GlobalStats::GlobalStats() http2_writes_begun{0}, http2_transport_stalls{0}, http2_stream_stalls{0}, + http2_hpack_hits{0}, + http2_hpack_misses{0}, cq_pluck_creates{0}, cq_next_creates{0}, cq_callback_creates{0}, @@ -466,6 +520,9 @@ HistogramView GlobalStats::histogram(Histogram which) const { case Histogram::kHttp2MetadataSize: return HistogramView{&Histogram_65536_26::BucketFor, kStatsTable2, 26, http2_metadata_size.buckets()}; + case Histogram::kHttp2HpackEntryLifetime: + return HistogramView{&Histogram_1800000_40::BucketFor, kStatsTable12, 40, + http2_hpack_entry_lifetime.buckets()}; case Histogram::kWrrSubchannelListSize: return HistogramView{&Histogram_10000_20::BucketFor, kStatsTable10, 20, wrr_subchannel_list_size.buckets()}; @@ -560,6 +617,10 @@ std::unique_ptr GlobalStatsCollector::Collect() const { data.http2_transport_stalls.load(std::memory_order_relaxed); result->http2_stream_stalls += data.http2_stream_stalls.load(std::memory_order_relaxed); + result->http2_hpack_hits += + data.http2_hpack_hits.load(std::memory_order_relaxed); + result->http2_hpack_misses += + data.http2_hpack_misses.load(std::memory_order_relaxed); result->cq_pluck_creates += data.cq_pluck_creates.load(std::memory_order_relaxed); result->cq_next_creates += @@ -598,6 +659,8 @@ std::unique_ptr GlobalStatsCollector::Collect() const { data.tcp_read_offer_iov_size.Collect(&result->tcp_read_offer_iov_size); data.http2_send_message_size.Collect(&result->http2_send_message_size); data.http2_metadata_size.Collect(&result->http2_metadata_size); + data.http2_hpack_entry_lifetime.Collect( + &result->http2_hpack_entry_lifetime); data.wrr_subchannel_list_size.Collect(&result->wrr_subchannel_list_size); data.wrr_subchannel_ready_size.Collect(&result->wrr_subchannel_ready_size); data.work_serializer_run_time_ms.Collect( @@ -664,6 +727,8 @@ std::unique_ptr GlobalStats::Diff(const GlobalStats& other) const { result->http2_transport_stalls = http2_transport_stalls - other.http2_transport_stalls; result->http2_stream_stalls = http2_stream_stalls - other.http2_stream_stalls; + result->http2_hpack_hits = http2_hpack_hits - other.http2_hpack_hits; + result->http2_hpack_misses = http2_hpack_misses - other.http2_hpack_misses; result->cq_pluck_creates = cq_pluck_creates - other.cq_pluck_creates; result->cq_next_creates = cq_next_creates - other.cq_next_creates; result->cq_callback_creates = cq_callback_creates - other.cq_callback_creates; @@ -695,6 +760,8 @@ std::unique_ptr GlobalStats::Diff(const GlobalStats& other) const { result->http2_send_message_size = http2_send_message_size - other.http2_send_message_size; result->http2_metadata_size = http2_metadata_size - other.http2_metadata_size; + result->http2_hpack_entry_lifetime = + http2_hpack_entry_lifetime - other.http2_hpack_entry_lifetime; result->wrr_subchannel_list_size = wrr_subchannel_list_size - other.wrr_subchannel_list_size; result->wrr_subchannel_ready_size = diff --git a/src/core/telemetry/stats_data.h b/src/core/telemetry/stats_data.h index 6220a118f20..89f360e2686 100644 --- a/src/core/telemetry/stats_data.h +++ b/src/core/telemetry/stats_data.h @@ -35,6 +35,7 @@ class Histogram_100000_20 { public: static int BucketFor(int value); const uint64_t* buckets() const { return buckets_; } + size_t bucket_count() const { return 20; } friend Histogram_100000_20 operator-(const Histogram_100000_20& left, const Histogram_100000_20& right); @@ -58,6 +59,7 @@ class Histogram_65536_26 { public: static int BucketFor(int value); const uint64_t* buckets() const { return buckets_; } + size_t bucket_count() const { return 26; } friend Histogram_65536_26 operator-(const Histogram_65536_26& left, const Histogram_65536_26& right); @@ -81,6 +83,7 @@ class Histogram_100_20 { public: static int BucketFor(int value); const uint64_t* buckets() const { return buckets_; } + size_t bucket_count() const { return 20; } friend Histogram_100_20 operator-(const Histogram_100_20& left, const Histogram_100_20& right); @@ -104,6 +107,7 @@ class Histogram_16777216_20 { public: static int BucketFor(int value); const uint64_t* buckets() const { return buckets_; } + size_t bucket_count() const { return 20; } friend Histogram_16777216_20 operator-(const Histogram_16777216_20& left, const Histogram_16777216_20& right); @@ -127,6 +131,7 @@ class Histogram_80_10 { public: static int BucketFor(int value); const uint64_t* buckets() const { return buckets_; } + size_t bucket_count() const { return 10; } friend Histogram_80_10 operator-(const Histogram_80_10& left, const Histogram_80_10& right); @@ -150,6 +155,7 @@ class Histogram_10000_20 { public: static int BucketFor(int value); const uint64_t* buckets() const { return buckets_; } + size_t bucket_count() const { return 20; } friend Histogram_10000_20 operator-(const Histogram_10000_20& left, const Histogram_10000_20& right); @@ -168,6 +174,30 @@ class HistogramCollector_10000_20 { private: std::atomic buckets_[20]{}; }; +class HistogramCollector_1800000_40; +class Histogram_1800000_40 { + public: + static int BucketFor(int value); + const uint64_t* buckets() const { return buckets_; } + size_t bucket_count() const { return 40; } + friend Histogram_1800000_40 operator-(const Histogram_1800000_40& left, + const Histogram_1800000_40& right); + + private: + friend class HistogramCollector_1800000_40; + uint64_t buckets_[40]{}; +}; +class HistogramCollector_1800000_40 { + public: + void Increment(int value) { + buckets_[Histogram_1800000_40::BucketFor(value)].fetch_add( + 1, std::memory_order_relaxed); + } + void Collect(Histogram_1800000_40* result) const; + + private: + std::atomic buckets_[40]{}; +}; struct GlobalStats { enum class Counter { kClientCallsCreated, @@ -185,6 +215,8 @@ struct GlobalStats { kHttp2WritesBegun, kHttp2TransportStalls, kHttp2StreamStalls, + kHttp2HpackHits, + kHttp2HpackMisses, kCqPluckCreates, kCqNextCreates, kCqCallbackCreates, @@ -213,6 +245,7 @@ struct GlobalStats { kTcpReadOfferIovSize, kHttp2SendMessageSize, kHttp2MetadataSize, + kHttp2HpackEntryLifetime, kWrrSubchannelListSize, kWrrSubchannelReadySize, kWorkSerializerRunTimeMs, @@ -259,6 +292,8 @@ struct GlobalStats { uint64_t http2_writes_begun; uint64_t http2_transport_stalls; uint64_t http2_stream_stalls; + uint64_t http2_hpack_hits; + uint64_t http2_hpack_misses; uint64_t cq_pluck_creates; uint64_t cq_next_creates; uint64_t cq_callback_creates; @@ -287,6 +322,7 @@ struct GlobalStats { Histogram_80_10 tcp_read_offer_iov_size; Histogram_16777216_20 http2_send_message_size; Histogram_65536_26 http2_metadata_size; + Histogram_1800000_40 http2_hpack_entry_lifetime; Histogram_10000_20 wrr_subchannel_list_size; Histogram_10000_20 wrr_subchannel_ready_size; Histogram_100000_20 work_serializer_run_time_ms; @@ -367,6 +403,12 @@ class GlobalStatsCollector { data_.this_cpu().http2_stream_stalls.fetch_add(1, std::memory_order_relaxed); } + void IncrementHttp2HpackHits() { + data_.this_cpu().http2_hpack_hits.fetch_add(1, std::memory_order_relaxed); + } + void IncrementHttp2HpackMisses() { + data_.this_cpu().http2_hpack_misses.fetch_add(1, std::memory_order_relaxed); + } void IncrementCqPluckCreates() { data_.this_cpu().cq_pluck_creates.fetch_add(1, std::memory_order_relaxed); } @@ -447,6 +489,9 @@ class GlobalStatsCollector { void IncrementHttp2MetadataSize(int value) { data_.this_cpu().http2_metadata_size.Increment(value); } + void IncrementHttp2HpackEntryLifetime(int value) { + data_.this_cpu().http2_hpack_entry_lifetime.Increment(value); + } void IncrementWrrSubchannelListSize(int value) { data_.this_cpu().wrr_subchannel_list_size.Increment(value); } @@ -526,6 +571,8 @@ class GlobalStatsCollector { std::atomic http2_writes_begun{0}; std::atomic http2_transport_stalls{0}; std::atomic http2_stream_stalls{0}; + std::atomic http2_hpack_hits{0}; + std::atomic http2_hpack_misses{0}; std::atomic cq_pluck_creates{0}; std::atomic cq_next_creates{0}; std::atomic cq_callback_creates{0}; @@ -551,6 +598,7 @@ class GlobalStatsCollector { HistogramCollector_80_10 tcp_read_offer_iov_size; HistogramCollector_16777216_20 http2_send_message_size; HistogramCollector_65536_26 http2_metadata_size; + HistogramCollector_1800000_40 http2_hpack_entry_lifetime; HistogramCollector_10000_20 wrr_subchannel_list_size; HistogramCollector_10000_20 wrr_subchannel_ready_size; HistogramCollector_100000_20 work_serializer_run_time_ms; diff --git a/src/core/telemetry/stats_data.yaml b/src/core/telemetry/stats_data.yaml index 3d96ddfff6c..6b1f04878d0 100644 --- a/src/core/telemetry/stats_data.yaml +++ b/src/core/telemetry/stats_data.yaml @@ -80,6 +80,14 @@ max: 65536 buckets: 26 doc: Number of bytes consumed by metadata, according to HPACK accounting rules +- counter: http2_hpack_hits + doc: Number of HPACK cache hits +- counter: http2_hpack_misses + doc: Number of HPACK cache misses (entries added but never used) +- histogram: http2_hpack_entry_lifetime + doc: Lifetime of HPACK entries in the cache (in milliseconds) + max: 1800000 + buckets: 40 # completion queues - counter: cq_pluck_creates doc: Number of completion queues created for cq_pluck (indicates sync api usage) diff --git a/src/core/util/unique_ptr_with_bitset.h b/src/core/util/unique_ptr_with_bitset.h new file mode 100644 index 00000000000..3bba0f670f1 --- /dev/null +++ b/src/core/util/unique_ptr_with_bitset.h @@ -0,0 +1,84 @@ +// Copyright 2024 gRPC authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef GRPC_SRC_CORE_UTIL_UNIQUE_PTR_WITH_BITSET_H +#define GRPC_SRC_CORE_UTIL_UNIQUE_PTR_WITH_BITSET_H + +#include +#include + +#include "absl/log/check.h" +#include "absl/numeric/bits.h" + +namespace grpc_core { + +// Like std::unique_ptr, but also includes a small bitset stored in the lower +// bits of the underlying T*. +template +class UniquePtrWithBitset { + public: + UniquePtrWithBitset() : p_(0) {} + // NOLINTNEXTLINE(google-explicit-constructor) + UniquePtrWithBitset(std::nullptr_t) : p_(0) {} + explicit UniquePtrWithBitset(T* p) : p_(reinterpret_cast(p)) {} + // NOLINTNEXTLINE(google-explicit-constructor) + UniquePtrWithBitset(std::unique_ptr&& p) + : UniquePtrWithBitset(p.release()) {} + ~UniquePtrWithBitset() { delete get(); } + UniquePtrWithBitset(const UniquePtrWithBitset&) = delete; + UniquePtrWithBitset& operator=(const UniquePtrWithBitset&) = delete; + UniquePtrWithBitset(UniquePtrWithBitset&& other) noexcept + : p_(std::exchange(other.p_, 0)) {} + UniquePtrWithBitset& operator=(UniquePtrWithBitset&& other) noexcept { + p_ = std::exchange(other.p_, 0); + return *this; + } + + T* get() const { return reinterpret_cast(p_ & ~kBitMask); } + T* operator->() const { return get(); } + T& operator*() const { return *get(); } + explicit operator bool() const { return get() != nullptr; } + void reset(T* p = nullptr) { + uintptr_t bits = p_ & kBitMask; + delete get(); + p_ = reinterpret_cast(p) | bits; + } + + void SetBit(size_t bit) { + DCHECK_LT(bit, kBits); + p_ |= 1 << bit; + } + void ClearBit(size_t bit) { + DCHECK_LT(bit, kBits); + p_ &= ~(1 << bit); + } + bool TestBit(size_t bit) const { + DCHECK_LT(bit, kBits); + return p_ & (1 << bit); + } + + friend bool operator==(const UniquePtrWithBitset& a, + const UniquePtrWithBitset& b) { + return a.p_ == b.p_; + } + + private: + static_assert(kBits <= absl::countr_zero(alignof(T)), "kBits too large"); + static constexpr uintptr_t kBitMask = (1 << kBits) - 1; + uintptr_t p_; +}; + +} // namespace grpc_core + +#endif // GRPC_SRC_CORE_UTIL_UNIQUE_PTR_WITH_BITSET_H diff --git a/test/core/transport/chttp2/hpack_parser_table_test.cc b/test/core/transport/chttp2/hpack_parser_table_test.cc index ea9741d1d31..8743b079006 100644 --- a/test/core/transport/chttp2/hpack_parser_table_test.cc +++ b/test/core/transport/chttp2/hpack_parser_table_test.cc @@ -28,11 +28,12 @@ #include "src/core/lib/iomgr/exec_ctx.h" #include "src/core/lib/slice/slice.h" +#include "src/core/telemetry/stats.h" #include "test/core/test_util/test_config.h" namespace grpc_core { namespace { -void AssertIndex(const HPackTable* tbl, uint32_t idx, const char* key, +void AssertIndex(HPackTable* tbl, uint32_t idx, const char* key, const char* value) { const auto* md = tbl->Lookup(idx); ASSERT_NE(md, nullptr); @@ -113,6 +114,8 @@ TEST(HpackParserTableTest, ManyAdditions) { ExecCtx exec_ctx; + auto stats_before = global_stats().Collect(); + for (i = 0; i < 100000; i++) { std::string key = absl::StrCat("K.", i); std::string value = absl::StrCat("VALUE.", i); @@ -134,6 +137,56 @@ TEST(HpackParserTableTest, ManyAdditions) { value.c_str()); } } + + auto stats_after = global_stats().Collect(); + + EXPECT_EQ(stats_after->http2_hpack_hits - stats_before->http2_hpack_hits, + 100000); + EXPECT_EQ(stats_after->http2_hpack_misses, stats_before->http2_hpack_misses); +} + +TEST(HpackParserTableTest, ManyUnusedAdditions) { + auto tbl = std::make_unique(); + int i; + + ExecCtx exec_ctx; + + auto stats_before = global_stats().Collect(); + const Timestamp start = Timestamp::Now(); + + for (i = 0; i < 100000; i++) { + std::string key = absl::StrCat("K.", i); + std::string value = absl::StrCat("VALUE.", i); + auto key_slice = Slice::FromCopiedString(key); + auto value_slice = Slice::FromCopiedString(value); + auto memento = HPackTable::Memento{ + ParsedMetadata( + ParsedMetadata::FromSlicePair{}, + std::move(key_slice), std::move(value_slice), + key.length() + value.length() + 32), + nullptr}; + ASSERT_TRUE(tbl->Add(std::move(memento))); + } + + tbl.reset(); + + auto stats_after = global_stats().Collect(); + const Timestamp end = Timestamp::Now(); + + EXPECT_EQ(stats_after->http2_hpack_hits, stats_before->http2_hpack_hits); + EXPECT_EQ(stats_after->http2_hpack_misses - stats_before->http2_hpack_misses, + 100000); + + size_t num_buckets_changed = 0; + const auto& lifetime_before = stats_before->http2_hpack_entry_lifetime; + const auto& lifetime_after = stats_after->http2_hpack_entry_lifetime; + for (size_t i = 0; i < lifetime_before.bucket_count(); i++) { + if (lifetime_before.buckets()[i] != lifetime_after.buckets()[i]) { + EXPECT_LE(i, lifetime_before.BucketFor((end - start).millis())); + num_buckets_changed++; + } + } + EXPECT_GT(num_buckets_changed, 0); } } // namespace grpc_core diff --git a/test/core/util/BUILD b/test/core/util/BUILD index a21eae8c2d2..41f7fc72549 100644 --- a/test/core/util/BUILD +++ b/test/core/util/BUILD @@ -112,6 +112,19 @@ grpc_cc_test( ], ) +grpc_cc_test( + name = "unique_ptr_with_bitset_test", + srcs = ["unique_ptr_with_bitset_test.cc"], + external_deps = ["gtest"], + language = "C++", + uses_event_engine = False, + uses_polling = False, + deps = [ + "//:gpr_platform", + "//src/core:unique_ptr_with_bitset", + ], +) + grpc_cc_test( name = "useful_test", srcs = ["useful_test.cc"], diff --git a/test/core/util/unique_ptr_with_bitset_test.cc b/test/core/util/unique_ptr_with_bitset_test.cc new file mode 100644 index 00000000000..e5b67a36ff9 --- /dev/null +++ b/test/core/util/unique_ptr_with_bitset_test.cc @@ -0,0 +1,60 @@ +// +// +// 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 "src/core/util/unique_ptr_with_bitset.h" + +#include + +#include +#include + +#include "gtest/gtest.h" + +#include + +namespace grpc_core { + +TEST(UniquePtrWithBitsetTest, Basic) { + UniquePtrWithBitset ptr; + EXPECT_EQ(ptr.get(), nullptr); + EXPECT_EQ(ptr.TestBit(0), false); + ptr.reset(new int(42)); + EXPECT_EQ(*ptr, 42); + EXPECT_EQ(ptr.TestBit(0), false); + ptr.SetBit(0); + EXPECT_EQ(ptr.TestBit(0), true); + ptr.reset(); + EXPECT_EQ(ptr.get(), nullptr); + EXPECT_EQ(ptr.TestBit(0), true); + ptr.ClearBit(0); + EXPECT_EQ(ptr.TestBit(0), false); + ptr.reset(new int(43)); + ptr.SetBit(0); + + UniquePtrWithBitset ptr2; + ptr2 = std::move(ptr); + EXPECT_EQ(*ptr2, 43); + EXPECT_EQ(ptr2.TestBit(0), true); +} + +} // namespace grpc_core + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/tools/codegen/core/gen_stats_data.py b/tools/codegen/core/gen_stats_data.py index b7e66f00107..44fdcb00abc 100755 --- a/tools/codegen/core/gen_stats_data.py +++ b/tools/codegen/core/gen_stats_data.py @@ -302,6 +302,10 @@ with open("src/core/telemetry/stats_data.h", "w") as H: print(" public:", file=H) print(" static int BucketFor(int value);", file=H) print(" const uint64_t* buckets() const { return buckets_; }", file=H) + print( + " size_t bucket_count() const { return %d; }" % shape.buckets, + file=H, + ) print( " friend Histogram_%d_%d operator-(const Histogram_%d_%d& left," " const Histogram_%d_%d& right);" diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index a1ec0b5c168..68987e28551 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -2956,6 +2956,7 @@ src/core/util/time.cc \ src/core/util/time_precise.cc \ src/core/util/time_precise.h \ src/core/util/tmpfile.h \ +src/core/util/unique_ptr_with_bitset.h \ src/core/util/upb_utils.h \ src/core/util/useful.h \ src/core/util/windows/cpu.cc \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index ccc8cabeb95..c0bdead5549 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -2736,6 +2736,7 @@ src/core/util/time.cc \ src/core/util/time_precise.cc \ src/core/util/time_precise.h \ src/core/util/tmpfile.h \ +src/core/util/unique_ptr_with_bitset.h \ src/core/util/upb_utils.h \ src/core/util/useful.h \ src/core/util/windows/cpu.cc \ diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index a056f7e79f1..d4d85942684 100644 --- a/tools/run_tests/generated/tests.json +++ b/tools/run_tests/generated/tests.json @@ -11419,6 +11419,30 @@ ], "uses_polling": false }, + { + "args": [], + "benchmark": false, + "ci_platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "gtest": true, + "language": "c++", + "name": "unique_ptr_with_bitset_test", + "platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "uses_polling": false + }, { "args": [], "benchmark": false,