Skip to content

Commit

Permalink
absl: Absl hash hook in a couple of places rather than hash functors (e…
Browse files Browse the repository at this point in the history
…nvoyproxy#8179)

Signed-off-by: Joshua Marantz <jmarantz@google.com>
  • Loading branch information
jmarantz committed Sep 24, 2019
1 parent 50d2320 commit a749aa4
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 27 deletions.
7 changes: 0 additions & 7 deletions source/common/common/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -500,13 +500,6 @@ template <typename Value> class IntervalSetImpl : public IntervalSet<Value> {
std::set<Interval, Compare> 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
Expand Down
43 changes: 25 additions & 18 deletions source/common/stats/symbol_table_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<absl::string_view, SharedSymbol, StringViewHash>;
using EncodeMap = absl::flat_hash_map<absl::string_view, SharedSymbol>;
using DecodeMap = absl::flat_hash_map<Symbol, InlineStringPtr>;
EncodeMap encode_map_ GUARDED_BY(lock_);
DecodeMap decode_map_ GUARDED_BY(lock_);
Expand Down Expand Up @@ -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 <typename H> 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<const char*>(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<const char*>(data());
return HashUtil::xxHash64(absl::string_view(cdata, dataSize()));
}
uint64_t hash() const { return absl::Hash<StatName>()(*this); }

bool operator==(const StatName& rhs) const {
const uint64_t sz = dataSize();
Expand All @@ -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<uint64_t>(size_and_data_[1]) << 8);
}

Expand Down Expand Up @@ -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 <class T>
using StatNameHashMap = absl::flat_hash_map<StatName, T, StatNameHash, StatNameCompare>;
template <class T> using StatNameHashMap = absl::flat_hash_map<StatName, T>;

// Hash-set of StatNames
using StatNameHashSet = absl::flat_hash_set<StatName, StatNameHash, StatNameCompare>;
using StatNameHashSet = absl::flat_hash_set<StatName>;

// Helper class for sorting StatNames.
struct StatNameLessThan {
Expand Down
4 changes: 2 additions & 2 deletions source/common/stats/tag_producer_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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<absl::string_view, std::vector<TagExtractorPtr>, StringViewHash>
tag_extractor_prefix_map_;
absl::flat_hash_map<absl::string_view, std::vector<TagExtractorPtr>> tag_extractor_prefix_map_;
std::vector<Tag> default_tags_;
};

Expand Down
1 change: 1 addition & 0 deletions test/common/stats/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
21 changes: 21 additions & 0 deletions test/common/stats/symbol_table_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit a749aa4

Please sign in to comment.