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

source/common/common/hash.cc

@@ -34,4 +34,40 @@ uint64_t MurmurHash::murmurHash2_64(absl::string_view key, uint64_t seed) {
   return hash;
 }
 
+CharStarSet::~CharStarSet() {
+  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) {
+  char* p = new char[str.size() + 1];
+  memcpy(p, str.data(), str.size());
+  p[str.size()] = '\0';
+  auto insertion = hash_set_.insert(p);
+  if (!insertion.second) {
+    delete[] p;
+    return *insertion.first;
+  }
+  return p;
+}
+
+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 Envoy

source/common/common/hash.h

@@ -77,16 +77,43 @@ 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>;
+
+// 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.
+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 Envoy

source/common/stats/thread_local_store.cc

@@ -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()) {
@@ -245,6 +274,9 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat(
   std::shared_ptr<StatType>* central_ref = nullptr;
   if (p != central_cache_map.end()) {
     central_ref = &(p->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_;
+    TlsCacheEntry& entry = parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_];
+    tls_cache = &entry.parent_histograms_;
     auto p = tls_cache->find(final_name.c_str());
     if (p != tls_cache->end()) {
       return *p->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());
   ParentHistogramImplSharedPtr* central_ref = nullptr;
   if (p != central_cache_.histograms_.end()) {
     central_ref = &p->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,12 +453,11 @@ 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_;

source/common/stats/thread_local_store.h

@@ -171,13 +171,35 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
   const Stats::StatsOptions& statsOptions() const override { return stats_options_; }
 
 private:
-  template <class Stat> using StatMap = CharStarHashMap<Stat>;
+  friend class ThreadLocalStoreTestScope;
+
+  template <class Stat> using StatMap = ConstCharStarHashMap<Stat>;
 
   struct TlsCacheEntry {
     StatMap<CounterSharedPtr> counters_;
     StatMap<GaugeSharedPtr> gauges_;
     StatMap<TlsHistogramSharedPtr> histograms_;
     StatMap<ParentHistogramSharedPtr> parent_histograms_;
+
+    // We keep a TLS cache of rejected stat names. This costs memory, but
+    // reduces runtime overhead running the matcher. Moreover, once symbol
+    // tables are integrated, rejection will need the fully elaborated string,
+    // and it we need to take a global symbol-table lock to run. We keep
+    // this char* map here in the TLS cache to avoid taking a lock to compute
+    // rejection.
+    //
+    // To avoid duplicate copies of stats, this map references const char*
+    // values from ThreadLocalStore::rejected_stats_, which is implemented
+    // as a absl::flat_hash_map<char*>, so the keys will be stable and safe
+    // to reference from other threads even as the hash-map resizes.
+    //
+    // Note that once a stat or rejected name enters into the TLS Cache, it
+    // remains in memory forever. This will effectively leak memory as scopes
+    // are dynamically removed from the configuration. This could be resolved by
+    // (a) using weak_ptr in the TLS cache and (b) proactively cleaning up
+    // expired items in each thread periodically. Alternatively, when a scope
+    // is removed, we could post() a cleanup request to each thread's TLS cache.
+    ConstCharStarHashSet rejected_stats_;
   };
 
   struct CentralCacheEntry {
@@ -223,7 +245,8 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
     template <class StatType>
     StatType&
     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);
 
     static std::atomic<uint64_t> next_scope_id_;
 
@@ -254,8 +277,11 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
   void mergeInternal(PostMergeCb mergeCb);
   absl::string_view truncateStatNameIfNeeded(absl::string_view name);
   bool rejects(const std::string& name) const;
+  bool rejectsAll() const { return stats_matcher_->rejectsAll(); }
   template <class StatMapClass, class StatListClass>
   void removeRejectedStats(StatMapClass& map, StatListClass& list);
+  bool checkAndRememberRejection(const std::string& name, ConstCharStarHashSet* tls_rejected_stats)
+      EXCLUSIVE_LOCKS_REQUIRED(lock_);
 
   const Stats::StatsOptions& stats_options_;
   StatDataAllocator& alloc_;
@@ -267,6 +293,7 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
   std::list<std::reference_wrapper<Sink>> timer_sinks_;
   TagProducerPtr tag_producer_;
   StatsMatcherPtr stats_matcher_;
+  CharStarSet rejected_stats_ GUARDED_BY(lock_); // See comment in TLSCacheEntry above.
   std::atomic<bool> shutting_down_{};
   std::atomic<bool> merge_in_progress_{};
   Counter& num_last_resort_stats_;

test/common/common/hash_test.cc

@@ -38,4 +38,11 @@ TEST(Hash, stdhash) {
 }
 #endif
 
+TEST(Hash, charStarSet) {
+  CharStarSet set;
+  const char* foo = set.insert("foo");
+  EXPECT_EQ(foo, set.find("foo"));
+  EXPECT_EQ(foo, set.find(std::string("foo")));
+}
+
 } // namespace Envoy