From 4ebc9c54f22b971b24a54bf5682dafe636b6072b Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Tue, 3 Sep 2019 10:54:12 -0400 Subject: [PATCH 1/2] Use implicit absl hashing in a couple of places, rather than explicit template args specifying hasher. Signed-off-by: Joshua Marantz --- source/common/common/utility.h | 9 +++++---- source/common/stats/symbol_table_impl.h | 16 +++++++--------- source/common/stats/tag_producer_impl.h | 4 ++-- 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 48c09be97548..ae81e7ab7c2d 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -487,11 +487,12 @@ template class IntervalSetImpl : public IntervalSet { }; /** - * Hashing functor for use with unordered_map and unordered_set with string_view as a key. + * Hashing functor for use with absl hash tables, which can use hash on + * string_view if this declaration is visible. */ -struct StringViewHash { - std::size_t operator()(const absl::string_view& k) const { return HashUtil::xxHash64(k); } -}; +template H AbslHashValue(H h, absl::string_view str) { + return H::combine(std::move(h), HashUtil::xxHash64(str)); +} /** * Hashing functor for use with enum class types. diff --git a/source/common/stats/symbol_table_impl.h b/source/common/stats/symbol_table_impl.h index dbbb633dcbe8..864c7480b7e0 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_); @@ -328,6 +328,10 @@ class StatName { return HashUtil::xxHash64(absl::string_view(cdata, dataSize())); } + template friend H AbslHashValue(H h, StatName stat_name) { + return H::combine(std::move(h), stat_name.hash()); + } + bool operator==(const StatName& rhs) const { const uint64_t sz = dataSize(); return sz == rhs.dataSize() && memcmp(data(), rhs.data(), sz * sizeof(uint8_t)) == 0; @@ -528,17 +532,11 @@ 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_; }; From 7ab37eeb775ede64720f5c4587db95d164bb618e Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Tue, 10 Sep 2019 17:18:17 -0400 Subject: [PATCH 2/2] Remove superfluous (and illegal) hash override for string_view, and simplify the one for StatName. Signed-off-by: Joshua Marantz --- source/common/common/utility.h | 8 ----- source/common/stats/symbol_table_impl.h | 35 +++++++++++++-------- test/common/stats/BUILD | 1 + test/common/stats/symbol_table_impl_test.cc | 21 +++++++++++++ 4 files changed, 44 insertions(+), 21 deletions(-) diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 4c66df0c0027..883bd5051970 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -500,14 +500,6 @@ template class IntervalSetImpl : public IntervalSet { std::set intervals_; // Intervals do not overlap or abut. }; -/** - * Hashing functor for use with absl hash tables, which can use hash on - * string_view if this declaration is visible. - */ -template H AbslHashValue(H h, absl::string_view str) { - return H::combine(std::move(h), HashUtil::xxHash64(str)); -} - /** * 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 864c7480b7e0..b83da159c035 100644 --- a/source/common/stats/symbol_table_impl.h +++ b/source/common/stats/symbol_table_impl.h @@ -317,20 +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())); - } - - template friend H AbslHashValue(H h, StatName stat_name) { - return H::combine(std::move(h), stat_name.hash()); - } + uint64_t hash() const { return absl::Hash()(*this); } bool operator==(const StatName& rhs) const { const uint64_t sz = dataSize(); @@ -343,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); } @@ -527,11 +541,6 @@ 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(); } -}; - // Value-templatized hash-map with StatName key. template using StatNameHashMap = absl::flat_hash_map; 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,