stats: remove storm of refcount bumping during metrics snapshot.

source/common/stats/allocator.h

@@ -24,6 +24,20 @@ class Allocator {
   Allocator(SymbolTable& symbol_table) : symbol_table_(symbol_table) {}
   virtual ~Allocator();
 
+private:
+  // We use a private 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.
+
   /**
    * @param name the full name of the stat.
    * @param tag_extracted_name the name of the stat with tag-values stripped out.
@@ -50,6 +64,8 @@ class Allocator {
    */
   TextReadoutSharedPtr makeTextReadout(StatName name, StatName tag_extracted_name,
                                        const StatNameTagVector& stat_name_tags);
+
+public:
   SymbolTable& symbolTable() { return symbol_table_; }
   const SymbolTable& constSymbolTable() const { return symbol_table_; }
 
@@ -111,9 +127,13 @@ class Allocator {
 
 private:
   template <class BaseClass> friend class StatsSharedImpl;
+  friend class AllocatorTest;
   friend class CounterImpl;
   friend class GaugeImpl;
+  friend class IsolatedStoreImpl;
+  friend class MetricImplTest;
   friend class TextReadoutImpl;
+  friend class ThreadLocalStoreImpl;
 
   // A mutex is needed here to protect both the stats_ object from both
   // alloc() and free() operations. Although alloc() operations are called under existing locking,

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_;

test/common/stats/allocator_test.cc

@@ -16,7 +16,6 @@
 
 namespace Envoy {
 namespace Stats {
-namespace {
 
 class AllocatorTest : public testing::Test {
 protected:
@@ -37,6 +36,25 @@ class AllocatorTest : public testing::Test {
     }
   }
 
+  // Wrappers for Allocator stats construction methods, held in the test fixture
+  // so they can be friended by AllocatorImpl. Those methods are protected in
+  // order to prevent filters from creating stats directly on the allocator;
+  // they should exclusively be managed by Scopes.
+  CounterSharedPtr makeCounter(StatName name, StatName tag_extracted_name,
+                               const StatNameTagVector& stat_name_tags) {
+    return alloc_.makeCounter(name, tag_extracted_name, stat_name_tags);
+  }
+
+  GaugeSharedPtr makeGauge(StatName name, StatName tag_extracted_name,
+                           const StatNameTagVector& stat_name_tags, Gauge::ImportMode import) {
+    return alloc_.makeGauge(name, tag_extracted_name, stat_name_tags, import);
+  }
+
+  TextReadoutSharedPtr makeTextReadout(StatName name, StatName tag_extracted_name,
+                                       const StatNameTagVector& stat_name_tags) {
+    return alloc_.makeTextReadout(name, tag_extracted_name, stat_name_tags);
+  }
+
   SymbolTableImpl symbol_table_;
   // Declare the pool before the allocator because the allocator could contain
   // a TestSinkPredicates object whose lifetime should be bounded by that of the pool.
@@ -48,9 +66,9 @@ class AllocatorTest : public testing::Test {
 // Allocate 2 counters of the same name, and you'll get the same object.
 TEST_F(AllocatorTest, CountersWithSameName) {
   StatName counter_name = makeStat("counter.name");
-  CounterSharedPtr c1 = alloc_.makeCounter(counter_name, StatName(), {});
+  CounterSharedPtr c1 = makeCounter(counter_name, StatName(), {});
   EXPECT_EQ(1, c1->use_count());
-  CounterSharedPtr c2 = alloc_.makeCounter(counter_name, StatName(), {});
+  CounterSharedPtr c2 = makeCounter(counter_name, StatName(), {});
   EXPECT_EQ(2, c1->use_count());
   EXPECT_EQ(2, c2->use_count());
   EXPECT_EQ(c1.get(), c2.get());
@@ -66,9 +84,9 @@ TEST_F(AllocatorTest, CountersWithSameName) {
 
 TEST_F(AllocatorTest, GaugesWithSameName) {
   StatName gauge_name = makeStat("gauges.name");
-  GaugeSharedPtr g1 = alloc_.makeGauge(gauge_name, StatName(), {}, Gauge::ImportMode::Accumulate);
+  GaugeSharedPtr g1 = makeGauge(gauge_name, StatName(), {}, Gauge::ImportMode::Accumulate);
   EXPECT_EQ(1, g1->use_count());
-  GaugeSharedPtr g2 = alloc_.makeGauge(gauge_name, StatName(), {}, Gauge::ImportMode::Accumulate);
+  GaugeSharedPtr g2 = makeGauge(gauge_name, StatName(), {}, Gauge::ImportMode::Accumulate);
   EXPECT_EQ(2, g1->use_count());
   EXPECT_EQ(2, g2->use_count());
   EXPECT_EQ(g1.get(), g2.get());
@@ -101,8 +119,8 @@ TEST_F(AllocatorTest, RefCountDecAllocRaceOrganic) {
     threads.push_back(thread_factory.createThread([&]() {
       go.WaitForNotification();
       for (uint32_t i = 0; i < iters; ++i) {
-        alloc_.makeCounter(counter_name, StatName(), {});
-        alloc_.makeGauge(gauge_name, StatName(), {}, Gauge::ImportMode::NeverImport);
+        makeCounter(counter_name, StatName(), {});
+        makeGauge(gauge_name, StatName(), {}, Gauge::ImportMode::NeverImport);
       }
     }));
   }
@@ -125,7 +143,7 @@ TEST_F(AllocatorTest, RefCountDecAllocRaceSynchronized) {
   alloc_.sync().enable();
   alloc_.sync().waitOn(Allocator::DecrementToZeroSyncPoint);
   Thread::ThreadPtr thread = thread_factory.createThread([&]() {
-    CounterSharedPtr counter = alloc_.makeCounter(counter_name, StatName(), {});
+    CounterSharedPtr counter = makeCounter(counter_name, StatName(), {});
     counter->inc();
     counter->reset(); // Blocks in thread synchronizer waiting on DecrementToZeroSyncPoint
   });
@@ -139,17 +157,17 @@ TEST_F(AllocatorTest, RefCountDecAllocRaceSynchronized) {
 
 TEST_F(AllocatorTest, HiddenGauge) {
   GaugeSharedPtr hidden_gauge =
-      alloc_.makeGauge(makeStat("hidden"), StatName(), {}, Gauge::ImportMode::HiddenAccumulate);
+      makeGauge(makeStat("hidden"), StatName(), {}, Gauge::ImportMode::HiddenAccumulate);
   EXPECT_EQ(hidden_gauge->importMode(), Gauge::ImportMode::HiddenAccumulate);
   EXPECT_TRUE(hidden_gauge->hidden());
 
   GaugeSharedPtr non_hidden_gauge =
-      alloc_.makeGauge(makeStat("non_hidden"), StatName(), {}, Gauge::ImportMode::Accumulate);
+      makeGauge(makeStat("non_hidden"), StatName(), {}, Gauge::ImportMode::Accumulate);
   EXPECT_NE(non_hidden_gauge->importMode(), Gauge::ImportMode::HiddenAccumulate);
   EXPECT_FALSE(non_hidden_gauge->hidden());
 
-  GaugeSharedPtr never_import_hidden_gauge = alloc_.makeGauge(
-      makeStat("never_import_hidden"), StatName(), {}, Gauge::ImportMode::NeverImport);
+  GaugeSharedPtr never_import_hidden_gauge =
+      makeGauge(makeStat("never_import_hidden"), StatName(), {}, Gauge::ImportMode::NeverImport);
   EXPECT_NE(never_import_hidden_gauge->importMode(), Gauge::ImportMode::HiddenAccumulate);
   EXPECT_FALSE(never_import_hidden_gauge->hidden());
 }
@@ -163,7 +181,7 @@ TEST_F(AllocatorTest, ForEachCounter) {
   for (size_t idx = 0; idx < num_stats; ++idx) {
     auto stat_name = makeStat(absl::StrCat("counter.", idx));
     stat_names.insert(stat_name);
-    counters.emplace_back(alloc_.makeCounter(stat_name, StatName(), {}));
+    counters.emplace_back(makeCounter(stat_name, StatName(), {}));
   }
 
   size_t num_counters = 0;
@@ -216,7 +234,7 @@ TEST_F(AllocatorTest, ForEachGauge) {
   for (size_t idx = 0; idx < num_stats; ++idx) {
     auto stat_name = makeStat(absl::StrCat("gauge.", idx));
     stat_names.insert(stat_name);
-    gauges.emplace_back(alloc_.makeGauge(stat_name, StatName(), {}, Gauge::ImportMode::Accumulate));
+    gauges.emplace_back(makeGauge(stat_name, StatName(), {}, Gauge::ImportMode::Accumulate));
   }
 
   size_t num_gauges = 0;
@@ -269,7 +287,7 @@ TEST_F(AllocatorTest, ForEachTextReadout) {
   for (size_t idx = 0; idx < num_stats; ++idx) {
     auto stat_name = makeStat(absl::StrCat("text_readout.", idx));
     stat_names.insert(stat_name);
-    text_readouts.emplace_back(alloc_.makeTextReadout(stat_name, StatName(), {}));
+    text_readouts.emplace_back(makeTextReadout(stat_name, StatName(), {}));
   }
 
   size_t num_text_readouts = 0;
@@ -326,7 +344,7 @@ TEST_F(AllocatorTest, ForEachWithNullSizeLambda) {
   // For each counter.
   for (size_t idx = 0; idx < num_stats; ++idx) {
     auto stat_name = makeStat(absl::StrCat("counter.", idx));
-    counters.emplace_back(alloc_.makeCounter(stat_name, StatName(), {}));
+    counters.emplace_back(makeCounter(stat_name, StatName(), {}));
   }
   size_t num_iterations = 0;
   alloc_.forEachCounter(nullptr, [&num_iterations](Counter& counter) {
@@ -338,7 +356,7 @@ TEST_F(AllocatorTest, ForEachWithNullSizeLambda) {
   // For each gauge.
   for (size_t idx = 0; idx < num_stats; ++idx) {
     auto stat_name = makeStat(absl::StrCat("gauge.", idx));
-    gauges.emplace_back(alloc_.makeGauge(stat_name, StatName(), {}, Gauge::ImportMode::Accumulate));
+    gauges.emplace_back(makeGauge(stat_name, StatName(), {}, Gauge::ImportMode::Accumulate));
   }
   num_iterations = 0;
   alloc_.forEachGauge(nullptr, [&num_iterations](Gauge& gauge) {
@@ -350,7 +368,7 @@ TEST_F(AllocatorTest, ForEachWithNullSizeLambda) {
   // For each text readout.
   for (size_t idx = 0; idx < num_stats; ++idx) {
     auto stat_name = makeStat(absl::StrCat("text_readout.", idx));
-    text_readouts.emplace_back(alloc_.makeTextReadout(stat_name, StatName(), {}));
+    text_readouts.emplace_back(makeTextReadout(stat_name, StatName(), {}));
   }
   num_iterations = 0;
   alloc_.forEachTextReadout(nullptr, [&num_iterations](TextReadout& text_readout) {
@@ -370,7 +388,7 @@ TEST_F(AllocatorTest, AskForDeletedStat) {
   std::vector<CounterSharedPtr> counters;
   for (size_t idx = 0; idx < num_stats; ++idx) {
     auto stat_name = makeStat(absl::StrCat("counter.", idx));
-    counters.emplace_back(alloc_.makeCounter(stat_name, StatName(), {}));
+    counters.emplace_back(makeCounter(stat_name, StatName(), {}));
   }
   // Reject a stat and remove it from "scope".
   StatName const rejected_counter_name = counters[4]->statName();
@@ -383,15 +401,15 @@ TEST_F(AllocatorTest, AskForDeletedStat) {
   rejected_counter.inc();
 
   // Make the deleted stat again.
-  CounterSharedPtr deleted_counter = alloc_.makeCounter(rejected_counter_name, StatName(), {});
+  CounterSharedPtr deleted_counter = makeCounter(rejected_counter_name, StatName(), {});
 
   EXPECT_EQ(deleted_counter->value(), 0);
   EXPECT_EQ(rejected_counter.value(), 2);
 
   std::vector<GaugeSharedPtr> gauges;
   for (size_t idx = 0; idx < num_stats; ++idx) {
     auto stat_name = makeStat(absl::StrCat("gauge.", idx));
-    gauges.emplace_back(alloc_.makeGauge(stat_name, StatName(), {}, Gauge::ImportMode::Accumulate));
+    gauges.emplace_back(makeGauge(stat_name, StatName(), {}, Gauge::ImportMode::Accumulate));
   }
   // Reject a stat and remove it from "scope".
   StatName const rejected_gauge_name = gauges[4]->statName();
@@ -404,15 +422,15 @@ TEST_F(AllocatorTest, AskForDeletedStat) {
 
   // Make the deleted stat again.
   GaugeSharedPtr deleted_gauge =
-      alloc_.makeGauge(rejected_gauge_name, StatName(), {}, Gauge::ImportMode::Accumulate);
+      makeGauge(rejected_gauge_name, StatName(), {}, Gauge::ImportMode::Accumulate);
 
   EXPECT_EQ(deleted_gauge->value(), 0);
   EXPECT_EQ(rejected_gauge.value(), 10);
 
   std::vector<TextReadoutSharedPtr> text_readouts;
   for (size_t idx = 0; idx < num_stats; ++idx) {
     auto stat_name = makeStat(absl::StrCat("text_readout.", idx));
-    text_readouts.emplace_back(alloc_.makeTextReadout(stat_name, StatName(), {}));
+    text_readouts.emplace_back(makeTextReadout(stat_name, StatName(), {}));
   }
   // Reject a stat and remove it from "scope".
   StatName const rejected_text_readout_name = text_readouts[4]->statName();
@@ -425,7 +443,7 @@ TEST_F(AllocatorTest, AskForDeletedStat) {
 
   // Make the deleted stat again.
   TextReadoutSharedPtr deleted_text_readout =
-      alloc_.makeTextReadout(rejected_text_readout_name, StatName(), {});
+      makeTextReadout(rejected_text_readout_name, StatName(), {});
 
   EXPECT_EQ(deleted_text_readout->value(), "");
   EXPECT_EQ(rejected_text_readout.value(), "deleted value");
@@ -447,9 +465,9 @@ TEST_F(AllocatorTest, ForEachSinkedCounter) {
     // sink every 3rd stat
     if ((idx + 1) % 3 == 0) {
       sink_predicates->add(stat_name);
-      sinked_counters.emplace_back(alloc_.makeCounter(stat_name, StatName(), {}));
+      sinked_counters.emplace_back(makeCounter(stat_name, StatName(), {}));
     } else {
-      unsinked_counters.emplace_back(alloc_.makeCounter(stat_name, StatName(), {}));
+      unsinked_counters.emplace_back(makeCounter(stat_name, StatName(), {}));
     }
   }
 
@@ -493,10 +511,10 @@ TEST_F(AllocatorTest, ForEachSinkedGauge) {
     if ((idx + 1) % 5 == 0) {
       sink_predicates->add(stat_name);
       sinked_gauges.emplace_back(
-          alloc_.makeGauge(stat_name, StatName(), {}, Gauge::ImportMode::Accumulate));
+          makeGauge(stat_name, StatName(), {}, Gauge::ImportMode::Accumulate));
     } else {
       unsinked_gauges.emplace_back(
-          alloc_.makeGauge(stat_name, StatName(), {}, Gauge::ImportMode::Accumulate));
+          makeGauge(stat_name, StatName(), {}, Gauge::ImportMode::Accumulate));
     }
   }
 
@@ -532,11 +550,9 @@ TEST_F(AllocatorTest, ForEachSinkedGaugeHidden) {
   size_t num_gauges = 0;
   size_t num_iterations = 0;
 
-  unhidden_gauge =
-      alloc_.makeGauge(unhidden_stat_name, StatName(), {}, Gauge::ImportMode::Accumulate);
+  unhidden_gauge = makeGauge(unhidden_stat_name, StatName(), {}, Gauge::ImportMode::Accumulate);
 
-  hidden_gauge =
-      alloc_.makeGauge(hidden_stat_name, StatName(), {}, Gauge::ImportMode::HiddenAccumulate);
+  hidden_gauge = makeGauge(hidden_stat_name, StatName(), {}, Gauge::ImportMode::HiddenAccumulate);
 
   alloc_.forEachSinkedGauge([&num_gauges](std::size_t size) { num_gauges = size; },
                             [&num_iterations, unhidden_stat_name](Gauge& gauge) {
@@ -565,11 +581,9 @@ TEST_F(AllocatorTest, ForEachSinkedGaugeHiddenPredicate) {
   size_t num_gauges = 0;
   size_t num_iterations = 0;
 
-  unhidden_gauge =
-      alloc_.makeGauge(unhidden_stat_name, StatName(), {}, Gauge::ImportMode::Accumulate);
+  unhidden_gauge = makeGauge(unhidden_stat_name, StatName(), {}, Gauge::ImportMode::Accumulate);
 
-  hidden_gauge =
-      alloc_.makeGauge(hidden_stat_name, StatName(), {}, Gauge::ImportMode::HiddenAccumulate);
+  hidden_gauge = makeGauge(hidden_stat_name, StatName(), {}, Gauge::ImportMode::HiddenAccumulate);
 
   alloc_.forEachSinkedGauge([&num_gauges](std::size_t size) { num_gauges = size; },
                             [&num_iterations, &sink_predicates](Gauge& gauge) {
@@ -596,9 +610,9 @@ TEST_F(AllocatorTest, ForEachSinkedTextReadout) {
     // sink every 2nd stat
     if ((idx + 1) % 2 == 0) {
       sink_predicates->add(stat_name);
-      sinked_text_readouts.emplace_back(alloc_.makeTextReadout(stat_name, StatName(), {}));
+      sinked_text_readouts.emplace_back(makeTextReadout(stat_name, StatName(), {}));
     } else {
-      unsinked_text_readouts.emplace_back(alloc_.makeTextReadout(stat_name, StatName(), {}));
+      unsinked_text_readouts.emplace_back(makeTextReadout(stat_name, StatName(), {}));
     }
   }
 
@@ -626,6 +640,5 @@ TEST_F(AllocatorTest, ForEachSinkedTextReadout) {
   EXPECT_EQ(num_iterations, 0);
 }
 
-} // namespace
 } // namespace Stats
 } // namespace Envoy