From a749aa486cf4930adb685cacc208dd14030303de Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 12 Sep 2019 12:32:27 -0400 Subject: [PATCH] absl: Absl hash hook in a couple of places rather than hash functors (#8179) Signed-off-by: Joshua Marantz --- source/common/common/utility.h | 7 ---- source/common/stats/symbol_table_impl.h | 43 ++++++++++++--------- source/common/stats/tag_producer_impl.h | 4 +- test/common/stats/BUILD | 1 + test/common/stats/symbol_table_impl_test.cc | 21 ++++++++++ 5 files changed, 49 insertions(+), 27 deletions(-) diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 9a74512da743..883bd5051970 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -500,13 +500,6 @@ template class IntervalSetImpl : public IntervalSet { std::set intervals_; // Intervals do not overlap or abut. }; -/** - * Hashing functor for use with unordered_map and unordered_set with string_view as a key. - */ -struct StringViewHash { - std::size_t operator()(const absl::string_view& k) const { return HashUtil::xxHash64(k); } -}; - /** * Hashing functor for use with enum class types. * This is needed for GCC 5.X; newer versions of GCC, as well as clang7, provide native hashing diff --git a/source/common/stats/symbol_table_impl.h b/source/common/stats/symbol_table_impl.h index dbbb633dcbe8..b83da159c035 100644 --- a/source/common/stats/symbol_table_impl.h +++ b/source/common/stats/symbol_table_impl.h @@ -229,7 +229,7 @@ class SymbolTableImpl : public SymbolTable { // Bitmap implementation. // The encode map stores both the symbol and the ref count of that symbol. // Using absl::string_view lets us only store the complete string once, in the decode map. - using EncodeMap = absl::flat_hash_map; + using EncodeMap = absl::flat_hash_map; using DecodeMap = absl::flat_hash_map; EncodeMap encode_map_ GUARDED_BY(lock_); DecodeMap decode_map_ GUARDED_BY(lock_); @@ -317,16 +317,31 @@ class StatName { // for lookup in a cache, and then on a miss need to store the data directly. StatName(const StatName& src, SymbolTable::Storage memory); + /** + * Defines default hash function so StatName can be used as a key in an absl + * hash-table without specifying a functor. See + * https://abseil.io/docs/cpp/guides/hash for details. + */ + template friend H AbslHashValue(H h, StatName stat_name) { + if (stat_name.empty()) { + return H::combine(std::move(h), absl::string_view()); + } + + // Casts the raw data as a string_view. Note that this string_view will not + // be in human-readable form, but it will be compatible with a string-view + // hasher. + const char* cdata = reinterpret_cast(stat_name.data()); + absl::string_view data_as_string_view = absl::string_view(cdata, stat_name.dataSize()); + return H::combine(std::move(h), data_as_string_view); + } + /** * Note that this hash function will return a different hash than that of * the elaborated string. * * @return uint64_t a hash of the underlying representation. */ - uint64_t hash() const { - const char* cdata = reinterpret_cast(data()); - return HashUtil::xxHash64(absl::string_view(cdata, dataSize())); - } + uint64_t hash() const { return absl::Hash()(*this); } bool operator==(const StatName& rhs) const { const uint64_t sz = dataSize(); @@ -339,6 +354,9 @@ class StatName { * overhead for the size itself. */ uint64_t dataSize() const { + if (size_and_data_ == nullptr) { + return 0; + } return size_and_data_[0] | (static_cast(size_and_data_[1]) << 8); } @@ -523,22 +541,11 @@ class StatNameList { SymbolTable::StoragePtr storage_; }; -// Helper class for constructing hash-tables with StatName keys. -struct StatNameHash { - size_t operator()(const StatName& a) const { return a.hash(); } -}; - -// Helper class for constructing hash-tables with StatName keys. -struct StatNameCompare { - bool operator()(const StatName& a, const StatName& b) const { return a == b; } -}; - // Value-templatized hash-map with StatName key. -template -using StatNameHashMap = absl::flat_hash_map; +template using StatNameHashMap = absl::flat_hash_map; // Hash-set of StatNames -using StatNameHashSet = absl::flat_hash_set; +using StatNameHashSet = absl::flat_hash_set; // Helper class for sorting StatNames. struct StatNameLessThan { diff --git a/source/common/stats/tag_producer_impl.h b/source/common/stats/tag_producer_impl.h index 6a4fcb07a6ef..e18f111e9238 100644 --- a/source/common/stats/tag_producer_impl.h +++ b/source/common/stats/tag_producer_impl.h @@ -17,6 +17,7 @@ #include "common/config/well_known_names.h" #include "common/protobuf/protobuf.h" +#include "absl/container/flat_hash_map.h" #include "absl/strings/string_view.h" namespace Envoy { @@ -97,8 +98,7 @@ class TagProducerImpl : public TagProducer { // Maps a prefix word extracted out of a regex to a vector of TagExtractors. Note that // the storage for the prefix string is owned by the TagExtractor, which, depending on // implementation, may need make a copy of the prefix. - std::unordered_map, StringViewHash> - tag_extractor_prefix_map_; + absl::flat_hash_map> tag_extractor_prefix_map_; std::vector default_tags_; }; diff --git a/test/common/stats/BUILD b/test/common/stats/BUILD index ab725c0a2608..b4b528194103 100644 --- a/test/common/stats/BUILD +++ b/test/common/stats/BUILD @@ -84,6 +84,7 @@ envoy_cc_test( envoy_cc_test( name = "symbol_table_impl_test", srcs = ["symbol_table_impl_test.cc"], + external_deps = ["abseil_hash_testing"], deps = [ ":stat_test_utility_lib", "//source/common/common:mutex_tracer_lib", diff --git a/test/common/stats/symbol_table_impl_test.cc b/test/common/stats/symbol_table_impl_test.cc index d6a533b092a5..4917caeca936 100644 --- a/test/common/stats/symbol_table_impl_test.cc +++ b/test/common/stats/symbol_table_impl_test.cc @@ -10,6 +10,7 @@ #include "test/test_common/logging.h" #include "test/test_common/utility.h" +#include "absl/hash/hash_testing.h" #include "absl/synchronization/blocking_counter.h" #include "gtest/gtest.h" @@ -565,6 +566,26 @@ TEST_P(StatNameTest, StatNameSet) { EXPECT_NE(dynamic2.data(), dynamic.data()); } +TEST_P(StatNameTest, StatNameEmptyEquivalent) { + StatName empty1; + StatName empty2 = makeStat(""); + StatName non_empty = makeStat("a"); + EXPECT_EQ(empty1, empty2); + EXPECT_EQ(empty1.hash(), empty2.hash()); + EXPECT_NE(empty1, non_empty); + EXPECT_NE(empty2, non_empty); + EXPECT_NE(empty1.hash(), non_empty.hash()); + EXPECT_NE(empty2.hash(), non_empty.hash()); +} + +TEST_P(StatNameTest, SupportsAbslHash) { + EXPECT_TRUE(absl::VerifyTypeImplementsAbslHashCorrectly({ + StatName(), + makeStat(""), + makeStat("hello.world"), + })); +} + // Tests the memory savings realized from using symbol tables with 1k // clusters. This test shows the memory drops from almost 8M to less than // 2M. Note that only SymbolTableImpl is tested for memory consumption,