Modernize CountedObject infrastructure:

- Replace the lazy singleton with a constinit registry of per-type
  static counters. The `getInstance` method is removed.
- A concurrency bug that would corrupt the list of counters during
  insertion has been fixed.
- Debug asserts can detect incorrect accounting that can result in
  counter under- or overflow.
- `CountedObject` can only be used as a CRTP base for its own type
  parameter.
- Track the current and maximum values of counter. This results in
  a change in the output of `get_counts`, with individual counters
  now reported as sub-objects of a `counters` object.

include/xrpl/basics/CountedObject.h

@@ -1,29 +1,31 @@
 #pragma once
 
+#include <xrpl/beast/utility/instrumentation.h>
+
 #include <boost/core/type_name.hpp>
 
+#include <algorithm>
 #include <atomic>
+#include <cstddef>
+#include <cstdint>
+#include <iterator>
 #include <string>
+#include <type_traits>
 #include <utility>
-#include <vector>
 
 namespace xrpl {
 
 /**
  * Manages all counted object types.
+ *
+ * Counters register themselves on a lock-free intrusive list maintained
+ * by this object when constructed. Because counters are never destroyed
+ * or removed, the ABA problem does not apply.
+ *
+ * The registry is iterable as a forward range.
  */
 class CountedObjects
 {
-public:
-    static CountedObjects&
-    getInstance() noexcept;
-
-    using Entry = std::pair<std::string, int>;
-    using List = std::vector<Entry>;
-
-    [[nodiscard]] List
-    getCounts(int minimumThreshold) const;
-
 public:
     /**
      * Implementation for @ref CountedObject.
@@ -33,68 +35,147 @@ public:
     class Counter
     {
     public:
-        Counter(std::string name) noexcept : name_(std::move(name)), count_(0)
-        {
-            // Insert ourselves at the front of the lock-free linked list
-            CountedObjects& instance = CountedObjects::getInstance();
-            Counter* head = nullptr;
+        Counter(std::string name) noexcept;
 
-            do
-            {
-                head = instance.head_.load();
-                next_ = head;
-            } while (instance.head_.exchange(this) != head);
+        // Counters are intrusive list nodes whose addresses are published
+        // in the registry; they must never be copied or moved. The atomic
+        // members already force this, but we make it explicit.
+        Counter(Counter const&) = delete;
+        Counter&
+        operator=(Counter const&) = delete;
+        Counter(Counter&&) = delete;
+        Counter&
+        operator=(Counter&&) = delete;
 
-            ++instance.count_;
-        }
-
-        ~Counter() noexcept = default;
-
-        int
+        std::uint32_t
         increment() noexcept
         {
-            return ++count_;
+            auto const newCount = count_.fetch_add(1, std::memory_order::relaxed) + 1;
+            XRPL_ASSERT(newCount != 0, "xrpl::CountedObjects::Counter::increment : no overflow");
+
+            auto maxCount = maxCount_.load(std::memory_order::relaxed);
+
+            while (newCount > maxCount &&
+                   !maxCount_.compare_exchange_weak(maxCount, newCount, std::memory_order::relaxed))
+            {
+            }
+
+            return newCount;
         }
 
-        int
+        std::uint32_t
         decrement() noexcept
         {
-            return --count_;
+            auto const prev = count_.fetch_sub(1, std::memory_order::relaxed);
+            XRPL_ASSERT(prev != 0, "xrpl::CountedObjects::Counter::decrement : no underflow");
+            return prev - 1;
         }
 
-        [[nodiscard]] int
-        getCount() const noexcept
+        [[nodiscard]] std::uint32_t
+        count() const noexcept
         {
-            return count_.load();
+            return count_.load(std::memory_order::relaxed);
         }
 
-        [[nodiscard]] Counter*
-        getNext() const noexcept
+        [[nodiscard]] std::uint32_t
+        max() const noexcept
         {
-            return next_;
+            return std::max(
+                count_.load(std::memory_order::relaxed),
+                maxCount_.load(std::memory_order::relaxed));
         }
 
         [[nodiscard]] std::string const&
-        getName() const noexcept
+        name() const noexcept
         {
             return name_;
         }
 
     private:
+        friend class CountedObjects;
+
+        Counter* next_ = nullptr;
+        std::atomic<std::uint32_t> count_ = 0;
+        std::atomic<std::uint32_t> maxCount_ = 0;
         std::string const name_;
-        std::atomic<int> count_;
-        Counter* next_;
     };
 
-private:
-    CountedObjects() noexcept;
-    ~CountedObjects() noexcept = default;
+    class Iterator
+    {
+    public:
+        using value_type = Counter const;
+        using reference = value_type&;
+        using pointer = value_type*;
+        using difference_type = std::ptrdiff_t;
+        using iterator_category = std::forward_iterator_tag;
+
+        explicit Iterator(Counter* c = nullptr) noexcept : current_(c)
+        {
+        }
+
+        reference
+        operator*() const noexcept
+        {
+            return *current_;
+        }
+
+        pointer
+        operator->() const noexcept
+        {
+            return current_;
+        }
+
+        Iterator&
+        operator++() noexcept
+        {
+            current_ = current_->next_;
+            return *this;
+        }
+
+        Iterator
+        operator++(int) noexcept
+        {
+            auto tmp = *this;
+            ++*this;
+            return tmp;
+        }
+
+        bool
+        operator==(Iterator const&) const noexcept = default;
+
+    private:
+        Counter* current_;
+    };
+
+    constexpr CountedObjects() noexcept = default;
+
+    [[nodiscard]] auto
+    begin() const noexcept
+    {
+        return Iterator{head_.load(std::memory_order::acquire)};
+    }
+
+    [[nodiscard]] auto
+    end() const noexcept
+    {
+        return Iterator{};
+    }
 
 private:
-    std::atomic<int> count_;
-    std::atomic<Counter*> head_;
+    std::atomic<Counter*> head_ = nullptr;
 };
 
+/** The global counted object registry. */
+inline constinit CountedObjects gCountedObjects;
+
+inline CountedObjects::Counter::Counter(std::string name) noexcept
+    : next_(gCountedObjects.head_.load(std::memory_order::relaxed)), name_(std::move(name))
+{
+    while (!gCountedObjects.head_.compare_exchange_weak(
+        next_, this, std::memory_order::release, std::memory_order::relaxed))
+        ;
+}
+
 //------------------------------------------------------------------------------
 
 /**
@@ -103,27 +184,37 @@ private:
  * Derived classes have their instances counted automatically. This is used
  * for reporting purposes.
  *
+ * The constructors are private and `Object` is befriended so that the
+ * CRTP parameter must be the deriving class itself: a copy-paste error
+ * like `class B : public CountedObject<A>` fails to compile instead of
+ * silently polluting A's count.
+ *
+ * @note This class has no move operations by design: a derived class's
+ *       move constructor falls back to the copy constructor for this
+ *       base, so the newly created instance is counted. This keeps the
+ *       invariant that count is the number of outstanding subobjects.
+ *
+ * @warning Counted objects constructed during dynamic initialization of
+ *          other translation units may have their increments discarded when
+ *          counter itself is dynamically initialized. Do not create counted
+ *          objects before main() begins.
+ *
  * @ingroup basics
  */
 template <class Object>
+    requires std::is_class_v<Object>
 class CountedObject
 {
-private:
-    static auto&
-    getCounter() noexcept
-    {
-        static CountedObjects::Counter kC{boost::core::type_name<Object>()};
-        return kC;
-    }
+    static inline CountedObjects::Counter counter{boost::core::type_name<Object>()};
 
     CountedObject() noexcept
     {
-        getCounter().increment();
+        counter.increment();
     }
 
     CountedObject(CountedObject const&) noexcept
     {
-        getCounter().increment();
+        counter.increment();
     }
 
     CountedObject&
@@ -132,7 +223,7 @@ private:
 public:
     ~CountedObject() noexcept
     {
-        getCounter().decrement();
+        counter.decrement();
     }
 
     friend Object;

include/xrpl/protocol/jss.h

@@ -392,6 +392,7 @@ JSS(max_ledger);                  // in/out: LedgerCleaner
 JSS(max_queue_size);              // out: TxQ
 JSS(max_spend_drops);             // out: AccountInfo
 JSS(max_spend_drops_total);       // out: AccountInfo
+JSS(maximum);
 JSS(mean);                        // out: get_aggregate_price
 JSS(median);                      // out: get_aggregate_price
 JSS(median_fee);                  // out: TxQ

src/libxrpl/basics/CountedObject.cpp

@@ -1,39 +0,0 @@
-#include <xrpl/basics/CountedObject.h>
-
-#include <algorithm>
-
-namespace xrpl {
-
-CountedObjects&
-CountedObjects::getInstance() noexcept
-{
-    static CountedObjects kInstance;
-
-    return kInstance;
-}
-
-CountedObjects::CountedObjects() noexcept : count_(0), head_(nullptr)
-{
-}
-
-CountedObjects::List
-CountedObjects::getCounts(int minimumThreshold) const
-{
-    List counts;
-
-    // When other operations are concurrent, the count
-    // might be temporarily less than the actual count.
-    counts.reserve(count_.load());
-
-    for (auto* ctr = head_.load(); ctr != nullptr; ctr = ctr->getNext())
-    {
-        if (ctr->getCount() >= minimumThreshold)
-            counts.emplace_back(ctr->getName(), ctr->getCount());
-    }
-
-    std::ranges::sort(counts);
-
-    return counts;
-}
-
-}  // namespace xrpl

src/test/app/Regression_test.cpp

@@ -305,12 +305,11 @@ struct Regression_test : public beast::unit_test::Suite
                 return digest.asUInt256();
             }();
 
-            auto const mapCounts = [&](CountedObjects::List const& list) {
-                std::map<std::string, int> result;
-                for (auto const& e : list)
-                {
-                    result[e.first] = e.second;
-                }
+            auto const mapCounts = [] {
+                std::map<std::string, std::uint32_t> result;
+
+                for (auto const& c : gCountedObjects)
+                    result.emplace(c.name(), c.count());
 
                 return result;
             };
@@ -319,11 +318,11 @@ struct Regression_test : public beast::unit_test::Suite
             {
                 auto& cache = env.app().getCachedSLEs();
                 cache.del(*digest, false);  // NOLINT(bugprone-unchecked-optional-access)
-                auto const beforeCounts = mapCounts(CountedObjects::getInstance().getCounts(0));
+                auto const beforeCounts = mapCounts();
 
                 env(check::cash(alice, bobIndex, check::DeliverMin(XRP(100))), Ter(tecNO_ENTRY));
 
-                auto const afterCounts = mapCounts(CountedObjects::getInstance().getCounts(0));
+                auto const afterCounts = mapCounts();
 
                 using namespace std::string_literals;
                 BEAST_EXPECT(

src/test/rpc/GetCounts_test.cpp

@@ -9,6 +9,7 @@
 #include <xrpl/json/json_value.h>
 #include <xrpl/protocol/jss.h>
 
+#include <algorithm>
 #include <thread>
 
 namespace xrpl {
@@ -52,36 +53,59 @@ class GetCounts_test : public beast::unit_test::Suite
             // check counts, default params
             result = env.rpc("get_counts")[jss::result];
             BEAST_EXPECT(result[jss::status] == "success");
-            // compare with values reported by CountedObjects
-            auto const& objectCounts = CountedObjects::getInstance().getCounts(10);
-            for (auto const& it : objectCounts)
+
+            // compare with values reported by the registry
+            auto const& counters = result[jss::counters];
+
+            for (auto const& c : gCountedObjects)
             {
-                BEAST_EXPECTS(result.isMember(it.first), it.first);
-                BEAST_EXPECTS(result[it.first].asInt() == it.second, it.first);
+                if (auto const count = c.count(); count >= 10)
+                {
+                    BEAST_EXPECTS(counters.isMember(c.name()), c.name());
+                    BEAST_EXPECTS(counters[c.name()][jss::current].asUInt() == count, c.name());
+                }
             }
+
             BEAST_EXPECT(!result.isMember(jss::local_txs));
         }
-
         {
             // make request with min threshold 100 and verify
-            // that only STObject and NodeObject are reported
+            // that only counters at or above the threshold are reported
             result = env.rpc("get_counts", "100")[jss::result];
             BEAST_EXPECT(result[jss::status] == "success");
 
-            // compare with values reported by CountedObjects
-            auto const& objectCounts = CountedObjects::getInstance().getCounts(100);
-            for (auto const& it : objectCounts)
-            {
-                BEAST_EXPECTS(result.isMember(it.first), it.first);
-                BEAST_EXPECTS(result[it.first].asInt() == it.second, it.first);
-            }
-            BEAST_EXPECT(!result.isMember("Transaction"));
-            BEAST_EXPECT(!result.isMember("STTx"));
-            BEAST_EXPECT(!result.isMember("STArray"));
-            BEAST_EXPECT(!result.isMember("HashRouterEntry"));
-            BEAST_EXPECT(!result.isMember("STLedgerEntry"));
-        }
+            auto const& counters = result[jss::counters];
 
+            // every registry counter at/above threshold must be reported, with
+            // a matching current value
+            for (auto const& c : gCountedObjects)
+            {
+                if (auto const count = c.count(); count >= 100)
+                {
+                    BEAST_EXPECTS(counters.isMember(c.name()), c.name());
+                    BEAST_EXPECTS(counters[c.name()][jss::current].asUInt() == count, c.name());
+                }
+            }
+
+            // conversely, every reported entry must be a known counter that met
+            // the threshold, and its maximum must be consistent
+            for (auto const& name : counters.getMemberNames())
+            {
+                auto const it = std::ranges::find_if(
+                    gCountedObjects, [&](auto const& c) { return c.name() == name; });
+                BEAST_EXPECTS(it != gCountedObjects.end(), name);
+
+                auto const& entry = counters[name];
+                BEAST_EXPECTS(entry[jss::current].asUInt() >= 100, name);
+                BEAST_EXPECTS(entry[jss::maximum].asUInt() >= entry[jss::current].asUInt(), name);
+            }
+
+            BEAST_EXPECT(!counters.isMember("xrpl::Transaction"));
+            BEAST_EXPECT(!counters.isMember("xrpl::STTx"));
+            BEAST_EXPECT(!counters.isMember("xrpl::STArray"));
+            BEAST_EXPECT(!counters.isMember("xrpl::HashRouterEntry"));
+            BEAST_EXPECT(!counters.isMember("xrpl::STLedgerEntry"));
+        }
         {
             // local_txs field will exist when there are open Txs
             env(pay(alice, bob, alice["USD"](5)));

src/xrpld/rpc/handlers/admin/status/GetCounts.cpp

@@ -4,6 +4,7 @@
 #include <xrpld/app/rdb/backend/SQLiteDatabase.h>
 #include <xrpld/rpc/Context.h>
 
+#include <xrpl/basics/CountedObject.h>
 #include <xrpl/basics/UptimeClock.h>
 #include <xrpl/json/json_forwards.h>
 #include <xrpl/json/json_value.h>
@@ -11,9 +12,11 @@
 #include <xrpl/protocol/jss.h>
 #include <xrpl/server/NetworkOPs.h>
 
+#include <algorithm>
 #include <chrono>
 #include <cstddef>
 #include <string>
+#include <utility>
 
 namespace xrpl {
 
@@ -43,16 +46,26 @@ textTime(
 }
 
 json::Value
-getCountsJson(Application& app, int minObjectCount)
+getCountsJson(Application& app, std::uint32_t minObjectCount)
 {
-    auto objectCounts = CountedObjects::getInstance().getCounts(minObjectCount);
+    json::Value ret = json::ValueType::Object;
 
-    json::Value ret(json::ValueType::Object);
+    ret[jss::counters] = [minObjectCount] {
+        json::Value ctrs = json::ValueType::Object;
 
-    for (auto const& [k, v] : objectCounts)
-    {
-        ret[k] = v;
-    }
+        for (auto const& c : gCountedObjects)
+        {
+            if (auto const count = c.count(); count >= minObjectCount)
+            {
+                json::Value obj(json::ValueType::Object);
+                obj[jss::current] = count;
+                obj[jss::maximum] = std::max(count, c.max());
+                ctrs[c.name()] = std::move(obj);
+            }
+        }
+
+        return ctrs;
+    }();
 
     if (app.config().useTxTables())
     {
@@ -113,7 +126,7 @@ getCountsJson(Application& app, int minObjectCount)
 json::Value
 doGetCounts(rpc::JsonContext& context)
 {
-    int minCount = 10;
+    std::uint32_t minCount = 10;
 
     if (context.params.isMember(jss::min_count))
         minCount = context.params[jss::min_count].asUInt();

src/xrpld/rpc/handlers/admin/status/GetCounts.h

@@ -4,9 +4,11 @@
 
 #include <xrpl/json/json_value.h>
 
+#include <cstdint>
+
 namespace xrpl {
 
 json::Value
-getCountsJson(Application& app, int minObjectCount);
+getCountsJson(Application& app, std::uint32_t minObjectCount);
 
 }  // namespace xrpl