This is an automated email from the ASF dual-hosted git repository.
cmcfarlen pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 402c393bea Refactor Metrics storage to preserve lifetime (#11214)
402c393bea is described below
commit 402c393beacb4b57ffb63f430f1176eb9b3a93e8
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]>
---
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);
