Fix tests and resync with envoyproxy#6161 and envoyproxy#6504

Signed-off-by: Joshua Marantz <jmarantz@google.com>

source/common/stats/scope_prefixer.cc

@@ -9,22 +9,39 @@ namespace Envoy {
 namespace Stats {
 
 ScopePrefixer::ScopePrefixer(absl::string_view prefix, Scope& scope)
-    : prefix_(Utility::sanitizeStatsName(prefix)), scope_(scope) {}
+    : scope_(scope), prefix_(Utility::sanitizeStatsName(prefix), symbolTable()) {}
+
+ScopePrefixer::ScopePrefixer(StatName prefix, Scope& scope)
+    : scope_(scope), prefix_(prefix, symbolTable()) {}
+
+ScopePrefixer::~ScopePrefixer() { prefix_.free(symbolTable()); }
+
+ScopePtr ScopePrefixer::createScopeFromStatName(StatName name) {
+  SymbolTable::StoragePtr joined = symbolTable().join({prefix_.statName(), name});
+  return std::make_unique<ScopePrefixer>(StatName(joined.get()), scope_);
+}
 
 ScopePtr ScopePrefixer::createScope(const std::string& name) {
-  return std::make_unique<ScopePrefixer>(prefix_ + name, scope_);
+  StatNameTempStorage stat_name_storage(Utility::sanitizeStatsName(name), symbolTable());
+  return createScopeFromStatName(stat_name_storage.statName());
 }
 
 Counter& ScopePrefixer::counterFromStatName(StatName name) {
-  return counter(symbolTable().toString(name));
+  Stats::SymbolTable::StoragePtr stat_name_storage =
+      scope_.symbolTable().join({prefix_.statName(), name});
+  return scope_.counterFromStatName(StatName(stat_name_storage.get()));
 }
 
 Gauge& ScopePrefixer::gaugeFromStatName(StatName name) {
-  return gauge(symbolTable().toString(name));
+  Stats::SymbolTable::StoragePtr stat_name_storage =
+      scope_.symbolTable().join({prefix_.statName(), name});
+  return scope_.gaugeFromStatName(StatName(stat_name_storage.get()));
 }
 
 Histogram& ScopePrefixer::histogramFromStatName(StatName name) {
-  return histogram(symbolTable().toString(name));
+  Stats::SymbolTable::StoragePtr stat_name_storage =
+      scope_.symbolTable().join({prefix_.statName(), name});
+  return scope_.histogramFromStatName(StatName(stat_name_storage.get()));
 }
 
 void ScopePrefixer::deliverHistogramToSinks(const Histogram& histograms, uint64_t val) {

source/common/stats/scope_prefixer.h

@@ -10,18 +10,30 @@ namespace Stats {
 class ScopePrefixer : public Scope {
 public:
   ScopePrefixer(absl::string_view prefix, Scope& scope);
+  ScopePrefixer(StatName prefix, Scope& scope);
+  ~ScopePrefixer() override;
+
+  ScopePtr createScopeFromStatName(StatName name);
 
   // Scope
   ScopePtr createScope(const std::string& name) override;
-  Counter& counter(const std::string& name) override { return scope_.counter(prefix_ + name); }
-  Gauge& gauge(const std::string& name) override { return scope_.gauge(prefix_ + name); }
-  Histogram& histogram(const std::string& name) override {
-    return scope_.histogram(prefix_ + name);
-  }
-  void deliverHistogramToSinks(const Histogram& histograms, uint64_t val) override;
   Counter& counterFromStatName(StatName name) override;
   Gauge& gaugeFromStatName(StatName name) override;
   Histogram& histogramFromStatName(StatName name) override;
+  void deliverHistogramToSinks(const Histogram& histograms, uint64_t val) override;
+
+  Counter& counter(const std::string& name) override {
+    StatNameTempStorage storage(name, symbolTable());
+    return counterFromStatName(storage.statName());
+  }
+  Gauge& gauge(const std::string& name) override {
+    StatNameTempStorage storage(name, symbolTable());
+    return gaugeFromStatName(storage.statName());
+  }
+  Histogram& histogram(const std::string& name) override {
+    StatNameTempStorage storage(name, symbolTable());
+    return histogramFromStatName(storage.statName());
+  }
 
   const Stats::StatsOptions& statsOptions() const override { return scope_.statsOptions(); }
   const SymbolTable& symbolTable() const override { return scope_.symbolTable(); }
@@ -30,8 +42,8 @@ class ScopePrefixer : public Scope {
   NullGaugeImpl& nullGauge(const std::string& str) override { return scope_.nullGauge(str); }
 
 private:
-  std::string prefix_;
   Scope& scope_;
+  StatNameStorage prefix_;
 };
 
 } // namespace Stats

source/common/stats/symbol_table_impl.cc

@@ -317,35 +317,32 @@ void StatNameStorage::free(SymbolTable& table) {
   bytes_.reset();
 }
 
-SharedStatNameStorageSet::~SharedStatNameStorageSet() {
-  // free() must be called before destructing SharedStatNameStorageSet to
-  // decrement references to all symbols.
+StatNameStorageSet::~StatNameStorageSet() {
+  // free() must be called before destructing StatNameStorageSet to decrement
+  // references to all symbols.
   ASSERT(empty());
 }
 
-void SharedStatNameStorageSet::free(SymbolTable& symbol_table) {
+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. So we must iterate over the set and free it. But we
-  // don't want to mutate objects while they are in a set, so we just copy them,
-  // which is easy because they are shared_ptr<StatNameStorage>.
-
-  size_t sz = size();
-  STACK_ARRAY(storage, SharedStatNameStorage, sz);
-  size_t i = 0;
-  for (const SharedStatNameStorage& name : *this) {
-    storage[i++] = name;
-  }
-  clear();
-
-  // Now that the associative container is clear, we can free all the referenced
-  // symbols.
-  for (i = 0; i < sz; ++i) {
-    SharedStatNameStorage& shared_stat_storage = storage[i];
-    RELEASE_ASSERT(shared_stat_storage.use_count() == 1, "Freeing symbol that's in use");
-    shared_stat_storage->free(symbol_table);
+  // 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.
+  //
+  // One risk here is 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 if 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 (!empty()) {
+    auto storage = extract(begin());
+    storage.value().free(symbol_table);
   }
 }
 

source/common/stats/symbol_table_impl.h

@@ -478,8 +478,6 @@ struct StatNameLessThan {
   const SymbolTable& symbol_table_;
 };
 
-using SharedStatNameStorage = std::shared_ptr<StatNameStorage>;
-
 struct HeterogeneousStatNameHash {
   // Specifying is_transparent indicates to the library infrastructure that
   // type-conversions should not be applied when calling find(), but instead
@@ -491,33 +489,32 @@ struct HeterogeneousStatNameHash {
   using is_transparent = void;
 
   size_t operator()(StatName a) const { return a.hash(); }
-  size_t operator()(const SharedStatNameStorage& a) const { return a->statName().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 SharedStatNameStorage& a, const SharedStatNameStorage& b) const {
-    return a->statName() == b->statName();
+  size_t operator()(const StatNameStorage& a, const StatNameStorage& b) const {
+    return a.statName() == b.statName();
   }
-  size_t operator()(StatName a, const SharedStatNameStorage& b) const { return a == b->statName(); }
-  size_t operator()(const SharedStatNameStorage& a, StatName b) const { return a->statName() == b; }
+  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 of shared_ptr<StatNameStorage>. We use a subclass 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-site 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 SharedStatNameStorageSet
-    : public absl::flat_hash_set<SharedStatNameStorage, HeterogeneousStatNameHash,
-                                 HeterogeneousStatNameEqual> {
+// Encapsulates a set<StatNameStorage>. We use a subclass 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 absl::flat_hash_set<StatNameStorage, HeterogeneousStatNameHash,
+                                                      HeterogeneousStatNameEqual> {
 public:
-  ~SharedStatNameStorageSet();
+  ~StatNameStorageSet();
   void free(SymbolTable& symbol_table);
 };
 

source/common/stats/thread_local_store.cc

@@ -199,11 +199,11 @@ void ThreadLocalStoreImpl::releaseScopeCrossThread(ScopeImpl* scope) {
 
   // This is called directly from the ScopeImpl destructor, but we can't delay
   // the destruction of scope->central_cache_.central_cache_.rejected_stats_
-  // to wait for all the TLS rejected_stats_ caches are destructed, as those
+  // to wait for all the TLS rejected_stats_ caches to be destructed, as those
   // reference elements of SharedStatNameStorageSet. So simply swap out the set
   // contents into a local that we can hold onto until the TLS cache is cleared
   // of all references.
-  auto rejected_stats = new SharedStatNameStorageSet;
+  auto rejected_stats = new StatNameStorageSet;
   rejected_stats->swap(scope->central_cache_.rejected_stats_);
   const uint64_t scope_id = scope->scope_id_;
   auto clean_central_cache = [this, rejected_stats]() {
@@ -281,21 +281,22 @@ class TagExtraction {
   std::string tag_extracted_name_;
 };
 
-bool ThreadLocalStoreImpl::checkAndRememberRejection(
-    StatName name, SharedStatNameStorageSet& central_rejected_stats,
-    StatNameHashSet* tls_rejected_stats) {
+bool ThreadLocalStoreImpl::checkAndRememberRejection(StatName name,
+                                                     StatNameStorageSet& central_rejected_stats,
+                                                     StatNameHashSet* tls_rejected_stats) {
   if (stats_matcher_->acceptsAll()) {
     return false;
   }
 
   auto iter = central_rejected_stats.find(name);
-  SharedStatNameStorage rejected_name;
+  const StatNameStorage* rejected_name = nullptr;
   if (iter != central_rejected_stats.end()) {
-    rejected_name = *iter;
+    rejected_name = &(*iter);
   } else {
     if (rejects(name)) {
-      rejected_name = std::make_shared<StatNameStorage>(name, symbolTable());
-      central_rejected_stats.insert(rejected_name);
+      auto insertion = central_rejected_stats.insert(StatNameStorage(name, symbolTable()));
+      const StatNameStorage& rejected_name_ref = *(insertion.first);
+      rejected_name = &rejected_name_ref;
     }
   }
   if (rejected_name != nullptr) {
@@ -310,7 +311,7 @@ bool ThreadLocalStoreImpl::checkAndRememberRejection(
 template <class StatType>
 StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat(
     StatName name, StatMap<std::shared_ptr<StatType>>& central_cache_map,
-    SharedStatNameStorageSet& central_rejected_stats, MakeStatFn<StatType> make_stat,
+    StatNameStorageSet& central_rejected_stats, MakeStatFn<StatType> make_stat,
     StatMap<std::shared_ptr<StatType>>* tls_cache, StatNameHashSet* tls_rejected_stats,
     StatType& null_stat) {
 

source/common/stats/thread_local_store.h

@@ -208,7 +208,7 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
     StatMap<CounterSharedPtr> counters_;
     StatMap<GaugeSharedPtr> gauges_;
     StatMap<ParentHistogramImplSharedPtr> histograms_;
-    SharedStatNameStorageSet rejected_stats_;
+    StatNameStorageSet rejected_stats_;
   };
 
   struct ScopeImpl : public TlsScope {
@@ -261,7 +261,7 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
      */
     template <class StatType>
     StatType& safeMakeStat(StatName name, StatMap<std::shared_ptr<StatType>>& central_cache_map,
-                           SharedStatNameStorageSet& central_rejected_stats,
+                           StatNameStorageSet& central_rejected_stats,
                            MakeStatFn<StatType> make_stat,
                            StatMap<std::shared_ptr<StatType>>* tls_cache,
                            StatNameHashSet* tls_rejected_stats, StatType& null_stat);
@@ -270,16 +270,6 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
                                 std::unique_ptr<StatNameTempStorage>& truncated_name_storage,
                                 std::vector<Tag>& tags, std::string& tag_extracted_name);
 
-    Counter& counterFromStatName(StatName name) override {
-      return counter(symbolTable().toString(name));
-    }
-
-    Gauge& gaugeFromStatName(StatName name) override { return gauge(symbolTable().toString(name)); }
-
-    Histogram& histogramFromStatName(StatName name) override {
-      return histogram(symbolTable().toString(name));
-    }
-
     static std::atomic<uint64_t> next_scope_id_;
 
     const uint64_t scope_id_;
@@ -307,7 +297,7 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
   bool rejectsAll() const { return stats_matcher_->rejectsAll(); }
   template <class StatMapClass, class StatListClass>
   void removeRejectedStats(StatMapClass& map, StatListClass& list);
-  bool checkAndRememberRejection(StatName name, SharedStatNameStorageSet& central_rejected_stats,
+  bool checkAndRememberRejection(StatName name, StatNameStorageSet& central_rejected_stats,
                                  StatNameHashSet* tls_rejected_stats);
 
   const Stats::StatsOptions& stats_options_;

test/common/stats/symbol_table_impl_test.cc

@@ -508,13 +508,15 @@ TEST_P(StatNameTest, MutexContentionOnExistingSymbols) {
 }
 
 TEST_P(StatNameTest, SharedStatNameStorageSet) {
-  SharedStatNameStorageSet set;
+  StatNameStorageSet set;
   {
-    auto foo = std::make_shared<StatNameStorage>("foo", *table_);
-    set.insert(foo);
-    StatNameTempStorage temp_foo("foo", *table_);
-    auto pos = set.find(temp_foo.statName());
-    EXPECT_EQ(pos->get(), foo.get());
+    for (int i = 0; i < 10; ++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());
+    }
   }
   set.free(*table_);
 }

test/common/stats/thread_local_store_test.cc

@@ -312,7 +312,7 @@ TEST_F(StatsThreadLocalStoreTest, ScopeDelete) {
   EXPECT_EQ(TestUtility::findByName(store_->source().cachedCounters(), "scope1.c1"), c1);
 
   EXPECT_CALL(main_thread_dispatcher_, post(_));
-  EXPECT_CALL(tls_, runOnAllThreads(_));
+  EXPECT_CALL(tls_, runOnAllThreads(_, _));
   scope1.reset();
   EXPECT_EQ(1UL, store_->counters().size());
   EXPECT_EQ(2UL, store_->source().cachedCounters().size());

test/integration/stats_integration_test.cc

@@ -195,8 +195,8 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithStats) {
 
   EXPECT_LT(start_mem, m1);
   EXPECT_LT(start_mem, m1001);
-  // As of 2019/03/20, m_per_cluster = 59015 (libstdc++)
-  EXPECT_LT(m_per_cluster, 59100);
+  // As of 2019/03/20, m_per_cluster = 40017 (libstdc++)
+  EXPECT_LT(m_per_cluster, 40500);
 }
 
 } // namespace

test/mocks/thread_local/mocks.cc

@@ -11,7 +11,9 @@ namespace ThreadLocal {
 
 MockInstance::MockInstance() {
   ON_CALL(*this, allocateSlot()).WillByDefault(Invoke(this, &MockInstance::allocateSlot_));
-  ON_CALL(*this, runOnAllThreads(_)).WillByDefault(Invoke(this, &MockInstance::runOnAllThreads_));
+  ON_CALL(*this, runOnAllThreads(_)).WillByDefault(Invoke(this, &MockInstance::runOnAllThreads1_));
+  ON_CALL(*this, runOnAllThreads(_, _))
+      .WillByDefault(Invoke(this, &MockInstance::runOnAllThreads2_));
   ON_CALL(*this, shutdownThread()).WillByDefault(Invoke(this, &MockInstance::shutdownThread_));
 }
 

test/mocks/thread_local/mocks.h

@@ -18,6 +18,7 @@ class MockInstance : public Instance {
   ~MockInstance();
 
   MOCK_METHOD1(runOnAllThreads, void(Event::PostCb cb));
+  MOCK_METHOD2(runOnAllThreads, void(Event::PostCb cb, Event::PostCb main_callback));
 
   // Server::ThreadLocal
   MOCK_METHOD0(allocateSlot, SlotPtr());
@@ -27,8 +28,8 @@ class MockInstance : public Instance {
   MOCK_METHOD0(dispatcher, Event::Dispatcher&());
 
   SlotPtr allocateSlot_() { return SlotPtr{new SlotImpl(*this, current_slot_++)}; }
-  void runOnAllThreads_(Event::PostCb cb) { cb(); }
-  void runOnAllThreads(Event::PostCb cb, Event::PostCb main_callback) {
+  void runOnAllThreads1_(Event::PostCb cb) { cb(); }
+  void runOnAllThreads2_(Event::PostCb cb, Event::PostCb main_callback) {
     cb();
     main_callback();
   }