stats: refactor to enable non-hot-restart stats to have distinct repr…

…esentation from hot-restart stats. (#3606)

Signed-off-by: Joshua Marantz <jmarantz@google.com>

include/envoy/server/hot_restart.h

@@ -94,7 +94,7 @@ class HotRestart {
   /**
    * @returns an allocator for stats.
    */
-  virtual Stats::RawStatDataAllocator& statsAllocator() PURE;
+  virtual Stats::StatDataAllocator& statsAllocator() PURE;
 };
 
 } // namespace Server

include/envoy/stats/stats.h

@@ -408,25 +408,38 @@ typedef std::unique_ptr<StoreRoot> StoreRootPtr;
 struct RawStatData;
 
 /**
- * Abstract interface for allocating a RawStatData.
+ * Abstract interface for allocating statistics. Implementations can
+ * be created utilizing a single fixed-size block suitable for
+ * shared-memory, or in the heap, allowing for pointers and sharing of
+ * substrings, with an opportunity for reduced memory consumption.
  */
-class RawStatDataAllocator {
+class StatDataAllocator {
 public:
-  virtual ~RawStatDataAllocator() {}
+  virtual ~StatDataAllocator() {}
 
   /**
-   * @return RawStatData* a raw stat data block for a given stat name or nullptr if there is no
-   *         more memory available for stats. The allocator should return a reference counted
-   *         data location by name if one already exists with the same name. This is used for
-   *         intra-process scope swapping as well as inter-process hot restart.
+   * @param name the full name of the stat.
+   * @param tag_extracted_name the name of the stat with tag-values stripped out.
+   * @param tags the extracted tag values.
+   * @return CounterSharedPtr a counter, or nullptr if allocation failed, in which case
+   *     tag_extracted_name and tags are not moved.
    */
-  virtual RawStatData* alloc(const std::string& name) PURE;
+  virtual CounterSharedPtr makeCounter(const std::string& name, std::string&& tag_extracted_name,
+                                       std::vector<Tag>&& tags) PURE;
 
   /**
-   * Free a raw stat data block. The allocator should handle reference counting and only truly
-   * free the block if it is no longer needed.
+   * @param name the full name of the stat.
+   * @param tag_extracted_name the name of the stat with tag-values stripped out.
+   * @param tags the extracted tag values.
+   * @return GaugeSharedPtr a gauge, or nullptr if allocation failed, in which case
+   *     tag_extracted_name and tags are not moved.
    */
-  virtual void free(RawStatData& data) PURE;
+  virtual GaugeSharedPtr makeGauge(const std::string& name, std::string&& tag_extracted_name,
+                                   std::vector<Tag>&& tags) PURE;
+
+  // TODO(jmarantz): create a parallel mechanism to instantiate histograms. At
+  // the moment, histograms don't fit the same pattern of counters and gaugaes
+  // as they are not actually created in the context of a stats allocator.
 };
 
 } // namespace Stats

source/common/stats/stats_impl.cc

@@ -363,5 +363,26 @@ void SourceImpl::clearCache() {
   histograms_.reset();
 }
 
+CounterSharedPtr RawStatDataAllocator::makeCounter(const std::string& name,
+                                                   std::string&& tag_extracted_name,
+                                                   std::vector<Tag>&& tags) {
+  RawStatData* data = alloc(name);
+  if (data == nullptr) {
+    return nullptr;
+  }
+  return std::make_shared<CounterImpl>(*data, *this, std::move(tag_extracted_name),
+                                       std::move(tags));
+}
+
+GaugeSharedPtr RawStatDataAllocator::makeGauge(const std::string& name,
+                                               std::string&& tag_extracted_name,
+                                               std::vector<Tag>&& tags) {
+  RawStatData* data = alloc(name);
+  if (data == nullptr) {
+    return nullptr;
+  }
+  return std::make_shared<GaugeImpl>(*data, *this, std::move(tag_extracted_name), std::move(tags));
+}
+
 } // namespace Stats
 } // namespace Envoy

source/common/stats/stats_impl.h

@@ -305,6 +305,35 @@ class MetricImpl : public virtual Metric {
   const std::vector<Tag> tags_;
 };
 
+/**
+ * Implements a StatDataAllocator that uses RawStatData -- capable of deploying
+ * in a shared memory block without internal pointers.
+ */
+class RawStatDataAllocator : public StatDataAllocator {
+public:
+  // StatDataAllocator
+  CounterSharedPtr makeCounter(const std::string& name, std::string&& tag_extracted_name,
+                               std::vector<Tag>&& tags) override;
+  GaugeSharedPtr makeGauge(const std::string& name, std::string&& tag_extracted_name,
+                           std::vector<Tag>&& tags) override;
+
+  /**
+   * @param name the full name of the stat.
+   * @return RawStatData* a raw stat data block for a given stat name or nullptr if there is no
+   *         more memory available for stats. The allocator should return a reference counted
+   *         data location by name if one already exists with the same name. This is used for
+   *         intra-process scope swapping as well as inter-process hot restart.
+   */
+  virtual RawStatData* alloc(const std::string& name) PURE;
+
+  /**
+   * Free a raw stat data block. The allocator should handle reference counting and only truly
+   * free the block if it is no longer needed.
+   * @param data the data returned by alloc().
+   */
+  virtual void free(RawStatData& data) PURE;
+};
+
 /**
  * Counter implementation that wraps a RawStatData.
  */
@@ -463,9 +492,9 @@ class HeapRawStatDataAllocator : public RawStatDataAllocator {
 /**
  * A stats cache template that is used by the isolated store.
  */
-template <class Base, class Impl> class IsolatedStatsCache {
+template <class Base> class IsolatedStatsCache {
 public:
-  typedef std::function<Impl*(const std::string& name)> Allocator;
+  typedef std::function<std::shared_ptr<Base>(const std::string& name)> Allocator;
 
   IsolatedStatsCache(Allocator alloc) : alloc_(alloc) {}
 
@@ -475,8 +504,8 @@ template <class Base, class Impl> class IsolatedStatsCache {
       return *stat->second;
     }
 
-    Impl* new_stat = alloc_(name);
-    stats_.emplace(name, std::shared_ptr<Impl>{new_stat});
+    std::shared_ptr<Base> new_stat = alloc_(name);
+    stats_.emplace(name, new_stat);
     return *new_stat;
   }
 
@@ -491,7 +520,7 @@ template <class Base, class Impl> class IsolatedStatsCache {
   }
 
 private:
-  std::unordered_map<std::string, std::shared_ptr<Impl>> stats_;
+  std::unordered_map<std::string, std::shared_ptr<Base>> stats_;
   Allocator alloc_;
 };
 
@@ -501,15 +530,19 @@ template <class Base, class Impl> class IsolatedStatsCache {
 class IsolatedStoreImpl : public Store {
 public:
   IsolatedStoreImpl()
-      : counters_([this](const std::string& name) -> CounterImpl* {
-          return new CounterImpl(*alloc_.alloc(name), alloc_, 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));
         }),
-        gauges_([this](const std::string& name) -> GaugeImpl* {
-          return new GaugeImpl(*alloc_.alloc(name), alloc_, std::string(name), std::vector<Tag>());
+        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) -> HistogramImpl* {
-          return new HistogramImpl(name, *this, std::string(name), std::vector<Tag>());
+        histograms_([this](const std::string& name) -> HistogramSharedPtr {
+          return std::make_shared<HistogramImpl>(name, *this, std::string(name),
+                                                 std::vector<Tag>());
         }) {}
 
   // Stats::Scope
@@ -552,9 +585,9 @@ class IsolatedStoreImpl : public Store {
   };
 
   HeapRawStatDataAllocator alloc_;
-  IsolatedStatsCache<Counter, CounterImpl> counters_;
-  IsolatedStatsCache<Gauge, GaugeImpl> gauges_;
-  IsolatedStatsCache<Histogram, HistogramImpl> histograms_;
+  IsolatedStatsCache<Counter> counters_;
+  IsolatedStatsCache<Gauge> gauges_;
+  IsolatedStatsCache<Histogram> histograms_;
 };
 
 } // namespace Stats

source/common/stats/thread_local_store.cc

@@ -12,7 +12,7 @@
 namespace Envoy {
 namespace Stats {
 
-ThreadLocalStoreImpl::ThreadLocalStoreImpl(RawStatDataAllocator& alloc)
+ThreadLocalStoreImpl::ThreadLocalStoreImpl(StatDataAllocator& alloc)
     : alloc_(alloc), default_scope_(createScope("")),
       tag_producer_(std::make_unique<TagProducerImpl>()),
       num_last_resort_stats_(default_scope_->counter("stats.overflow")), source_(*this) {}
@@ -155,35 +155,15 @@ void ThreadLocalStoreImpl::clearScopeFromCaches(uint64_t scope_id) {
   }
 }
 
-ThreadLocalStoreImpl::SafeAllocData ThreadLocalStoreImpl::safeAlloc(const std::string& name) {
-  RawStatData* data = alloc_.alloc(name);
-  if (!data) {
-    // If we run out of stat space from the allocator (which can happen if for example allocations
-    // are coming from a fixed shared memory region, we need to deal with this case the best we
-    // can. We must pass back the right allocator so that free() happens on the heap.
-    num_last_resort_stats_.inc();
-    return {*heap_allocator_.alloc(name), heap_allocator_};
-  } else {
-    return {*data, alloc_};
-  }
-}
-
 std::atomic<uint64_t> ThreadLocalStoreImpl::ScopeImpl::next_scope_id_;
 
 ThreadLocalStoreImpl::ScopeImpl::~ScopeImpl() { parent_.releaseScopeCrossThread(this); }
 
-Counter& ThreadLocalStoreImpl::ScopeImpl::counter(const std::string& name) {
-  // Determine the final name based on the prefix and the passed name.
-  std::string final_name = prefix_ + name;
-
-  // We now try to acquire a *reference* to the TLS cache shared pointer. This might remain null
-  // if we don't have TLS initialized currently. The de-referenced pointer might be null if there
-  // is no cache entry.
-  CounterSharedPtr* tls_ref = nullptr;
-  if (!parent_.shutting_down_ && parent_.tls_) {
-    tls_ref =
-        &parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_].counters_[final_name];
-  }
+template <class StatType>
+StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat(
+    const std::string& name,
+    std::unordered_map<std::string, std::shared_ptr<StatType>>& central_cache_map,
+    MakeStatFn<StatType> make_stat, std::shared_ptr<StatType>* tls_ref) {
 
   // If we have a valid cache entry, return it.
   if (tls_ref && *tls_ref) {
@@ -193,13 +173,19 @@ Counter& ThreadLocalStoreImpl::ScopeImpl::counter(const std::string& name) {
   // 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_);
-  CounterSharedPtr& central_ref = central_cache_.counters_[final_name];
+  std::shared_ptr<StatType>& central_ref = central_cache_map[name];
   if (!central_ref) {
-    SafeAllocData alloc = parent_.safeAlloc(final_name);
     std::vector<Tag> tags;
-    std::string tag_extracted_name = parent_.getTagsForName(final_name, tags);
-    central_ref.reset(
-        new CounterImpl(alloc.data_, alloc.free_, std::move(tag_extracted_name), std::move(tags)));
+    std::string tag_extracted_name = parent_.getTagsForName(name, tags);
+    std::shared_ptr<StatType> stat =
+        make_stat(parent_.alloc_, name, std::move(tag_extracted_name), std::move(tags));
+    if (stat == nullptr) {
+      parent_.num_last_resort_stats_.inc();
+      stat =
+          make_stat(parent_.heap_allocator_, name, std::move(tag_extracted_name), std::move(tags));
+      ASSERT(stat != nullptr);
+    }
+    central_ref = stat;
   }
 
   // If we have a TLS location to store or allocation into, do it.
@@ -211,6 +197,28 @@ Counter& ThreadLocalStoreImpl::ScopeImpl::counter(const std::string& name) {
   return *central_ref;
 }
 
+Counter& ThreadLocalStoreImpl::ScopeImpl::counter(const std::string& name) {
+  // Determine the final name based on the prefix and the passed name.
+  std::string final_name = prefix_ + name;
+
+  // We now try to acquire a *reference* to the TLS cache shared pointer. This might remain null
+  // if we don't have TLS initialized currently. The de-referenced pointer might be null if there
+  // is no cache entry.
+  CounterSharedPtr* tls_ref = nullptr;
+  if (!parent_.shutting_down_ && parent_.tls_) {
+    tls_ref =
+        &parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_].counters_[final_name];
+  }
+
+  return safeMakeStat<Counter>(
+      final_name, central_cache_.counters_,
+      [](StatDataAllocator& allocator, const std::string& name, std::string&& tag_extracted_name,
+         std::vector<Tag>&& tags) -> CounterSharedPtr {
+        return allocator.makeCounter(name, std::move(tag_extracted_name), std::move(tags));
+      },
+      tls_ref);
+}
+
 void ThreadLocalStoreImpl::ScopeImpl::deliverHistogramToSinks(const Histogram& histogram,
                                                               uint64_t value) {
   // Thread local deliveries must be blocked outright for histograms and timers during shutdown.
@@ -236,25 +244,13 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gauge(const std::string& name) {
     tls_ref = &parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_].gauges_[final_name];
   }
 
-  if (tls_ref && *tls_ref) {
-    return **tls_ref;
-  }
-
-  Thread::LockGuard lock(parent_.lock_);
-  GaugeSharedPtr& central_ref = central_cache_.gauges_[final_name];
-  if (!central_ref) {
-    SafeAllocData alloc = parent_.safeAlloc(final_name);
-    std::vector<Tag> tags;
-    std::string tag_extracted_name = parent_.getTagsForName(final_name, tags);
-    central_ref.reset(
-        new GaugeImpl(alloc.data_, alloc.free_, std::move(tag_extracted_name), std::move(tags)));
-  }
-
-  if (tls_ref) {
-    *tls_ref = central_ref;
-  }
-
-  return *central_ref;
+  return safeMakeStat<Gauge>(
+      final_name, central_cache_.gauges_,
+      [](StatDataAllocator& allocator, const std::string& name, std::string&& tag_extracted_name,
+         std::vector<Tag>&& tags) -> GaugeSharedPtr {
+        return allocator.makeGauge(name, std::move(tag_extracted_name), std::move(tags));
+      },
+      tls_ref);
 }
 
 Histogram& ThreadLocalStoreImpl::ScopeImpl::histogram(const std::string& name) {

source/common/stats/thread_local_store.h

@@ -163,7 +163,7 @@ class TlsScope : public Scope {
  */
 class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRoot {
 public:
-  ThreadLocalStoreImpl(RawStatDataAllocator& alloc);
+  ThreadLocalStoreImpl(StatDataAllocator& alloc);
   ~ThreadLocalStoreImpl();
 
   // Stats::Scope
@@ -225,6 +225,29 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
     Histogram& histogram(const std::string& name) override;
     Histogram& tlsHistogram(const std::string& name, ParentHistogramImpl& parent) override;
 
+    template <class StatType>
+    using MakeStatFn =
+        std::function<std::shared_ptr<StatType>(StatDataAllocator&, const std::string& name,
+                                                std::string&& tag_extracted_name,
+                                                std::vector<Tag>&& tags)>;
+
+    /**
+     * Makes a stat either by looking it up in the central cache,
+     * generating it from the the parent allocator, or as a last
+     * result, creating it with the heap allocator.
+     *
+     * @param name the full name of the stat (not tag extracted).
+     * @param central_cache_map a map from name to the desired object in the central cache.
+     * @param make_stat a function to generate the stat object, called if it's not in cache.
+     * @param tls_ref possibly null reference to a cache entry for this stat, which will be
+     *     used if non-empty, or filled in if empty (and non-null).
+     */
+    template <class StatType>
+    StatType&
+    safeMakeStat(const std::string& name,
+                 std::unordered_map<std::string, std::shared_ptr<StatType>>& central_cache_map,
+                 MakeStatFn<StatType> make_stat, std::shared_ptr<StatType>* tls_ref);
+
     static std::atomic<uint64_t> next_scope_id_;
 
     const uint64_t scope_id_;
@@ -244,18 +267,12 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
     std::unordered_map<uint64_t, TlsCacheEntry> scope_cache_;
   };
 
-  struct SafeAllocData {
-    RawStatData& data_;
-    RawStatDataAllocator& free_;
-  };
-
   std::string getTagsForName(const std::string& name, std::vector<Tag>& tags) const;
   void clearScopeFromCaches(uint64_t scope_id);
   void releaseScopeCrossThread(ScopeImpl* scope);
-  SafeAllocData safeAlloc(const std::string& name);
   void mergeInternal(PostMergeCb mergeCb);
 
-  RawStatDataAllocator& alloc_;
+  StatDataAllocator& alloc_;
   Event::Dispatcher* main_thread_dispatcher_{};
   ThreadLocal::SlotPtr tls_;
   mutable Thread::MutexBasicLockable lock_;

source/server/hot_restart_impl.h

@@ -130,7 +130,7 @@ class HotRestartImpl : public HotRestart,
   std::string version() override;
   Thread::BasicLockable& logLock() override { return log_lock_; }
   Thread::BasicLockable& accessLogLock() override { return access_log_lock_; }
-  Stats::RawStatDataAllocator& statsAllocator() override { return *this; }
+  Stats::StatDataAllocator& statsAllocator() override { return *this; }
 
   /**
    * envoy --hot_restart_version doesn't initialize Envoy, but computes the version string