WIP Integration of SymbolTable into HeapStatData / ThreadLocalStore

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,23 @@ 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(); }
+
+  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,14 +54,19 @@ 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()); }
+    // size_t operator()(const HeapStatData* a) const { return HashUtil::xxHash64(a->key()); }
+    size_t operator()(const HeapStatData* a) const { return a->name_ptr_->hash(); }
   };
   struct HeapStatCompare_ {
-    bool operator()(const HeapStatData* a, const HeapStatData* b) const {
-      return (a->key() == b->key());
-    }
+    bool operator()(const HeapStatData* a, const HeapStatData* b) const { return (*a == *b); }
   };
 
   // TODO(jmarantz): See https://github.com/envoyproxy/envoy/pull/3927 and
@@ -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/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

@@ -73,6 +73,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 std::string(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_); }
 
@@ -92,7 +92,7 @@ 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_); }
 

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());

source/common/stats/symbol_table_impl.h

@@ -11,6 +11,9 @@
 #include "envoy/stats/symbol_table.h"
 
 #include "common/common/assert.h"
+#include "common/common/hash.h"
+#include "common/common/lock_guard.h"
+#include "common/common/thread.h"
 #include "common/common/utility.h"
 
 #include "absl/strings/str_join.h"
@@ -115,6 +118,8 @@ class SymbolTableImpl : public SymbolTable {
   // TODO(ambuc): There might be an optimization here relating to storing ranges of freed symbols
   // using an Envoy::IntervalSet.
   std::stack<Symbol> pool_;
+
+  mutable Thread::MutexBasicLockable lock_; // This must be called during free()
 };
 
 /**
@@ -127,6 +132,11 @@ class StatNameImpl : public StatName {
       : symbol_vec_(symbol_vec), symbol_table_(symbol_table) {}
   ~StatNameImpl() override { symbol_table_.free(symbol_vec_); }
   std::string toString() const override { return symbol_table_.decode(symbol_vec_); }
+  uint64_t hash() const override { return HashUtil::hashVector(symbol_vec_); }
+  bool operator==(const StatName& rhs) const override {
+    const StatNameImpl& r = dynamic_cast<const StatNameImpl&>(rhs);
+    return symbol_vec_ == r.symbol_vec_;
+  }
 
 private:
   friend class StatNameTest;
@@ -135,5 +145,16 @@ class StatNameImpl : public StatName {
   SymbolTableImpl& symbol_table_;
 };
 
+struct StatNamePtrHash_ {
+  size_t operator()(const StatNamePtr& a) const { return a->hash(); }
+};
+
+struct StatNamePtrCompare_ {
+  bool operator()(const StatNamePtr& a, const StatNamePtr& b) const {
+    // We really need to compare the underlying StatNames.
+    return (*a.get() == *b.get());
+  }
+};
+
 } // namespace Stats
 } // namespace Envoy