stats: cache the results of the stats-matcher class.

source/common/common/hash.h

@@ -77,16 +77,18 @@ class MurmurHash {
   static inline uint64_t shift_mix(uint64_t v) { return v ^ (v >> 47); }
 };
 
-struct CharStarHash {
+struct ConstCharStarHash {
   size_t operator()(const char* a) const { return HashUtil::xxHash64(a); }
 };
 
-struct CharStarEqual {
+struct ConstCharStarEqual {
   size_t operator()(const char* a, const char* b) const { return strcmp(a, b) == 0; }
 };
 
 template <class Value>
-using CharStarHashMap = absl::flat_hash_map<const char*, Value, CharStarHash, CharStarEqual>;
-using CharStarHashSet = absl::flat_hash_set<const char*, CharStarHash, CharStarEqual>;
+using ConstCharStarHashMap =
+    absl::flat_hash_map<const char*, Value, ConstCharStarHash, ConstCharStarEqual>;
+using ConstCharStarHashSet =
+    absl::flat_hash_set<const char*, ConstCharStarHash, ConstCharStarEqual>;
 
 } // namespace Envoy

source/common/stats/BUILD

@@ -8,6 +8,17 @@ load(
 
 envoy_package()
 
+envoy_cc_library(
+    name = "char_star_set_lib",
+    srcs = ["char_star_set.cc"],
+    hdrs = ["char_star_set.h"],
+    external_deps = ["abseil_base"],
+    deps = [
+        "//source/common/common:hash_lib",
+        "//source/common/common:utility_lib",
+    ],
+)
+
 envoy_cc_library(
     name = "heap_stat_data_lib",
     srcs = ["heap_stat_data.cc"],
@@ -190,6 +201,7 @@ envoy_cc_library(
     srcs = ["thread_local_store.cc"],
     hdrs = ["thread_local_store.h"],
     deps = [
+        ":char_star_set_lib",
         ":heap_stat_data_lib",
         ":stats_lib",
         ":stats_matcher_lib",

source/common/stats/char_star_set.cc

@@ -0,0 +1,49 @@
+#include "common/stats/char_star_set.h"
+
+#include "common/common/utility.h"
+
+namespace Envoy {
+namespace Stats {
+
+CharStarSet::~CharStarSet() {
+  // It's generally safer to clear associative containers before mutating
+  // elements of them (e.g. by deleting them), unless you know the container
+  // internals well, 2-phase deletes like are more obviously correct.
+  std::vector<char*> keys;
+  keys.reserve(hash_set_.size());
+  for (char* p : hash_set_) {
+    keys.push_back(p);
+  }
+
+  hash_set_.clear();
+  for (char* p : keys) {
+    delete[] p;
+  }
+}
+
+const char* CharStarSet::insert(absl::string_view str) {
+  size_t bytes = str.size() + 1;
+  char* ptr = new char[bytes];
+  StringUtil::strlcpy(ptr, str.data(), bytes);
+  auto insertion = hash_set_.insert(ptr);
+  if (!insertion.second) {
+    delete[] ptr;
+    return *insertion.first;
+  }
+  return ptr;
+}
+
+const char* CharStarSet::find(const char* str) const {
+  // The const_cast is necessary because hash_set_ is declared as a
+  // flat_hash_set<char*>, and the find() method does not add a 'const'
+  // qualifier to its key template type. As long as we don't modify the returned
+  // iterator it will not actually mutate the key, and the const_cast is safe.
+  auto iter = hash_set_.find(const_cast<char*>(str));
+  if (iter == hash_set_.end()) {
+    return nullptr;
+  }
+  return *iter;
+}
+
+} // namespace Stats
+} // namespace Envoy

source/common/stats/char_star_set.h

@@ -0,0 +1,41 @@
+#pragma once
+
+#include "common/common/hash.h"
+
+#include "absl/container/flat_hash_set.h"
+
+namespace Envoy {
+namespace Stats {
+
+// Implements a set of NUL-terminated char*, with the property that once
+// inserted, the char* pointers remain stable for the life of the set. Note
+// there is currently no 'erase' method.
+//
+// Also note that this class may not have long-term value; it may be removed
+// once SymbolTables are integrated (#6161). With that, the current sole
+// use of this class, as a cache for disallowed stats from StatsMatcher.
+// could be replaced by a StatNameHashSet or similar.
+class CharStarSet {
+public:
+  CharStarSet() = default;
+  ~CharStarSet();
+  CharStarSet(const CharStarSet&) = delete;
+  CharStarSet& operator=(const CharStarSet&) = delete;
+
+  // Inserts a nul-terminated string, returning a stable char* reference to it.
+  const char* insert(absl::string_view str);
+
+  // Finds a stable reference for the specified string, returning nullptr if
+  // it's not in the set yet.
+  const char* find(const std::string& str) const { return find(str.c_str()); }
+  const char* find(const char* str) const;
+
+  // Returns the number of retained strings.
+  size_t size() { return hash_set_.size(); }
+
+private:
+  absl::flat_hash_set<char*, ConstCharStarHash, ConstCharStarEqual> hash_set_;
+};
+
+} // namespace Stats
+} // namespace Envoy

source/common/stats/thread_local_store.cc

@@ -58,10 +58,10 @@ void ThreadLocalStoreImpl::removeRejectedStats(StatMapClass& map, StatListClass&
     }
   }
   for (const char* stat_name : remove_list) {
-    auto p = map.find(stat_name);
-    ASSERT(p != map.end());
-    list.push_back(p->second); // Save SharedPtr to the list to avoid invalidating refs to stat.
-    map.erase(p);
+    auto iter = map.find(stat_name);
+    ASSERT(iter != map.end());
+    list.push_back(iter->second); // Save SharedPtr to the list to avoid invalidating refs to stat.
+    map.erase(iter);
   }
 }
 
@@ -75,7 +75,7 @@ bool ThreadLocalStoreImpl::rejects(const std::string& name) const {
 std::vector<CounterSharedPtr> ThreadLocalStoreImpl::counters() const {
   // Handle de-dup due to overlapping scopes.
   std::vector<CounterSharedPtr> ret;
-  CharStarHashSet names;
+  ConstCharStarHashSet names;
   Thread::LockGuard lock(lock_);
   for (ScopeImpl* scope : scopes_) {
     for (auto& counter : scope->central_cache_.counters_) {
@@ -98,7 +98,7 @@ ScopePtr ThreadLocalStoreImpl::createScope(const std::string& name) {
 std::vector<GaugeSharedPtr> ThreadLocalStoreImpl::gauges() const {
   // Handle de-dup due to overlapping scopes.
   std::vector<GaugeSharedPtr> ret;
-  CharStarHashSet names;
+  ConstCharStarHashSet names;
   Thread::LockGuard lock(lock_);
   for (ScopeImpl* scope : scopes_) {
     for (auto& gauge : scope->central_cache_.gauges_) {
@@ -217,12 +217,41 @@ std::atomic<uint64_t> ThreadLocalStoreImpl::ScopeImpl::next_scope_id_;
 
 ThreadLocalStoreImpl::ScopeImpl::~ScopeImpl() { parent_.releaseScopeCrossThread(this); }
 
+bool ThreadLocalStoreImpl::checkAndRememberRejection(const std::string& name,
+                                                     ConstCharStarHashSet* tls_rejected_stats) {
+  if (stats_matcher_->acceptsAll()) {
+    return false;
+  }
+
+  const char* rejected_name = rejected_stats_.find(name);
+  if (rejected_name == nullptr) {
+    if (rejects(name)) {
+      rejected_name = rejected_stats_.insert(name);
+    }
+  }
+  if (rejected_name != nullptr) {
+    if (tls_rejected_stats != nullptr) {
+      tls_rejected_stats->insert(rejected_name);
+    }
+    return true;
+  }
+  return false;
+}
+
 template <class StatType>
 StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat(
     const std::string& name, StatMap<std::shared_ptr<StatType>>& central_cache_map,
-    MakeStatFn<StatType> make_stat, StatMap<std::shared_ptr<StatType>>* tls_cache) {
+    MakeStatFn<StatType> make_stat, StatMap<std::shared_ptr<StatType>>* tls_cache,
+    ConstCharStarHashSet* tls_rejected_stats, StatType& null_stat) {
 
   const char* stat_key = name.c_str();
+
+  // We do name-rejections on the full name, prior to truncation.
+  if (tls_rejected_stats != nullptr &&
+      tls_rejected_stats->find(stat_key) != tls_rejected_stats->end()) {
+    return null_stat;
+  }
+
   std::unique_ptr<std::string> truncation_buffer;
   absl::string_view truncated_name = parent_.truncateStatNameIfNeeded(name);
   if (truncated_name.size() < name.size()) {
@@ -241,10 +270,13 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat(
   // We must now look in the central store so we must be locked. We grab a reference to the
   // central store location. It might contain nothing. In this case, we allocate a new stat.
   Thread::LockGuard lock(parent_.lock_);
-  auto p = central_cache_map.find(stat_key);
+  auto iter = central_cache_map.find(stat_key);
   std::shared_ptr<StatType>* central_ref = nullptr;
-  if (p != central_cache_map.end()) {
-    central_ref = &(p->second);
+  if (iter != central_cache_map.end()) {
+    central_ref = &(iter->second);
+  } else if (parent_.checkAndRememberRejection(name, tls_rejected_stats)) {
+    // Note that again we do the name-rejection lookup on the untruncated name.
+    return null_stat;
   } else {
     // If we had to truncate, warn now that we've missed all caches.
     if (truncation_buffer != nullptr) {
@@ -285,6 +317,10 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat(
 }
 
 Counter& ThreadLocalStoreImpl::ScopeImpl::counter(const std::string& name) {
+  if (parent_.rejectsAll()) {
+    return null_counter_;
+  }
+
   // Determine the final name based on the prefix and the passed name.
   //
   // Note that we can do map.find(final_name.c_str()), but we cannot do
@@ -295,15 +331,15 @@ Counter& ThreadLocalStoreImpl::ScopeImpl::counter(const std::string& name) {
   // strategy costs an extra hash lookup for each miss, but saves time
   // re-copying the string and significant memory overhead.
   std::string final_name = prefix_ + name;
-  if (parent_.rejects(final_name)) {
-    return null_counter_;
-  }
 
   // We now find the TLS cache. This might remain null if we don't have TLS
   // initialized currently.
   StatMap<CounterSharedPtr>* tls_cache = nullptr;
+  ConstCharStarHashSet* tls_rejected_stats = nullptr;
   if (!parent_.shutting_down_ && parent_.tls_) {
-    tls_cache = &parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_].counters_;
+    TlsCacheEntry& entry = parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_];
+    tls_cache = &entry.counters_;
+    tls_rejected_stats = &entry.rejected_stats_;
   }
 
   return safeMakeStat<Counter>(
@@ -312,7 +348,7 @@ Counter& ThreadLocalStoreImpl::ScopeImpl::counter(const std::string& name) {
          std::vector<Tag>&& tags) -> CounterSharedPtr {
         return allocator.makeCounter(name, std::move(tag_extracted_name), std::move(tags));
       },
-      tls_cache);
+      tls_cache, tls_rejected_stats, null_counter_);
 }
 
 void ThreadLocalStoreImpl::ScopeImpl::deliverHistogramToSinks(const Histogram& histogram,
@@ -332,6 +368,10 @@ void ThreadLocalStoreImpl::ScopeImpl::deliverHistogramToSinks(const Histogram& h
 }
 
 Gauge& ThreadLocalStoreImpl::ScopeImpl::gauge(const std::string& name) {
+  if (parent_.rejectsAll()) {
+    return null_gauge_;
+  }
+
   // See comments in counter(). There is no super clean way (via templates or otherwise) to
   // share this code so I'm leaving it largely duplicated for now.
   //
@@ -341,13 +381,13 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gauge(const std::string& name) {
   // do a find() first, using that if it succeeds. If it fails, then after we
   // construct the stat we can insert it into the required maps.
   std::string final_name = prefix_ + name;
-  if (parent_.rejects(final_name)) {
-    return null_gauge_;
-  }
 
   StatMap<GaugeSharedPtr>* tls_cache = nullptr;
+  ConstCharStarHashSet* tls_rejected_stats = nullptr;
   if (!parent_.shutting_down_ && parent_.tls_) {
-    tls_cache = &parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_].gauges_;
+    TlsCacheEntry& entry = parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_];
+    tls_cache = &entry.gauges_;
+    tls_rejected_stats = &entry.rejected_stats_;
   }
 
   return safeMakeStat<Gauge>(
@@ -356,10 +396,14 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gauge(const std::string& name) {
          std::vector<Tag>&& tags) -> GaugeSharedPtr {
         return allocator.makeGauge(name, std::move(tag_extracted_name), std::move(tags));
       },
-      tls_cache);
+      tls_cache, tls_rejected_stats, null_gauge_);
 }
 
 Histogram& ThreadLocalStoreImpl::ScopeImpl::histogram(const std::string& name) {
+  if (parent_.rejectsAll()) {
+    return null_histogram_;
+  }
+
   // See comments in counter(). There is no super clean way (via templates or otherwise) to
   // share this code so I'm leaving it largely duplicated for now.
   //
@@ -369,25 +413,29 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogram(const std::string& name) {
   // do a find() first, using that if it succeeds. If it fails, then after we
   // construct the stat we can insert it into the required maps.
   std::string final_name = prefix_ + name;
-  if (parent_.rejects(final_name)) {
-    return null_histogram_;
-  }
 
   StatMap<ParentHistogramSharedPtr>* tls_cache = nullptr;
+  ConstCharStarHashSet* tls_rejected_stats = nullptr;
   if (!parent_.shutting_down_ && parent_.tls_) {
-    tls_cache =
-        &parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_].parent_histograms_;
-    auto p = tls_cache->find(final_name.c_str());
-    if (p != tls_cache->end()) {
-      return *p->second;
+    TlsCacheEntry& entry = parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_];
+    tls_cache = &entry.parent_histograms_;
+    auto iter = tls_cache->find(final_name.c_str());
+    if (iter != tls_cache->end()) {
+      return *iter->second;
+    }
+    tls_rejected_stats = &entry.rejected_stats_;
+    if (tls_rejected_stats->find(final_name.c_str()) != tls_rejected_stats->end()) {
+      return null_histogram_;
     }
   }
 
   Thread::LockGuard lock(parent_.lock_);
-  auto p = central_cache_.histograms_.find(final_name.c_str());
+  auto iter = central_cache_.histograms_.find(final_name.c_str());
   ParentHistogramImplSharedPtr* central_ref = nullptr;
-  if (p != central_cache_.histograms_.end()) {
-    central_ref = &p->second;
+  if (iter != central_cache_.histograms_.end()) {
+    central_ref = &iter->second;
+  } else if (parent_.checkAndRememberRejection(final_name, tls_rejected_stats)) {
+    return null_histogram_;
   } else {
     std::vector<Tag> tags;
     std::string tag_extracted_name = parent_.getTagsForName(final_name, tags);
@@ -405,18 +453,17 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogram(const std::string& name) {
 
 Histogram& ThreadLocalStoreImpl::ScopeImpl::tlsHistogram(const std::string& name,
                                                          ParentHistogramImpl& parent) {
-  if (parent_.rejects(name)) {
-    return null_histogram_;
-  }
+  // tlsHistogram() is generally not called for a histogram that is rejected by
+  // the matcher, so no further rejection-checking is needed at this level.
+  // TlsHistogram inherits its reject/accept status from ParentHistogram.
 
   // See comments in counter() which explains the logic here.
-
   StatMap<TlsHistogramSharedPtr>* tls_cache = nullptr;
   if (!parent_.shutting_down_ && parent_.tls_) {
     tls_cache = &parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_].histograms_;
-    auto p = tls_cache->find(name.c_str());
-    if (p != tls_cache->end()) {
-      return *p->second;
+    auto iter = tls_cache->find(name.c_str());
+    if (iter != tls_cache->end()) {
+      return *iter->second;
     }
   }