stats: comments clean up

envoy/stats/symbol_table.h

@@ -56,7 +56,7 @@ class SymbolTable {
    * into the SymbolTable, which will not be optimal, but in practice appears
    * to be pretty good.
    *
-   * This is exposed in the interface for the benefit of join(), which which is
+   * This is exposed in the interface for the benefit of join(), which is
    * used in the hot-path to append two stat-names into a temp without taking
    * locks. This is used then in thread-local cache lookup, so that once warm,
    * no locks are taken when looking up stats.
@@ -128,7 +128,7 @@ class SymbolTable {
    *
    * @param names A pointer to the first name in an array, allocated by the caller.
    * @param num_names The number of names.
-   * @param symbol_table The symbol table in which to encode the names.
+   * @param list The StatNameList representing the stat names.
    */
   virtual void populateList(const StatName* names, uint32_t num_names, StatNameList& list) PURE;
 

source/common/stats/allocator_impl.cc

@@ -45,8 +45,8 @@ void AllocatorImpl::debugPrint() {
 // which we need in order to clean up the counter and gauge maps in that class
 // when they are destroyed.
 //
-// We implement the RefcountInterface API, using 16 bits that would otherwise be
-// wasted in the alignment padding next to flags_.
+// We implement the RefcountInterface API to avoid weak counter and destructor overhead in
+// shared_ptr.
 template <class BaseClass> class StatsSharedImpl : public MetricImpl<BaseClass> {
 public:
   StatsSharedImpl(StatName name, AllocatorImpl& alloc, StatName tag_extracted_name,

source/common/stats/thread_local_store.cc

@@ -532,14 +532,6 @@ Counter& ThreadLocalStoreImpl::ScopeImpl::counterFromStatNameWithTags(
   }
 
   // Determine the final name based on the prefix and the passed name.
-  //
-  // Note that we can do map.find(final_name.c_str()), but we cannot do
-  // map[final_name.c_str()] as the char*-keyed maps would then save the pointer
-  // to a temporary, and address sanitization errors would follow. Instead we
-  // must do a find() first, using the value if it succeeds. If it fails, then
-  // after we construct the stat we can insert it into the required maps. This
-  // strategy costs an extra hash lookup for each miss, but saves time
-  // re-copying the string and significant memory overhead.
   TagUtility::TagStatNameJoiner joiner(prefix_.statName(), name, stat_name_tags, symbolTable());
   Stats::StatName final_stat_name = joiner.nameWithTags();
 
@@ -593,12 +585,6 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gaugeFromStatNameWithTags(
 
   // See comments in counter(). There is no super clean way (via templates or otherwise) to
   // share this code so I'm leaving it largely duplicated for now.
-  //
-  // Note that we can do map.find(final_name.c_str()), but we cannot do
-  // map[final_name.c_str()] as the char*-keyed maps would then save the pointer to
-  // a temporary, and address sanitization errors would follow. Instead we must
-  // do a find() first, using that if it succeeds. If it fails, then after we
-  // construct the stat we can insert it into the required maps.
   TagUtility::TagStatNameJoiner joiner(prefix_.statName(), name, stat_name_tags, symbolTable());
   StatName final_stat_name = joiner.nameWithTags();
 
@@ -610,7 +596,7 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gaugeFromStatNameWithTags(
   StatRefMap<Gauge>* tls_cache = nullptr;
   StatNameHashSet* tls_rejected_stats = nullptr;
   if (!parent_.shutting_down_ && parent_.tls_cache_) {
-    TlsCacheEntry& entry = parent_.tlsCache().scope_cache_[this->scope_id_];
+    TlsCacheEntry& entry = parent_.tlsCache().insertScope(this->scope_id_);
     tls_cache = &entry.gauges_;
     tls_rejected_stats = &entry.rejected_stats_;
   }
@@ -635,13 +621,6 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatNameWithTags(
 
   // See comments in counter(). There is no super clean way (via templates or otherwise) to
   // share this code so I'm leaving it largely duplicated for now.
-  //
-  // Note that we can do map.find(final_name.c_str()), but we cannot do
-  // map[final_name.c_str()] as the char*-keyed maps would then save the pointer to
-  // a temporary, and address sanitization errors would follow. Instead we must
-  // do a find() first, using that if it succeeds. If it fails, then after we
-  // construct the stat we can insert it into the required maps.
-
   TagUtility::TagStatNameJoiner joiner(prefix_.statName(), name, stat_name_tags, symbolTable());
   StatName final_stat_name = joiner.nameWithTags();
 
@@ -653,7 +632,7 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatNameWithTags(
   StatNameHashMap<ParentHistogramSharedPtr>* tls_cache = nullptr;
   StatNameHashSet* tls_rejected_stats = nullptr;
   if (!parent_.shutting_down_ && parent_.tls_cache_) {
-    TlsCacheEntry& entry = parent_.tlsCache().scope_cache_[this->scope_id_];
+    TlsCacheEntry& entry = parent_.tlsCache().insertScope(this->scope_id_);
     tls_cache = &entry.parent_histograms_;
     auto iter = tls_cache->find(final_stat_name);
     if (iter != tls_cache->end()) {
@@ -713,14 +692,6 @@ TextReadout& ThreadLocalStoreImpl::ScopeImpl::textReadoutFromStatNameWithTags(
   }
 
   // Determine the final name based on the prefix and the passed name.
-  //
-  // Note that we can do map.find(final_name.c_str()), but we cannot do
-  // map[final_name.c_str()] as the char*-keyed maps would then save the pointer
-  // to a temporary, and address sanitization errors would follow. Instead we
-  // must do a find() first, using the value if it succeeds. If it fails, then
-  // after we construct the stat we can insert it into the required maps. This
-  // strategy costs an extra hash lookup for each miss, but saves time
-  // re-copying the string and significant memory overhead.
   TagUtility::TagStatNameJoiner joiner(prefix_.statName(), name, stat_name_tags, symbolTable());
   Stats::StatName final_stat_name = joiner.nameWithTags();
 

source/common/stats/utility.cc

@@ -68,11 +68,6 @@ struct ElementVisitor {
 
 namespace Utility {
 
-ScopePtr scopeFromElements(Scope& scope, const ElementVec& elements) {
-  ElementVisitor visitor(scope.symbolTable(), elements);
-  return scope.scopeFromStatName(visitor.statName());
-}
-
 ScopePtr scopeFromStatNames(Scope& scope, const StatNameVec& elements) {
   SymbolTable::StoragePtr joined = scope.symbolTable().join(elements);
   return scope.scopeFromStatName(StatName(joined.get()));

source/common/stats/utility.h

@@ -63,22 +63,6 @@ std::string sanitizeStatsName(absl::string_view name);
  */
 absl::optional<StatName> findTag(const Metric& metric, StatName find_tag_name);
 
-/**
- * Creates a nested scope from a vector of tokens which are used to create the
- * name. The tokens can be specified as DynamicName or StatName. For
- * tokens specified as DynamicName, a dynamic StatName will be created. See
- * https://github.com/envoyproxy/envoy/blob/main/source/docs/stats.md#dynamic-stat-tokens
- * for more detail on why symbolic StatNames are preferred when possible.
- *
- * See also scopeFromStatNames, which is slightly faster but does not allow
- * passing DynamicName(string)s as names.
- *
- * @param scope The scope in which to create the counter.
- * @param elements The vector of mixed DynamicName and StatName
- * @return A scope named using the joined elements.
- */
-ScopePtr scopeFromElements(Scope& scope, const ElementVec& elements);
-
 /**
  * Creates a nested scope from a vector of StatNames which are used to create the
  * name.