stats: Add a new class StatNamePool to abstract some StatName memory management.

source/common/stats/fake_symbol_table_impl.h

@@ -109,9 +109,8 @@ class FakeSymbolTableImpl : public SymbolTable {
   SymbolTable::StoragePtr join(const std::vector<StatName>& names) const override {
     std::vector<absl::string_view> strings;
     for (StatName name : names) {
-      absl::string_view str = toStringView(name);
-      if (!str.empty()) {
-        strings.push_back(str);
+      if (!name.empty()) {
+        strings.push_back(toStringView(name));
       }
     }
     return encodeHelper(absl::StrJoin(strings, "."));

source/common/stats/symbol_table_impl.cc

@@ -317,6 +317,18 @@ void StatNameStorage::free(SymbolTable& table) {
   bytes_.reset();
 }
 
+void StatNamePool::clear() {
+  for (StatNameStorage& storage : storage_vector_) {
+    storage.free(symbol_table_);
+  }
+  storage_vector_.clear();
+}
+
+StatName StatNamePool::add(absl::string_view str) {
+  storage_vector_.push_back(Stats::StatNameStorage(str, symbol_table_));
+  return StatName(storage_vector_.back().bytes());
+}
+
 StatNameStorageSet::~StatNameStorageSet() {
   // free() must be called before destructing StatNameStorageSet to decrement
   // references to all symbols.

source/common/stats/symbol_table_impl.h

@@ -287,6 +287,8 @@ class StatNameStorage {
    */
   inline StatName statName() const;
 
+  uint8_t* bytes() { return bytes_.get(); }
+
 private:
   SymbolTable::StoragePtr bytes_;
 };
@@ -353,10 +355,15 @@ class StatName {
 #endif
 
   /**
-   * @return uint8_t* A pointer to the first byte of data (skipping over size bytes).
+   * @return A pointer to the first byte of data (skipping over size bytes).
    */
   const uint8_t* data() const { return size_and_data_ + StatNameSizeEncodingBytes; }
 
+  /**
+   * @return whether this is empty.
+   */
+  bool empty() const { return size_and_data_ == nullptr || dataSize() == 0; }
+
 private:
   const uint8_t* size_and_data_;
 };
@@ -400,6 +407,41 @@ class StatNameManagedStorage : public StatNameStorage {
   SymbolTable& symbol_table_;
 };
 
+/**
+ * Maintains storage for a collection of StatName objects. Like
+ * StatNameManagedStorage, this has an RAII usage model, taking
+ * care of decrementing ref-counts in the SymbolTable for all
+ * contained StatNames on destruction or on clear();
+ *
+ * Example usage:
+ *   StatNamePool pool(symbol_table);
+ *   StatName name1 = pool.add("name1");
+ *   StatName name2 = pool.add("name2");
+ */
+class StatNamePool {
+public:
+  explicit StatNamePool(SymbolTable& symbol_table) : symbol_table_(symbol_table) {}
+  ~StatNamePool() { clear(); }
+
+  /**
+   * Removes all StatNames from the pool.
+   */
+  void clear();
+
+  /**
+   * @param name the name to add the container.
+   * @return the StatName held in the container for this name.
+   */
+  StatName add(absl::string_view name);
+
+private:
+  // We keep the stat names in a vector of StatNameStorage, storing the
+  // SymbolTable reference separately. This saves 8 bytes per StatName,
+  // at the cost of having a destructor that calls clear().
+  SymbolTable& symbol_table_;
+  std::vector<StatNameStorage> storage_vector_;
+};
+
 // Represents an ordered container of StatNames. The encoding for each StatName
 // is byte-packed together, so this carries less overhead than allocating the
 // storage separately. The tradeoff is there is no random access; you can only

test/common/stats/heap_stat_data_test.cc

@@ -13,29 +13,23 @@ namespace {
 
 class HeapStatDataTest : public testing::Test {
 protected:
-  HeapStatDataTest() : alloc_(symbol_table_) {}
+  HeapStatDataTest() : alloc_(symbol_table_), pool_(symbol_table_) {}
   ~HeapStatDataTest() { clearStorage(); }
 
   StatNameStorage makeStatStorage(absl::string_view name) {
     return StatNameStorage(name, symbol_table_);
   }
 
-  StatName makeStat(absl::string_view name) {
-    stat_name_storage_.emplace_back(makeStatStorage(name));
-    return stat_name_storage_.back().statName();
-  }
+  StatName makeStat(absl::string_view name) { return pool_.add(name); }
 
   void clearStorage() {
-    for (auto& stat_name_storage : stat_name_storage_) {
-      stat_name_storage.free(symbol_table_);
-    }
-    stat_name_storage_.clear();
+    pool_.clear();
     EXPECT_EQ(0, symbol_table_.numSymbols());
   }
 
   FakeSymbolTableImpl symbol_table_;
   HeapStatDataAllocator alloc_;
-  std::vector<StatNameStorage> stat_name_storage_;
+  StatNamePool pool_;
 };
 
 // No truncation occurs in the implementation of HeapStatData.

test/common/stats/isolated_store_impl_test.cc

@@ -13,27 +13,16 @@ namespace Stats {
 
 class StatsIsolatedStoreImplTest : public testing::Test {
 protected:
-  ~StatsIsolatedStoreImplTest() override { clearStorage(); }
-
-  void clearStorage() {
-    for (auto& stat_name_storage : stat_name_storage_) {
-      stat_name_storage.free(store_.symbolTable());
-    }
-    stat_name_storage_.clear();
+  StatsIsolatedStoreImplTest() : pool_(store_.symbolTable()) {}
+  ~StatsIsolatedStoreImplTest() override {
+    pool_.clear();
     EXPECT_EQ(0, store_.symbolTable().numSymbols());
   }
 
-  StatName makeStatName(absl::string_view name) {
-    stat_name_storage_.emplace_back(makeStatStorage(name));
-    return stat_name_storage_.back().statName();
-  }
-
-  StatNameStorage makeStatStorage(absl::string_view name) {
-    return StatNameStorage(name, store_.symbolTable());
-  }
+  StatName makeStatName(absl::string_view name) { return pool_.add(name); }
 
   IsolatedStoreImpl store_;
-  std::vector<StatNameStorage> stat_name_storage_;
+  StatNamePool pool_;
 };
 
 TEST_F(StatsIsolatedStoreImplTest, All) {

test/common/stats/symbol_table_impl_test.cc

@@ -45,14 +45,13 @@ class StatNameTest : public testing::TestWithParam<SymbolTableType> {
       table_ = std::move(table);
       break;
     }
+    pool_ = std::make_unique<StatNamePool>(*table_);
   }
+
   ~StatNameTest() override { clearStorage(); }
 
   void clearStorage() {
-    for (auto& stat_name_storage : stat_name_storage_) {
-      stat_name_storage.free(*table_);
-    }
-    stat_name_storage_.clear();
+    pool_->clear();
     EXPECT_EQ(0, table_->numSymbols());
   }
 
@@ -67,18 +66,12 @@ class StatNameTest : public testing::TestWithParam<SymbolTableType> {
     return table_->toString(makeStat(stat_name));
   }
 
-  StatNameStorage makeStatStorage(absl::string_view name) { return StatNameStorage(name, *table_); }
-
-  StatName makeStat(absl::string_view name) {
-    stat_name_storage_.emplace_back(makeStatStorage(name));
-    return stat_name_storage_.back().statName();
-  }
+  StatName makeStat(absl::string_view name) { return pool_->add(name); }
 
   FakeSymbolTableImpl* fake_symbol_table_{nullptr};
   SymbolTableImpl* real_symbol_table_{nullptr};
   std::unique_ptr<SymbolTable> table_;
-
-  std::vector<StatNameStorage> stat_name_storage_;
+  std::unique_ptr<StatNamePool> pool_;
 };
 
 INSTANTIATE_TEST_CASE_P(StatNameTest, StatNameTest,
@@ -93,6 +86,12 @@ TEST_P(StatNameTest, TestArbitrarySymbolRoundtrip) {
   }
 }
 
+TEST_P(StatNameTest, TestEmpty) {
+  EXPECT_TRUE(makeStat("").empty());
+  EXPECT_FALSE(makeStat("x").empty());
+  EXPECT_TRUE(StatName().empty());
+}
+
 TEST_P(StatNameTest, Test100KSymbolsRoundtrip) {
   for (int i = 0; i < 100 * 1000; ++i) {
     const std::string stat_name = absl::StrCat("symbol_", i);
@@ -170,7 +169,7 @@ TEST_P(StatNameTest, TestSameValueOnPartialFree) {
   // This should hold true for components as well. Since "foo" persists even when "foo.bar" is
   // freed, we expect both instances of "foo" to have the same symbol.
   makeStat("foo");
-  StatNameStorage stat_foobar_1(makeStatStorage("foo.bar"));
+  StatNameStorage stat_foobar_1("foo.bar", *table_);
   SymbolVec stat_foobar_1_symbols = getSymbols(stat_foobar_1.statName());
   stat_foobar_1.free(*table_);
   StatName stat_foobar_2(makeStat("foo.bar"));
@@ -226,22 +225,26 @@ TEST_P(StatNameTest, TestShrinkingExpectation) {
   // ::size() is a public function, but should only be used for testing.
   size_t table_size_0 = table_->numSymbols();
 
-  StatNameStorage stat_a(makeStatStorage("a"));
+  auto make_stat_storage = [this](absl::string_view name) -> StatNameStorage {
+    return StatNameStorage(name, *table_);
+  };
+
+  StatNameStorage stat_a(make_stat_storage("a"));
   size_t table_size_1 = table_->numSymbols();
 
-  StatNameStorage stat_aa(makeStatStorage("a.a"));
+  StatNameStorage stat_aa(make_stat_storage("a.a"));
   EXPECT_EQ(table_size_1, table_->numSymbols());
 
-  StatNameStorage stat_ab(makeStatStorage("a.b"));
+  StatNameStorage stat_ab(make_stat_storage("a.b"));
   size_t table_size_2 = table_->numSymbols();
 
-  StatNameStorage stat_ac(makeStatStorage("a.c"));
+  StatNameStorage stat_ac(make_stat_storage("a.c"));
   size_t table_size_3 = table_->numSymbols();
 
-  StatNameStorage stat_acd(makeStatStorage("a.c.d"));
+  StatNameStorage stat_acd(make_stat_storage("a.c.d"));
   size_t table_size_4 = table_->numSymbols();
 
-  StatNameStorage stat_ace(makeStatStorage("a.c.e"));
+  StatNameStorage stat_ace(make_stat_storage("a.c.e"));
   size_t table_size_5 = table_->numSymbols();
   EXPECT_GE(table_size_5, table_size_4);
 

test/common/stats/thread_local_store_test.cc

@@ -379,33 +379,15 @@ TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) {
 
 class LookupWithStatNameTest : public testing::Test {
 public:
-  LookupWithStatNameTest() : alloc_(symbol_table_), store_(alloc_) {}
-  ~LookupWithStatNameTest() override {
-    store_.shutdownThreading();
-    clearStorage();
-  }
-
-  void clearStorage() {
-    for (auto& stat_name_storage : stat_name_storage_) {
-      stat_name_storage.free(store_.symbolTable());
-    }
-    stat_name_storage_.clear();
-    EXPECT_EQ(0, store_.symbolTable().numSymbols());
-  }
-
-  StatName makeStatName(absl::string_view name) {
-    stat_name_storage_.emplace_back(makeStatStorage(name));
-    return stat_name_storage_.back().statName();
-  }
+  LookupWithStatNameTest() : alloc_(symbol_table_), store_(alloc_), pool_(symbol_table_) {}
+  ~LookupWithStatNameTest() override { store_.shutdownThreading(); }
 
-  StatNameStorage makeStatStorage(absl::string_view name) {
-    return StatNameStorage(name, store_.symbolTable());
-  }
+  StatName makeStatName(absl::string_view name) { return pool_.add(name); }
 
   Stats::FakeSymbolTableImpl symbol_table_;
   HeapStatDataAllocator alloc_;
   ThreadLocalStoreImpl store_;
-  std::vector<StatNameStorage> stat_name_storage_;
+  StatNamePool pool_;
 };
 
 TEST_F(LookupWithStatNameTest, All) {