From c27c7477f5bef6646c5fc57f3e75d486da884812 Mon Sep 17 00:00:00 2001
From: Joshua Marantz <jmarantz@google.com>
Date: Sun, 19 May 2019 21:58:20 -0400
Subject: [PATCH 1/8] initial checkin

Signed-off-by: Joshua Marantz <jmarantz@google.com>
---
 include/envoy/stats/symbol_table.h           | 17 +++++++
 source/common/common/hash.h                  | 10 ++--
 source/common/stats/fake_symbol_table_impl.h |  5 ++
 source/common/stats/symbol_table_impl.cc     | 42 ++++++++++++++--
 source/common/stats/symbol_table_impl.h      | 50 +++++++++++++++++++-
 source/common/stats/thread_local_store.h     | 12 +++--
 test/common/stats/symbol_table_impl_test.cc  | 10 ++++
 7 files changed, 133 insertions(+), 13 deletions(-)

diff --git a/include/envoy/stats/symbol_table.h b/include/envoy/stats/symbol_table.h
index 4b4c8a4c4fe9e..0cf1a9cc94eba 100644
--- a/include/envoy/stats/symbol_table.h
+++ b/include/envoy/stats/symbol_table.h
@@ -22,6 +22,8 @@ class StatName;
 
 class StatNameList;
 
+using StringViewVector = std::vector<absl::string_view>;
+
 /**
  * SymbolTable manages a namespace optimized for stat names, exploiting their
  * typical composition from "."-separated tokens, with a significant overlap
@@ -146,6 +148,7 @@ class SymbolTable {
   friend struct HeapStatData;
   friend class StatNameStorage;
   friend class StatNameList;
+  friend class StringStatNameMap;
 
   // The following methods are private, but are called by friend classes
   // StatNameStorage and StatNameList, which must be friendly with SymbolTable
@@ -182,6 +185,20 @@ class SymbolTable {
    *
    */
   virtual StoragePtr encode(absl::string_view name) PURE;
+
+  /**
+   * Returns a vector of absl:string_view containing the symbols in the
+   * StatName. The string_view elements reference memory that is held in the
+   * SymbolTable as long as a reference to the symbols is retained. This takes
+   * a lock on the symbol table.
+   */
+  virtual StringViewVector statNameToStringVector(const StatName& stat_name) const PURE;
+
+  /**
+   * Splits a string_view into multple symbols. This does not require a lock on
+   * the symbol table.
+   */
+  virtual StringViewVector splitString(absl::string_view) const PURE;
 };
 
 using SharedSymbolTable = std::shared_ptr<SymbolTable>;
diff --git a/source/common/common/hash.h b/source/common/common/hash.h
index b6c50e63ac74f..87a59b9a39a1b 100644
--- a/source/common/common/hash.h
+++ b/source/common/common/hash.h
@@ -82,7 +82,7 @@ struct ConstCharStarHash {
 };
 
 struct ConstCharStarEqual {
-  size_t operator()(const char* a, const char* b) const { return strcmp(a, b) == 0; }
+  bool operator()(const char* a, const char* b) const { return strcmp(a, b) == 0; }
 };
 
 template <class Value>
@@ -111,10 +111,10 @@ struct HeterogeneousStringEqual {
   // See description for HeterogeneousStringHash::is_transparent.
   using is_transparent = void;
 
-  size_t operator()(absl::string_view a, absl::string_view b) const { return a == b; }
-  size_t operator()(const SharedString& a, const SharedString& b) const { return *a == *b; }
-  size_t operator()(absl::string_view a, const SharedString& b) const { return a == *b; }
-  size_t operator()(const SharedString& a, absl::string_view b) const { return *a == b; }
+  bool operator()(absl::string_view a, absl::string_view b) const { return a == b; }
+  bool operator()(const SharedString& a, const SharedString& b) const { return *a == *b; }
+  bool operator()(absl::string_view a, const SharedString& b) const { return a == *b; }
+  bool operator()(const SharedString& a, absl::string_view b) const { return *a == b; }
 };
 
 // We use heterogeneous hash/equal functors to do a find() without constructing
diff --git a/source/common/stats/fake_symbol_table_impl.h b/source/common/stats/fake_symbol_table_impl.h
index 096cb42d4d72b..80198a817794e 100644
--- a/source/common/stats/fake_symbol_table_impl.h
+++ b/source/common/stats/fake_symbol_table_impl.h
@@ -125,6 +125,11 @@ class FakeSymbolTableImpl : public SymbolTable {
     fn(toStringView(stat_name));
   }
 
+  StringViewVector statNameToStringVector(const StatName& stat_name) const override {
+    return {toStringView(stat_name)};
+  }
+  StringViewVector splitString(absl::string_view s) const override { return {s}; }
+
 private:
   absl::string_view toStringView(const StatName& stat_name) const {
     return {reinterpret_cast<const char*>(stat_name.data()),
diff --git a/source/common/stats/symbol_table_impl.cc b/source/common/stats/symbol_table_impl.cc
index 9b85687a66bcf..1d65687a00a1e 100644
--- a/source/common/stats/symbol_table_impl.cc
+++ b/source/common/stats/symbol_table_impl.cc
@@ -118,7 +118,7 @@ void SymbolTableImpl::addTokensToEncoding(const absl::string_view name, Encoding
 
   // We want to hold the lock for the minimum amount of time, so we do the
   // string-splitting and prepare a temp vector of Symbol first.
-  std::vector<absl::string_view> tokens = absl::StrSplit(name, '.');
+  StringViewVector tokens = absl::StrSplit(name, '.');
   std::vector<Symbol> symbols;
   symbols.reserve(tokens.size());
 
@@ -153,7 +153,11 @@ void SymbolTableImpl::callWithStringView(StatName stat_name,
 }
 
 std::string SymbolTableImpl::decodeSymbolVec(const SymbolVec& symbols) const {
-  std::vector<absl::string_view> name_tokens;
+  return absl::StrJoin(decodeSymbolVecTokens(symbols), ".");
+}
+
+StringViewVector SymbolTableImpl::decodeSymbolVecTokens(const SymbolVec& symbols) const {
+  StringViewVector name_tokens;
   name_tokens.reserve(symbols.size());
   {
     // Hold the lock only while decoding symbols.
@@ -162,7 +166,7 @@ std::string SymbolTableImpl::decodeSymbolVec(const SymbolVec& symbols) const {
       name_tokens.push_back(fromSymbol(symbol));
     }
   }
-  return absl::StrJoin(name_tokens, ".");
+  return name_tokens;
 }
 
 void SymbolTableImpl::incRefCount(const StatName& stat_name) {
@@ -427,5 +431,37 @@ void StatNameList::clear(SymbolTable& symbol_table) {
   storage_.reset();
 }
 
+size_t StringViewVectorHash::operator()(const StringViewVector& a) const {
+  uint64_t hash = 0;
+  for (absl::string_view sv : a) {
+    hash = 63 * hash + HashUtil::xxHash64(sv);
+  }
+  return hash;
+}
+
+StringViewVector SymbolTableImpl::statNameToStringVector(const StatName& stat_name) const {
+  return decodeSymbolVecTokens(Encoding::decodeSymbols(stat_name.data(), stat_name.dataSize()));
+}
+
+/*struct StringViewVectorCompare {
+  size_t operator()(const StringViewVector& a, const StringViewVector& b) const;
+};
+
+
+bool StringViewVectorCompare operator()(const StringViewVector& a,
+                                        const StringViewVector& b) const {
+  return
+  if (a.size() != b.size()) {
+    return false;
+  }
+  for (size_t i = 0; i < a.size(); ++i) {
+    if (a[i] != b[i]) {
+      return false;
+    }
+  }
+  return true;
+}
+*/
+
 } // namespace Stats
 } // namespace Envoy
diff --git a/source/common/stats/symbol_table_impl.h b/source/common/stats/symbol_table_impl.h
index 366963f790c77..74ab9a50c42ff 100644
--- a/source/common/stats/symbol_table_impl.h
+++ b/source/common/stats/symbol_table_impl.h
@@ -139,6 +139,11 @@ class SymbolTableImpl : public SymbolTable {
   void callWithStringView(StatName stat_name,
                           const std::function<void(absl::string_view)>& fn) const override;
 
+  StringViewVector statNameToStringVector(const StatName& stat_name) const override;
+  StringViewVector splitString(absl::string_view s) const override {
+    return absl::StrSplit(s, '.');
+  }
+
 #ifndef ENVOY_CONFIG_COVERAGE
   void debugPrint() const override;
 #endif
@@ -183,6 +188,7 @@ class SymbolTableImpl : public SymbolTable {
    * @return std::string the retrieved stat name.
    */
   std::string decodeSymbolVec(const SymbolVec& symbols) const;
+  StringViewVector decodeSymbolVecTokens(const SymbolVec& symbols) const;
 
   /**
    * Convenience function for encode(), symbolizing one string segment at a time.
@@ -542,13 +548,14 @@ struct HeterogeneousStatNameHash {
 
   size_t operator()(StatName a) const { return a.hash(); }
   size_t operator()(const StatNameStorage& a) const { return a.statName().hash(); }
+  size_t operator()(absl::string_view str) const;
 };
 
 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()(StatName a, StatName b) const { return a == b; }
   size_t operator()(const StatNameStorage& a, const StatNameStorage& b) const {
     return a.statName() == b.statName();
   }
@@ -610,5 +617,46 @@ class StatNameStorageSet {
   HashSet hash_set_;
 };
 
+struct StringViewVectorHash {
+  size_t operator()(const StringViewVector& a) const;
+};
+
+class StringStatNameMap {
+public:
+  explicit StringStatNameMap(SymbolTable& symbol_table) : symbol_table_(symbol_table) {}
+
+  /**
+   * This does not take a lock in the symbol table.
+   */
+  absl::optional<StatName> find(absl::string_view name) const {
+    absl::optional<StatName> ret;
+
+    StringViewVector v = symbol_table_.splitString(name);
+    auto iter = string_vector_stat_name_map_.find(v);
+    if (iter != string_vector_stat_name_map_.end()) {
+      ret = iter->second;
+    }
+    return ret;
+  }
+
+  /**
+   * Inserts stat_name into the StatNameStorageMap. Caller must guarantee that
+   * the backing-store for StatName lasts longer than the StatNameStorageMap.
+   *
+   * Note: this take a lock in the symbol table, so it's useful to call find() first.
+   */
+  void insert(StatName stat_name) {
+    StringViewVector v = symbol_table_.statNameToStringVector(stat_name);
+    string_vector_stat_name_map_[v] = stat_name;
+  }
+
+private:
+  using StringVectorStatNameMap =
+      absl::flat_hash_map<StringViewVector, StatName, StringViewVectorHash>;
+
+  StringVectorStatNameMap string_vector_stat_name_map_;
+  SymbolTable& symbol_table_;
+};
+
 } // namespace Stats
 } // namespace Envoy
diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h
index 8a062b2269574..b7ff6e7532e71 100644
--- a/source/common/stats/thread_local_store.h
+++ b/source/common/stats/thread_local_store.h
@@ -227,10 +227,10 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
 
     // We keep a TLS cache of rejected stat names. This costs memory, but
     // reduces runtime overhead running the matcher. Moreover, once symbol
-    // tables are integrated, rejection will need the fully elaborated string,
-    // and it we need to take a global symbol-table lock to run. We keep this
-    // StatName set here in the TLS cache to avoid taking a lock to compute
-    // rejection.
+    // tables are integrated, checking rejection without a cache would require
+    // the fully elaborated string, requiring acquisition of the global
+    // symbol-table lock. We keep this StatName set here in the TLS cache to
+    // avoid taking a lock to compute rejection.
     StatNameHashSet rejected_stats_;
   };
 
@@ -239,6 +239,10 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
     StatMap<GaugeSharedPtr> gauges_;
     StatMap<ParentHistogramImplSharedPtr> histograms_;
     StatNameStorageSet rejected_stats_;
+
+    // Strings should in most cases be converted to symbols at construction
+    // time. However, for a
+    absl::flat_hash_map<std::string, StatName> hot_path_symbols_;
   };
 
   struct ScopeImpl : public TlsScope {
diff --git a/test/common/stats/symbol_table_impl_test.cc b/test/common/stats/symbol_table_impl_test.cc
index a3ace4270941c..c0f1442be4300 100644
--- a/test/common/stats/symbol_table_impl_test.cc
+++ b/test/common/stats/symbol_table_impl_test.cc
@@ -537,6 +537,16 @@ TEST_P(StatNameTest, SharedStatNameStorageSetSwap) {
   set2.free(*table_);
 }
 
+TEST_P(StatNameTest, StatNameStringMap) {
+  StatNameManagedStorage foo_bar("foo.bar", *table_); // Must outlive the map.
+  {
+    StringStatNameMap map(*table_);
+    EXPECT_FALSE(map.find("foo.bar"));
+    map.insert(foo_bar.statName());
+    EXPECT_EQ(map.find("foo.bar"), foo_bar.statName());
+  }
+}
+
 // 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,

From 077cae7edf8f600a0a8a55f2b880cc14ea5b726c Mon Sep 17 00:00:00 2001
From: Joshua Marantz <jmarantz@google.com>
Date: Mon, 20 May 2019 08:50:18 -0400
Subject: [PATCH 2/8] integrate with ThreadLocalStore.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
---
 include/envoy/stats/scope.h                   | 15 +++++
 source/common/stats/isolated_store_impl.cc    | 13 ++++
 source/common/stats/isolated_store_impl.h     |  5 ++
 source/common/stats/scope_prefixer.cc         | 12 +++-
 source/common/stats/scope_prefixer.h          |  3 +
 source/common/stats/symbol_table_impl.h       | 11 ++--
 source/common/stats/thread_local_store.cc     | 64 ++++++++++++++++---
 source/common/stats/thread_local_store.h      | 20 ++++--
 test/common/stats/isolated_store_impl_test.cc |  3 +
 test/common/stats/symbol_table_impl_test.cc   |  8 +--
 test/common/stats/thread_local_store_test.cc  |  6 ++
 test/mocks/stats/mocks.cc                     | 11 ++++
 test/mocks/stats/mocks.h                      |  4 ++
 13 files changed, 147 insertions(+), 28 deletions(-)

diff --git a/include/envoy/stats/scope.h b/include/envoy/stats/scope.h
index 863d5f7d45909..86c210f3d69d5 100644
--- a/include/envoy/stats/scope.h
+++ b/include/envoy/stats/scope.h
@@ -113,6 +113,21 @@ class Scope {
    */
   virtual const SymbolTable& symbolTable() const PURE;
   virtual SymbolTable& symbolTable() PURE;
+
+  /**
+   * Performs fast conversion of string to StatName in the hot-path, at the
+   * expense of significant per-name-per-thread memory overhead. This incurs
+   * a lock only the first time this string is referenced from a thread. It
+   * should only be used names that cannot be determined during startup or xDS
+   * update. Forthose names that can be determined at that time, the StatNames
+   * should be collected via StatNamePool or StatNameManagedStorage once, and
+   * then used later on in the hot-path to compose fully elaborated StatName
+   * objects via SYmbolTable::join(), which is lock-free.
+   *
+   * @param name The name of the stat or fragment of stat.
+   * @return the stat name allocated from symbolTable().
+   */
+  virtual StatName fastMemoryIntensiveStatNameLookup(absl::string_view name) PURE;
 };
 
 } // namespace Stats
diff --git a/source/common/stats/isolated_store_impl.cc b/source/common/stats/isolated_store_impl.cc
index 905ff98db06d0..60f12e177f7b6 100644
--- a/source/common/stats/isolated_store_impl.cc
+++ b/source/common/stats/isolated_store_impl.cc
@@ -36,9 +36,22 @@ IsolatedStoreImpl::IsolatedStoreImpl(SymbolTable& symbol_table)
       }),
       null_gauge_(symbol_table) {}
 
+IsolatedStoreImpl::~IsolatedStoreImpl() { stat_name_set_.free(symbolTable()); }
+
 ScopePtr IsolatedStoreImpl::createScope(const std::string& name) {
   return std::make_unique<ScopePrefixer>(name, *this);
 }
 
+StatName IsolatedStoreImpl::fastMemoryIntensiveStatNameLookup(absl::string_view name) {
+  absl::optional<StatName> stat_name = string_stat_name_map_.find(name, symbolTable());
+  if (!stat_name) {
+    StatNameStorage storage(name, symbolTable());
+    auto insertion = stat_name_set_.insert(std::move(storage));
+    ASSERT(insertion.second); // If the name is not in the map, it should not be in the set.
+    stat_name = insertion.first->statName();
+  }
+  return *stat_name;
+}
+
 } // namespace Stats
 } // namespace Envoy
diff --git a/source/common/stats/isolated_store_impl.h b/source/common/stats/isolated_store_impl.h
index d658565e63d9a..961322be06804 100644
--- a/source/common/stats/isolated_store_impl.h
+++ b/source/common/stats/isolated_store_impl.h
@@ -68,6 +68,7 @@ class IsolatedStoreImpl : public StoreImpl {
 public:
   IsolatedStoreImpl();
   explicit IsolatedStoreImpl(SymbolTable& symbol_table);
+  ~IsolatedStoreImpl() override;
 
   // Stats::Scope
   Counter& counterFromStatName(StatName name) override { return counters_.get(name); }
@@ -110,6 +111,8 @@ class IsolatedStoreImpl : public StoreImpl {
     return histogramFromStatName(storage.statName());
   }
 
+  StatName fastMemoryIntensiveStatNameLookup(absl::string_view name) override;
+
 private:
   IsolatedStoreImpl(std::unique_ptr<SymbolTable>&& symbol_table);
 
@@ -119,6 +122,8 @@ class IsolatedStoreImpl : public StoreImpl {
   IsolatedStatsCache<Gauge> gauges_;
   IsolatedStatsCache<Histogram> histograms_;
   NullGaugeImpl null_gauge_;
+  StatNameStorageSet stat_name_set_;
+  StringStatNameMap string_stat_name_map_;
 };
 
 } // namespace Stats
diff --git a/source/common/stats/scope_prefixer.cc b/source/common/stats/scope_prefixer.cc
index 44314d967de1c..97a7851483f55 100644
--- a/source/common/stats/scope_prefixer.cc
+++ b/source/common/stats/scope_prefixer.cc
@@ -5,14 +5,18 @@
 #include "common/stats/symbol_table_impl.h"
 #include "common/stats/utility.h"
 
+#include "absl/strings/str_cat.h"
+
 namespace Envoy {
 namespace Stats {
 
 ScopePrefixer::ScopePrefixer(absl::string_view prefix, Scope& scope)
-    : scope_(scope), prefix_(Utility::sanitizeStatsName(prefix), symbolTable()) {}
+    : scope_(scope), prefix_(Utility::sanitizeStatsName(prefix), symbolTable()),
+      prefix_string_(std::string(prefix)) {}
 
 ScopePrefixer::ScopePrefixer(StatName prefix, Scope& scope)
-    : scope_(scope), prefix_(prefix, symbolTable()) {}
+    : scope_(scope), prefix_(prefix, symbolTable()),
+      prefix_string_(symbolTable().toString(prefix)) {}
 
 ScopePrefixer::~ScopePrefixer() { prefix_.free(symbolTable()); }
 
@@ -62,5 +66,9 @@ void ScopePrefixer::deliverHistogramToSinks(const Histogram& histograms, uint64_
   scope_.deliverHistogramToSinks(histograms, val);
 }
 
+StatName ScopePrefixer::fastMemoryIntensiveStatNameLookup(absl::string_view name) {
+  return scope_.fastMemoryIntensiveStatNameLookup(absl::StrCat(prefix_string_, name));
+}
+
 } // namespace Stats
 } // namespace Envoy
diff --git a/source/common/stats/scope_prefixer.h b/source/common/stats/scope_prefixer.h
index 51c1828393913..6d0094993a1b4 100644
--- a/source/common/stats/scope_prefixer.h
+++ b/source/common/stats/scope_prefixer.h
@@ -45,9 +45,12 @@ class ScopePrefixer : public Scope {
 
   NullGaugeImpl& nullGauge(const std::string& str) override { return scope_.nullGauge(str); }
 
+  StatName fastMemoryIntensiveStatNameLookup(absl::string_view name) override;
+
 private:
   Scope& scope_;
   StatNameStorage prefix_;
+  std::string prefix_string_;
 };
 
 } // namespace Stats
diff --git a/source/common/stats/symbol_table_impl.h b/source/common/stats/symbol_table_impl.h
index 74ab9a50c42ff..cb1d4b6e45ff4 100644
--- a/source/common/stats/symbol_table_impl.h
+++ b/source/common/stats/symbol_table_impl.h
@@ -623,15 +623,13 @@ struct StringViewVectorHash {
 
 class StringStatNameMap {
 public:
-  explicit StringStatNameMap(SymbolTable& symbol_table) : symbol_table_(symbol_table) {}
-
   /**
    * This does not take a lock in the symbol table.
    */
-  absl::optional<StatName> find(absl::string_view name) const {
+  absl::optional<StatName> find(absl::string_view name, const SymbolTable& symbol_table) const {
     absl::optional<StatName> ret;
 
-    StringViewVector v = symbol_table_.splitString(name);
+    StringViewVector v = symbol_table.splitString(name);
     auto iter = string_vector_stat_name_map_.find(v);
     if (iter != string_vector_stat_name_map_.end()) {
       ret = iter->second;
@@ -645,8 +643,8 @@ class StringStatNameMap {
    *
    * Note: this take a lock in the symbol table, so it's useful to call find() first.
    */
-  void insert(StatName stat_name) {
-    StringViewVector v = symbol_table_.statNameToStringVector(stat_name);
+  void insert(StatName stat_name, const SymbolTable& symbol_table) {
+    StringViewVector v = symbol_table.statNameToStringVector(stat_name);
     string_vector_stat_name_map_[v] = stat_name;
   }
 
@@ -655,7 +653,6 @@ class StringStatNameMap {
       absl::flat_hash_map<StringViewVector, StatName, StringViewVectorHash>;
 
   StringVectorStatNameMap string_vector_stat_name_map_;
-  SymbolTable& symbol_table_;
 };
 
 } // namespace Stats
diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc
index 8a31348ca748a..e9c9864ac420e 100644
--- a/source/common/stats/thread_local_store.cc
+++ b/source/common/stats/thread_local_store.cc
@@ -32,9 +32,9 @@ ThreadLocalStoreImpl::~ThreadLocalStoreImpl() {
   ASSERT(shutting_down_ || !threading_ever_initialized_);
   default_scope_.reset();
   ASSERT(scopes_.empty());
-  for (StatNameStorageSet* rejected_stats : rejected_stats_purgatory_) {
-    rejected_stats->free(symbolTable());
-    delete rejected_stats;
+  for (StatNameStorageSet* stats : stats_purgatory_) {
+    stats->free(symbolTable());
+    delete stats;
   }
 }
 
@@ -206,7 +206,15 @@ void ThreadLocalStoreImpl::releaseScopeCrossThread(ScopeImpl* scope) {
   //
   // We use a raw pointer here as it's easier to capture it in in the lambda.
   auto rejected_stats = new StatNameStorageSet;
+  auto hot_path_stats = new StatNameStorageSet;
   rejected_stats->swap(scope->central_cache_.rejected_stats_);
+  hot_path_stats->swap(scope->central_cache_.hot_path_stats_);
+  auto cleanup_stats = [rejected_stats, hot_path_stats, this]() {
+    rejected_stats->free(symbolTable());
+    hot_path_stats->free(symbolTable());
+    delete rejected_stats;
+    delete hot_path_stats;
+  };
 
   // This can happen from any thread. We post() back to the main thread which will initiate the
   // cache flush operation.
@@ -221,22 +229,22 @@ void ThreadLocalStoreImpl::releaseScopeCrossThread(ScopeImpl* scope) {
     // be cleared out in the ThreadLocalStoreImpl destructor. We'd prefer
     // to release the memory immediately, however, in which case we remove
     // the rejected stats set from purgatory.
-    rejected_stats_purgatory_.insert(rejected_stats);
-    auto clean_central_cache = [this, rejected_stats]() {
+    stats_purgatory_.insert(rejected_stats);
+    stats_purgatory_.insert(hot_path_stats);
+    auto clean_central_cache = [this, rejected_stats, hot_path_stats, cleanup_stats]() {
       {
         Thread::LockGuard lock(lock_);
-        rejected_stats_purgatory_.erase(rejected_stats);
+        stats_purgatory_.erase(rejected_stats);
+        stats_purgatory_.erase(hot_path_stats);
       }
-      rejected_stats->free(symbolTable());
-      delete rejected_stats;
+      cleanup_stats();
     };
     lock.release();
     main_thread_dispatcher_->post([this, clean_central_cache, scope_id]() {
       clearScopeFromCaches(scope_id, clean_central_cache);
     });
   } else {
-    rejected_stats->free(symbolTable());
-    delete rejected_stats;
+    cleanup_stats();
   }
 }
 
@@ -507,6 +515,42 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatName(StatName name)
   return **central_ref;
 }
 
+StatName
+ThreadLocalStoreImpl::ScopeImpl::fastMemoryIntensiveStatNameLookup(absl::string_view name) {
+  absl::optional<StatName> stat_name;
+
+  // Try to populate stat_name from the TLS cache, without taking any locks.
+  StringStatNameMap* tls_cache = nullptr;
+  if (!parent_.shutting_down_ && parent_.tls_) {
+    TlsCacheEntry& entry = parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_];
+    tls_cache = &entry.hot_path_stats_map_;
+    stat_name = tls_cache->find(name, symbolTable());
+    if (stat_name) {
+      return *stat_name;
+    }
+  }
+
+  // If that doesn't work, try the central cache.
+  Thread::LockGuard lock(parent_.lock_);
+  stat_name = central_cache_.hot_path_stats_map_.find(name, symbolTable());
+  if (!stat_name) {
+    StatNameStorage storage(name, symbolTable());
+    auto insertion = central_cache_.hot_path_stats_.insert(std::move(storage));
+    stat_name = insertion.first->statName();
+    if (insertion.second) {
+      central_cache_.hot_path_stats_map_.insert(*stat_name, symbolTable());
+    } else {
+      // A race between threads can cause two threads to simultaneously
+      // try to insert the same name.
+      storage.free(symbolTable());
+    }
+  }
+  if (tls_cache != nullptr) {
+    tls_cache->insert(*stat_name, symbolTable());
+  }
+  return *stat_name;
+}
+
 absl::optional<std::reference_wrapper<const Counter>>
 ThreadLocalStoreImpl::ScopeImpl::findCounter(StatName name) const {
   return findStatLockHeld<Counter>(name, central_cache_.counters_);
diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h
index b7ff6e7532e71..392cc51dfa6e1 100644
--- a/source/common/stats/thread_local_store.h
+++ b/source/common/stats/thread_local_store.h
@@ -160,6 +160,10 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
   }
   Histogram& histogram(const std::string& name) override { return default_scope_->histogram(name); }
   NullGaugeImpl& nullGauge(const std::string&) override { return null_gauge_; }
+  StatName fastMemoryIntensiveStatNameLookup(absl::string_view name) override {
+    return default_scope_->fastMemoryIntensiveStatNameLookup(name);
+  }
+
   const SymbolTable& symbolTable() const override { return alloc_.symbolTable(); }
   SymbolTable& symbolTable() override { return alloc_.symbolTable(); }
   const TagProducer& tagProducer() const { return *tag_producer_; }
@@ -232,6 +236,12 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
     // symbol-table lock. We keep this StatName set here in the TLS cache to
     // avoid taking a lock to compute rejection.
     StatNameHashSet rejected_stats_;
+
+    // We keep a TLS cache of names that can only be discovered on the hot-path,
+    // that we need to turn into stats. StringStatNameMap contains only
+    // references to separately maintained stats, and the backing store for this
+    // is in CentralAcacheEntry.hot_path_stats_.
+    StringStatNameMap hot_path_stats_map_;
   };
 
   struct CentralCacheEntry {
@@ -239,10 +249,8 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
     StatMap<GaugeSharedPtr> gauges_;
     StatMap<ParentHistogramImplSharedPtr> histograms_;
     StatNameStorageSet rejected_stats_;
-
-    // Strings should in most cases be converted to symbols at construction
-    // time. However, for a
-    absl::flat_hash_map<std::string, StatName> hot_path_symbols_;
+    StatNameStorageSet hot_path_stats_; // Backing store for TlsCacheEntry.hot_path_stats_map_.
+    StringStatNameMap hot_path_stats_map_;
   };
 
   struct ScopeImpl : public TlsScope {
@@ -323,6 +331,8 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
                                 std::unique_ptr<StatNameManagedStorage>& truncated_name_storage,
                                 std::vector<Tag>& tags, std::string& tag_extracted_name);
 
+    StatName fastMemoryIntensiveStatNameLookup(absl::string_view name) override;
+
     static std::atomic<uint64_t> next_scope_id_;
 
     const uint64_t scope_id_;
@@ -384,7 +394,7 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
   std::vector<GaugeSharedPtr> deleted_gauges_;
   std::vector<HistogramSharedPtr> deleted_histograms_;
 
-  absl::flat_hash_set<StatNameStorageSet*> rejected_stats_purgatory_ GUARDED_BY(lock_);
+  absl::flat_hash_set<StatNameStorageSet*> stats_purgatory_ GUARDED_BY(lock_);
 };
 
 } // namespace Stats
diff --git a/test/common/stats/isolated_store_impl_test.cc b/test/common/stats/isolated_store_impl_test.cc
index d8b7d6a985e14..0a57efb43f1bb 100644
--- a/test/common/stats/isolated_store_impl_test.cc
+++ b/test/common/stats/isolated_store_impl_test.cc
@@ -109,6 +109,9 @@ TEST_F(StatsIsolatedStoreImplTest, AllWithSymbolTable) {
 
   EXPECT_EQ(4UL, store_.counters().size());
   EXPECT_EQ(2UL, store_.gauges().size());
+
+  StatName fast_lookup = store_.fastMemoryIntensiveStatNameLookup("fast.lookup");
+  EXPECT_EQ("fast.lookup", store_.symbolTable().toString(fast_lookup));
 }
 
 TEST_F(StatsIsolatedStoreImplTest, LongStatName) {
diff --git a/test/common/stats/symbol_table_impl_test.cc b/test/common/stats/symbol_table_impl_test.cc
index c0f1442be4300..da8947ecd8171 100644
--- a/test/common/stats/symbol_table_impl_test.cc
+++ b/test/common/stats/symbol_table_impl_test.cc
@@ -540,10 +540,10 @@ TEST_P(StatNameTest, SharedStatNameStorageSetSwap) {
 TEST_P(StatNameTest, StatNameStringMap) {
   StatNameManagedStorage foo_bar("foo.bar", *table_); // Must outlive the map.
   {
-    StringStatNameMap map(*table_);
-    EXPECT_FALSE(map.find("foo.bar"));
-    map.insert(foo_bar.statName());
-    EXPECT_EQ(map.find("foo.bar"), foo_bar.statName());
+    StringStatNameMap map;
+    EXPECT_FALSE(map.find("foo.bar", *table_));
+    map.insert(foo_bar.statName(), *table_);
+    EXPECT_EQ(map.find("foo.bar", *table_), foo_bar.statName());
   }
 }
 
diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc
index 6954a28147f1e..b65ffc78f5219 100644
--- a/test/common/stats/thread_local_store_test.cc
+++ b/test/common/stats/thread_local_store_test.cc
@@ -221,6 +221,9 @@ TEST_F(StatsThreadLocalStoreTest, NoTls) {
   EXPECT_EQ(&g1, store_->gauges().front().get()); // front() ok when size()==1
   EXPECT_EQ(2L, store_->gauges().front().use_count());
 
+  StatName fast_lookup = store_->fastMemoryIntensiveStatNameLookup("fast.lookup");
+  EXPECT_EQ("fast.lookup", symbol_table_.toString(fast_lookup));
+
   store_->shutdownThreading();
 }
 
@@ -273,6 +276,9 @@ TEST_F(StatsThreadLocalStoreTest, Tls) {
   EXPECT_EQ(1UL, store_->gauges().size());
   EXPECT_EQ(&g1, store_->gauges().front().get()); // front() ok when size()==1
   EXPECT_EQ(2L, store_->gauges().front().use_count());
+
+  StatName fast_lookup = store_->fastMemoryIntensiveStatNameLookup("fast.lookup");
+  EXPECT_EQ("fast.lookup", symbol_table_.toString(fast_lookup));
 }
 
 TEST_F(StatsThreadLocalStoreTest, BasicScope) {
diff --git a/test/mocks/stats/mocks.cc b/test/mocks/stats/mocks.cc
index 18c7e1a08bc0f..f6e4afa5d6f41 100644
--- a/test/mocks/stats/mocks.cc
+++ b/test/mocks/stats/mocks.cc
@@ -125,6 +125,17 @@ MockStore::MockStore() : StoreImpl(*fake_symbol_table_) {
 }
 MockStore::~MockStore() {}
 
+StatName MockStore::fastMemoryIntensiveStatNameLookup(absl::string_view name) {
+  absl::optional<StatName> stat_name = string_stat_name_map_.find(name, symbolTable());
+  if (!stat_name) {
+    StatNameStorage storage(name, symbolTable());
+    auto insertion = stat_name_set_.insert(std::move(storage));
+    ASSERT(insertion.second); // If the name is not in the map, it should not be in the set.
+    stat_name = insertion.first->statName();
+  }
+  return *stat_name;
+}
+
 MockIsolatedStatsStore::MockIsolatedStatsStore()
     : IsolatedStoreImpl(Test::Global<Stats::FakeSymbolTableImpl>::get()) {}
 MockIsolatedStatsStore::~MockIsolatedStatsStore() {}
diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h
index 23d6dd2738cd6..e7693b73a0f1f 100644
--- a/test/mocks/stats/mocks.h
+++ b/test/mocks/stats/mocks.h
@@ -207,12 +207,16 @@ class MockStore : public SymbolTableProvider, public StoreImpl {
     return histogram(symbol_table_->toString(name));
   }
 
+  StatName fastMemoryIntensiveStatNameLookup(absl::string_view name) override;
+
   SymbolTable& symbolTable() override { return symbol_table_.get(); }
   const SymbolTable& symbolTable() const override { return symbol_table_.get(); }
 
   Test::Global<FakeSymbolTableImpl> symbol_table_;
   testing::NiceMock<MockCounter> counter_;
   std::vector<std::unique_ptr<MockHistogram>> histograms_;
+  StatNameStorageSet stat_name_set_;
+  StringStatNameMap string_stat_name_map_;
 };
 
 /**

From ba77b7a0810878978dc4646cc13a22e2c309699a Mon Sep 17 00:00:00 2001
From: Joshua Marantz <jmarantz@google.com>
Date: Mon, 20 May 2019 09:17:36 -0400
Subject: [PATCH 3/8] Remove superfluous functions and outline some non-trivial
 functions.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
---
 source/common/stats/symbol_table_impl.cc | 37 +++++++++++-------------
 source/common/stats/symbol_table_impl.h  | 26 +++--------------
 2 files changed, 21 insertions(+), 42 deletions(-)

diff --git a/source/common/stats/symbol_table_impl.cc b/source/common/stats/symbol_table_impl.cc
index 1d65687a00a1e..1b5de673753db 100644
--- a/source/common/stats/symbol_table_impl.cc
+++ b/source/common/stats/symbol_table_impl.cc
@@ -156,6 +156,10 @@ std::string SymbolTableImpl::decodeSymbolVec(const SymbolVec& symbols) const {
   return absl::StrJoin(decodeSymbolVecTokens(symbols), ".");
 }
 
+StringViewVector SymbolTableImpl::statNameToStringVector(const StatName& stat_name) const {
+  return decodeSymbolVecTokens(Encoding::decodeSymbols(stat_name.data(), stat_name.dataSize()));
+}
+
 StringViewVector SymbolTableImpl::decodeSymbolVecTokens(const SymbolVec& symbols) const {
   StringViewVector name_tokens;
   name_tokens.reserve(symbols.size());
@@ -439,29 +443,22 @@ size_t StringViewVectorHash::operator()(const StringViewVector& a) const {
   return hash;
 }
 
-StringViewVector SymbolTableImpl::statNameToStringVector(const StatName& stat_name) const {
-  return decodeSymbolVecTokens(Encoding::decodeSymbols(stat_name.data(), stat_name.dataSize()));
-}
+absl::optional<StatName> StringStatNameMap::find(absl::string_view name,
+                                                 const SymbolTable& symbol_table) const {
+  absl::optional<StatName> ret;
 
-/*struct StringViewVectorCompare {
-  size_t operator()(const StringViewVector& a, const StringViewVector& b) const;
-};
-
-
-bool StringViewVectorCompare operator()(const StringViewVector& a,
-                                        const StringViewVector& b) const {
-  return
-  if (a.size() != b.size()) {
-    return false;
-  }
-  for (size_t i = 0; i < a.size(); ++i) {
-    if (a[i] != b[i]) {
-      return false;
-    }
+  StringViewVector v = symbol_table.splitString(name);
+  auto iter = string_vector_stat_name_map_.find(v);
+  if (iter != string_vector_stat_name_map_.end()) {
+    ret = iter->second;
   }
-  return true;
+  return ret;
+}
+
+void StringStatNameMap::insert(StatName stat_name, const SymbolTable& symbol_table) {
+  StringViewVector v = symbol_table.statNameToStringVector(stat_name);
+  string_vector_stat_name_map_[v] = stat_name;
 }
-*/
 
 } // namespace Stats
 } // namespace Envoy
diff --git a/source/common/stats/symbol_table_impl.h b/source/common/stats/symbol_table_impl.h
index cb1d4b6e45ff4..862bdc7a51fa1 100644
--- a/source/common/stats/symbol_table_impl.h
+++ b/source/common/stats/symbol_table_impl.h
@@ -514,17 +514,11 @@ struct StatNameHash {
   size_t operator()(const StatName& a) const { return a.hash(); }
 };
 
-// Helper class for constructing hash-tables with StatName keys.
-struct StatNameCompare {
-  bool operator()(const StatName& a, const StatName& b) const { return a == b; }
-};
-
 // Value-templatized hash-map with StatName key.
-template <class T>
-using StatNameHashMap = absl::flat_hash_map<StatName, T, StatNameHash, StatNameCompare>;
+template <class T> using StatNameHashMap = absl::flat_hash_map<StatName, T, StatNameHash>;
 
 // Hash-set of StatNames
-using StatNameHashSet = absl::flat_hash_set<StatName, StatNameHash, StatNameCompare>;
+using StatNameHashSet = absl::flat_hash_set<StatName, StatNameHash>;
 
 // Helper class for sorting StatNames.
 struct StatNameLessThan {
@@ -626,16 +620,7 @@ class StringStatNameMap {
   /**
    * This does not take a lock in the symbol table.
    */
-  absl::optional<StatName> find(absl::string_view name, const SymbolTable& symbol_table) const {
-    absl::optional<StatName> ret;
-
-    StringViewVector v = symbol_table.splitString(name);
-    auto iter = string_vector_stat_name_map_.find(v);
-    if (iter != string_vector_stat_name_map_.end()) {
-      ret = iter->second;
-    }
-    return ret;
-  }
+  absl::optional<StatName> find(absl::string_view name, const SymbolTable& symbol_table) const;
 
   /**
    * Inserts stat_name into the StatNameStorageMap. Caller must guarantee that
@@ -643,10 +628,7 @@ class StringStatNameMap {
    *
    * Note: this take a lock in the symbol table, so it's useful to call find() first.
    */
-  void insert(StatName stat_name, const SymbolTable& symbol_table) {
-    StringViewVector v = symbol_table.statNameToStringVector(stat_name);
-    string_vector_stat_name_map_[v] = stat_name;
-  }
+  void insert(StatName stat_name, const SymbolTable& symbol_table);
 
 private:
   using StringVectorStatNameMap =

From 32e2e4ec2c3406e8f26cd3e348acefc67155c218 Mon Sep 17 00:00:00 2001
From: Joshua Marantz <jmarantz@google.com>
Date: Mon, 20 May 2019 12:58:41 -0400
Subject: [PATCH 4/8] Get all tests passing.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
---
 include/envoy/stats/symbol_table.h           |  7 ++---
 source/common/stats/fake_symbol_table_impl.h |  4 +--
 source/common/stats/scope_prefixer.cc        |  2 +-
 source/common/stats/symbol_table_impl.cc     | 29 +++++++++++++-------
 source/common/stats/symbol_table_impl.h      | 28 ++++++++++++-------
 test/integration/server.h                    |  9 ++++++
 test/integration/stats_integration_test.cc   |  3 +-
 7 files changed, 54 insertions(+), 28 deletions(-)

diff --git a/include/envoy/stats/symbol_table.h b/include/envoy/stats/symbol_table.h
index 0cf1a9cc94eba..5566eff8813cc 100644
--- a/include/envoy/stats/symbol_table.h
+++ b/include/envoy/stats/symbol_table.h
@@ -22,8 +22,6 @@ class StatName;
 
 class StatNameList;
 
-using StringViewVector = std::vector<absl::string_view>;
-
 /**
  * SymbolTable manages a namespace optimized for stat names, exploiting their
  * typical composition from "."-separated tokens, with a significant overlap
@@ -192,13 +190,14 @@ class SymbolTable {
    * SymbolTable as long as a reference to the symbols is retained. This takes
    * a lock on the symbol table.
    */
-  virtual StringViewVector statNameToStringVector(const StatName& stat_name) const PURE;
+  virtual std::vector<absl::string_view>
+  statNameToStringVector(const StatName& stat_name) const PURE;
 
   /**
    * Splits a string_view into multple symbols. This does not require a lock on
    * the symbol table.
    */
-  virtual StringViewVector splitString(absl::string_view) const PURE;
+  virtual std::vector<absl::string_view> splitString(absl::string_view) const PURE;
 };
 
 using SharedSymbolTable = std::shared_ptr<SymbolTable>;
diff --git a/source/common/stats/fake_symbol_table_impl.h b/source/common/stats/fake_symbol_table_impl.h
index 80198a817794e..e03fcbd8bfc73 100644
--- a/source/common/stats/fake_symbol_table_impl.h
+++ b/source/common/stats/fake_symbol_table_impl.h
@@ -125,10 +125,10 @@ class FakeSymbolTableImpl : public SymbolTable {
     fn(toStringView(stat_name));
   }
 
-  StringViewVector statNameToStringVector(const StatName& stat_name) const override {
+  std::vector<absl::string_view> statNameToStringVector(const StatName& stat_name) const override {
     return {toStringView(stat_name)};
   }
-  StringViewVector splitString(absl::string_view s) const override { return {s}; }
+  std::vector<absl::string_view> splitString(absl::string_view s) const override { return {s}; }
 
 private:
   absl::string_view toStringView(const StatName& stat_name) const {
diff --git a/source/common/stats/scope_prefixer.cc b/source/common/stats/scope_prefixer.cc
index 97a7851483f55..0fbd607813239 100644
--- a/source/common/stats/scope_prefixer.cc
+++ b/source/common/stats/scope_prefixer.cc
@@ -16,7 +16,7 @@ ScopePrefixer::ScopePrefixer(absl::string_view prefix, Scope& scope)
 
 ScopePrefixer::ScopePrefixer(StatName prefix, Scope& scope)
     : scope_(scope), prefix_(prefix, symbolTable()),
-      prefix_string_(symbolTable().toString(prefix)) {}
+      prefix_string_(symbolTable().toString(prefix) + ".") {}
 
 ScopePrefixer::~ScopePrefixer() { prefix_.free(symbolTable()); }
 
diff --git a/source/common/stats/symbol_table_impl.cc b/source/common/stats/symbol_table_impl.cc
index 1b5de673753db..1ce2dcb904e59 100644
--- a/source/common/stats/symbol_table_impl.cc
+++ b/source/common/stats/symbol_table_impl.cc
@@ -118,7 +118,7 @@ void SymbolTableImpl::addTokensToEncoding(const absl::string_view name, Encoding
 
   // We want to hold the lock for the minimum amount of time, so we do the
   // string-splitting and prepare a temp vector of Symbol first.
-  StringViewVector tokens = absl::StrSplit(name, '.');
+  std::vector<absl::string_view> tokens = absl::StrSplit(name, '.');
   std::vector<Symbol> symbols;
   symbols.reserve(tokens.size());
 
@@ -156,12 +156,14 @@ std::string SymbolTableImpl::decodeSymbolVec(const SymbolVec& symbols) const {
   return absl::StrJoin(decodeSymbolVecTokens(symbols), ".");
 }
 
-StringViewVector SymbolTableImpl::statNameToStringVector(const StatName& stat_name) const {
+std::vector<absl::string_view>
+SymbolTableImpl::statNameToStringVector(const StatName& stat_name) const {
   return decodeSymbolVecTokens(Encoding::decodeSymbols(stat_name.data(), stat_name.dataSize()));
 }
 
-StringViewVector SymbolTableImpl::decodeSymbolVecTokens(const SymbolVec& symbols) const {
-  StringViewVector name_tokens;
+std::vector<absl::string_view>
+SymbolTableImpl::decodeSymbolVecTokens(const SymbolVec& symbols) const {
+  std::vector<absl::string_view> name_tokens;
   name_tokens.reserve(symbols.size());
   {
     // Hold the lock only while decoding symbols.
@@ -435,7 +437,7 @@ void StatNameList::clear(SymbolTable& symbol_table) {
   storage_.reset();
 }
 
-size_t StringViewVectorHash::operator()(const StringViewVector& a) const {
+size_t StringViewVectorHash::operator()(const std::vector<absl::string_view>& a) const {
   uint64_t hash = 0;
   for (absl::string_view sv : a) {
     hash = 63 * hash + HashUtil::xxHash64(sv);
@@ -447,17 +449,24 @@ absl::optional<StatName> StringStatNameMap::find(absl::string_view name,
                                                  const SymbolTable& symbol_table) const {
   absl::optional<StatName> ret;
 
-  StringViewVector v = symbol_table.splitString(name);
-  auto iter = string_vector_stat_name_map_.find(v);
-  if (iter != string_vector_stat_name_map_.end()) {
+  // TODO(jmarantz)(: change the hash/compare mechanism to avoid needing to
+  // allocate memory during the find() operation. We should be able to
+  // iterate over the "."-separated tokens to compute the hash-value without
+  // having to have the allocated split vector first. Similar for compare.
+  //
+  // The only nuance -- and the reason to leave this as is for now, is that
+  // when using FakeSymbolTable, there is no actual splitting by token.
+  std::vector<absl::string_view> v = symbol_table.splitString(name);
+  auto iter = map_.find(v);
+  if (iter != map_.end()) {
     ret = iter->second;
   }
   return ret;
 }
 
 void StringStatNameMap::insert(StatName stat_name, const SymbolTable& symbol_table) {
-  StringViewVector v = symbol_table.statNameToStringVector(stat_name);
-  string_vector_stat_name_map_[v] = stat_name;
+  std::vector<absl::string_view> v = symbol_table.statNameToStringVector(stat_name);
+  map_[v] = stat_name;
 }
 
 } // namespace Stats
diff --git a/source/common/stats/symbol_table_impl.h b/source/common/stats/symbol_table_impl.h
index 862bdc7a51fa1..f8f92ba5f31e4 100644
--- a/source/common/stats/symbol_table_impl.h
+++ b/source/common/stats/symbol_table_impl.h
@@ -139,8 +139,8 @@ class SymbolTableImpl : public SymbolTable {
   void callWithStringView(StatName stat_name,
                           const std::function<void(absl::string_view)>& fn) const override;
 
-  StringViewVector statNameToStringVector(const StatName& stat_name) const override;
-  StringViewVector splitString(absl::string_view s) const override {
+  std::vector<absl::string_view> statNameToStringVector(const StatName& stat_name) const override;
+  std::vector<absl::string_view> splitString(absl::string_view s) const override {
     return absl::StrSplit(s, '.');
   }
 
@@ -188,7 +188,7 @@ class SymbolTableImpl : public SymbolTable {
    * @return std::string the retrieved stat name.
    */
   std::string decodeSymbolVec(const SymbolVec& symbols) const;
-  StringViewVector decodeSymbolVecTokens(const SymbolVec& symbols) const;
+  std::vector<absl::string_view> decodeSymbolVecTokens(const SymbolVec& symbols) const;
 
   /**
    * Convenience function for encode(), symbolizing one string segment at a time.
@@ -549,7 +549,6 @@ 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();
   }
@@ -612,13 +611,21 @@ class StatNameStorageSet {
 };
 
 struct StringViewVectorHash {
-  size_t operator()(const StringViewVector& a) const;
+  size_t operator()(const std::vector<absl::string_view>& a) const;
 };
 
+// Captures a map from string_view to StatName. The backing store for both of
+// these must be separately managed, and live beyond the map.
+//
+// This is intended for use as a TLS cache in thread_local_store.cc, with the
+// backing store held in sets in the CentralCacheEntry.
 class StringStatNameMap {
 public:
   /**
    * This does not take a lock in the symbol table.
+   * @param name the string stat-name to find.
+   * @param symbol_table the symbol-table used to help split the string into tokens.
+   * @return the StatName, if found.
    */
   absl::optional<StatName> find(absl::string_view name, const SymbolTable& symbol_table) const;
 
@@ -626,15 +633,16 @@ class StringStatNameMap {
    * Inserts stat_name into the StatNameStorageMap. Caller must guarantee that
    * the backing-store for StatName lasts longer than the StatNameStorageMap.
    *
-   * Note: this take a lock in the symbol table, so it's useful to call find() first.
+   * Note: this takes a lock in the symbol table, in order to do
+   * symbol-string-lookups in its lock-protected symbol map.
+   *
+   * @param stat_name the name to insert.
+   * @param symbol_table the symbol-table used to find stable strings for the tokens in stat names.
    */
   void insert(StatName stat_name, const SymbolTable& symbol_table);
 
 private:
-  using StringVectorStatNameMap =
-      absl::flat_hash_map<StringViewVector, StatName, StringViewVectorHash>;
-
-  StringVectorStatNameMap string_vector_stat_name_map_;
+  absl::flat_hash_map<std::vector<absl::string_view>, StatName, StringViewVectorHash> map_;
 };
 
 } // namespace Stats
diff --git a/test/integration/server.h b/test/integration/server.h
index aa1e3865913bb..9c3b1e22c90f6 100644
--- a/test/integration/server.h
+++ b/test/integration/server.h
@@ -123,6 +123,10 @@ class TestScopeWrapper : public Scope {
   const SymbolTable& symbolTable() const override { return wrapped_scope_->symbolTable(); }
   SymbolTable& symbolTable() override { return wrapped_scope_->symbolTable(); }
 
+  StatName fastMemoryIntensiveStatNameLookup(absl::string_view name) override {
+    return wrapped_scope_->fastMemoryIntensiveStatNameLookup(name);
+  }
+
 private:
   Thread::MutexBasicLockable& lock_;
   ScopePtr wrapped_scope_;
@@ -197,6 +201,11 @@ class TestIsolatedStoreImpl : public StoreRoot {
     return store_.histograms();
   }
 
+  StatName fastMemoryIntensiveStatNameLookup(absl::string_view name) override {
+    Thread::LockGuard lock(lock_);
+    return store_.fastMemoryIntensiveStatNameLookup(name);
+  }
+
   // Stats::StoreRoot
   void addSink(Sink&) override {}
   void setTagProducer(TagProducerPtr&&) override {}
diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc
index 6fdd74e70856b..3e5b5e566bef7 100644
--- a/test/integration/stats_integration_test.cc
+++ b/test/integration/stats_integration_test.cc
@@ -212,7 +212,8 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithStats) {
   // 2019/04/24  6161     49415               Pack tags and tag extracted names
   // 2019/05/07  6794     49957               Stats for excluded hosts in cluster
 
-  EXPECT_EQ(m_per_cluster, 49957);
+  // EXPECT_EQ(m_per_cluster, 49957);
+  EXPECT_EQ(m_per_cluster, 50134);
 }
 
 } // namespace

From 9950d20b9af3382930a0edec97df7b3e7e43e88c Mon Sep 17 00:00:00 2001
From: Joshua Marantz <jmarantz@google.com>
Date: Mon, 20 May 2019 13:01:28 -0400
Subject: [PATCH 5/8] Spelling & doxygen.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
---
 include/envoy/stats/symbol_table.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/envoy/stats/symbol_table.h b/include/envoy/stats/symbol_table.h
index 5566eff8813cc..10db6a388efc9 100644
--- a/include/envoy/stats/symbol_table.h
+++ b/include/envoy/stats/symbol_table.h
@@ -194,10 +194,13 @@ class SymbolTable {
   statNameToStringVector(const StatName& stat_name) const PURE;
 
   /**
-   * Splits a string_view into multple symbols. This does not require a lock on
+   * Splits a string_view into multiple symbols. This does not require a lock on
    * the symbol table.
+   *
+   * @param str the string to split, based on the semantics of the SymbolTable.
+   * @return the string, split into tokens.
    */
-  virtual std::vector<absl::string_view> splitString(absl::string_view) const PURE;
+  virtual std::vector<absl::string_view> splitString(absl::string_view str) const PURE;
 };
 
 using SharedSymbolTable = std::shared_ptr<SymbolTable>;

From d239b0e2b4c95f8da0c6bcd14fec1ba5898b3b3d Mon Sep 17 00:00:00 2001
From: Joshua Marantz <jmarantz@google.com>
Date: Mon, 20 May 2019 13:06:51 -0400
Subject: [PATCH 6/8] more spelling.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
---
 include/envoy/stats/scope.h              | 2 +-
 source/common/stats/thread_local_store.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/envoy/stats/scope.h b/include/envoy/stats/scope.h
index 86c210f3d69d5..2cb88130d8069 100644
--- a/include/envoy/stats/scope.h
+++ b/include/envoy/stats/scope.h
@@ -122,7 +122,7 @@ class Scope {
    * update. Forthose names that can be determined at that time, the StatNames
    * should be collected via StatNamePool or StatNameManagedStorage once, and
    * then used later on in the hot-path to compose fully elaborated StatName
-   * objects via SYmbolTable::join(), which is lock-free.
+   * objects via SymbolTable::join(), which is lock-free.
    *
    * @param name The name of the stat or fragment of stat.
    * @return the stat name allocated from symbolTable().
diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h
index 392cc51dfa6e1..def480bace642 100644
--- a/source/common/stats/thread_local_store.h
+++ b/source/common/stats/thread_local_store.h
@@ -240,7 +240,7 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
     // We keep a TLS cache of names that can only be discovered on the hot-path,
     // that we need to turn into stats. StringStatNameMap contains only
     // references to separately maintained stats, and the backing store for this
-    // is in CentralAcacheEntry.hot_path_stats_.
+    // is in CentralCacheEntry.hot_path_stats_.
     StringStatNameMap hot_path_stats_map_;
   };
 

From 8e9df9189fd7c66fb365e72cd84e826ecaa1cf9c Mon Sep 17 00:00:00 2001
From: Joshua Marantz <jmarantz@google.com>
Date: Mon, 20 May 2019 14:59:35 -0400
Subject: [PATCH 7/8] cleanup and simplify per clang-tidy.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
---
 source/common/stats/thread_local_store.cc | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc
index e9c9864ac420e..70f66a1e78cb0 100644
--- a/source/common/stats/thread_local_store.cc
+++ b/source/common/stats/thread_local_store.cc
@@ -531,18 +531,15 @@ ThreadLocalStoreImpl::ScopeImpl::fastMemoryIntensiveStatNameLookup(absl::string_
   }
 
   // If that doesn't work, try the central cache.
-  Thread::LockGuard lock(parent_.lock_);
-  stat_name = central_cache_.hot_path_stats_map_.find(name, symbolTable());
-  if (!stat_name) {
-    StatNameStorage storage(name, symbolTable());
-    auto insertion = central_cache_.hot_path_stats_.insert(std::move(storage));
-    stat_name = insertion.first->statName();
-    if (insertion.second) {
+  {
+    Thread::LockGuard lock(parent_.lock_);
+    stat_name = central_cache_.hot_path_stats_map_.find(name, symbolTable());
+    if (!stat_name) {
+      StatNameStorage storage(name, symbolTable());
+      auto insertion = central_cache_.hot_path_stats_.insert(std::move(storage));
+      stat_name = insertion.first->statName();
+      ASSERT(insertion.second); // hot_path_stats_map_ and hot_path_stats_ are lock-protected.
       central_cache_.hot_path_stats_map_.insert(*stat_name, symbolTable());
-    } else {
-      // A race between threads can cause two threads to simultaneously
-      // try to insert the same name.
-      storage.free(symbolTable());
     }
   }
   if (tls_cache != nullptr) {

From b496faa6f854efa41a94ac29f4a108391db980e4 Mon Sep 17 00:00:00 2001
From: Joshua Marantz <jmarantz@google.com>
Date: Tue, 21 May 2019 15:56:17 -0400
Subject: [PATCH 8/8] hit more lines of the tls cache handling code.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
---
 test/common/stats/thread_local_store_test.cc | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc
index b65ffc78f5219..fa70601f14fc6 100644
--- a/test/common/stats/thread_local_store_test.cc
+++ b/test/common/stats/thread_local_store_test.cc
@@ -267,6 +267,12 @@ TEST_F(StatsThreadLocalStoreTest, Tls) {
   EXPECT_EQ(&g1, store_->gauges().front().get()); // front() ok when size()==1
   EXPECT_EQ(3L, store_->gauges().front().use_count());
 
+  StatName fast_lookup = store_->fastMemoryIntensiveStatNameLookup("fast.lookup");
+  // Request the same string so that we trigger the cache 'hit' case.
+  StatName fast_lookup2 = store_->fastMemoryIntensiveStatNameLookup("fast.lookup");
+  EXPECT_EQ(fast_lookup, fast_lookup2);
+  EXPECT_EQ("fast.lookup", symbol_table_.toString(fast_lookup));
+
   store_->shutdownThreading();
   tls_.shutdownThread();
 
@@ -276,9 +282,6 @@ TEST_F(StatsThreadLocalStoreTest, Tls) {
   EXPECT_EQ(1UL, store_->gauges().size());
   EXPECT_EQ(&g1, store_->gauges().front().get()); // front() ok when size()==1
   EXPECT_EQ(2L, store_->gauges().front().use_count());
-
-  StatName fast_lookup = store_->fastMemoryIntensiveStatNameLookup("fast.lookup");
-  EXPECT_EQ("fast.lookup", symbol_table_.toString(fast_lookup));
 }
 
 TEST_F(StatsThreadLocalStoreTest, BasicScope) {