HeapStatData with a distinct allocation mechanism for RawStatData

include/envoy/stats/stats.h

@@ -480,6 +480,11 @@ class StatDataAllocator {
   virtual GaugeSharedPtr makeGauge(const std::string& name, std::string&& tag_extracted_name,
                                    std::vector<Tag>&& tags) PURE;
 
+  /**
+   * @return const StatsOptions& the options used to initialize the allocator.
+   */
+  virtual const StatsOptions& statsOptions() const 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.

source/common/stats/stats_impl.cc

@@ -135,35 +135,47 @@ bool TagExtractorImpl::extractTag(const std::string& stat_name, std::vector<Tag>
   return false;
 }
 
-RawStatData* HeapRawStatDataAllocator::alloc(const std::string& name) {
-  uint64_t num_bytes_to_allocate = RawStatData::structSize(name.size());
-  RawStatData* data = static_cast<RawStatData*>(::calloc(num_bytes_to_allocate, 1));
-  if (data == nullptr) {
-    throw std::bad_alloc();
-  }
-  data->checkAndInit(name, num_bytes_to_allocate);
+HeapStatData::HeapStatData(absl::string_view key) : name_(key.data(), key.size()) {}
 
+HeapStatData* HeapStatDataAllocator::alloc(const std::string& name) {
+  auto data = std::make_unique<HeapStatData>(truncateStatName(name));
   Thread::ReleasableLockGuard lock(mutex_);
-  auto ret = stats_.insert(data);
-  RawStatData* existing_data = *ret.first;
+  auto ret = stats_.insert(data.get());
+  HeapStatData* existing_data = *ret.first;
   lock.release();
 
-  if (!ret.second) {
-    ::free(data);
-    ++existing_data->ref_count_;
-    return existing_data;
-  } else {
-    return data;
+  if (ret.second) {
+    return data.release();
+  }
+  ++existing_data->ref_count_;
+  return existing_data;
+}
+
+void HeapStatDataAllocator::free(HeapStatData& data) {
+  ASSERT(data.ref_count_ > 0);
+  if (--data.ref_count_ > 0) {
+    return;
+  }
+
+  {
+    Thread::LockGuard lock(mutex_);
+    size_t key_removed = stats_.erase(&data);
+    ASSERT(key_removed == 1);
   }
+
+  delete &data;
 }
 
 /**
- * Counter implementation that wraps a RawStatData.
+ * Counter implementation that wraps a StatData. StatData must have data members:
+ *    int64_t value_;
+ *    int64_t pending_increment_;
+ *    int16_t flags_;
  */
-class CounterImpl : public Counter, public MetricImpl {
+template <class StatData> class CounterImpl : public Counter, public MetricImpl {
 public:
-  CounterImpl(RawStatData& data, RawStatDataAllocator& alloc, std::string&& tag_extracted_name,
-              std::vector<Tag>&& tags)
+  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),
         alloc_(alloc) {}
   ~CounterImpl() { alloc_.free(data_); }
@@ -182,17 +194,17 @@ class CounterImpl : public Counter, public MetricImpl {
   uint64_t value() const override { return data_.value_; }
 
 private:
-  RawStatData& data_;
-  RawStatDataAllocator& alloc_;
+  StatData& data_;
+  StatDataAllocatorImpl<StatData>& alloc_;
 };
 
 /**
- * Gauge implementation that wraps a RawStatData.
+ * Gauge implementation that wraps a StatData.
  */
-class GaugeImpl : public Gauge, public MetricImpl {
+template <class StatData> class GaugeImpl : public Gauge, public MetricImpl {
 public:
-  GaugeImpl(RawStatData& data, RawStatDataAllocator& alloc, std::string&& tag_extracted_name,
-            std::vector<Tag>&& tags)
+  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),
         alloc_(alloc) {}
   ~GaugeImpl() { alloc_.free(data_); }
@@ -217,8 +229,8 @@ class GaugeImpl : public Gauge, public MetricImpl {
   bool used() const override { return data_.flags_ & Flags::Used; }
 
 private:
-  RawStatData& data_;
-  RawStatDataAllocator& alloc_;
+  StatData& data_;
+  StatDataAllocatorImpl<StatData>& alloc_;
 };
 
 TagProducerImpl::TagProducerImpl(const envoy::config::metrics::v2::StatsConfig& config) {
@@ -319,20 +331,17 @@ TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v2::StatsCon
   return names;
 }
 
-void HeapRawStatDataAllocator::free(RawStatData& data) {
-  ASSERT(data.ref_count_ > 0);
-  if (--data.ref_count_ > 0) {
-    return;
-  }
-
-  size_t key_removed;
-  {
-    Thread::LockGuard lock(mutex_);
-    key_removed = stats_.erase(&data);
+template <class StatData>
+absl::string_view StatDataAllocatorImpl<StatData>::truncateStatName(absl::string_view key) {
+  const uint64_t max_width = stats_options_.maxNameLength();
+  if (key.size() > max_width) {
+    ENVOY_LOG_MISC(
+        warn,
+        "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", key,
+        key.size(), max_width);
+    return absl::string_view(key.data(), max_width);
   }
-
-  ASSERT(key_removed == 1);
-  ::free(&data);
+  return key;
 }
 
 void RawStatData::initialize(absl::string_view key, uint64_t xfer_size) {
@@ -420,26 +429,32 @@ 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);
+template <class StatData>
+CounterSharedPtr StatDataAllocatorImpl<StatData>::makeCounter(const std::string& name,
+                                                              std::string&& tag_extracted_name,
+                                                              std::vector<Tag>&& tags) {
+  StatData* data = alloc(name);
   if (data == nullptr) {
     return nullptr;
   }
-  return std::make_shared<CounterImpl>(*data, *this, std::move(tag_extracted_name),
-                                       std::move(tags));
+  return std::make_shared<CounterImpl<StatData>>(*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);
+template <class StatData>
+GaugeSharedPtr StatDataAllocatorImpl<StatData>::makeGauge(const std::string& name,
+                                                          std::string&& tag_extracted_name,
+                                                          std::vector<Tag>&& tags) {
+  StatData* data = alloc(name);
   if (data == nullptr) {
     return nullptr;
   }
-  return std::make_shared<GaugeImpl>(*data, *this, std::move(tag_extracted_name), std::move(tags));
+  return std::make_shared<GaugeImpl<StatData>>(*data, *this, std::move(tag_extracted_name),
+                                               std::move(tags));
 }
 
+template class StatDataAllocatorImpl<HeapStatData>;
+template class StatDataAllocatorImpl<RawStatData>;
+
 } // namespace Stats
 } // namespace Envoy

source/common/stats/stats_impl.h

@@ -283,12 +283,14 @@ 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 {
+template <class StatData> class StatDataAllocatorImpl : public StatDataAllocator {
 public:
+  /**
+   * @param stats_options The stat optinos, which must live longer than the allocator.
+   */
+  explicit StatDataAllocatorImpl(const StatsOptions& stats_options)
+      : stats_options_(stats_options) {}
+
   // StatDataAllocator
   CounterSharedPtr makeCounter(const std::string& name, std::string&& tag_extracted_name,
                                std::vector<Tag>&& tags) override;
@@ -297,21 +299,40 @@ class RawStatDataAllocator : public StatDataAllocator {
 
   /**
    * @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.
+   * @return StatData* a 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;
+  virtual StatData* 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;
+  virtual void free(StatData& data) PURE;
+
+  /**
+   * Truncates a stat name based on the options passed into the constructor,
+   * issuing a warning if a truncation was needed.
+   *
+   * @param stat_name The original stat name.
+   * @return absl::string_view The stat name, truncated if needed to meet limits in stat_options_.
+   */
+  absl::string_view truncateStatName(absl::string_view stat_name);
+
+  /**
+   * @return const StatsOptions& The options passed into the constructor.
+   */
+  const StatsOptions& statsOptions() const override { return stats_options_; }
+
+private:
+  const StatsOptions& stats_options_;
 };
 
+using RawStatDataAllocator = StatDataAllocatorImpl<RawStatData>;
+
 /**
  * Implementation of HistogramStatistics for circllhist.
  */
@@ -373,30 +394,61 @@ class SourceImpl : public Source {
 };
 
 /**
- * Implementation of RawStatDataAllocator that uses an unordered set to store
- * RawStatData pointers.
+ * This structure is an alternate backing store for both CounterImpl and GaugeImpl. It is designed
+ * so that it can be allocated efficiently from the heap on demand.
+ */
+struct HeapStatData {
+  explicit HeapStatData(absl::string_view key);
+
+  /**
+   * Returns the name as a string_view. This is required by BlockMemoryHashSet.
+   */
+  absl::string_view key() const { return name_; }
+
+  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::atomic<uint32_t> unused_{0};
+  std::string name_;
+};
+
+/**
+ * Implementation of StatDataAllocator using a pure heap-based strategy, so that
+ * Envoy implementations that do not require hot-restart can use less memory.
  */
-class HeapRawStatDataAllocator : public RawStatDataAllocator {
+class HeapStatDataAllocator : public StatDataAllocatorImpl<HeapStatData> {
 public:
-  // RawStatDataAllocator
-  ~HeapRawStatDataAllocator() { ASSERT(stats_.empty()); }
-  RawStatData* alloc(const std::string& name) override;
-  void free(RawStatData& data) override;
+  explicit HeapStatDataAllocator(const StatsOptions& options) : StatDataAllocatorImpl(options) {}
+  // HeapStatDataAllocator() {}
+  ~HeapStatDataAllocator() { ASSERT(stats_.empty()); }
+
+  // StatDataAllocator
+  HeapStatData* alloc(const std::string& name) override;
+  void free(HeapStatData& data) override;
 
 private:
-  struct RawStatDataHash_ {
-    size_t operator()(const RawStatData* a) const { return HashUtil::xxHash64(a->key()); }
+  struct HeapStatHash_ {
+    size_t operator()(const HeapStatData* a) const { return HashUtil::xxHash64(a->key()); }
   };
-  struct RawStatDataCompare_ {
-    bool operator()(const RawStatData* a, const RawStatData* b) const {
+  struct HeapStatCompare_ {
+    bool operator()(const HeapStatData* a, const HeapStatData* b) const {
       return (a->key() == b->key());
     }
   };
-  typedef std::unordered_set<RawStatData*, RawStatDataHash_, RawStatDataCompare_> StringRawDataSet;
 
-  // An unordered set of RawStatData pointers which keys off the key()
+  // TODO(jmarantz): storing the set of stats as a string-set wastes an
+  // enourmous amount of of memory, because all the stats are composed using the
+  // same set of cluster names and patterns found in
+  // source/common/config/well_known_names.cc. This should eventually be changed to
+  // store a symbol-table of decomposed stat segments (e.g. split on "." to start) and
+  // arrays referencing those segments. The segments can be ref-counted so they
+  // no longer consume memory once deleted.
+  typedef std::unordered_set<HeapStatData*, HeapStatHash_, HeapStatCompare_> StatSet;
+
+  // An unordered set of HeapStatData pointers which keys off the key()
   // field in each object. This necessitates a custom comparator and hasher.
-  StringRawDataSet stats_ GUARDED_BY(mutex_);
+  StatSet stats_ GUARDED_BY(mutex_);
   // A mutex is needed here to protect the stats_ object from both alloc() and free() operations.
   // 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.
@@ -443,8 +495,16 @@ template <class Base> class IsolatedStatsCache {
  */
 class IsolatedStoreImpl : public Store {
 public:
-  IsolatedStoreImpl()
-      : counters_([this](const std::string& name) -> CounterSharedPtr {
+  // TODO(jmarantz): It appears that if the configuartion overrides the max
+  // option length, this isolated store will not see the adjustment. I am
+  // not sure if that's a problem. Ideally I'd like to take the stat_options
+  // here we can creating the StatAllocator, but it's not immediately obvious
+  // where to pull that from. I think this issue existed starting with
+  // https://github.com/envoyproxy/envoy/pull/3629, which removed the shadowing
+  // of option size in a static.
+
+  /* explicit */ IsolatedStoreImpl(/*Stats::StatsOptions& stats_options*/)
+      : alloc_(stats_options_), 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));
@@ -500,11 +560,11 @@ class IsolatedStoreImpl : public Store {
     const std::string prefix_;
   };
 
-  HeapRawStatDataAllocator alloc_;
+  StatsOptionsImpl stats_options_;
+  HeapStatDataAllocator alloc_;
   IsolatedStatsCache<Counter> counters_;
   IsolatedStatsCache<Gauge> gauges_;
   IsolatedStatsCache<Histogram> histograms_;
-  const Stats::StatsOptionsImpl stats_options_;
 };
 
 } // namespace Stats

source/common/stats/thread_local_store.cc

@@ -14,7 +14,7 @@ namespace Stats {
 
 ThreadLocalStoreImpl::ThreadLocalStoreImpl(const Stats::StatsOptions& stats_options,
                                            StatDataAllocator& alloc)
-    : stats_options_(stats_options), alloc_(alloc), default_scope_(createScope("")),
+    : alloc_(alloc), heap_allocator_(stats_options), default_scope_(createScope("")),
       tag_producer_(std::make_unique<TagProducerImpl>()),
       num_last_resort_stats_(default_scope_->counter("stats.overflow")), source_(*this) {}