diff --git a/envoy/stats/symbol_table.h b/envoy/stats/symbol_table.h index e7f171b6f8c91..e7fcb3526b487 100644 --- a/envoy/stats/symbol_table.h +++ b/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; diff --git a/source/common/stats/allocator_impl.cc b/source/common/stats/allocator_impl.cc index 3c64b6ba49511..8418bfe4eddb7 100644 --- a/source/common/stats/allocator_impl.cc +++ b/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 StatsSharedImpl : public MetricImpl { public: StatsSharedImpl(StatName name, AllocatorImpl& alloc, StatName tag_extracted_name, diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 3b29f8b5d4df5..22cceb93959c5 100644 --- a/source/common/stats/thread_local_store.cc +++ b/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* 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* 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(); diff --git a/source/common/stats/utility.cc b/source/common/stats/utility.cc index a4e5574713e23..69fe60d516ba1 100644 --- a/source/common/stats/utility.cc +++ b/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())); diff --git a/source/common/stats/utility.h b/source/common/stats/utility.h index 2ef817b61c171..5c067f628f103 100644 --- a/source/common/stats/utility.h +++ b/source/common/stats/utility.h @@ -63,22 +63,6 @@ std::string sanitizeStatsName(absl::string_view name); */ absl::optional 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.