replace LedgerIndexMap with xrpl::Mutex

include/xrpl/ledger/LedgerIndexMap.h

@@ -1,92 +0,0 @@
-#pragma once
-
-#include <algorithm>
-#include <mutex>
-#include <optional>
-#include <unordered_map>
-#include <utility>
-
-namespace xrpl {
-
-template <class Key, class Mapped>
-class LedgerIndexMap
-{
-public:
-    LedgerIndexMap() = default;
-    explicit LedgerIndexMap(std::size_t reserve_capacity)
-    {
-        data_.reserve(reserve_capacity);
-    }
-
-    LedgerIndexMap(LedgerIndexMap const&) = delete;
-    LedgerIndexMap&
-    operator=(LedgerIndexMap const&) = delete;
-    LedgerIndexMap(LedgerIndexMap&&) = delete;
-    LedgerIndexMap&
-    operator=(LedgerIndexMap&&) = delete;
-
-    [[nodiscard]] std::optional<Mapped>
-    get(Key const& k) const
-    {
-        std::lock_guard lock(mutex_);
-        auto it = data_.find(k);
-        return it == data_.end() ? std::nullopt : std::optional<Mapped>{it->second};
-    }
-
-    bool
-    put(Key const& k, Mapped value)
-    {
-        std::lock_guard lock(mutex_);
-        return data_.insert_or_assign(k, std::move(value)).second;
-    }
-
-    bool
-    contains(Key const& k) const
-    {
-        std::lock_guard lock(mutex_);
-        return data_.find(k) != data_.end();
-    }
-
-    std::size_t
-    size() const
-    {
-        std::lock_guard lock(mutex_);
-        return data_.size();
-    }
-
-    bool
-    empty() const
-    {
-        std::lock_guard lock(mutex_);
-        return data_.empty();
-    }
-
-    void
-    reserve(std::size_t n)
-    {
-        std::lock_guard lock(mutex_);
-        data_.reserve(n);
-    }
-
-    void
-    rehash(std::size_t n)
-    {
-        std::lock_guard lock(mutex_);
-        data_.rehash(n);
-    }
-
-    std::size_t
-    eraseBefore(Key const& cutoff)
-    {
-        std::lock_guard lock(mutex_);
-        auto const before = data_.size();
-        std::erase_if(data_, [&](auto const& kv) { return kv.first < cutoff; });
-        return before - data_.size();
-    }
-
-private:
-    std::unordered_map<Key, Mapped> data_;
-    mutable std::mutex mutex_;
-};
-
-}  // namespace xrpl

src/tests/libxrpl/ledger/LedgerIndexMap.cpp

@@ -1,191 +0,0 @@
-#include <xrpl/ledger/LedgerIndexMap.h>
-
-#include <gtest/gtest.h>
-
-#include <cstdint>
-#include <string>
-#include <vector>
-
-using namespace xrpl;
-
-using TestMap = LedgerIndexMap<int, std::string>;
-
-TEST(LedgerIndexMap, DefaultEmpty)
-{
-    TestMap m;
-    EXPECT_EQ(m.size(), 0);
-    EXPECT_TRUE(m.empty());
-    EXPECT_FALSE(m.get(42).has_value());
-    EXPECT_FALSE(m.contains(42));
-}
-
-TEST(LedgerIndexMap, PutInsertsValue)
-{
-    TestMap m;
-    bool inserted = m.put(10, "");
-    EXPECT_TRUE(inserted);
-    EXPECT_EQ(m.size(), 1);
-    EXPECT_TRUE(m.contains(10));
-    auto got = m.get(10);
-    ASSERT_TRUE(got.has_value());
-    EXPECT_TRUE(got->empty());
-}
-
-TEST(LedgerIndexMap, PutWithValue)
-{
-    TestMap m;
-    bool inserted = m.put(7, "seven");
-    EXPECT_TRUE(inserted);
-    EXPECT_EQ(m.size(), 1);
-    auto got = m.get(7);
-    ASSERT_TRUE(got.has_value());
-    EXPECT_EQ(*got, "seven");
-}
-
-TEST(LedgerIndexMap, InsertThroughPut)
-{
-    TestMap m;
-    bool inserted = m.put(20, "twenty");
-    EXPECT_TRUE(inserted);
-    auto got = m.get(20);
-    ASSERT_TRUE(got.has_value());
-    EXPECT_EQ(*got, "twenty");
-    EXPECT_EQ(m.size(), 1);
-}
-
-TEST(LedgerIndexMap, OverwriteExistingKeyWithPut)
-{
-    TestMap m;
-    bool inserted1 = m.put(5, "five");
-    EXPECT_TRUE(inserted1);
-    EXPECT_EQ(m.size(), 1);
-    bool inserted2 = m.put(5, "FIVE");
-    EXPECT_FALSE(inserted2);  // Not a new insertion, it's an update
-    EXPECT_EQ(m.size(), 1);
-    auto got = m.get(5);
-    ASSERT_TRUE(got.has_value());
-    EXPECT_EQ(*got, "FIVE");
-}
-
-TEST(LedgerIndexMap, OnceFoundOneNotFound)
-{
-    TestMap m;
-    m.put(1, "one");
-    EXPECT_TRUE(m.get(1).has_value());
-    EXPECT_FALSE(m.get(2).has_value());
-}
-
-TEST(LedgerIndexMap, TryEraseBeforeNothingToDo)
-{
-    TestMap m;
-    m.put(10, "a");
-    m.put(11, "b");
-    m.put(12, "c");
-    EXPECT_EQ(m.eraseBefore(10), 0);
-    EXPECT_EQ(m.size(), 3);
-    EXPECT_TRUE(m.contains(10));
-    EXPECT_TRUE(m.contains(11));
-    EXPECT_TRUE(m.contains(12));
-}
-
-TEST(LedgerIndexMap, EraseBeforeRemovesSeveralEntries)
-{
-    TestMap m;
-    m.put(10, "a");
-    m.put(11, "b");
-    m.put(12, "c");
-    m.put(13, "d");
-    EXPECT_EQ(m.eraseBefore(12), 2);
-    EXPECT_FALSE(m.contains(10));
-    EXPECT_FALSE(m.contains(11));
-    EXPECT_TRUE(m.contains(12));
-    EXPECT_TRUE(m.contains(13));
-    EXPECT_EQ(m.size(), 2);
-}
-
-TEST(LedgerIndexMap, EraseBeforeRemovesAllEntries)
-{
-    TestMap m;
-    m.put(1, "x");
-    m.put(2, "y");
-    EXPECT_EQ(m.eraseBefore(1000), 2);
-    EXPECT_EQ(m.size(), 0);
-    EXPECT_TRUE(m.empty());
-}
-
-TEST(LedgerIndexMap, EraseBeforeSameCallSecondTimeNothingToDo)
-{
-    TestMap m;
-    m.put(10, "a");
-    m.put(11, "b");
-    m.put(12, "c");
-    EXPECT_EQ(m.eraseBefore(12), 2);
-    EXPECT_TRUE(m.contains(12));
-    EXPECT_EQ(m.eraseBefore(12), 0);
-    EXPECT_EQ(m.size(), 1);
-}
-
-TEST(LedgerIndexMap, EraseBeforeSingleEntryRemoved)
-{
-    TestMap m;
-    m.put(10, "v1");
-    m.put(10, "v2");
-    m.put(10, "v3");
-    EXPECT_EQ(m.size(), 1);
-    EXPECT_EQ(m.eraseBefore(11), 1);
-    EXPECT_EQ(m.size(), 0);
-}
-
-TEST(LedgerIndexMap, EraseBeforeOutlierStillRemovedInOneCall)
-{
-    TestMap m;
-    m.put(10, "a");
-    m.put(12, "c");
-    m.put(11, "b");  // out-of-order insert
-
-    EXPECT_EQ(m.eraseBefore(12), 2);  // removes 10 and 11
-    EXPECT_FALSE(m.contains(10));
-    EXPECT_FALSE(m.contains(11));
-    EXPECT_TRUE(m.contains(12));
-    EXPECT_EQ(m.size(), 1);
-}
-
-TEST(LedgerIndexMap, EraseBeforeEraseInTwoSteps)
-{
-    TestMap m;
-    for (int k : {10, 11, 12, 13})
-        m.put(k, std::to_string(k));
-
-    EXPECT_EQ(m.eraseBefore(11), 1);
-    EXPECT_FALSE(m.contains(10));
-    EXPECT_EQ(m.size(), 3);
-
-    EXPECT_EQ(m.eraseBefore(13), 2);
-    EXPECT_FALSE(m.contains(11));
-    EXPECT_FALSE(m.contains(12));
-    EXPECT_TRUE(m.contains(13));
-    EXPECT_EQ(m.size(), 1);
-}
-
-TEST(LedgerIndexMap, RehashDoesNotLoseEntries)
-{
-    TestMap m;
-    for (int k = 0; k < 16; ++k)
-        m.put(k, "v" + std::to_string(k));
-
-    m.reserve(64);
-    m.rehash(32);
-
-    for (int k = 0; k < 16; ++k)
-    {
-        auto v = m.get(k);
-        ASSERT_TRUE(v.has_value());
-        EXPECT_EQ(*v, "v" + std::to_string(k));
-    }
-
-    EXPECT_EQ(m.eraseBefore(8), 8);
-    for (int k = 0; k < 8; ++k)
-        EXPECT_FALSE(m.contains(k));
-    for (int k = 8; k < 16; ++k)
-        EXPECT_TRUE(m.contains(k));
-}

src/xrpld/app/ledger/LedgerHistory.cpp

@@ -12,22 +12,21 @@ LedgerHistory::LedgerHistory(beast::insight::Collector::ptr const& collector, Ap
     : app_(app)
     , collector_(collector)
     , mismatch_counter_(collector->make_counter("ledger.history", "mismatch"))
-    , m_ledgers_by_hash(
-          "LedgerCache",
-          app_.config().getValueFor(SizedItem::ledgerSize),
-          std::chrono::seconds{app_.config().getValueFor(SizedItem::ledgerAge)},
-          stopwatch(),
-          app_.journal("TaggedCache"))
     , m_consensus_validated(
           "ConsensusValidated",
           64,
           std::chrono::minutes{5},
           stopwatch(),
           app_.journal("TaggedCache"))
-    , mLedgersByIndex(
-          512)  // Index map reserve capacity, aligned with typical ledger_history retention.
     , j_(app.journal("LedgerHistory"))
 {
+    auto lock = ledger_maps_.lock();
+    lock->by_hash = std::make_unique<LedgerMaps::LedgersByHash>(
+        "LedgerCache",
+        app_.config().getValueFor(SizedItem::ledgerSize),
+        std::chrono::seconds{app_.config().getValueFor(SizedItem::ledgerAge)},
+        stopwatch(),
+        app_.journal("TaggedCache"));
 }
 
 bool
@@ -39,10 +38,11 @@ LedgerHistory::insert(std::shared_ptr<Ledger const> const& ledger, bool validate
     XRPL_ASSERT(
         ledger->stateMap().getHash().isNonZero(), "xrpl::LedgerHistory::insert : nonzero hash");
 
+    auto lock = ledger_maps_.lock();
     bool const alreadyHad =
-        m_ledgers_by_hash.canonicalize_replace_cache(ledger->header().hash, ledger);
+        lock->by_hash->canonicalize_replace_cache(ledger->header().hash, ledger);
     if (validated)
-        mLedgersByIndex.put(ledger->header().seq, ledger->header().hash);
+        lock->by_index[ledger->header().seq] = ledger->header().hash;
 
     return alreadyHad;
 }
@@ -50,20 +50,31 @@ LedgerHistory::insert(std::shared_ptr<Ledger const> const& ledger, bool validate
 LedgerHash
 LedgerHistory::getLedgerHash(LedgerIndex index)
 {
-    if (auto p = mLedgersByIndex.get(index))
-        return *p;
+    auto lock = ledger_maps_.lock();
+    if (auto it = lock->by_index.find(index); it != lock->by_index.end())
+        return it->second;
     return {};
 }
 
 std::shared_ptr<Ledger const>
 LedgerHistory::getLedgerBySeq(LedgerIndex index)
 {
-    if (auto p = mLedgersByIndex.get(index))
+    uint256 hash;
     {
-        uint256 const hash = *p;
-        return getLedgerByHash(hash);
+        auto lock = ledger_maps_.lock();
+        if (auto it = lock->by_index.find(index); it != lock->by_index.end())
+        {
+            hash = it->second;
+        }
+        else
+        {
+            hash = {};
+        }
     }
 
+    if (!hash.isZero())
+        return getLedgerByHash(hash);
+
     std::shared_ptr<Ledger const> ret = loadByIndex(index, app_);
 
     if (!ret)
@@ -74,10 +85,11 @@ LedgerHistory::getLedgerBySeq(LedgerIndex index)
 
     {
         // Add this ledger to the local tracking by index
+        auto lock = ledger_maps_.lock();
         XRPL_ASSERT(
             ret->isImmutable(), "xrpl::LedgerHistory::getLedgerBySeq : immutable result ledger");
-        m_ledgers_by_hash.canonicalize_replace_client(ret->header().hash, ret);
-        mLedgersByIndex.put(ret->header().seq, ret->header().hash);
+        lock->by_hash->canonicalize_replace_client(ret->header().hash, ret);
+        lock->by_index[ret->header().seq] = ret->header().hash;
         return (ret->header().seq == index) ? ret : nullptr;
     }
 }
@@ -85,7 +97,11 @@ LedgerHistory::getLedgerBySeq(LedgerIndex index)
 std::shared_ptr<Ledger const>
 LedgerHistory::getLedgerByHash(LedgerHash const& hash)
 {
-    auto ret = m_ledgers_by_hash.fetch(hash);
+    std::shared_ptr<Ledger const> ret;
+    {
+        auto lock = ledger_maps_.lock();
+        ret = lock->by_hash->fetch(hash);
+    }
 
     if (ret)
     {
@@ -110,7 +126,10 @@ LedgerHistory::getLedgerByHash(LedgerHash const& hash)
     XRPL_ASSERT(
         ret->header().hash == hash,
         "xrpl::LedgerHistory::getLedgerByHash : loaded ledger hash match");
-    m_ledgers_by_hash.canonicalize_replace_client(ret->header().hash, ret);
+    {
+        auto lock = ledger_maps_.lock();
+        lock->by_hash->canonicalize_replace_client(ret->header().hash, ret);
+    }
     XRPL_ASSERT(
         ret->header().hash == hash, "xrpl::LedgerHistory::getLedgerByHash : result hash match");
 
@@ -462,11 +481,12 @@ LedgerHistory::validatedLedger(
 bool
 LedgerHistory::fixIndex(LedgerIndex ledgerIndex, LedgerHash const& ledgerHash)
 {
-    if (auto cur = mLedgersByIndex.get(ledgerIndex))
+    auto lock = ledger_maps_.lock();
+    if (auto it = lock->by_index.find(ledgerIndex); it != lock->by_index.end())
     {
-        if (*cur != ledgerHash)
+        if (it->second != ledgerHash)
         {
-            mLedgersByIndex.put(ledgerIndex, ledgerHash);
+            lock->by_index[ledgerIndex] = ledgerHash;
             return false;
         }
     }
@@ -477,21 +497,44 @@ void
 LedgerHistory::clearLedgerCachePrior(LedgerIndex seq)
 {
     std::size_t hashesCleared = 0;
-    for (LedgerHash it : m_ledgers_by_hash.getKeys())
-    {
-        auto const ledger = getLedgerByHash(it);
-        if (!ledger || ledger->header().seq < seq)
-        {
-            m_ledgers_by_hash.del(it, false);
-            ++hashesCleared;
-        }
-    }
-    JLOG(j_.debug()) << "LedgersByHash: cleared " << hashesCleared << " entries before seq " << seq
-                     << " (total now " << m_ledgers_by_hash.size() << ")";
+    std::size_t indexesCleared = 0;
+    std::size_t cacheSize = 0;
+    std::size_t indexSize = 0;
 
-    std::size_t const indexesCleared = mLedgersByIndex.eraseBefore(seq);
-    JLOG(j_.debug()) << "LedgerIndexMap: cleared " << indexesCleared << " index entries before seq "
-                     << seq << " (total now " << mLedgersByIndex.size() << ")";
+    {
+        auto lock = ledger_maps_.lock();
+        for (LedgerHash it : lock->by_hash->getKeys())
+        {
+            auto const ledger = getLedgerByHash(it);
+            if (!ledger || ledger->header().seq < seq)
+            {
+                lock->by_hash->del(it, false);
+                ++hashesCleared;
+            }
+        }
+        cacheSize = lock->by_hash->size();
+
+        auto it = lock->by_index.begin();
+        while (it != lock->by_index.end())
+        {
+            if (it->first < seq)
+            {
+                it = lock->by_index.erase(it);
+                ++indexesCleared;
+            }
+            else
+            {
+                ++it;
+            }
+        }
+        indexSize = lock->by_index.size();
+    }
+
+    JLOG(j_.debug()) << "LedgersByHash: cleared " << hashesCleared << " entries before seq " << seq
+                     << " (total now " << cacheSize << ")";
+
+    JLOG(j_.debug()) << "LedgersByIndex: cleared " << indexesCleared << " index entries before seq "
+                     << seq << " (total now " << indexSize << ")";
 }
 
 }  // namespace xrpl

src/xrpld/app/ledger/LedgerHistory.h

@@ -3,10 +3,12 @@
 #include <xrpld/app/ledger/Ledger.h>
 #include <xrpld/app/main/Application.h>
 
+#include <xrpl/basics/Mutex.hpp>
 #include <xrpl/beast/insight/Collector.h>
-#include <xrpl/ledger/LedgerIndexMap.h>
 #include <xrpl/protocol/RippleLedgerHash.h>
 
+#include <map>
+#include <memory>
 #include <optional>
 
 namespace xrpl {
@@ -31,7 +33,8 @@ public:
     float
     getCacheHitRate()
     {
-        return m_ledgers_by_hash.getHitRate();
+        auto lock = ledger_maps_.lock();
+        return lock->by_hash->getHitRate();
     }
 
     /** Get a ledger given its sequence number */
@@ -54,7 +57,10 @@ public:
     void
     sweep()
     {
-        m_ledgers_by_hash.sweep();
+        {
+            auto lock = ledger_maps_.lock();
+            lock->by_hash->sweep();
+        }
         m_consensus_validated.sweep();
     }
 
@@ -102,9 +108,15 @@ private:
     beast::insight::Collector::ptr collector_;
     beast::insight::Counter mismatch_counter_;
 
-    using LedgersByHash = TaggedCache<LedgerHash, Ledger const>;
+    struct LedgerMaps
+    {
+        using LedgersByHash = TaggedCache<LedgerHash, Ledger const>;
 
-    LedgersByHash m_ledgers_by_hash;
+        std::unique_ptr<LedgersByHash> by_hash;
+        std::map<LedgerIndex, LedgerHash> by_index;  // validated ledgers
+    };
+
+    xrpl::Mutex<LedgerMaps, std::recursive_mutex> ledger_maps_;
 
     // Maps ledger indexes to the corresponding hashes
     // For debug and logging purposes
@@ -124,9 +136,6 @@ private:
     using ConsensusValidated = TaggedCache<LedgerIndex, cv_entry>;
     ConsensusValidated m_consensus_validated;
 
-    // Maps ledger indexes to the corresponding hash.
-    xrpl::LedgerIndexMap<LedgerIndex, LedgerHash> mLedgersByIndex;  // validated ledgers
-
     beast::Journal j_;
 };