stats: optimize StatName hot paths in symbol table (envoyproxy#44448)

Optimizations to StatName to make it more friendly for use as a key in a
map. Specifically we improve `operator==` and similar calls and clean up
some duplicate unnecessary work across the file.

(nb: if preferred i can resubmit the commits as separated PRs, do let me
know)

Key changes:
- Inline `dataSize()`, `decodeNumber()`, `encodingSizeBytes()` into the
header to make inlining easier
- Fast-path `decodeNumber()` for values <= 127 - skip loop for values
with length less than 127
- Branch-free `encodingSizeBytes()` via `std::countl_zero`
- `StatNameList::front()` + `MetricHelper::statName()` bypass - avoid
`std::function` allocation and iterator setup for the common "get first
element"
- `operator==` fast-path pointer-identity check and early size
comparison before `memcmp`; `AbslHashValue` also avoids redundant varint
decode
- `empty()` checks first byte instead of full varint decode - `0x00`
uniquely encodes zero length (`d040567`)

Cumulative benchmark results (`--benchmark_min_time=5s`):

| Benchmark | Before | After | Improvement |
|---|---|---|---|
| `BM_StatsNoTls` | 5697 us | 5077 us | **-10.9%** |
| `BM_StatsWithTls` | 6023 us | 5453 us | **-9.5%** |
| `BM_StatsWithTlsAndRejectionsWithDot` | 2889 us | 2715 us | **-6.1%**
|
| `BM_StatsWithTlsAndRejectionsWithoutDot` | 5486 us | 4866 us |
**-11.3%** |
| `bmCreateRace` | 49.2 ms | 49.7 ms | ~flat |
| `bmJoinStatNames` | 101 ns | 87.6 ns | **-13.3%** |
| `bmJoinElements` | 233 ns | 208 ns | **-10.7%** |
| `bmCompareElements` | 34.3 ns | 33.5 ns | **-2.3%** |
| `bmSortByStatNames` | 49976 us | 50547 us | ~flat |
| `bmStdSort` | 60993 us | 56058 us | **-8.1%** |
| `bmStatNameMapLookup` | 22.4 ns | 8.52 ns | **-62.0%** |
| `bmStatNameMapLookupDifferentPointer` | 22.4 ns | 11.8 ns | **-47.3%**
|
| `bmStatNameMapLookupMiss` | 12.1 ns | 6.70 ns | **-44.6%** |
| `bmSortStrings` | 20443 us | 20488 us | ~flat |
| `bmSetStrings` | 38079 us | 38100 us | ~flat |

Optimizations were developed and benchmarked incrementally (see
individual commit messages). The map-lookup benchmarks were added in the
first commit to establish a baseline.

Generative AI (Claude) was used as a development aid for this change. I
understand all code in the PR.

Risk Level: Medium - all changes are to internal implementation details
with no API or behavioral changes. The varint encoding format is
unchanged. But, it is a core component.

Testing:
- Existing `bazel test //test/common/stats/...` passes incl config=asan.
- New `StatNameEqualityFastPaths` test covers pointer-identity,
null-vs-zero-length, and null-vs-non-empty cases
- Benchmarked with `symbol_table_benchmark` and
`thread_local_store_speed_test`

Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

---------

Signed-off-by: Jason Koch <jkoch@netflix.com>

Author: jasonk000

source/common/stats/metric_impl.cc

@@ -32,33 +32,9 @@ MetricHelper::MetricHelper(StatName name, StatName tag_extracted_name,
   symbol_table.populateList(names.begin(), num_names, stat_names_);
 }
 
-StatName MetricHelper::statName() const {
-  StatName stat_name;
-  stat_names_.iterate([&stat_name](StatName s) -> bool {
-    stat_name = s;
-    return false; // Returning 'false' stops the iteration.
-  });
-  return stat_name;
-}
+StatName MetricHelper::statName() const { return stat_names_.at(0); }
 
-StatName MetricHelper::tagExtractedStatName() const {
-  // The name is the first element in stat_names_. The second is the
-  // tag-extracted-name. We don't have random access in that format,
-  // so we iterate through them, skipping the first element (name),
-  // and terminating the iteration after capturing the tag-extracted
-  // name by returning false from the lambda.
-  StatName tag_extracted_stat_name;
-  bool skip = true;
-  stat_names_.iterate([&tag_extracted_stat_name, &skip](StatName s) -> bool {
-    if (skip) {
-      skip = false;
-      return true;
-    }
-    tag_extracted_stat_name = s;
-    return false; // Returning 'false' stops the iteration.
-  });
-  return tag_extracted_stat_name;
-}
+StatName MetricHelper::tagExtractedStatName() const { return stat_names_.at(1); }
 
 void MetricHelper::iterateTagStatNames(const Metric::TagStatNameIterFn& fn) const {
   enum { Name, TagExtractedName, TagName, TagValue } state = Name;

source/common/stats/symbol_table.cc

@@ -14,13 +14,6 @@
 namespace Envoy {
 namespace Stats {
 
-// Masks used for variable-length encoding of arbitrary-sized integers into a
-// uint8-array. The integers are typically small, so we try to store them in as
-// few bytes as possible. The bottom 7 bits hold values, and the top bit is used
-// to determine whether another byte is needed for more data.
-static constexpr uint32_t SpilloverMask = 0x80;
-static constexpr uint32_t Low7Bits = 0x7f;
-
 // When storing Symbol arrays, we disallow Symbol 0, which is the only Symbol
 // that will decode into uint8_t array starting (and ending) with {0}. Thus we
 // can use a leading 0 in the first byte to indicate that what follows is
@@ -31,13 +24,6 @@ static constexpr uint32_t Low7Bits = 0x7f;
 static constexpr Symbol FirstValidSymbol = 1;
 static constexpr uint8_t LiteralStringIndicator = 0;
 
-size_t StatName::dataSize() const {
-  if (size_and_data_ == nullptr) {
-    return 0;
-  }
-  return SymbolTable::Encoding::decodeNumber(size_and_data_).first;
-}
-
 #ifndef ENVOY_CONFIG_COVERAGE
 void StatName::debugPrint() {
   // TODO(jmarantz): capture this functionality (always prints regardless of
@@ -105,17 +91,6 @@ void SymbolTable::Encoding::addSymbols(const std::vector<Symbol>& symbols) {
   }
 }
 
-std::pair<uint64_t, size_t> SymbolTable::Encoding::decodeNumber(const uint8_t* encoding) {
-  uint64_t number = 0;
-  uint64_t uc = SpilloverMask;
-  const uint8_t* start = encoding;
-  for (uint32_t shift = 0; (uc & SpilloverMask) != 0; ++encoding, shift += 7) {
-    uc = static_cast<uint32_t>(*encoding);
-    number |= (uc & Low7Bits) << shift;
-  }
-  return std::make_pair(number, encoding - start);
-}
-
 SymbolVec SymbolTable::Encoding::decodeSymbols(StatName stat_name) {
   SymbolVec symbol_vec;
   symbol_vec.reserve(stat_name.dataSize());

source/common/stats/symbol_table.h

@@ -192,8 +192,31 @@ class SymbolTable final {
      *
      * @param The encoded byte array, written previously by appendEncoding.
      * @return A pair containing the decoded number, and the number of bytes consumed from encoding.
+     *
+     * Defined inline in the header to allow the compiler to inline on hot paths.
      */
-    static std::pair<uint64_t, size_t> decodeNumber(const uint8_t* encoding);
+    static std::pair<uint64_t, size_t> decodeNumber(const uint8_t* encoding) {
+      // fast-path for the case 127 or less
+      if (encoding[0] < SpilloverMask) {
+        return {encoding[0], 1};
+      }
+
+      uint64_t number = 0;
+      uint64_t uc = SpilloverMask;
+      const uint8_t* start = encoding;
+      for (uint32_t shift = 0; (uc & SpilloverMask) != 0; ++encoding, shift += 7) {
+        uc = static_cast<uint32_t>(*encoding);
+        number |= (uc & Low7Bits) << shift;
+      }
+      return std::make_pair(number, encoding - start);
+    }
+
+    // Masks used for variable-length encoding of arbitrary-sized integers into a
+    // uint8-array. The integers are typically small, so we try to store them in as
+    // few bytes as possible. The bottom 7 bits hold values, and the top bit is used
+    // to determine whether another byte is needed for more data.
+    static constexpr uint32_t SpilloverMask = 0x80;
+    static constexpr uint32_t Low7Bits = 0x7f;
 
   private:
     friend class StatName;
@@ -587,6 +610,14 @@ class StatName {
   uint64_t hash() const { return absl::Hash<StatName>()(*this); }
 
   bool operator==(const StatName& rhs) const {
+    if (size_and_data_ == rhs.size_and_data_) {
+      return true;
+    }
+
+    if (size_and_data_ == nullptr || rhs.size_and_data_ == nullptr) {
+      return empty() && rhs.empty();
+    }
+
     return dataAsStringView() == rhs.dataAsStringView();
   }
   bool operator!=(const StatName& rhs) const { return !(*this == rhs); }
@@ -595,7 +626,7 @@ class StatName {
    * @return size_t the number of bytes in the symbol array, excluding the
    *                overhead for the size itself.
    */
-  size_t dataSize() const;
+  size_t dataSize() const { return dataAsStringView().size(); }
 
   /**
    * @return size_t the number of bytes in the symbol array, including the
@@ -624,7 +655,8 @@ class StatName {
    * @param mem_block_builder the builder to receive the storage.
    */
   void appendDataToMemBlock(MemBlockBuilder<uint8_t>& storage) {
-    storage.appendData(absl::MakeSpan(data(), dataSize()));
+    auto sv = dataAsStringView();
+    storage.appendData(absl::MakeSpan(reinterpret_cast<const uint8_t*>(sv.data()), sv.size()));
   }
 
 #ifndef ENVOY_CONFIG_COVERAGE
@@ -635,18 +667,19 @@ class StatName {
    * @return A pointer to the first byte of data (skipping over size bytes).
    */
   const uint8_t* data() const {
-    if (size_and_data_ == nullptr) {
-      return nullptr;
-    }
-    return size_and_data_ + SymbolTable::Encoding::encodingSizeBytes(dataSize());
+    return reinterpret_cast<const uint8_t*>(dataAsStringView().data());
   }
 
   const uint8_t* dataIncludingSize() const { return size_and_data_; }
 
   /**
    * @return whether this is empty.
    */
-  bool empty() const { return size_and_data_ == nullptr || dataSize() == 0; }
+  bool empty() const {
+    // Avoid a full varint decode: it is sufficient to know the first byte,
+    // since 0x00 uniquely encodes zero
+    return size_and_data_ == nullptr || size_and_data_[0] == 0;
+  }
 
   /**
    * Determines whether this starts with the prefix. Note: dynamic segments
@@ -660,13 +693,16 @@ class StatName {
 
 private:
   /**
-   * Casts the raw data as a string_view. Note that this string_view will not
-   * be in human-readable form, but it will be compatible with a string-view
-   * hasher and comparator.
+   * Casts the raw data as a string_view, decoding the varint length prefix.
+   * All accessors that need the data pointer and/or size should delegate to
+   * this method so the decode happens in exactly one place.
    */
   absl::string_view dataAsStringView() const {
-    return {reinterpret_cast<const char*>(data()),
-            static_cast<absl::string_view::size_type>(dataSize())};
+    if (size_and_data_ == nullptr) {
+      return {};
+    }
+    const auto [data_size, prefix_size] = SymbolTable::Encoding::decodeNumber(size_and_data_);
+    return {reinterpret_cast<const char*>(size_and_data_ + prefix_size), data_size};
   }
 
   const uint8_t* size_and_data_{nullptr};
@@ -863,6 +899,27 @@ class StatNameList {
    */
   void clear(SymbolTable& symbol_table);
 
+  /**
+   * @return the number of StatNames in the list, or 0 if not populated.
+   */
+  uint32_t size() const { return populated() ? storage_[0] : 0; }
+
+  /**
+   * @return the StatName at the given index. This is O(index): each element
+   *         has a variable-length encoding, so callers that need to visit
+   *         many elements should prefer iterate().
+   *
+   * Requires: populated() && index < size().
+   */
+  StatName at(uint32_t index) const {
+    ASSERT(populated() && index < storage_[0]);
+    const uint8_t* p = &storage_[1];
+    for (uint32_t i = 0; i < index; ++i) {
+      p += StatName(p).size();
+    }
+    return StatName(p);
+  }
+
 private:
   friend class SymbolTable;