stats: add/test heterogenous set of StatNameStorage objects.

source/common/stats/symbol_table_impl.cc

@@ -317,6 +317,36 @@ void StatNameStorage::free(SymbolTable& table) {
   bytes_.reset();
 }
 
+StatNameStorageSet::~StatNameStorageSet() {
+  // free() must be called before destructing StatNameStorageSet to decrement
+  // references to all symbols.
+  ASSERT(hash_set_.empty());
+}
+
+void StatNameStorageSet::free(SymbolTable& symbol_table) {
+  // We must free() all symbols referenced in the set, otherwise the symbols
+  // will leak when the flat_hash_map superclass is destructed. They cannot
+  // self-destruct without an explicit free() as each individual StatNameStorage
+  // object does not have a reference to the symbol table, which would waste 8
+  // bytes per stat-name. The easiest way to safely free all the contents of the
+  // symbol table set is to use flat_hash_map::extract(), which removes and
+  // returns an element from the set without destructing the element
+  // immediately. This gives us a chance to call free() on each one before they
+  // are destroyed.
+  //
+  // There's a performance risk here, if removing elements via
+  // flat_hash_set::begin() is inefficient to use in a loop like this. One can
+  // imagine a hash-table implementation where the performance of this
+  // usage-model would be poor. However, tests with 100k elements appeared to
+  // run quickly when compiled for optimization, so at present this is not a
+  // performance issue.
+
+  while (!hash_set_.empty()) {
+    auto storage = hash_set_.extract(hash_set_.begin());
+    storage.value().free(symbol_table);
+  }
+}
+
 SymbolTable::StoragePtr SymbolTableImpl::join(const std::vector<StatName>& stat_names) const {
   uint64_t num_bytes = 0;
   for (StatName stat_name : stat_names) {

source/common/stats/symbol_table_impl.h

@@ -132,7 +132,7 @@ class SymbolTableImpl : public SymbolTable {
   bool lessThan(const StatName& a, const StatName& b) const override;
   void free(const StatName& stat_name) override;
   void incRefCount(const StatName& stat_name) override;
-  SymbolTable::StoragePtr join(const std::vector<StatName>& stat_names) const override;
+  StoragePtr join(const std::vector<StatName>& stat_names) const override;
   void populateList(const absl::string_view* names, uint32_t num_names,
                     StatNameList& list) override;
   StoragePtr encode(absl::string_view name) override;
@@ -476,5 +476,85 @@ struct StatNameLessThan {
   const SymbolTable& symbol_table_;
 };
 
+struct HeterogeneousStatNameHash {
+  // Specifying is_transparent indicates to the library infrastructure that
+  // type-conversions should not be applied when calling find(), but instead
+  // pass the actual types of the contained and searched-for objects directly to
+  // these functors. See
+  // https://en.cppreference.com/w/cpp/utility/functional/less_void for an
+  // official reference, and https://abseil.io/tips/144 for a description of
+  // using it in the context of absl.
+  using is_transparent = void;
+
+  size_t operator()(StatName a) const { return a.hash(); }
+  size_t operator()(const StatNameStorage& a) const { return a.statName().hash(); }
+};
+
+struct HeterogeneousStatNameEqual {
+  // See description for HeterogeneousStatNameHash::is_transparent.
+  using is_transparent = void;
+
+  size_t operator()(StatName a, StatName b) const { return a == b; }
+  size_t operator()(const StatNameStorage& a, const StatNameStorage& b) const {
+    return a.statName() == b.statName();
+  }
+  size_t operator()(StatName a, const StatNameStorage& b) const { return a == b.statName(); }
+  size_t operator()(const StatNameStorage& a, StatName b) const { return a.statName() == b; }
+};
+
+// Encapsulates a set<StatNameStorage>. We use containment here rather than a
+// 'using' alias because we need to ensure that when the set is destructed,
+// StatNameStorage::free(symbol_table) is called on each entry. It is a little
+// easier at the call-sites in thread_local_store.cc to implement this an
+// explicit free() method, analogous to StatNameStorage::free(), compared to
+// storing a SymbolTable reference in the class and doing the free in the
+// destructor, like StatNameTempStorage.
+class StatNameStorageSet {
+public:
+  using HashSet =
+      absl::flat_hash_set<StatNameStorage, HeterogeneousStatNameHash, HeterogeneousStatNameEqual>;
+  using iterator = HashSet::iterator;
+
+  ~StatNameStorageSet();
+
+  /**
+   * Releases all symbols held in this set. Must be called prior to destruction.
+   *
+   * @param symbol_table The symbol table that owns the symbols.
+   */
+  void free(SymbolTable& symbol_table);
+
+  /**
+   * @param storage The StatNameStorage to add to the set.
+   */
+  std::pair<HashSet::iterator, bool> insert(StatNameStorage&& storage) {
+    return hash_set_.insert(std::move(storage));
+  }
+
+  /**
+   * @param stat_name The stat_name to find.
+   * @return the iterator pointing to the stat_name, or end() if not found.
+   */
+  iterator find(StatName stat_name) { return hash_set_.find(stat_name); }
+
+  /**
+   * @return the end-marker.
+   */
+  iterator end() { return hash_set_.end(); }
+
+  /**
+   * @param set the storage set to swap with.
+   */
+  void swap(StatNameStorageSet& set) { hash_set_.swap(set.hash_set_); }
+
+  /**
+   * @return the number of elements in the set.
+   */
+  size_t size() const { return hash_set_.size(); }
+
+private:
+  HashSet hash_set_;
+};
+
 } // namespace Stats
 } // namespace Envoy

test/common/stats/symbol_table_impl_test.cc

@@ -507,6 +507,44 @@ TEST_P(StatNameTest, MutexContentionOnExistingSymbols) {
   }
 }
 
+TEST_P(StatNameTest, SharedStatNameStorageSetInsertAndFind) {
+  StatNameStorageSet set;
+  const int iters = 10;
+  for (int i = 0; i < iters; ++i) {
+    std::string foo = absl::StrCat("foo", i);
+    auto insertion = set.insert(StatNameStorage(foo, *table_));
+    StatNameTempStorage temp_foo(foo, *table_);
+    auto found = set.find(temp_foo.statName());
+    EXPECT_EQ(found->statName().data(), insertion.first->statName().data());
+  }
+  StatNameTempStorage bar("bar", *table_);
+  EXPECT_EQ(set.end(), set.find(bar.statName()));
+  EXPECT_EQ(iters, set.size());
+  set.free(*table_);
+}
+
+TEST_P(StatNameTest, SharedStatNameStorageSetSwap) {
+  StatNameStorageSet set1, set2;
+  set1.insert(StatNameStorage("foo", *table_));
+  EXPECT_EQ(1, set1.size());
+  EXPECT_EQ(0, set2.size());
+  set1.swap(set2);
+  EXPECT_EQ(0, set1.size());
+  EXPECT_EQ(1, set2.size());
+  set2.free(*table_);
+}
+
+TEST_P(StatNameTest, SharedStatNameStorageSetIfDestroyedWithoutFree) {
+#ifndef NDEBUG
+  EXPECT_DEATH(
+      {
+        StatNameStorageSet set;
+        set.insert(StatNameStorage("foo", *table_));
+      },
+      "");
+#endif
+}
+
 // Tests the memory savings realized from using symbol tables with 1k
 // clusters. This test shows the memory drops from almost 8M to less than
 // 2M. Note that only SymbolTableImpl is tested for memory consumption,