This is an automated email from the ASF dual-hosted git repository. cmcfarlen pushed a commit to branch 10.0.x in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit 3c4ab456a346f6a00721f334cf9ceb5163e4f6d7 Author: Chris McFarlen <[email protected]> AuthorDate: Wed Apr 10 12:05:49 2024 -0500 Refactor Metrics storage to preserve lifetime (#11214) * Refactor Metrics storage to preserve lifetime * cleanup, make storage class * PR address --------- Co-authored-by: Chris McFarlen <[email protected]> (cherry picked from commit 402c393beacb4b57ffb63f430f1176eb9b3a93e8) --- include/tsutil/Metrics.h | 126 ++++++++++++++++++++++++++++++++++------------- src/tsutil/Metrics.cc | 34 +++++++------ 2 files changed, 110 insertions(+), 50 deletions(-) diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index 77b18fd39c..a143cf041f 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -27,7 +27,6 @@ #include <unordered_map> #include <tuple> #include <mutex> -#include <thread> #include <atomic> #include <cstdint> #include <string> @@ -103,29 +102,33 @@ public: Metrics &operator=(Metrics &&) = delete; Metrics(Metrics &&) = delete; - virtual ~Metrics() - { - for (size_t i = 0; i <= _cur_blob; ++i) { - delete _blobs[i]; - } - } - - Metrics() - { - _blobs[0] = new NamesAndAtomics(); - release_assert(_blobs[0]); - release_assert(0 == _create("proxy.process.api.metrics.bad_id")); // Reserve slot 0 for errors, this should always be 0 - } + virtual ~Metrics() {} // The singleton instance, owned by the Metrics class static Metrics &instance(); // Yes, we don't return objects here, but rather ID's and atomic's directly. Treat // the std::atomic<int64_t> as the underlying class for a single metric, and be happy. - IdType lookup(const std::string_view name) const; - AtomicType *lookup(const std::string_view name, IdType *out_id) const; - AtomicType *lookup(IdType id, std::string_view *out_name = nullptr) const; - bool rename(IdType id, const std::string_view name); + IdType + lookup(const std::string_view name) const + { + return _storage->lookup(name); + } + AtomicType * + lookup(const std::string_view name, IdType *out_id) const + { + return _storage->lookup(name, out_id); + } + AtomicType * + lookup(IdType id, std::string_view *out_name = nullptr) const + { + return _storage->lookup(id, out_name); + } + bool + rename(IdType id, const std::string_view name) + { + return _storage->rename(id, name); + } AtomicType & operator[](IdType id) @@ -155,14 +158,16 @@ public: return (metric ? metric->_value.fetch_sub(val, MEMORY_ORDER) : NOT_FOUND); } - std::string_view name(IdType id) const; + std::string_view + name(IdType id) const + { + return _storage->name(id); + } bool valid(IdType id) const { - auto [blob, entry] = _splitID(id); - - return (id >= 0 && ((blob < _cur_blob && entry < MAX_SIZE) || (blob == _cur_blob && entry <= _cur_off))); + return _storage->valid(id); } // Static methods to encapsulate access to the atomic's @@ -232,10 +237,7 @@ public: iterator end() const { - _mutex.lock(); - int16_t blob = _cur_blob; - int16_t offset = _cur_off; - _mutex.unlock(); + auto [blob, offset] = _storage->current(); return iterator(*this, _makeId(blob, offset)); } @@ -254,8 +256,17 @@ public: private: // These are private, to assure that we don't use them by accident creating naked metrics - IdType _create(const std::string_view name); - SpanType _createSpan(size_t size, IdType *id = nullptr); + IdType + _create(const std::string_view name) + { + return _storage->create(name); + } + + SpanType + _createSpan(size_t size, IdType *id = nullptr) + { + return _storage->createSpan(size, id); + } // These are little helpers around managing the ID's static constexpr std::tuple<uint16_t, uint16_t> @@ -276,13 +287,60 @@ private: return _makeId(std::get<0>(id), std::get<1>(id)); } - void _addBlob(); + class Storage + { + BlobStorage _blobs; + uint16_t _cur_blob = 0; + uint16_t _cur_off = 0; + LookupTable _lookups; + mutable std::mutex _mutex; + + public: + Storage(const Storage &) = delete; + Storage &operator=(const Storage &) = delete; + + Storage() + { + _blobs[0] = new NamesAndAtomics(); + release_assert(_blobs[0]); + release_assert(0 == create("proxy.process.api.metrics.bad_id")); // Reserve slot 0 for errors, this should always be 0 + } + + ~Storage() + { + for (size_t i = 0; i <= _cur_blob; ++i) { + delete _blobs[i]; + } + } + + IdType create(const std::string_view name); + void addBlob(); + IdType lookup(const std::string_view name) const; + AtomicType *lookup(const std::string_view name, IdType *out_id) const; + AtomicType *lookup(Metrics::IdType id, std::string_view *out_name = nullptr) const; + std::string_view name(IdType id) const; + SpanType createSpan(size_t size, IdType *id = nullptr); + bool rename(IdType id, const std::string_view name); + + std::pair<int16_t, int16_t> + current() const + { + std::lock_guard lock(_mutex); + return {_cur_blob, _cur_off}; + } + + bool + valid(IdType id) const + { + auto [blob, entry] = _splitID(id); + + return (id >= 0 && ((blob < _cur_blob && entry < MAX_SIZE) || (blob == _cur_blob && entry <= _cur_off))); + } + }; + + Metrics(std::shared_ptr<Storage> &str) : _storage(str) {} - mutable std::mutex _mutex; - LookupTable _lookups; - BlobStorage _blobs; - uint16_t _cur_blob = 0; - uint16_t _cur_off = 0; + std::shared_ptr<Storage> _storage; public: // These are sort of factory classes, using the Metrics singleton for all storage etc. diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index af8f3669d7..9c43aebcc9 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -22,22 +22,24 @@ */ #include "tsutil/Assert.h" +#include <memory> #include "tsutil/Metrics.h" namespace ts { -// This is the singleton instance of the metrics class. Metrics & Metrics::instance() { - static Metrics _instance; + // This is the singleton instance of the metrics storage class. + static std::shared_ptr<Storage> _metrics_store = std::make_shared<Storage>(); + thread_local Metrics _instance(_metrics_store); return _instance; } void -Metrics::_addBlob() // The mutex must be held before calling this! +Metrics::Storage::addBlob() // The mutex must be held before calling this! { auto blob = new Metrics::NamesAndAtomics(); @@ -49,9 +51,9 @@ Metrics::_addBlob() // The mutex must be held before calling this! } Metrics::IdType -Metrics::_create(std::string_view name) +Metrics::Storage::create(std::string_view name) { - std::lock_guard<std::mutex> lock(_mutex); + std::lock_guard lock(_mutex); auto it = _lookups.find(name); if (it != _lookups.end()) { @@ -66,16 +68,16 @@ Metrics::_create(std::string_view name) _lookups.emplace(std::get<0>(names[_cur_off]), id); if (++_cur_off >= MAX_SIZE) { - _addBlob(); // This resets _cur_off to 0 as well + addBlob(); // This resets _cur_off to 0 as well } return id; } Metrics::IdType -Metrics::lookup(const std::string_view name) const +Metrics::Storage::lookup(const std::string_view name) const { - std::lock_guard<std::mutex> lock(_mutex); + std::lock_guard lock(_mutex); auto it = _lookups.find(name); if (it != _lookups.end()) { @@ -86,7 +88,7 @@ Metrics::lookup(const std::string_view name) const } Metrics::AtomicType * -Metrics::lookup(Metrics::IdType id, std::string_view *out_name) const +Metrics::Storage::lookup(Metrics::IdType id, std::string_view *out_name) const { auto [blob_ix, offset] = _splitID(id); Metrics::NamesAndAtomics *blob = _blobs[blob_ix]; @@ -105,7 +107,7 @@ Metrics::lookup(Metrics::IdType id, std::string_view *out_name) const } Metrics::AtomicType * -Metrics::lookup(const std::string_view name, Metrics::IdType *out_id) const +Metrics::Storage::lookup(const std::string_view name, Metrics::IdType *out_id) const { Metrics::IdType id = lookup(name); Metrics::AtomicType *result = nullptr; @@ -122,7 +124,7 @@ Metrics::lookup(const std::string_view name, Metrics::IdType *out_id) const } std::string_view -Metrics::name(Metrics::IdType id) const +Metrics::Storage::name(Metrics::IdType id) const { auto [blob_ix, offset] = _splitID(id); Metrics::NamesAndAtomics *blob = _blobs[blob_ix]; @@ -139,13 +141,13 @@ Metrics::name(Metrics::IdType id) const } Metrics::SpanType -Metrics::_createSpan(size_t size, Metrics::IdType *id) +Metrics::Storage::createSpan(size_t size, Metrics::IdType *id) { release_assert(size <= MAX_SIZE); - std::lock_guard<std::mutex> lock(_mutex); + std::lock_guard lock(_mutex); if (_cur_off + size > MAX_SIZE) { - _addBlob(); + addBlob(); } Metrics::IdType span_start = _makeId(_cur_blob, _cur_off); @@ -163,7 +165,7 @@ Metrics::_createSpan(size_t size, Metrics::IdType *id) } bool -Metrics::rename(Metrics::IdType id, std::string_view name) +Metrics::Storage::rename(Metrics::IdType id, std::string_view name) { auto [blob_ix, offset] = _splitID(id); Metrics::NamesAndAtomics *blob = _blobs[blob_ix]; @@ -174,7 +176,7 @@ Metrics::rename(Metrics::IdType id, std::string_view name) } std::string &cur = std::get<0>(std::get<0>(*blob)[offset]); - std::lock_guard<std::mutex> lock(_mutex); + std::lock_guard lock(_mutex); if (cur.length() > 0) { _lookups.erase(cur);
