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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Sep 12, 2019
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4ebc9c5
Use implicit absl hashing in a couple of places, rather than explicit…
jmarantz Sep 3, 2019
1fa7942
Merge branch 'master' into absl-hash-hook
jmarantz Sep 3, 2019
4bbe713
Merge branch 'master' into absl-hash-hook
jmarantz Sep 8, 2019
d863fc0
Merge branch 'master' into absl-hash-hook
jmarantz Sep 9, 2019
7ab37ee
Remove superfluous (and illegal) hash override for string_view, and s…
jmarantz Sep 10, 2019
cfa62e3
Merge branch 'master' into absl-hash-hook
jmarantz Sep 11, 2019
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
9 changes: 5 additions & 4 deletions source/common/common/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,11 +501,12 @@ template <typename Value> class IntervalSetImpl : public IntervalSet<Value> {
};

/**
* 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 <typename H> H AbslHashValue(H h, absl::string_view str) {
jmarantz marked this conversation as resolved.
Show resolved Hide resolved
return H::combine(std::move(h), HashUtil::xxHash64(str));
}

/**
* Hashing functor for use with enum class types.
Expand Down
16 changes: 7 additions & 9 deletions source/common/stats/symbol_table_impl.h
View file
Open in desktop
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 @@ -328,6 +328,10 @@ class StatName {
return HashUtil::xxHash64(absl::string_view(cdata, dataSize()));
}

template <typename H> friend H AbslHashValue(H h, StatName stat_name) {
jmarantz marked this conversation as resolved.
Show resolved Hide resolved
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;
Expand Down Expand Up @@ -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 <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
View file
Open in desktop
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