WIP Integration of SymbolTable into HeapStatData / ThreadLocalStore

include/envoy/stats/stats.h

@@ -39,6 +39,9 @@ class Metric {
    * Indicates whether this metric has been updated since the server was started.
    */
   virtual bool used() const PURE;
+
+  virtual uint64_t hash() const PURE;
+  virtual bool operator==(const Metric& rhs) const PURE;
 };
 
 /**

include/envoy/stats/symbol_table.h

@@ -18,6 +18,8 @@ class StatName {
 public:
   virtual ~StatName(){};
   virtual std::string toString() const PURE;
+  virtual uint64_t hash() const PURE;
+  virtual bool operator==(const StatName& rhs) const PURE;
 };
 
 using StatNamePtr = std::unique_ptr<StatName>;

source/common/common/hash.h

@@ -1,6 +1,7 @@
 #pragma once
 
 #include <string>
+#include <vector>
 
 #include "absl/strings/ascii.h"
 #include "absl/strings/string_view.h"
@@ -35,6 +36,15 @@ class HashUtil {
     };
     return hash;
   }
+
+  // https://stackoverflow.com/a/27216842
+  static uint64_t hashVector(std::vector<uint32_t> const& vec) {
+    std::size_t seed = vec.size();
+    for (auto& i : vec) {
+      seed ^= i + 0x9e3779b9 + (seed << 6) + (seed >> 2);
+    }
+    return seed;
+  }
 };
 
 } // namespace Envoy

source/common/stats/BUILD

@@ -14,6 +14,7 @@ envoy_cc_library(
     hdrs = ["heap_stat_data.h"],
     deps = [
         ":stat_data_allocator_lib",
+        ":symbol_table_lib",
         "//source/common/common:assert_lib",
         "//source/common/common:hash_lib",
         "//source/common/common:thread_annotations",
@@ -125,6 +126,9 @@ envoy_cc_library(
     deps = [
         "//include/envoy/stats:symbol_table_interface",
         "//source/common/common:assert_lib",
+        "//source/common/common:hash_lib",
+        "//source/common/common:lock_guard_lib",
+        "//source/common/common:thread_lib",
         "//source/common/common:utility_lib",
     ],
 )

source/common/stats/heap_stat_data.cc

@@ -6,7 +6,7 @@
 namespace Envoy {
 namespace Stats {
 
-HeapStatData::HeapStatData(absl::string_view key) : name_(key.data(), key.size()) {}
+HeapStatData::HeapStatData(StatNamePtr name_ptr) : name_ptr_(std::move(name_ptr)) {}
 
 HeapStatDataAllocator::HeapStatDataAllocator() {}
 
@@ -15,8 +15,8 @@ HeapStatDataAllocator::~HeapStatDataAllocator() { ASSERT(stats_.empty()); }
 HeapStatData* HeapStatDataAllocator::alloc(absl::string_view name) {
   // Any expected truncation of name is done at the callsite. No truncation is
   // required to use this allocator.
-  auto data = std::make_unique<HeapStatData>(name);
   Thread::ReleasableLockGuard lock(mutex_);
+  auto data = std::make_unique<HeapStatData>(table_.encode(name));
   auto ret = stats_.insert(data.get());
   HeapStatData* existing_data = *ret.first;
   lock.release();

source/common/stats/heap_stat_data.h

@@ -5,9 +5,11 @@
 #include <unordered_set>
 
 #include "common/common/hash.h"
+#include "common/common/lock_guard.h"
 #include "common/common/thread.h"
 #include "common/common/thread_annotations.h"
 #include "common/stats/stat_data_allocator_impl.h"
+#include "common/stats/symbol_table_impl.h"
 
 namespace Envoy {
 namespace Stats {
@@ -17,18 +19,24 @@ namespace Stats {
  * so that it can be allocated efficiently from the heap on demand.
  */
 struct HeapStatData {
-  explicit HeapStatData(absl::string_view key);
+  explicit HeapStatData(StatNamePtr name_ptr);
 
   /**
    * @returns absl::string_view the name as a string_view.
    */
-  absl::string_view key() const { return name_; }
+  std::string key() const { return name(); }
+  std::string name() const { return name_ptr_->toString(); }
+
+  uint64_t hash() const { return name_ptr_->hash(); }
+  bool operator==(const HeapStatData& rhs) const {
+    return *(name_ptr_.get()) == *(rhs.name_ptr_.get());
+  }
 
   std::atomic<uint64_t> value_{0};
   std::atomic<uint64_t> pending_increment_{0};
   std::atomic<uint16_t> flags_{0};
   std::atomic<uint16_t> ref_count_{1};
-  std::string name_;
+  StatNamePtr name_ptr_;
 };
 
 /**
@@ -47,20 +55,24 @@ class HeapStatDataAllocator : public StatDataAllocatorImpl<HeapStatData> {
   // StatDataAllocator
   bool requiresBoundedStatNameSize() const override { return false; }
 
+  // SymbolTable
+  StatNamePtr encode(absl::string_view sv) {
+    Thread::LockGuard lock(mutex_);
+    return table_.encode(sv);
+  }
+
 private:
-  struct HeapStatHash_ {
-    size_t operator()(const HeapStatData* a) const { return HashUtil::xxHash64(a->key()); }
+  struct HeapStatDataHash_ {
+    size_t operator()(const HeapStatData* a) const { return a->hash(); }
   };
-  struct HeapStatCompare_ {
-    bool operator()(const HeapStatData* a, const HeapStatData* b) const {
-      return (a->key() == b->key());
-    }
+  struct HeapStatDataCompare_ {
+    bool operator()(const HeapStatData* a, const HeapStatData* b) const { return (*a == *b); }
   };
 
   // TODO(jmarantz): See https://github.com/envoyproxy/envoy/pull/3927 and
   //  https://github.com/envoyproxy/envoy/issues/3585, which can help reorganize
   // the heap stats using a ref-counted symbol table to compress the stat strings.
-  typedef std::unordered_set<HeapStatData*, HeapStatHash_, HeapStatCompare_> StatSet;
+  typedef std::unordered_set<HeapStatData*, HeapStatDataHash_, HeapStatDataCompare_> StatSet;
 
   // An unordered set of HeapStatData pointers which keys off the key()
   // field in each object. This necessitates a custom comparator and hasher.
@@ -69,6 +81,8 @@ class HeapStatDataAllocator : public StatDataAllocatorImpl<HeapStatData> {
   // Although alloc() operations are called under existing locking, free() operations are made from
   // the destructors of the individual stat objects, which are not protected by locks.
   Thread::MutexBasicLockable mutex_;
+
+  SymbolTableImpl table_ GUARDED_BY(mutex_);
 };
 
 } // namespace Stats

source/common/stats/histogram_impl.h

@@ -7,6 +7,7 @@
 #include "envoy/stats/stats.h"
 #include "envoy/stats/store.h"
 
+#include "common/common/hash.h"
 #include "common/common/non_copyable.h"
 #include "common/stats/metric_impl.h"
 
@@ -53,6 +54,13 @@ class HistogramImpl : public Histogram, public MetricImpl {
 
   bool used() const override { return true; }
 
+  // Metric
+  uint64_t hash() const { return HashUtil::xxHash64(name()); }
+  bool operator==(const Metric& rhs) const {
+    const HistogramImpl& r = dynamic_cast<const HistogramImpl&>(rhs);
+    return (name() == r.name());
+  }
+
 private:
   // This is used for delivering the histogram data to sinks.
   Store& parent_;

source/common/stats/isolated_store_impl.cc

@@ -13,19 +13,26 @@ namespace Envoy {
 namespace Stats {
 
 IsolatedStoreImpl::IsolatedStoreImpl()
-    : counters_([this](const std::string& name) -> CounterSharedPtr {
-        std::string tag_extracted_name = name;
-        std::vector<Tag> tags;
-        return alloc_.makeCounter(name, std::move(tag_extracted_name), std::move(tags));
-      }),
-      gauges_([this](const std::string& name) -> GaugeSharedPtr {
-        std::string tag_extracted_name = name;
-        std::vector<Tag> tags;
-        return alloc_.makeGauge(name, std::move(tag_extracted_name), std::move(tags));
-      }),
-      histograms_([this](const std::string& name) -> HistogramSharedPtr {
-        return std::make_shared<HistogramImpl>(name, *this, std::string(name), std::vector<Tag>());
-      }) {}
+    : counters_(
+          [this](const std::string& name) -> CounterSharedPtr {
+            std::string tag_extracted_name = name;
+            std::vector<Tag> tags;
+            return alloc_.makeCounter(name, std::move(tag_extracted_name), std::move(tags));
+          },
+          alloc_),
+      gauges_(
+          [this](const std::string& name) -> GaugeSharedPtr {
+            std::string tag_extracted_name = name;
+            std::vector<Tag> tags;
+            return alloc_.makeGauge(name, std::move(tag_extracted_name), std::move(tags));
+          },
+          alloc_),
+      histograms_(
+          [this](const std::string& name) -> HistogramSharedPtr {
+            return std::make_shared<HistogramImpl>(name, *this, std::string(name),
+                                                   std::vector<Tag>());
+          },
+          alloc_) {}
 
 struct IsolatedScopeImpl : public Scope {
   IsolatedScopeImpl(IsolatedStoreImpl& parent, const std::string& prefix)

source/common/stats/isolated_store_impl.h

@@ -8,10 +8,12 @@
 #include "envoy/stats/stats.h"
 #include "envoy/stats/stats_options.h"
 #include "envoy/stats/store.h"
+#include "envoy/stats/symbol_table.h"
 
 #include "common/common/utility.h"
 #include "common/stats/heap_stat_data.h"
 #include "common/stats/stats_options_impl.h"
+#include "common/stats/symbol_table_impl.h"
 #include "common/stats/utility.h"
 
 namespace Envoy {
@@ -24,16 +26,18 @@ template <class Base> class IsolatedStatsCache {
 public:
   typedef std::function<std::shared_ptr<Base>(const std::string& name)> Allocator;
 
-  IsolatedStatsCache(Allocator alloc) : alloc_(alloc) {}
+  IsolatedStatsCache(Allocator alloc, HeapStatDataAllocator& heap_alloc)
+      : alloc_(alloc), heap_alloc_(heap_alloc) {}
 
   Base& get(const std::string& name) {
-    auto stat = stats_.find(name);
+    StatNamePtr ptr = heap_alloc_.encode(name);
+    auto stat = stats_.find(ptr);
     if (stat != stats_.end()) {
       return *stat->second;
     }
 
     std::shared_ptr<Base> new_stat = alloc_(name);
-    stats_.emplace(name, new_stat);
+    stats_.emplace(std::move(ptr), new_stat);
     return *new_stat;
   }
 
@@ -48,8 +52,10 @@ template <class Base> class IsolatedStatsCache {
   }
 
 private:
-  std::unordered_map<std::string, std::shared_ptr<Base>> stats_;
+  std::unordered_map<StatNamePtr, std::shared_ptr<Base>, StatNamePtrHash_, StatNamePtrCompare_>
+      stats_;
   Allocator alloc_;
+  HeapStatDataAllocator& heap_alloc_;
 };
 
 class IsolatedStoreImpl : public Store {

source/common/stats/raw_stat_data.h

@@ -63,6 +63,11 @@ struct RawStatData {
    * Returns a hash of the key. This is required by BlockMemoryHashSet.
    */
   static uint64_t hash(absl::string_view key) { return HashUtil::xxHash64(key); }
+  uint64_t hash() { return HashUtil::xxHash64(absl::string_view(name_)); }
+
+  bool operator==(const RawStatData& rhs) const {
+    return (name_ == rhs.name_);
+  }
 
   /**
    * Returns true if object is in use.
@@ -73,6 +78,7 @@ struct RawStatData {
    * Returns the name as a string_view with no truncation.
    */
   absl::string_view key() const { return absl::string_view(name_); }
+  std::string name() const { return name_; }
 
   std::atomic<uint64_t> value_;
   std::atomic<uint64_t> pending_increment_;

source/common/stats/stat_data_allocator_impl.h

@@ -63,7 +63,7 @@ template <class StatData> class CounterImpl : public Counter, public MetricImpl
 public:
   CounterImpl(StatData& data, StatDataAllocatorImpl<StatData>& alloc,
               std::string&& tag_extracted_name, std::vector<Tag>&& tags)
-      : MetricImpl(data.name_, std::move(tag_extracted_name), std::move(tags)), data_(data),
+      : MetricImpl(data.name(), std::move(tag_extracted_name), std::move(tags)), data_(data),
         alloc_(alloc) {}
   ~CounterImpl() { alloc_.free(data_); }
 
@@ -80,19 +80,49 @@ template <class StatData> class CounterImpl : public Counter, public MetricImpl
   bool used() const override { return data_.flags_ & Flags::Used; }
   uint64_t value() const override { return data_.value_; }
 
+  uint64_t hash() const { return data_.hash(); }
+  bool operator==(const Metric& rhs) const {
+    const CounterImpl& r = dynamic_cast<const CounterImpl&>(rhs);
+    return (data_ == r.data_);
+  }
+
 private:
   StatData& data_;
   StatDataAllocatorImpl<StatData>& alloc_;
 };
 
+struct MetricSharedPtrHash_ {
+  uint64_t operator()(const std::shared_ptr<Metric> a) const {
+    return a.get()->hash();
+  }
+};
+
+struct MetricSharedPtrCompare_ {
+  bool operator()(const std::shared_ptr<Metric> a, const std::shared_ptr<Metric> b) const {
+    return (*a.get() == *b.get());
+  }
+};
+
+////struct CounterSharedPtrHash_ {
+////  uint64_t operator()(const CounterSharedPtr a) const {
+////    return a.get()->hash();
+////  }
+////};
+////
+////struct CounterSharedPtrCompare_ {
+////  bool operator()(const CounterSharedPtr a, const CounterSharedPtr b) const {
+////    return (*a.get() == *b.get());
+////  }
+////};
+
 /**
  * Gauge implementation that wraps a StatData.
  */
 template <class StatData> class GaugeImpl : public Gauge, public MetricImpl {
 public:
   GaugeImpl(StatData& data, StatDataAllocatorImpl<StatData>& alloc,
             std::string&& tag_extracted_name, std::vector<Tag>&& tags)
-      : MetricImpl(data.name_, std::move(tag_extracted_name), std::move(tags)), data_(data),
+      : MetricImpl(data.name(), std::move(tag_extracted_name), std::move(tags)), data_(data),
         alloc_(alloc) {}
   ~GaugeImpl() { alloc_.free(data_); }
 
@@ -115,11 +145,29 @@ template <class StatData> class GaugeImpl : public Gauge, public MetricImpl {
   virtual uint64_t value() const override { return data_.value_; }
   bool used() const override { return data_.flags_ & Flags::Used; }
 
+  uint64_t hash() const { return data_.hash(); }
+  bool operator==(const Metric& rhs) const {
+    const GaugeImpl& r = dynamic_cast<const GaugeImpl&>(rhs);
+    return (data_ == r.data_);
+  }
+
 private:
   StatData& data_;
   StatDataAllocatorImpl<StatData>& alloc_;
 };
 
+////struct GaugeSharedPtrHash_ {
+////  uint64_t operator()(const GaugeSharedPtr a) const {
+////    return a.get()->hash();
+////  }
+////};
+////
+////struct GaugeSharedPtrCompare_ {
+////  bool operator()(const GaugeSharedPtr a, const GaugeSharedPtr b) const {
+////    return (*a.get() == *b.get());
+////  }
+////};
+
 template <class StatData>
 CounterSharedPtr StatDataAllocatorImpl<StatData>::makeCounter(absl::string_view name,
                                                               std::string&& tag_extracted_name,

source/common/stats/symbol_table_impl.cc

@@ -31,6 +31,7 @@ std::string SymbolTableImpl::decode(const SymbolVec& symbol_vec) const {
 }
 
 void SymbolTableImpl::free(const SymbolVec& symbol_vec) {
+  Thread::LockGuard lock(lock_);
   for (const Symbol symbol : symbol_vec) {
     auto decode_search = decode_map_.find(symbol);
     ASSERT(decode_search != decode_map_.end());