Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

absl: Absl hash hook in a couple of places rather than hash functors #8179

Merged
merged 6 commits into from
Sep 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should consider if you still want to use these aliases or not. Either is fine, just posing it as a question

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My inclination is to keep them. I am curious: is there any runtime benefit to having this default hasher, compared to specifying the functor? Or it's just nicer because the type is simpler to type?

I guess one functional difference is that we're using absl's default hash algorithm rather than xxhash, which maybe is tuned for the swiss tables better?


// 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