stats: remove storm of refcount bumping during metrics snapshot.

envoy/stats/allocator.h

@@ -30,6 +30,23 @@ class Allocator {
 public:
   virtual ~Allocator() = default;
 
+protected:
+  // We use a protected section for makeCounter, makeGauge, and makeTextReadout.
+  // We do not want general filter code to be creating stats in this way, they
+  // must go through the Scope interface.
+  //
+  // This allows the MetricSnapshotImpl to hold onto references to the stats
+  // holding onto references to the Scopes, rather than holding onto each
+  // individual stat. In other words, all the stats must be owned by one or
+  // more scopes.
+  //
+  // The alternative is to have that function hold onto reference to every stat,
+  // which requires incrementing and decrementing reference counts to each stat,
+  // which is very slow, particularly on ARM processors.
+  //
+  // Note that we must 'friend' a different set of classes from AllocatorImpl.
+  friend class ThreadLocalStoreImpl;
+
   /**
    * @param name the full name of the stat.
    * @param tag_extracted_name the name of the stat with tag-values stripped out.
@@ -57,6 +74,8 @@ class Allocator {
    */
   virtual TextReadoutSharedPtr makeTextReadout(StatName name, StatName tag_extracted_name,
                                                const StatNameTagVector& stat_name_tags) PURE;
+
+public:
   virtual const SymbolTable& constSymbolTable() const PURE;
   virtual SymbolTable& symbolTable() PURE;
 

source/common/stats/allocator_impl.h

@@ -23,14 +23,27 @@ class AllocatorImpl : public Allocator {
   AllocatorImpl(SymbolTable& symbol_table) : symbol_table_(symbol_table) {}
   ~AllocatorImpl() override;
 
-  // Allocator
+protected:
+  // Please refer to the comment on the protected section in Allocator.
+  //
+  // TODO(jmarantz): consider dropping the pure interface for Allocator,
+  // merging AllocatorImpl and Allocator. In the distant past there were
+  // two implementations of Allocator, one in shared memory, but this is
+  // no longer needed.
+  friend class AllocatorImplTest;
+  friend class IsolatedStoreImpl;
+  friend class MetricImplTest;
+
   CounterSharedPtr makeCounter(StatName name, StatName tag_extracted_name,
                                const StatNameTagVector& stat_name_tags) override;
   GaugeSharedPtr makeGauge(StatName name, StatName tag_extracted_name,
                            const StatNameTagVector& stat_name_tags,
                            Gauge::ImportMode import_mode) override;
   TextReadoutSharedPtr makeTextReadout(StatName name, StatName tag_extracted_name,
                                        const StatNameTagVector& stat_name_tags) override;
+
+public:
+  // Allocator
   SymbolTable& symbolTable() override { return symbol_table_; }
   const SymbolTable& constSymbolTable() const override { return symbol_table_; }
 

source/server/server.cc

@@ -171,45 +171,26 @@ void InstanceBase::failHealthcheck(bool fail) {
 MetricSnapshotImpl::MetricSnapshotImpl(Stats::Store& store,
                                        Upstream::ClusterManager& cluster_manager,
                                        TimeSource& time_source) {
+  // Capture references to all the scopes in this, which will in turn keep all
+  // of the contained counters, gauges, text readouts, and histograms live for
+  // the duration of the MetricsSnapshot.
+  store.forEachScope(
+      [this](std::size_t size) { scopes_.reserve(size); },
+      [this](const Stats::Scope& scope) { scopes_.push_back(scope.getConstShared()); });
   store.forEachSinkedCounter(
-      [this](std::size_t size) {
-        snapped_counters_.reserve(size);
-        counters_.reserve(size);
-      },
-      [this](Stats::Counter& counter) {
-        snapped_counters_.push_back(Stats::CounterSharedPtr(&counter));
-        counters_.push_back({counter.latch(), counter});
-      });
+      [this](std::size_t size) { counters_.reserve(size); },
+      [this](Stats::Counter& counter) { counters_.push_back({counter.latch(), counter}); });
 
-  store.forEachSinkedGauge(
-      [this](std::size_t size) {
-        snapped_gauges_.reserve(size);
-        gauges_.reserve(size);
-      },
-      [this](Stats::Gauge& gauge) {
-        snapped_gauges_.push_back(Stats::GaugeSharedPtr(&gauge));
-        gauges_.push_back(gauge);
-      });
+  store.forEachSinkedGauge([this](std::size_t size) { gauges_.reserve(size); },
+                           [this](Stats::Gauge& gauge) { gauges_.push_back(gauge); });
 
   store.forEachSinkedHistogram(
-      [this](std::size_t size) {
-        snapped_histograms_.reserve(size);
-        histograms_.reserve(size);
-      },
-      [this](Stats::ParentHistogram& histogram) {
-        snapped_histograms_.push_back(Stats::ParentHistogramSharedPtr(&histogram));
-        histograms_.push_back(histogram);
-      });
+      [this](std::size_t size) { histograms_.reserve(size); },
+      [this](Stats::ParentHistogram& histogram) { histograms_.push_back(histogram); });
 
   store.forEachSinkedTextReadout(
-      [this](std::size_t size) {
-        snapped_text_readouts_.reserve(size);
-        text_readouts_.reserve(size);
-      },
-      [this](Stats::TextReadout& text_readout) {
-        snapped_text_readouts_.push_back(Stats::TextReadoutSharedPtr(&text_readout));
-        text_readouts_.push_back(text_readout);
-      });
+      [this](std::size_t size) { text_readouts_.reserve(size); },
+      [this](Stats::TextReadout& text_readout) { text_readouts_.push_back(text_readout); });
 
   Upstream::HostUtility::forEachHostMetric(
       cluster_manager,

source/server/server.h

@@ -491,13 +491,10 @@ class MetricSnapshotImpl : public Stats::MetricSnapshot {
   SystemTime snapshotTime() const override { return snapshot_time_; }
 
 private:
-  std::vector<Stats::CounterSharedPtr> snapped_counters_;
+  std::vector<Stats::ConstScopeSharedPtr> scopes_;
   std::vector<CounterSnapshot> counters_;
-  std::vector<Stats::GaugeSharedPtr> snapped_gauges_;
   std::vector<std::reference_wrapper<const Stats::Gauge>> gauges_;
-  std::vector<Stats::ParentHistogramSharedPtr> snapped_histograms_;
   std::vector<std::reference_wrapper<const Stats::ParentHistogram>> histograms_;
-  std::vector<Stats::TextReadoutSharedPtr> snapped_text_readouts_;
   std::vector<std::reference_wrapper<const Stats::TextReadout>> text_readouts_;
   std::vector<Stats::PrimitiveCounterSnapshot> host_counters_;
   std::vector<Stats::PrimitiveGaugeSnapshot> host_gauges_;