Speedups to FakeSymbolTableTest based on microbenchmarks from envoypr…

…oxy#6161.

Signed-off-by: Joshua Marantz <[email protected]>

include/envoy/stats/BUILD

@@ -31,6 +31,9 @@ envoy_cc_library(
 envoy_cc_library(
     name = "symbol_table_interface",
     hdrs = ["symbol_table.h"],
+    deps = [
+        "//source/common/common:hash_lib",
+    ],
 )
 
 envoy_cc_library(

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.
    */
@@ -130,11 +107,39 @@ 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.
+   * @param num_names The number of names.
+   * @param symbol_table The symbol table in which to encode the names.
+   */
+  virtual void populateList(absl::string_view* names, int32_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 any benefit
+   * from the improved representation.
+   *
+   * @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;
 
@@ -158,6 +163,8 @@ class SymbolTable {
    * @param stat_name the stat name.
    */
   virtual void incRefCount(const StatName& stat_name) PURE;
+
+  virtual StoragePtr copyToBytes(absl::string_view name) PURE;
 };
 
 using SharedSymbolTable = std::shared_ptr<SymbolTable>;

source/common/stats/BUILD

@@ -130,6 +130,7 @@ envoy_cc_library(
         "//include/envoy/stats:symbol_table_interface",
         "//source/common/common:assert_lib",
         "//source/common/common:logger_lib",
+        "//source/common/common:stack_array",
         "//source/common/common:thread_lib",
         "//source/common/common:utility_lib",
     ],

source/common/stats/fake_symbol_table_impl.h

@@ -49,7 +49,42 @@ namespace Stats {
  */
 class FakeSymbolTableImpl : public SymbolTable {
 public:
-  SymbolEncoding encode(absl::string_view name) override { return encodeHelper(name); }
+  // SymbolTable
+  void populateList(absl::string_view* names, int32_t num_names, StatNameList& list) override {
+    // This implementation of populateList is similar to
+    // SymboLableImpl::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.
+
+    RELEASE_ASSERT(num_names < 256, "Maximum number elements in a StatNameList exceeded");
+
+    // First encode all the names.
+    size_t total_size_bytes = 1 + num_names * StatNameSizeEncodingBytes;
+
+    for (int32_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<uint8_t[]>(total_size_bytes);
+    uint8_t* p = &storage[0];
+    *p++ = num_names;
+    for (int32_t i = 0; i < num_names; ++i) {
+      auto& name = names[i];
+      size_t sz = name.size();
+      p = saveLengthToBytesReturningNext(sz, p);
+      if (!name.empty()) {
+        memcpy(p, name.data(), sz * sizeof(uint8_t));
+        p += sz;
+      }
+    }
+    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));
@@ -75,22 +110,38 @@ class FakeSymbolTableImpl : public SymbolTable {
   void debugPrint() const override {}
 #endif
 
+  StoragePtr copyToBytes(absl::string_view name) override {
+    auto bytes = std::make_unique<uint8_t[]>(name.size() + StatNameSizeEncodingBytes);
+    uint8_t* buffer = saveLengthToBytesReturningNext(name.size(), bytes.get());
+    memcpy(buffer, name.data(), name.size());
+    return bytes;
+  }
+
+  void callWithStringView(StatName stat_name,
+                          const std::function<void(absl::string_view)>& fn) const override {
+    fn(toStringView(stat_name));
+  }
+
 private:
-  SymbolEncoding encodeHelper(absl::string_view name) const {
-    SymbolEncoding encoding;
-    encoding.addStringForFakeSymbolTable(name);
-    return encoding;
+  // Saves the specified length into the byte array, returning the next byte.
+  // There is no guarantee that bytes will be aligned, so we can't cast to a
+  // uint16_t* and assign, but must individually copy the bytes.
+  static uint8_t* saveLengthToBytesReturningNext(uint64_t length, uint8_t* bytes) {
+    ASSERT(length < StatNameMaxSize);
+    *bytes++ = length & 0xff;
+    *bytes++ = length >> 8;
+    return bytes;
   }
 
   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());
-    return bytes;
+  StoragePtr stringToStorage(absl::string_view name) const {
+    auto storage = std::make_unique<Storage>(name.size() + 2);
+    uint8_t* p = saveLengthToBytesReturningNext(name.size(), storage.get());
+    memcpy(p, name.data(), name.size());
+    return storage;
   }
 };
 

source/common/stats/symbol_table_impl.cc

@@ -30,7 +30,7 @@ void StatName::debugPrint() {
     for (uint64_t i = 0; i < nbytes; ++i) {
       absl::StrAppend(&msg, " ", static_cast<uint64_t>(data()[i]));
     }
-    SymbolVec encoding = SymbolEncoding::decodeSymbols(data(), dataSize());
+    SymbolVec encoding = SymbolTableImpl::Encoding::decodeSymbols(data(), dataSize());
     absl::StrAppend(&msg, ", numSymbols=", encoding.size(), ":");
     for (Symbol symbol : encoding) {
       absl::StrAppend(&msg, " ", symbol);
@@ -40,9 +40,9 @@ void StatName::debugPrint() {
 }
 #endif
 
-SymbolEncoding::~SymbolEncoding() { ASSERT(vec_.empty()); }
+SymbolTableImpl::Encoding::~Encoding() { ASSERT(vec_.empty()); }
 
-void SymbolEncoding::addSymbol(Symbol symbol) {
+void SymbolTableImpl::Encoding::addSymbol(Symbol symbol) {
   // UTF-8-like encoding where a value 127 or less gets written as a single
   // byte. For higher values we write the low-order 7 bits with a 1 in
   // the high-order bit. Then we right-shift 7 bits and keep adding more bytes
@@ -60,14 +60,8 @@ void SymbolEncoding::addSymbol(Symbol symbol) {
   } while (symbol != 0);
 }
 
-void SymbolEncoding::addStringForFakeSymbolTable(absl::string_view str) {
-  if (!str.empty()) {
-    vec_.resize(str.size());
-    memcpy(&vec_[0], str.data(), str.size());
-  }
-}
-
-SymbolVec SymbolEncoding::decodeSymbols(const SymbolTable::Storage array, uint64_t size) {
+SymbolVec SymbolTableImpl::Encoding::decodeSymbols(const SymbolTable::Storage array,
+                                                   uint64_t size) {
   SymbolVec symbol_vec;
   Symbol symbol = 0;
   for (uint32_t shift = 0; size > 0; --size, ++array) {
@@ -98,7 +92,7 @@ static inline uint8_t* saveLengthToBytesReturningNext(uint64_t length, uint8_t*
   return bytes;
 }
 
-uint64_t SymbolEncoding::moveToStorage(SymbolTable::Storage symbol_array) {
+uint64_t SymbolTableImpl::Encoding::moveToStorage(SymbolTable::Storage symbol_array) {
   uint64_t sz = size();
   symbol_array = saveLengthToBytesReturningNext(sz, symbol_array);
   if (sz != 0) {
@@ -123,11 +117,9 @@ SymbolTableImpl::~SymbolTableImpl() {
 // TODO(ambuc): There is a possible performance optimization here for avoiding
 // the encoding of IPs / numbers if they appear in stat names. We don't want to
 // waste time symbolizing an integer as an integer, if we can help it.
-SymbolEncoding SymbolTableImpl::encode(const absl::string_view name) {
-  SymbolEncoding encoding;
-
+void SymbolTableImpl::encode(const absl::string_view name, Encoding& encoding) {
   if (name.empty()) {
-    return encoding;
+    return;
   }
 
   // We want to hold the lock for the minimum amount of time, so we do the
@@ -149,7 +141,6 @@ SymbolEncoding SymbolTableImpl::encode(const absl::string_view name) {
   for (Symbol symbol : symbols) {
     encoding.addSymbol(symbol);
   }
-  return encoding;
 }
 
 uint64_t SymbolTableImpl::numSymbols() const {
@@ -159,7 +150,13 @@ uint64_t SymbolTableImpl::numSymbols() const {
 }
 
 std::string SymbolTableImpl::toString(const StatName& stat_name) const {
-  return decodeSymbolVec(SymbolEncoding::decodeSymbols(stat_name.data(), stat_name.dataSize()));
+  return decodeSymbolVec(
+      SymbolTableImpl::Encoding::decodeSymbols(stat_name.data(), stat_name.dataSize()));
+}
+
+void SymbolTableImpl::callWithStringView(StatName stat_name,
+                                         const std::function<void(absl::string_view)>& fn) const {
+  fn(toString(stat_name));
 }
 
 std::string SymbolTableImpl::decodeSymbolVec(const SymbolVec& symbols) const {
@@ -177,7 +174,8 @@ std::string SymbolTableImpl::decodeSymbolVec(const SymbolVec& symbols) const {
 
 void SymbolTableImpl::incRefCount(const StatName& stat_name) {
   // Before taking the lock, decode the array of symbols from the SymbolTable::Storage.
-  SymbolVec symbols = SymbolEncoding::decodeSymbols(stat_name.data(), stat_name.dataSize());
+  SymbolVec symbols =
+      SymbolTableImpl::Encoding::decodeSymbols(stat_name.data(), stat_name.dataSize());
 
   Thread::LockGuard lock(lock_);
   for (Symbol symbol : symbols) {
@@ -193,7 +191,8 @@ void SymbolTableImpl::incRefCount(const StatName& stat_name) {
 
 void SymbolTableImpl::free(const StatName& stat_name) {
   // Before taking the lock, decode the array of symbols from the SymbolTable::Storage.
-  SymbolVec symbols = SymbolEncoding::decodeSymbols(stat_name.data(), stat_name.dataSize());
+  SymbolVec symbols =
+      SymbolTableImpl::Encoding::decodeSymbols(stat_name.data(), stat_name.dataSize());
 
   Thread::LockGuard lock(lock_);
   for (Symbol symbol : symbols) {
@@ -265,8 +264,8 @@ bool SymbolTableImpl::lessThan(const StatName& a, const StatName& b) const {
   // If this becomes a performance bottleneck (e.g. during sorting), we could
   // provide an iterator-like interface for incrementally decoding the symbols
   // without allocating memory.
-  SymbolVec av = SymbolEncoding::decodeSymbols(a.data(), a.dataSize());
-  SymbolVec bv = SymbolEncoding::decodeSymbols(b.data(), b.dataSize());
+  SymbolVec av = SymbolTableImpl::Encoding::decodeSymbols(a.data(), a.dataSize());
+  SymbolVec bv = SymbolTableImpl::Encoding::decodeSymbols(b.data(), b.dataSize());
 
   // Calling fromSymbol requires holding the lock, as it needs read-access to
   // the maps that are written when adding new symbols.
@@ -296,12 +295,17 @@ void SymbolTableImpl::debugPrint() const {
 }
 #endif
 
-StatNameStorage::StatNameStorage(absl::string_view name, SymbolTable& table) {
-  SymbolEncoding encoding = table.encode(name);
-  bytes_ = std::make_unique<uint8_t[]>(encoding.bytesRequired());
-  encoding.moveToStorage(bytes_.get());
+SymbolTable::StoragePtr SymbolTableImpl::copyToBytes(absl::string_view name) {
+  Encoding encoding;
+  encode(name, encoding);
+  auto bytes = std::make_unique<uint8_t[]>(encoding.bytesRequired());
+  encoding.moveToStorage(bytes.get());
+  return bytes;
 }
 
+StatNameStorage::StatNameStorage(absl::string_view name, SymbolTable& table)
+    : bytes_(table.copyToBytes(name)) {}
+
 StatNameStorage::StatNameStorage(StatName src, SymbolTable& table) {
   uint64_t size = src.size();
   bytes_ = std::make_unique<uint8_t[]>(size);
@@ -337,34 +341,34 @@ SymbolTable::StoragePtr SymbolTableImpl::join(const std::vector<StatName>& stat_
   return bytes;
 }
 
-StatNameList::~StatNameList() { ASSERT(!populated()); }
-
-void StatNameList::populate(const std::vector<absl::string_view>& names,
-                            SymbolTable& symbol_table) {
-  RELEASE_ASSERT(names.size() < 256, "Maximum number elements in a StatNameList exceeded");
+void SymbolTableImpl::populateList(absl::string_view* names, int32_t num_names,
+                                   StatNameList& list) {
+  RELEASE_ASSERT(num_names < 256, "Maximum number elements in a StatNameList exceeded");
 
   // First encode all the names.
   size_t total_size_bytes = 1; /* one byte for holding the number of names */
-  std::vector<SymbolEncoding> encodings;
-  encodings.resize(names.size());
-  int index = 0;
-  for (auto& name : names) {
-    SymbolEncoding encoding = symbol_table.encode(name);
+
+  STACK_ARRAY(encodings, Encoding, num_names);
+  for (int32_t i = 0; i < num_names; ++i) {
+    Encoding& encoding = encodings[i];
+    encode(names[i], encoding);
     total_size_bytes += encoding.bytesRequired();
-    encodings[index++].swap(encoding);
   }
 
   // Now allocate the exact number of bytes required and move the encodings
   // into storage.
-  storage_ = std::make_unique<uint8_t[]>(total_size_bytes);
-  uint8_t* p = &storage_[0];
-  *p++ = encodings.size();
+  auto storage = std::make_unique<uint8_t[]>(total_size_bytes);
+  uint8_t* p = &storage[0];
+  *p++ = num_names;
   for (auto& encoding : encodings) {
     p += encoding.moveToStorage(p);
   }
-  ASSERT(p == &storage_[0] + total_size_bytes);
+  ASSERT(p == &storage[0] + total_size_bytes);
+  list.moveStorageIntoList(std::move(storage));
 }
 
+StatNameList::~StatNameList() { ASSERT(!populated()); }
+
 void StatNameList::iterate(const std::function<bool(StatName)>& f) const {
   uint8_t* p = &storage_[0];
   uint32_t num_elements = *p++;