[DNR]Fix stat destructor benchmarking

source/common/stats/thread_local_store.cc

@@ -1130,15 +1130,21 @@ void ThreadLocalStoreImpl::evictUnused() {
         MetricBag metrics(scope->scope_id_);
         CentralCacheEntrySharedPtr& central_cache = scope->centralCacheMutableNoThreadAnalysis();
         auto filter_unused = []<typename T>(StatNameHashMap<T>& unused_metrics) {
-          return [&unused_metrics](std::pair<StatName, T> kv) {
+          return [&unused_metrics](const std::pair<const StatName, T>& kv) {
             const auto& [name, metric] = kv;
+            // We assume the gauge is used as an `updown` counter here and if the value is 0, it is
+            // unused.
+            if constexpr (std::is_same_v<T, GaugeSharedPtr>) {
+              if (metric->value() != 0) {
+                return false;
+              }
+            }
             if (metric->used()) {
               metric->markUnused();
               return false;
-            } else {
-              unused_metrics.try_emplace(name, metric);
-              return true;
             }
+            unused_metrics.try_emplace(name, metric);
+            return true;
           };
         };
         absl::erase_if(central_cache->counters_, filter_unused(metrics.counters_));

source/extensions/access_loggers/stats/BUILD

@@ -19,6 +19,7 @@ envoy_cc_library(
         "//source/common/access_log:access_log_lib",
         "//source/common/formatter:substitution_format_string_lib",
         "//source/common/matcher:matcher_lib",
+        "//source/common/stats:tag_utility_lib",
         "//source/extensions/access_loggers/common:access_log_base",
         "//source/extensions/matching/actions/transform_stat:transform_stat_lib",
         "@envoy_api//envoy/data/accesslog/v3:pkg_cc_proto",

source/extensions/access_loggers/stats/stats.cc

@@ -7,6 +7,7 @@
 
 #include "source/common/formatter/substitution_formatter.h"
 #include "source/common/stats/symbol_table.h"
+#include "source/common/stats/tag_utility.h"
 
 namespace Envoy {
 namespace Extensions {
@@ -19,64 +20,81 @@ using Extensions::Matching::Actions::TransformStat::ActionContext;
 
 class AccessLogState : public StreamInfo::FilterState::Object {
 public:
-  AccessLogState(Stats::ScopeSharedPtr scope) : scope_(std::move(scope)) {}
+  AccessLogState(std::shared_ptr<const StatsAccessLog> parent) : parent_(std::move(parent)) {}
 
-  // When the request is destroyed, we need to subtract the value from the gauge.
-  // We need to look up the gauge again in the scope because it might have been evicted.
-  // The gauge object itself is kept alive by the shared_ptr in the state, so we can access its
-  // name and tags to re-lookup/re-create it in the scope.
   ~AccessLogState() override {
-    for (const auto& [gauge_ptr, state] : inflight_gauges_) {
-      // TODO(taoxuy):  make this as an accessor of the
-      // Stat class.
-      Stats::StatNameTagVector tag_names;
-      state.gauge_->iterateTagStatNames(
-          [&tag_names](Stats::StatName name, Stats::StatName value) -> bool {
-            tag_names.emplace_back(name, value);
-            return true;
-          });
-
-      // Using state.gauge_->statName() directly would be incorrect because it returns the fully
-      // qualified name (including tags). Passing this full name to scope_->gaugeFromStatName(...)
-      // would cause the scope to attempt tag extraction on the full name. Since the tags in
-      // AccessLogState are often dynamic and not configured in the global tag extractors, this
-      // extraction would likely fail to identify the tags correctly, resulting in a gauge with a
-      // different identity (the full name as the stat name and no tags).
-      auto& gauge = scope_->gaugeFromStatNameWithTags(
-          state.gauge_->tagExtractedStatName(), tag_names, Stats::Gauge::ImportMode::Accumulate);
-      gauge.sub(state.value_);
+    for (const auto& p : inflight_gauges_) {
+      Stats::Gauge& gauge_stat = parent_->scope().gaugeFromStatName(p.first, p.second.import_mode_);
+      gauge_stat.sub(p.second.value_);
     }
   }
 
-  void addInflightGauge(Stats::Gauge* gauge, uint64_t value) {
-    inflight_gauges_.try_emplace(gauge, Stats::GaugeSharedPtr(gauge), value);
-  }
+  // Adds an incremental value to an existing gauge, or creates it if that gauge doesn't exist.
+  // Zero values are ignored.
+  void addInflightGauge(Stats::StatName stat_name, Stats::StatNameTagVectorOptConstRef tags,
+                        Stats::Gauge::ImportMode import_mode, uint64_t value,
+                        std::vector<Stats::StatNameDynamicStorage> tags_storage) {
+    if (value == 0) {
+      return;
+    }
 
-  absl::optional<uint64_t> removeInflightGauge(Stats::Gauge* gauge) {
-    auto it = inflight_gauges_.find(gauge);
+    Stats::TagUtility::TagStatNameJoiner joiner(Stats::StatName(), stat_name, tags, parent_->scope().symbolTable());
+    Stats::StatName joined_name = joiner.nameWithTags();
+
+    auto it = inflight_gauges_.find(joined_name);
     if (it == inflight_gauges_.end()) {
-      return absl::nullopt;
+      Stats::StatName persistent_joined_name = joiner.nameWithTags();
+      auto [new_it, inserted] = inflight_gauges_.emplace(
+          persistent_joined_name, InflightGauge{std::move(tags_storage), 0, import_mode,
+                                                std::move(joiner)});
+      it = new_it;
+    }
+    it->second.value_ += value;
+    parent_->scope().gaugeFromStatName(joined_name, import_mode).add(value);
+  }
+
+  // Removes an amount from an existing gauge, allowing the gauge to be evicted if the value reaches
+  // 0.
+  void removeInflightGauge(Stats::StatName stat_name, Stats::StatNameTagVectorOptConstRef tags,
+                           Stats::Gauge::ImportMode import_mode, uint64_t value) {
+    if (value == 0) {
+      return;
+    }
+
+    Stats::TagUtility::TagStatNameJoiner joiner(Stats::StatName(), stat_name, tags, parent_->scope().symbolTable());
+    Stats::StatName joined_name = joiner.nameWithTags();
+
+    // Create the gauge so it gets registered in the stat store (expected by some tests and stats
+    // logic)
+    Stats::Gauge& gauge_stat =
+        parent_->scope().gaugeFromStatName(joined_name, import_mode);
+
+    auto it = inflight_gauges_.find(joined_name);
+    if (it != inflight_gauges_.end()) {
+      ENVOY_BUG(it->second.value_ >= value, "Connection gauge underflow in removeInflightGauge");
+      it->second.value_ -= value;
+      gauge_stat.sub(value);
+      if (it->second.value_ == 0) {
+        inflight_gauges_.erase(it);
+      }
     }
-    uint64_t value = it->second.value_;
-    inflight_gauges_.erase(it);
-    return value;
   }
 
   static constexpr absl::string_view key() { return "envoy.access_loggers.stats.access_log_state"; }
 
 private:
-  struct State {
-    State(Stats::GaugeSharedPtr gauge, uint64_t value) : gauge_(std::move(gauge)), value_(value) {}
+  // Hold a shared_ptr to the parent to ensure the parent and its members exist for the lifetime of
+  // AccessLogState.
+  std::shared_ptr<const StatsAccessLog> parent_;
 
-    Stats::GaugeSharedPtr gauge_;
+  struct InflightGauge {
+    std::vector<Stats::StatNameDynamicStorage> tags_storage_;
     uint64_t value_;
+    Stats::Gauge::ImportMode import_mode_;
+    Stats::TagUtility::TagStatNameJoiner joiner_;
   };
 
-  Stats::ScopeSharedPtr scope_;
-
-  // The map key holds a raw pointer to the gauge. The value holds a ref-counted pointer to ensure
-  // the gauge is not destroyed if it is evicted from the stats scope.
-  absl::flat_hash_map<Stats::Gauge*, State> inflight_gauges_;
+  absl::flat_hash_map<Stats::StatName, InflightGauge> inflight_gauges_;
 };
 
 Formatter::FormatterProviderPtr
@@ -138,6 +156,7 @@ class TagActionValidationVisitor
 
 } // namespace
 
+
 StatsAccessLog::StatsAccessLog(const envoy::extensions::access_loggers::stats::v3::Config& config,
                                Server::Configuration::GenericFactoryContext& context,
                                AccessLog::FilterPtr&& filter,
@@ -417,31 +436,26 @@ void StatsAccessLog::emitLogForGauge(const Gauge& gauge, const Formatter::Contex
   Stats::Gauge::ImportMode import_mode = op == Gauge::OperationType::SET
                                              ? Stats::Gauge::ImportMode::NeverImport
                                              : Stats::Gauge::ImportMode::Accumulate;
-  auto& gauge_stat = scope_->gaugeFromStatNameWithTags(gauge.stat_.name_, tags, import_mode);
 
-  if (op == Gauge::OperationType::PAIRED_ADD || op == Gauge::OperationType::PAIRED_SUBTRACT) {
+  if (op == Gauge::OperationType::SET) {
+    Stats::Gauge& gauge_stat =
+        scope_->gaugeFromStatNameWithTags(gauge.stat_.name_, tags, import_mode);
+    gauge_stat.set(value);
+  } else if (op == Gauge::OperationType::PAIRED_ADD ||
+             op == Gauge::OperationType::PAIRED_SUBTRACT) {
     auto& filter_state = const_cast<StreamInfo::FilterState&>(stream_info.filterState());
     if (!filter_state.hasData<AccessLogState>(AccessLogState::key())) {
-      filter_state.setData(AccessLogState::key(), std::make_shared<AccessLogState>(scope_),
-                           StreamInfo::FilterState::StateType::Mutable,
-                           StreamInfo::FilterState::LifeSpan::Request);
+      filter_state.setData(
+          AccessLogState::key(), std::make_shared<AccessLogState>(shared_from_this()),
+          StreamInfo::FilterState::StateType::Mutable, StreamInfo::FilterState::LifeSpan::Request);
     }
     auto* state = filter_state.getDataMutable<AccessLogState>(AccessLogState::key());
 
     if (op == Gauge::OperationType::PAIRED_ADD) {
-      state->addInflightGauge(&gauge_stat, value);
-      gauge_stat.add(value);
+      state->addInflightGauge(gauge.stat_.name_, tags, import_mode, value, std::move(storage));
     } else {
-      absl::optional<uint64_t> added_value = state->removeInflightGauge(&gauge_stat);
-      if (added_value.has_value()) {
-        gauge_stat.sub(added_value.value());
-      }
+      state->removeInflightGauge(gauge.stat_.name_, tags, import_mode, value);
     }
-    return;
-  }
-
-  if (op == Gauge::OperationType::SET) {
-    gauge_stat.set(value);
   }
 }
 

source/extensions/access_loggers/stats/stats.h

@@ -6,6 +6,7 @@
 #include "envoy/stats/tag.h"
 
 #include "source/common/matcher/matcher.h"
+#include "source/common/stats/symbol_table.h"
 #include "source/extensions/access_loggers/common/access_log_base.h"
 #include "source/extensions/matching/actions/transform_stat/transform_stat.h"
 
@@ -14,8 +15,11 @@ namespace Extensions {
 namespace AccessLoggers {
 namespace StatsAccessLog {
 
-class StatsAccessLog : public AccessLoggers::Common::ImplBase {
+
+class StatsAccessLog : public AccessLoggers::Common::ImplBase,
+                       public std::enable_shared_from_this<StatsAccessLog> {
 public:
+  Stats::Scope& scope() const { return *scope_; }
   StatsAccessLog(const envoy::extensions::access_loggers::stats::v3::Config& config,
                  Server::Configuration::GenericFactoryContext& context,
                  AccessLog::FilterPtr&& filter,
@@ -103,6 +107,7 @@ class StatsAccessLog : public AccessLoggers::Common::ImplBase {
 };
 
 } // namespace StatsAccessLog
+
 } // namespace AccessLoggers
 } // namespace Extensions
 } // namespace Envoy

test/common/stats/thread_local_store_test.cc

@@ -536,6 +536,7 @@ TEST_F(StatsThreadLocalStoreTest, StatsNumLimitsWithEviction) {
     store_->mergeHistograms([]() -> void {});
 
     // First eviction marks stats as unused.
+    g1.sub(1); // Make value 0 so it can be marked unused
     store_->evictUnused();
     EXPECT_FALSE(c1.used());
     EXPECT_FALSE(g1.used());
@@ -706,6 +707,7 @@ TEST_F(StatsThreadLocalStoreTest, Eviction) {
     store_->mergeHistograms([]() -> void {});
 
     // Eviction only marks unused but does not remove the counters.
+    g1.set(0); // Make value 0 so it can be marked unused
     store_->evictUnused();
 
     EXPECT_EQ(&c1, &scope->counterFromString("c1"));
@@ -716,7 +718,7 @@ TEST_F(StatsThreadLocalStoreTest, Eviction) {
     EXPECT_EQ(&g1, &scope->gaugeFromString("g1", Gauge::ImportMode::Accumulate));
     EXPECT_EQ(&g1, &scope1->gaugeFromString("g1", Gauge::ImportMode::Accumulate));
     EXPECT_FALSE(g1.used());
-    EXPECT_EQ(5, g1.value());
+    EXPECT_EQ(0, g1.value());
     EXPECT_EQ(1UL, store_->gauges().size());
 
     EXPECT_EQ(&t1, &scope->textReadoutFromString("t1"));
@@ -731,6 +733,8 @@ TEST_F(StatsThreadLocalStoreTest, Eviction) {
     EXPECT_EQ(1UL, store_->histograms().size());
   }
 
+  // Gauge value was already made zero above
+
   // Eviction removes here.
   EXPECT_CALL(tls_, runOnAllThreads(_, _)).Times(testing::AtLeast(1));
   store_->evictUnused();
@@ -778,6 +782,7 @@ TEST_F(StatsThreadLocalStoreTest, EvictionGaugesInterleavedOperations) {
   // 2. MarkUnused / Evict
   // First pass marks unused. Note that evictUnused() only removes if it was ALREADY unused.
   // Since we just used it (g1.add(10)), the first call will only mark it as unused.
+  g1.sub(10); // Make value 0 so it can be marked unused
   store_->evictUnused();
   EXPECT_FALSE(g1.used());
   EXPECT_EQ(1UL, store_->gauges().size());
@@ -795,7 +800,7 @@ TEST_F(StatsThreadLocalStoreTest, EvictionGaugesInterleavedOperations) {
 
   // 3. Interleaved PAIRED_ADD (add) on the held reference
   g1_ref->add(5);
-  EXPECT_EQ(15, g1_ref->value());
+  EXPECT_EQ(5, g1_ref->value());
   EXPECT_TRUE(g1_ref->used());
 
   // 4. Re-resolve and PAIRED_SUBTRACT (sub)
@@ -805,17 +810,71 @@ TEST_F(StatsThreadLocalStoreTest, EvictionGaugesInterleavedOperations) {
   EXPECT_EQ(g1_ref.get(), &g1_resurrected);
 
   // Value should be preserved
-  EXPECT_EQ(15, g1_resurrected.value());
+  EXPECT_EQ(5, g1_resurrected.value());
 
   // Perform subtract
-  g1_resurrected.sub(15);
+  g1_resurrected.sub(5);
   EXPECT_EQ(0, g1_resurrected.value());
 
   tls_.shutdownGlobalThreading();
   store_->shutdownThreading();
   tls_.shutdownThread();
 }
 
+TEST_F(StatsThreadLocalStoreTest, EvictionGauges) {
+  InSequence s;
+  store_->initializeThreading(main_thread_dispatcher_, tls_);
+
+  ScopeSharedPtr scope = store_->rootScope()->createScope("scope.", /*evictable=*/true);
+
+  // 1. Create gauge and add to make it non-zero
+  Gauge& g1 = scope->gaugeFromString("g1", Gauge::ImportMode::Accumulate);
+  g1.add(10);
+  EXPECT_EQ(10, g1.value());
+  EXPECT_TRUE(g1.used());
+
+  // First pass marks unused. Note that evictUnused() only removes if it was ALREADY unused.
+  store_->evictUnused();
+  EXPECT_TRUE(g1.used()); // Value was non-zero, it should stay used.
+  EXPECT_EQ(1UL, store_->gauges().size());
+
+  // Second pass would normally evict from scope cache if it was zero, but since it's non-zero,
+  // it should stay.
+  store_->evictUnused();
+
+  // Verify STILL in scope
+  StatNameManagedStorage g1_name("scope.g1", symbol_table_);
+  auto found_gauge = scope->findGauge(g1_name.statName());
+  ASSERT_TRUE(found_gauge.has_value());
+  EXPECT_EQ(&g1, &(found_gauge.value().get()));
+
+  // Verify still in store (allocator)
+  EXPECT_EQ(1UL, store_->gauges().size());
+
+  // 2. Now subtract to make it zero and evict
+  g1.sub(10);
+  EXPECT_EQ(0, g1.value());
+
+  // First pass marks unused
+  store_->evictUnused();
+  main_thread_dispatcher_.run(Event::Dispatcher::RunType::NonBlock);
+
+  // Second pass actually evicts it here
+  EXPECT_CALL(tls_, runOnAllThreads(_, _)).Times(testing::AtLeast(1));
+  store_->evictUnused();
+  main_thread_dispatcher_.run(Event::Dispatcher::RunType::NonBlock);
+
+  // Verify removed from scope
+  EXPECT_FALSE(scope->findGauge(g1_name.statName()).has_value());
+
+  // Verify removed from store
+  EXPECT_EQ(0UL, store_->gauges().size());
+
+  tls_.shutdownGlobalThreading();
+  store_->shutdownThreading();
+  tls_.shutdownThread();
+}
+
 TEST_F(StatsThreadLocalStoreTest, NestedScopes) {
   InSequence s;
   store_->initializeThreading(main_thread_dispatcher_, tls_);

test/extensions/access_loggers/stats/BUILD

@@ -1,5 +1,6 @@
 load(
     "//bazel:envoy_build_system.bzl",
+    "envoy_cc_benchmark_binary",
     "envoy_package",
 )
 load(
@@ -25,6 +26,7 @@ envoy_extension_cc_test(
         "//test/mocks/server:factory_context_mocks",
         "//test/mocks/stream_info:stream_info_mocks",
         "//test/test_common:logging_lib",
+        "@abseil-cpp//absl/hash:hash_testing",
     ],
 )
 
@@ -42,3 +44,14 @@ envoy_extension_cc_test(
         "@envoy_api//envoy/extensions/filters/network/http_connection_manager/v3:pkg_cc_proto",
     ],
 )
+envoy_cc_benchmark_binary(
+    name = "stats_speed_test",
+    srcs = ["stats_speed_test.cc"],
+    rbe_pool = "6gig",
+    deps = [
+        "//source/common/stats:isolated_store_lib",
+        "//source/common/stats:symbol_table_lib",
+        "//source/common/stats:tag_utility_lib",
+        "@benchmark",
+    ],
+)