[DRAFT] Refcount stats across scopes

source/common/common/BUILD

@@ -179,8 +179,8 @@ envoy_cc_library(
 )
 
 envoy_cc_library(
-    name = "shared_memory_hash_set_lib",
-    hdrs = ["shared_memory_hash_set.h"],
+    name = "block_memory_hash_set_lib",
+    hdrs = ["block_memory_hash_set.h"],
     deps = [
         ":assert_lib",
         ":logger_lib",

source/common/common/shared_memory_hash_set.h → source/common/common/block_memory_hash_set.h

@@ -15,18 +15,18 @@
 namespace Envoy {
 
 /**
- * Initialization parameters for SharedMemoryHashSet. The options are duplicated
+ * Initialization parameters for BlockMemoryHashSet. The options are duplicated
  * to the control-block after init, to aid with sanity checking when attaching
  * an existing memory segment.
  */
-struct SharedMemoryHashSetOptions {
+struct BlockMemoryHashSetOptions {
   std::string toString() const {
     return fmt::format("capacity={}, num_slots={}", capacity, num_slots);
   }
-  bool operator==(const SharedMemoryHashSetOptions& that) const {
+  bool operator==(const BlockMemoryHashSetOptions& that) const {
     return capacity == that.capacity && num_slots == that.num_slots;
   }
-  bool operator!=(const SharedMemoryHashSetOptions& that) const { return !(*this == that); }
+  bool operator!=(const BlockMemoryHashSetOptions& that) const { return !(*this == that); }
 
   uint32_t capacity;  // how many values can be stored.
   uint32_t num_slots; // determines speed of hash vs size efficiency.
@@ -45,7 +45,7 @@ struct SharedMemoryHashSetOptions {
  * term storage, but not across machine architectures, as it doesn't
  * use network byte order for storing ints.
  */
-template <class Value> class SharedMemoryHashSet : public Logger::Loggable<Logger::Id::config> {
+template <class Value> class BlockMemoryHashSet : public Logger::Loggable<Logger::Id::config> {
 public:
   /**
    * Sentinal used for next_cell links to indicate end-of-list.
@@ -58,31 +58,31 @@ template <class Value> class SharedMemoryHashSet : public Logger::Loggable<Logge
   /**
    * Constructs a map control structure given a set of options, which cannot be changed.
    * @param options describes the parameters comtrolling set layout.
-   * @param init true if the shared-memory should be initialized on construction. If false,
+   * @param init true if the memory should be initialized on construction. If false,
    *             the data in the table will be sanity checked, and an exception thrown if
    *             it is incoherent or mismatches the passed-in options.
    * @param memory the memory buffer for the set data.
    *
    * Note that no locking of any kind is done by this class; this must be done at the
    * call-site to support concurrent access.
    */
-  SharedMemoryHashSet(const SharedMemoryHashSetOptions& options, bool init, uint8_t* memory)
+  BlockMemoryHashSet(const BlockMemoryHashSetOptions& options, bool init, uint8_t* memory)
       : cells_(nullptr), control_(nullptr), slots_(nullptr) {
     mapMemorySegments(options, memory);
     if (init) {
       initialize(options);
     } else if (!attach(options)) {
-      throw EnvoyException("SharedMemoryHashSet: Incompatible memory block");
+      throw EnvoyException("BlockMemoryHashSet: Incompatible memory block");
     }
   }
 
   /**
    * Returns the numbers of byte required for the hash-table, based on
    * the control structure. This must be used to allocate the
-   * backing-store (eg) in shared memory, which we do after
+   * backing-store (eg) in memory, which we do after
    * constructing the object with the desired sizing.
    */
-  static uint32_t numBytes(const SharedMemoryHashSetOptions& options) {
+  static uint32_t numBytes(const BlockMemoryHashSetOptions& options) {
     uint32_t size =
         cellOffset(options.capacity) + sizeof(Control) + options.num_slots * sizeof(uint32_t);
     return align(size);
@@ -93,7 +93,7 @@ template <class Value> class SharedMemoryHashSet : public Logger::Loggable<Logge
   /**
    * Returns the options structure that was used to construct the set.
    */
-  const SharedMemoryHashSetOptions& options() const { return control_->options; }
+  const BlockMemoryHashSetOptions& options() const { return control_->options; }
 
   /** Examines the data structures to see if they are sane, assert-failing on any trouble. */
   void sanityCheck() {
@@ -221,14 +221,14 @@ template <class Value> class SharedMemoryHashSet : public Logger::Loggable<Logge
   }
 
 private:
-  friend class SharedMemoryHashSetTest;
+  friend class BlockMemoryHashSetTest;
 
   /**
    * Initializes a hash-map on raw memory. No expectations are made about the state of the memory
    * coming in.
    * @param memory
    */
-  void initialize(const SharedMemoryHashSetOptions& options) {
+  void initialize(const BlockMemoryHashSetOptions& options) {
     control_->hash_signature = Value::hash(signatureStringToHash());
     control_->num_bytes = numBytes(options);
     control_->options = options;
@@ -250,18 +250,18 @@ template <class Value> class SharedMemoryHashSet : public Logger::Loggable<Logge
   }
 
   /**
-   * Attempts to attach to an existing shared memory segment. Does a (relatively) quick
+   * Attempts to attach to an existing memory segment. Does a (relatively) quick
    * sanity check to make sure the options copied to the provided memory match, and also
    * that the slot, cell, and key-string structures look sane.
    */
-  bool attach(const SharedMemoryHashSetOptions& options) {
+  bool attach(const BlockMemoryHashSetOptions& options) {
     if (numBytes(options) != control_->num_bytes) {
-      ENVOY_LOG(error, "SharedMemoryHashSet unexpected memory size {} != {}", numBytes(options),
+      ENVOY_LOG(error, "BlockMemoryHashSet unexpected memory size {} != {}", numBytes(options),
                 control_->num_bytes);
       return false;
     }
     if (Value::hash(signatureStringToHash()) != control_->hash_signature) {
-      ENVOY_LOG(error, "SharedMemoryHashSet hash signature mismatch.");
+      ENVOY_LOG(error, "BlockMemoryHashSet hash signature mismatch.");
       return false;
     }
     sanityCheck();
@@ -290,11 +290,11 @@ template <class Value> class SharedMemoryHashSet : public Logger::Loggable<Logge
    * Represents control-values for the hash-table.
    */
   struct Control {
-    SharedMemoryHashSetOptions options; // Options established at map construction time.
-    uint64_t hash_signature;            // Hash of a constant signature string.
-    uint32_t num_bytes;                 // Bytes allocated on behalf of the map.
-    uint32_t size;                      // Number of values currently stored.
-    uint32_t free_cell_index;           // Offset of first free cell.
+    BlockMemoryHashSetOptions options; // Options established at map construction time.
+    uint64_t hash_signature;           // Hash of a constant signature string.
+    uint32_t num_bytes;                // Bytes allocated on behalf of the map.
+    uint32_t size;                     // Number of values currently stored.
+    uint32_t free_cell_index;          // Offset of first free cell.
   };
 
   /**
@@ -340,8 +340,8 @@ template <class Value> class SharedMemoryHashSet : public Logger::Loggable<Logge
     return *reinterpret_cast<Cell*>(ptr);
   }
 
-  /** Maps out the segments of shared memory for us to work with. */
-  void mapMemorySegments(const SharedMemoryHashSetOptions& options, uint8_t* memory) {
+  /** Maps out the segments of memory for us to work with. */
+  void mapMemorySegments(const BlockMemoryHashSetOptions& options, uint8_t* memory) {
     // Note that we are not examining or mutating memory here, just looking at the pointer,
     // so we don't need to hold any locks.
     cells_ = reinterpret_cast<Cell*>(memory); // First because Value may need to be aligned.
@@ -351,7 +351,7 @@ template <class Value> class SharedMemoryHashSet : public Logger::Loggable<Logge
     slots_ = reinterpret_cast<uint32_t*>(memory);
   }
 
-  // Pointers into shared memory. Cells go first, because Value may need a more aggressive
+  // Pointers into memory. Cells go first, because Value may need a more aggressive
   // aligmnment.
   Cell* cells_;
   Control* control_;

source/common/stats/BUILD

@@ -17,6 +17,7 @@ envoy_cc_library(
         "//include/envoy/server:options_interface",
         "//include/envoy/stats:stats_interface",
         "//source/common/common:assert_lib",
+        "//source/common/common:block_memory_hash_set_lib",
         "//source/common/common:hash_lib",
         "//source/common/common:perf_annotation_lib",
         "//source/common/common:utility_lib",

source/common/stats/stats_impl.cc

@@ -149,13 +149,53 @@ bool TagExtractorImpl::extractTag(const std::string& stat_name, std::vector<Tag>
   return false;
 }
 
+RawStatData* BlockRawStatDataAllocator::alloc(const std::string& name) {
+  std::unique_lock<Thread::BasicLockable> lock(stat_lock_);
+
+  absl::string_view key = name;
+  if (key.size() > Stats::RawStatData::maxNameLength()) {
+    key.remove_suffix(key.size() - Stats::RawStatData::maxNameLength());
+  }
+  auto value_created = stats_set_->insert(key);
+  Stats::RawStatData* data = value_created.first;
+  if (data == nullptr) {
+    return nullptr;
+  }
+  // For new entries (value-created.second==true), BlockMemoryHashSet calls Value::initialize()
+  // automatically, but on recycled entries (value-created.second==false) we need to bump the
+  // ref-count.
+  if (!value_created.second) {
+    ++data->ref_count_;
+  }
+  return data;
+}
+
+void BlockRawStatDataAllocator::free(RawStatData& data) {
+  // We must hold the lock since the reference decrement can race with an initialize above.
+  std::unique_lock<Thread::BasicLockable> lock(stat_lock_);
+
+  ASSERT(data.ref_count_ > 0);
+  if (--data.ref_count_ > 0) {
+    return;
+  }
+  bool key_removed = stats_set_->remove(data.key());
+  ASSERT(key_removed);
+  memset(&data, 0, Stats::RawStatData::size());
+}
+
 RawStatData* HeapRawStatDataAllocator::alloc(const std::string& name) {
   // This must be zero-initialized
   RawStatData* data = static_cast<RawStatData*>(::calloc(RawStatData::size(), 1));
   data->initialize(name);
   return data;
 }
 
+void HeapRawStatDataAllocator::free(RawStatData& data) {
+  // This allocator does not ever have concurrent access to the raw data.
+  ASSERT(data.ref_count_ == 1);
+  ::free(&data);
+}
+
 TagProducerImpl::TagProducerImpl(const envoy::config::metrics::v2::StatsConfig& config)
     : TagProducerImpl() {
   // To check name conflict.
@@ -255,12 +295,6 @@ TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v2::StatsCon
   return names;
 }
 
-void HeapRawStatDataAllocator::free(RawStatData& data) {
-  // This allocator does not ever have concurrent access to the raw data.
-  ASSERT(data.ref_count_ == 1);
-  ::free(&data);
-}
-
 void RawStatData::initialize(absl::string_view key) {
   ASSERT(!initialized());
   ASSERT(key.size() <= maxNameLength());

source/common/stats/stats_impl.h

@@ -17,6 +17,7 @@
 #include "envoy/stats/stats.h"
 
 #include "common/common/assert.h"
+#include "common/common/block_memory_hash_set.h"
 #include "common/common/hash.h"
 #include "common/common/utility.h"
 #include "common/protobuf/protobuf.h"
@@ -26,6 +27,8 @@
 namespace Envoy {
 namespace Stats {
 
+typedef BlockMemoryHashSet<Stats::RawStatData> RawStatDataSet;
+
 class TagExtractorImpl : public TagExtractor {
 public:
   /**
@@ -217,19 +220,19 @@ struct RawStatData {
 
   /**
    * Returns the size of this struct, accounting for the length of name_
-   * and padding for alignment. This is required by SharedMemoryHashSet.
+   * and padding for alignment. This is required by BlockMemoryHashSet.
    */
   static size_t size();
 
   /**
    * Initializes this object to have the specified key,
    * a refcount of 1, and all other values zero. This is required by
-   * SharedMemoryHashSet.
+   * BlockMemoryHashSet.
    */
   void initialize(absl::string_view key);
 
   /**
-   * Returns a hash of the key. This is required by SharedMemoryHashSet.
+   * Returns a hash of the key. This is required by BlockMemoryHashSet.
    */
   static uint64_t hash(absl::string_view key) { return HashUtil::xxHash64(key); }
 
@@ -239,7 +242,7 @@ struct RawStatData {
   bool initialized() { return name_[0] != '\0'; }
 
   /**
-   * Returns the name as a string_view. This is required by SharedMemoryHashSet.
+   * Returns the name as a string_view. This is required by BlockMemoryHashSet.
    */
   absl::string_view key() const {
     return absl::string_view(name_, strnlen(name_, maxNameLength()));
@@ -370,12 +373,43 @@ class HistogramImpl : public Histogram, public MetricImpl {
 };
 
 /**
- * Implementation of RawStatDataAllocator that just allocates a new structure in memory and returns
- * it.
+ * Implementation of RawStatDataAllocator which accepts a block of (potentially shared) memory.
+ */
+class BlockRawStatDataAllocator : public RawStatDataAllocator {
+public:
+  /**
+   * Creates a RawStatDataAllocator with an underlying MemoryHashMap to track stat names. Optionally
+   * works with shared memory, as in HotRestartImpl. Thread-safe but requires a lock as argument.
+   * @param options initialization parameters for BlockMemoryHashSet
+   * @param init true if the shared-memory should be initialized on construction. See the
+   * BlockMemoryHashSet constructor.
+   * @param memory the memory buffer for the set data.
+   * @param stat_lock a lock for thread safety.
+   * @return BlockRawStatDataAllocator newly constructed BlockRawStatDataAllocator.
+   */
+  BlockRawStatDataAllocator(const BlockMemoryHashSetOptions& options, bool init, uint8_t* memory,
+                            Thread::BasicLockable& stat_lock)
+      : stat_lock_(stat_lock) {
+    stats_set_ = std::make_unique<RawStatDataSet>(options, init, memory);
+  }
+
+  RawStatData* alloc(const std::string& name);
+  void free(RawStatData& data);
+  std::string version() { return stats_set_->version(); }
+
+private:
+  std::unique_ptr<RawStatDataSet> stats_set_;
+  // We must hold the stat lock when attaching to an existing memory segment
+  // because it might be actively written to while we sanityCheck it.
+  Thread::BasicLockable& stat_lock_;
+};
+
+/**
+ * Heap-based implementation of RawStatDataAllocator that just allocates a new structure in memory
+ * and returns it.
  */
 class HeapRawStatDataAllocator : public RawStatDataAllocator {
 public:
-  // RawStatDataAllocator
   RawStatData* alloc(const std::string& name) override;
   void free(RawStatData& data) override;
 };

source/exe/main_common.cc

@@ -53,7 +53,7 @@ MainCommonBase::MainCommonBase(OptionsImpl& options) : options_(options) {
     }
 #endif
     if (restarter_.get() == nullptr) {
-      restarter_.reset(new Server::HotRestartNopImpl());
+      restarter_.reset(new Server::HotRestartNopImpl(options_));
     }
 
     tls_.reset(new ThreadLocal::InstanceImpl);
@@ -69,7 +69,7 @@ MainCommonBase::MainCommonBase(OptionsImpl& options) : options_(options) {
     break;
   }
   case Server::Mode::Validate:
-    restarter_.reset(new Server::HotRestartNopImpl());
+    restarter_.reset(new Server::HotRestartNopImpl(options_));
     Logger::Registry::initialize(options_.logLevel(), options_.logFormat(), restarter_->logLock());
     break;
   }

source/server/BUILD

@@ -116,7 +116,7 @@ envoy_cc_library(
         "//include/envoy/server:options_interface",
         "//source/common/api:os_sys_calls_lib",
         "//source/common/common:assert_lib",
-        "//source/common/common:shared_memory_hash_set_lib",
+        "//source/common/common:block_memory_hash_set_lib",
         "//source/common/common:utility_lib",
         "//source/common/network:utility_lib",
         "//source/common/stats:stats_lib",
@@ -128,6 +128,7 @@ envoy_cc_library(
     hdrs = ["hot_restart_nop_impl.h"],
     deps = [
         "//include/envoy/server:hot_restart_interface",
+        "//source/server:hot_restart_lib",
     ],
 )