Speedups to FakeSymbolTableTest based on microbenchmarks from #6161.

include/envoy/stats/symbol_table.h

@@ -1,5 +1,6 @@
 #pragma once
 
+#include <functional>
 #include <memory>
 #include <vector>
 
@@ -19,15 +20,7 @@ namespace Stats {
  */
 class StatName;
 
-/**
- * Intermediate representation for a stat-name. This helps store multiple names
- * in a single packed allocation. First we encode each desired name, then sum
- * their sizes for the single packed allocation. This is used to store
- * MetricImpl's tags and tagExtractedName. Like StatName, we don't want to pay
- * a vptr overhead per object, and the representation is shared between the
- * SymbolTable implementations, so this is just a pre-declare.
- */
-class SymbolEncoding;
+class StatNameList;
 
 /**
  * SymbolTable manages a namespace optimized for stat names, exploiting their
@@ -59,22 +52,6 @@ class SymbolTable {
 
   virtual ~SymbolTable() = default;
 
-  /**
-   * Encodes a stat name using the symbol table, returning a SymbolEncoding. The
-   * SymbolEncoding is not intended for long-term storage, but is used to help
-   * allocate a StatName with the correct amount of storage.
-   *
-   * When a name is encoded, it bumps reference counts held in the table for
-   * each symbol. The caller is responsible for creating a StatName using this
-   * SymbolEncoding and ultimately disposing of it by calling
-   * SymbolTable::free(). Users are protected from leaking symbols into the pool
-   * by ASSERTions in the SymbolTable destructor.
-   *
-   * @param name The name to encode.
-   * @return SymbolEncoding the encoded symbols.
-   */
-  virtual SymbolEncoding encode(absl::string_view name) PURE;
-
   /**
    * @return uint64_t the number of symbols in the symbol table.
    */
@@ -116,9 +93,9 @@ class SymbolTable {
    * decode/encode into the elaborated form, and does not require locking the
    * SymbolTable.
    *
-   * The caveat is that this representation does not bump reference counts on
-   * the referenced Symbols in the SymbolTable, so it's only valid as long for
-   * the lifetime of the joined StatNames.
+   * Note that this method does not bump reference counts on the referenced
+   * Symbols in the SymbolTable, so it's only valid as long for the lifetime of
+   * the joined StatNames.
    *
    * This is intended for use doing cached name lookups of scoped stats, where
    * the scope prefix and the names to combine it with are already in StatName
@@ -130,14 +107,50 @@ class SymbolTable {
    */
   virtual StoragePtr join(const std::vector<StatName>& stat_names) const PURE;
 
+  /**
+   * Populates a StatNameList from a list of encodings. This is not done at
+   * construction time to enable StatNameList to be instantiated directly in
+   * a class that doesn't have a live SymbolTable when it is constructed.
+   *
+   * @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.
+   */
+  virtual void populateList(const absl::string_view* names, uint32_t num_names,
+                            StatNameList& list) PURE;
+
 #ifndef ENVOY_CONFIG_COVERAGE
   virtual void debugPrint() const PURE;
 #endif
 
+  /**
+   * Calls the provided function with a string-view representation of the
+   * elaborated name. This is useful during the interim period when we
+   * are using FakeSymbolTableImpl, to avoid an extra allocation. Once
+   * we migrate to using SymbolTableImpl, this interface will no longer
+   * be helpful and can be removed. The reason it's useful now is that
+   * it makes up, in part, for some extra runtime overhead that is spent
+   * on the SymbolTable abstraction and API, without getting full benefit
+   * from the improved representation.
+   *
+   * TODO(#6307): Remove this when the transition from FakeSymbolTableImpl to
+   * SymbolTableImpl is complete.
+   *
+   * @param stat_name The stat name.
+   * @param fn The function to call with the elaborated stat name as a string_view.
+   */
+  virtual void callWithStringView(StatName stat_name,
+                                  const std::function<void(absl::string_view)>& fn) const PURE;
+
 private:
+  friend struct HeapStatData;
   friend class StatNameStorage;
   friend class StatNameList;
 
+  // The following methods are private, but are called by friend classes
+  // StatNameStorage and StatNameList, which must be friendly with SymbolTable
+  // in order to manage the reference-counted symbols they own.
+
   /**
    * Since SymbolTable does manual reference counting, a client of SymbolTable
    * must manually call free(symbol_vec) when it is freeing the backing store
@@ -158,6 +171,17 @@ class SymbolTable {
    * @param stat_name the stat name.
    */
   virtual void incRefCount(const StatName& stat_name) PURE;
+
+  /**
+   * Encodes 'name' into the symbol table. Bumps reference counts for referenced
+   * symbols. The caller must manage the storage, and is responsible for calling
+   * SymbolTable::free() to release the reference counts.
+   *
+   * @param name The name to encode.
+   * @return The encoded name, transferring ownership to the caller.
+   *
+   */
+  virtual StoragePtr encode(absl::string_view name) PURE;
 };
 
 using SharedSymbolTable = std::shared_ptr<SymbolTable>;

source/common/stats/fake_symbol_table_impl.h

@@ -44,12 +44,57 @@ namespace Stats {
  * that backs each StatName, so there is no sharing or memory savings, but also
  * no state associated with the SymbolTable, and thus no locks needed.
  *
- * TODO(jmarantz): delete this class once SymbolTable is fully deployed in the
+ * TODO(#6307): delete this class once SymbolTable is fully deployed in the
  * Envoy codebase.
  */
 class FakeSymbolTableImpl : public SymbolTable {
 public:
-  SymbolEncoding encode(absl::string_view name) override { return encodeHelper(name); }
+  // SymbolTable
+  void populateList(const absl::string_view* names, uint32_t num_names,
+                    StatNameList& list) override {
+    // This implementation of populateList is similar to
+    // SymbolTableImpl::populateList. This variant is more efficient for
+    // FakeSymbolTableImpl, because it avoid "encoding" each name in names. The
+    // strings are laid out abutting each other with 2-byte length prefixes, so
+    // encoding isn't needed, and doing a dummy encoding step would cost one
+    // memory allocation per element, adding significant overhead as measured by
+    // thread_local_store_speed_test.
+
+    // We encode the number of names in a single byte, thus there must be less
+    // than 256 of them.
+    RELEASE_ASSERT(num_names < 256, "Maximum number elements in a StatNameList exceeded");
+
+    // First encode all the names. The '1' here represents the number of
+    // names. The num_names * StatNameSizeEncodingBytes reserves space for the
+    // lengths of each name.
+    size_t total_size_bytes = 1 + num_names * StatNameSizeEncodingBytes;
+
+    for (uint32_t i = 0; i < num_names; ++i) {
+      total_size_bytes += names[i].size();
+    }
+
+    // Now allocate the exact number of bytes required and move the encodings
+    // into storage.
+    auto storage = std::make_unique<Storage>(total_size_bytes);
+    uint8_t* p = &storage[0];
+    *p++ = num_names;
+    for (uint32_t i = 0; i < num_names; ++i) {
+      auto& name = names[i];
+      size_t sz = name.size();
+      p = SymbolTableImpl::writeLengthReturningNext(sz, p);
+      if (!name.empty()) {
+        memcpy(p, name.data(), sz * sizeof(uint8_t));
+        p += sz;
+      }
+    }
+
+    // This assertion double-checks the arithmetic where we computed
+    // total_size_bytes. After appending all the encoded data into the
+    // allocated byte array, we should wind up with a pointer difference of
+    // total_size_bytes from the beginning of the allocation.
+    ASSERT(p == &storage[0] + total_size_bytes);
+    list.moveStorageIntoList(std::move(storage));
+  }
 
   std::string toString(const StatName& stat_name) const override {
     return std::string(toStringView(stat_name));
@@ -60,6 +105,7 @@ class FakeSymbolTableImpl : public SymbolTable {
   }
   void free(const StatName&) override {}
   void incRefCount(const StatName&) override {}
+  StoragePtr encode(absl::string_view name) override { return encodeHelper(name); }
   SymbolTable::StoragePtr join(const std::vector<StatName>& names) const override {
     std::vector<absl::string_view> strings;
     for (StatName name : names) {
@@ -68,28 +114,27 @@ class FakeSymbolTableImpl : public SymbolTable {
         strings.push_back(str);
       }
     }
-    return stringToStorage(absl::StrJoin(strings, "."));
+    return encodeHelper(absl::StrJoin(strings, "."));
   }
 
 #ifndef ENVOY_CONFIG_COVERAGE
   void debugPrint() const override {}
 #endif
 
-private:
-  SymbolEncoding encodeHelper(absl::string_view name) const {
-    SymbolEncoding encoding;
-    encoding.addStringForFakeSymbolTable(name);
-    return encoding;
+  void callWithStringView(StatName stat_name,
+                          const std::function<void(absl::string_view)>& fn) const override {
+    fn(toStringView(stat_name));
   }
 
+private:
   absl::string_view toStringView(const StatName& stat_name) const {
     return {reinterpret_cast<const char*>(stat_name.data()), stat_name.dataSize()};
   }
 
-  SymbolTable::StoragePtr stringToStorage(absl::string_view name) const {
-    SymbolEncoding encoding = encodeHelper(name);
-    auto bytes = std::make_unique<uint8_t[]>(encoding.bytesRequired());
-    encoding.moveToStorage(bytes.get());
+  StoragePtr encodeHelper(absl::string_view name) const {
+    auto bytes = std::make_unique<Storage>(name.size() + StatNameSizeEncodingBytes);
+    uint8_t* buffer = SymbolTableImpl::writeLengthReturningNext(name.size(), bytes.get());
+    memcpy(buffer, name.data(), name.size());
     return bytes;
   }
 };