HeapStatData with a distinct allocation mechanism for RawStatData (envoyproxy#3710)

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

Author: jmarantz

include/envoy/stats/stats.h

@@ -467,7 +467,7 @@ class StatDataAllocator {
    * @return CounterSharedPtr a counter, or nullptr if allocation failed, in which case
    *     tag_extracted_name and tags are not moved.
    */
-  virtual CounterSharedPtr makeCounter(const std::string& name, std::string&& tag_extracted_name,
+  virtual CounterSharedPtr makeCounter(absl::string_view name, std::string&& tag_extracted_name,
                                        std::vector<Tag>&& tags) PURE;
 
   /**
@@ -477,9 +477,14 @@ class StatDataAllocator {
    * @return GaugeSharedPtr a gauge, or nullptr if allocation failed, in which case
    *     tag_extracted_name and tags are not moved.
    */
-  virtual GaugeSharedPtr makeGauge(const std::string& name, std::string&& tag_extracted_name,
+  virtual GaugeSharedPtr makeGauge(absl::string_view name, std::string&& tag_extracted_name,
                                    std::vector<Tag>&& tags) PURE;
 
+  /**
+   * Determines whether this stats allocator requires bounded stat-name size.
+   */
+  virtual bool requiresBoundedStatNameSize() 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/common/block_memory_hash_set.h

@@ -165,7 +165,7 @@ template <class Value> class BlockMemoryHashSet : public Logger::Loggable<Logger
     cell.next_cell_index = slots_[slot];
     slots_[slot] = cell_index;
     value = &cell.value;
-    value->truncateAndInit(key, stats_options_);
+    value->initialize(key, stats_options_);
     ++control_->size;
     return ValueCreatedPair(value, true);
   }

source/common/stats/stats_impl.cc

@@ -135,35 +135,35 @@ 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(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 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;
 }
 
 /**
- * Counter implementation that wraps a RawStatData.
+ * Counter implementation that wraps a StatData. StatData must have data members:
+ *    std::atomic<int64_t> value_;
+ *    std::atomic<int64_t> pending_increment_;
+ *    std::atomic<int16_t> flags_;
+ *    std::atomic<int16_t> ref_count_;
  */
-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 +182,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 +217,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,47 +319,28 @@ TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v2::StatsCon
   return names;
 }
 
-void HeapRawStatDataAllocator::free(RawStatData& data) {
+// TODO(jmarantz): move this below HeapStatDataAllocator::alloc.
+void HeapStatDataAllocator::free(HeapStatData& 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);
+    size_t key_removed = stats_.erase(&data);
+    ASSERT(key_removed == 1);
   }
 
-  ASSERT(key_removed == 1);
-  ::free(&data);
+  delete &data;
 }
 
-void RawStatData::initialize(absl::string_view key, uint64_t xfer_size) {
+void RawStatData::initialize(absl::string_view key, const StatsOptions& stats_options) {
   ASSERT(!initialized());
+  ASSERT(key.size() <= stats_options.maxNameLength());
   ref_count_ = 1;
-  memcpy(name_, key.data(), xfer_size);
-  name_[xfer_size] = '\0';
-}
-
-void RawStatData::checkAndInit(absl::string_view key, uint64_t num_bytes_allocated) {
-  uint64_t xfer_size = key.size();
-  ASSERT(structSize(xfer_size) <= num_bytes_allocated);
-
-  initialize(key, xfer_size);
-}
-
-void RawStatData::truncateAndInit(absl::string_view key, const StatsOptions& stats_options) {
-  if (key.size() > stats_options.maxNameLength()) {
-    ENVOY_LOG_MISC(
-        warn,
-        "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", key,
-        key.size(), stats_options.maxNameLength());
-  }
-
-  // key is not necessarily nul-terminated, but we want to make sure name_ is.
-  uint64_t xfer_size = std::min(stats_options.maxNameLength(), key.size());
-  initialize(key, xfer_size);
+  memcpy(name_, key.data(), key.size());
+  name_[key.size()] = '\0';
 }
 
 HistogramStatisticsImpl::HistogramStatisticsImpl(const histogram_t* histogram_ptr)
@@ -420,26 +401,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(absl::string_view 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(absl::string_view 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