From c57cd8b23ead8a092ff28a7be67c23d610e29c46 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 22 Aug 2025 17:30:08 -0400 Subject: [PATCH 1/2] Revert "perf: Move mutex to the partition level (#5486)" This reverts commit 94decc753b515e7499808ca0d5b9e24d172c691e. --- include/xrpl/basics/SHAMapHash.h | 1 + include/xrpl/basics/TaggedCache.h | 26 ++- include/xrpl/basics/TaggedCache.ipp | 197 +++++++++--------- .../xrpl/basics/partitioned_unordered_map.h | 12 -- include/xrpl/protocol/Protocol.h | 1 + src/test/basics/TaggedCache_test.cpp | 24 +-- src/xrpld/app/ledger/LedgerHistory.cpp | 15 +- src/xrpld/rpc/handlers/GetCounts.cpp | 2 +- 8 files changed, 143 insertions(+), 135 deletions(-) diff --git a/include/xrpl/basics/SHAMapHash.h b/include/xrpl/basics/SHAMapHash.h index 1ec326409c..2d2dcdc3ef 100644 --- a/include/xrpl/basics/SHAMapHash.h +++ b/include/xrpl/basics/SHAMapHash.h @@ -21,6 +21,7 @@ #define RIPPLE_BASICS_SHAMAP_HASH_H_INCLUDED #include +#include #include diff --git a/include/xrpl/basics/TaggedCache.h b/include/xrpl/basics/TaggedCache.h index 7eace6fe72..99c91fe393 100644 --- a/include/xrpl/basics/TaggedCache.h +++ b/include/xrpl/basics/TaggedCache.h @@ -90,6 +90,9 @@ public: int getCacheSize() const; + int + getTrackSize() const; + float getHitRate(); @@ -167,6 +170,9 @@ public: bool retrieve(key_type const& key, T& data); + mutex_type& + peekMutex(); + std::vector getKeys() const; @@ -187,14 +193,11 @@ public: private: SharedPointerType - initialFetch(key_type const& key); + initialFetch(key_type const& key, std::lock_guard const& l); void collect_metrics(); - Mutex& - lockPartition(key_type const& key) const; - private: struct Stats { @@ -297,8 +300,8 @@ private: [[maybe_unused]] clock_type::time_point const& now, typename KeyValueCacheType::map_type& partition, SweptPointersVector& stuffToSweep, - std::atomic& allRemoval, - Mutex& partitionLock); + std::atomic& allRemovals, + std::lock_guard const&); [[nodiscard]] std::thread sweepHelper( @@ -307,12 +310,14 @@ private: typename KeyOnlyCacheType::map_type& partition, SweptPointersVector&, std::atomic& allRemovals, - Mutex& partitionLock); + std::lock_guard const&); beast::Journal m_journal; clock_type& m_clock; Stats m_stats; + mutex_type mutable m_mutex; + // Used for logging std::string m_name; @@ -323,11 +328,10 @@ private: clock_type::duration const m_target_age; // Number of items cached - std::atomic m_cache_count; + int m_cache_count; cache_type m_cache; // Hold strong reference to recent objects - std::atomic m_hits; - std::atomic m_misses; - mutable std::vector partitionLocks_; + std::uint64_t m_hits; + std::uint64_t m_misses; }; } // namespace ripple diff --git a/include/xrpl/basics/TaggedCache.ipp b/include/xrpl/basics/TaggedCache.ipp index c909ec6ad1..16a3f7587a 100644 --- a/include/xrpl/basics/TaggedCache.ipp +++ b/include/xrpl/basics/TaggedCache.ipp @@ -22,7 +22,6 @@ #include #include -#include namespace ripple { @@ -61,7 +60,6 @@ inline TaggedCache< , m_hits(0) , m_misses(0) { - partitionLocks_ = std::vector(m_cache.partitions()); } template < @@ -107,13 +105,8 @@ TaggedCache< KeyEqual, Mutex>::size() const { - std::size_t totalSize = 0; - for (size_t i = 0; i < partitionLocks_.size(); ++i) - { - std::lock_guard lock(partitionLocks_[i]); - totalSize += m_cache.map()[i].size(); - } - return totalSize; + std::lock_guard lock(m_mutex); + return m_cache.size(); } template < @@ -136,7 +129,32 @@ TaggedCache< KeyEqual, Mutex>::getCacheSize() const { - return m_cache_count.load(std::memory_order_relaxed); + std::lock_guard lock(m_mutex); + return m_cache_count; +} + +template < + class Key, + class T, + bool IsKeyCache, + class SharedWeakUnionPointer, + class SharedPointerType, + class Hash, + class KeyEqual, + class Mutex> +inline int +TaggedCache< + Key, + T, + IsKeyCache, + SharedWeakUnionPointer, + SharedPointerType, + Hash, + KeyEqual, + Mutex>::getTrackSize() const +{ + std::lock_guard lock(m_mutex); + return m_cache.size(); } template < @@ -159,10 +177,9 @@ TaggedCache< KeyEqual, Mutex>::getHitRate() { - auto const hits = m_hits.load(std::memory_order_relaxed); - auto const misses = m_misses.load(std::memory_order_relaxed); - float const total = float(hits + misses); - return hits * (100.0f / std::max(1.0f, total)); + std::lock_guard lock(m_mutex); + auto const total = static_cast(m_hits + m_misses); + return m_hits * (100.0f / std::max(1.0f, total)); } template < @@ -185,12 +202,9 @@ TaggedCache< KeyEqual, Mutex>::clear() { - for (auto& mutex : partitionLocks_) - mutex.lock(); + std::lock_guard lock(m_mutex); m_cache.clear(); - for (auto& mutex : partitionLocks_) - mutex.unlock(); - m_cache_count.store(0, std::memory_order_relaxed); + m_cache_count = 0; } template < @@ -213,9 +227,11 @@ TaggedCache< KeyEqual, Mutex>::reset() { - clear(); - m_hits.store(0, std::memory_order_relaxed); - m_misses.store(0, std::memory_order_relaxed); + std::lock_guard lock(m_mutex); + m_cache.clear(); + m_cache_count = 0; + m_hits = 0; + m_misses = 0; } template < @@ -239,7 +255,7 @@ TaggedCache< KeyEqual, Mutex>::touch_if_exists(KeyComparable const& key) { - std::lock_guard lock(lockPartition(key)); + std::lock_guard lock(m_mutex); auto const iter(m_cache.find(key)); if (iter == m_cache.end()) { @@ -281,6 +297,8 @@ TaggedCache< auto const start = std::chrono::steady_clock::now(); { + std::lock_guard lock(m_mutex); + if (m_target_size == 0 || (static_cast(m_cache.size()) <= m_target_size)) { @@ -312,13 +330,12 @@ TaggedCache< m_cache.map()[p], allStuffToSweep[p], allRemovals, - partitionLocks_[p])); + lock)); } for (std::thread& worker : workers) worker.join(); - int removals = allRemovals.load(std::memory_order_relaxed); - m_cache_count.fetch_sub(removals, std::memory_order_relaxed); + m_cache_count -= allRemovals; } // At this point allStuffToSweep will go out of scope outside the lock // and decrement the reference count on each strong pointer. @@ -352,8 +369,7 @@ TaggedCache< { // Remove from cache, if !valid, remove from map too. Returns true if // removed from cache - - std::lock_guard lock(lockPartition(key)); + std::lock_guard lock(m_mutex); auto cit = m_cache.find(key); @@ -366,7 +382,7 @@ TaggedCache< if (entry.isCached()) { - m_cache_count.fetch_sub(1, std::memory_order_relaxed); + --m_cache_count; entry.ptr.convertToWeak(); ret = true; } @@ -404,16 +420,17 @@ TaggedCache< { // Return canonical value, store if needed, refresh in cache // Return values: true=we had the data already + std::lock_guard lock(m_mutex); - std::lock_guard lock(lockPartition(key)); auto cit = m_cache.find(key); + if (cit == m_cache.end()) { m_cache.emplace( std::piecewise_construct, std::forward_as_tuple(key), std::forward_as_tuple(m_clock.now(), data)); - m_cache_count.fetch_add(1, std::memory_order_relaxed); + ++m_cache_count; return false; } @@ -462,12 +479,12 @@ TaggedCache< data = cachedData; } - m_cache_count.fetch_add(1, std::memory_order_relaxed); + ++m_cache_count; return true; } entry.ptr = data; - m_cache_count.fetch_add(1, std::memory_order_relaxed); + ++m_cache_count; return false; } @@ -543,11 +560,10 @@ TaggedCache< KeyEqual, Mutex>::fetch(key_type const& key) { - std::lock_guard lock(lockPartition(key)); - - auto ret = initialFetch(key); + std::lock_guard l(m_mutex); + auto ret = initialFetch(key, l); if (!ret) - m_misses.fetch_add(1, std::memory_order_relaxed); + ++m_misses; return ret; } @@ -611,8 +627,8 @@ TaggedCache< Mutex>::insert(key_type const& key) -> std::enable_if_t { + std::lock_guard lock(m_mutex); clock_type::time_point const now(m_clock.now()); - std::lock_guard lock(lockPartition(key)); auto [it, inserted] = m_cache.emplace( std::piecewise_construct, std::forward_as_tuple(key), @@ -652,6 +668,29 @@ TaggedCache< return true; } +template < + class Key, + class T, + bool IsKeyCache, + class SharedWeakUnionPointer, + class SharedPointerType, + class Hash, + class KeyEqual, + class Mutex> +inline auto +TaggedCache< + Key, + T, + IsKeyCache, + SharedWeakUnionPointer, + SharedPointerType, + Hash, + KeyEqual, + Mutex>::peekMutex() -> mutex_type& +{ + return m_mutex; +} + template < class Key, class T, @@ -675,13 +714,10 @@ TaggedCache< std::vector v; { + std::lock_guard lock(m_mutex); v.reserve(m_cache.size()); - for (std::size_t i = 0; i < partitionLocks_.size(); ++i) - { - std::lock_guard lock(partitionLocks_[i]); - for (auto const& entry : m_cache.map()[i]) - v.push_back(entry.first); - } + for (auto const& _ : m_cache) + v.push_back(_.first); } return v; @@ -707,12 +743,11 @@ TaggedCache< KeyEqual, Mutex>::rate() const { - auto const hits = m_hits.load(std::memory_order_relaxed); - auto const misses = m_misses.load(std::memory_order_relaxed); - auto const tot = hits + misses; + std::lock_guard lock(m_mutex); + auto const tot = m_hits + m_misses; if (tot == 0) - return 0.0; - return double(hits) / tot; + return 0; + return double(m_hits) / tot; } template < @@ -736,16 +771,18 @@ TaggedCache< KeyEqual, Mutex>::fetch(key_type const& digest, Handler const& h) { - std::lock_guard lock(lockPartition(digest)); - - if (auto ret = initialFetch(digest)) - return ret; + { + std::lock_guard l(m_mutex); + if (auto ret = initialFetch(digest, l)) + return ret; + } auto sle = h(); if (!sle) return {}; - m_misses.fetch_add(1, std::memory_order_relaxed); + std::lock_guard l(m_mutex); + ++m_misses; auto const [it, inserted] = m_cache.emplace(digest, Entry(m_clock.now(), std::move(sle))); if (!inserted) @@ -772,10 +809,9 @@ TaggedCache< SharedPointerType, Hash, KeyEqual, - Mutex>::initialFetch(key_type const& key) + Mutex>:: + initialFetch(key_type const& key, std::lock_guard const& l) { - std::lock_guard lock(lockPartition(key)); - auto cit = m_cache.find(key); if (cit == m_cache.end()) return {}; @@ -783,7 +819,7 @@ TaggedCache< Entry& entry = cit->second; if (entry.isCached()) { - m_hits.fetch_add(1, std::memory_order_relaxed); + ++m_hits; entry.touch(m_clock.now()); return entry.ptr.getStrong(); } @@ -791,13 +827,12 @@ TaggedCache< if (entry.isCached()) { // independent of cache size, so not counted as a hit - m_cache_count.fetch_add(1, std::memory_order_relaxed); + ++m_cache_count; entry.touch(m_clock.now()); return entry.ptr.getStrong(); } m_cache.erase(cit); - return {}; } @@ -826,11 +861,10 @@ TaggedCache< { beast::insight::Gauge::value_type hit_rate(0); { - auto const hits = m_hits.load(std::memory_order_relaxed); - auto const misses = m_misses.load(std::memory_order_relaxed); - auto const total = hits + misses; + std::lock_guard lock(m_mutex); + auto const total(m_hits + m_misses); if (total != 0) - hit_rate = (hits * 100) / total; + hit_rate = (m_hits * 100) / total; } m_stats.hit_rate.set(hit_rate); } @@ -861,16 +895,12 @@ TaggedCache< typename KeyValueCacheType::map_type& partition, SweptPointersVector& stuffToSweep, std::atomic& allRemovals, - Mutex& partitionLock) + std::lock_guard const&) { return std::thread([&, this]() { - beast::setCurrentThreadName("sweep-KVCache"); - int cacheRemovals = 0; int mapRemovals = 0; - std::lock_guard lock(partitionLock); - // Keep references to all the stuff we sweep // so that we can destroy them outside the lock. stuffToSweep.reserve(partition.size()); @@ -954,16 +984,12 @@ TaggedCache< typename KeyOnlyCacheType::map_type& partition, SweptPointersVector&, std::atomic& allRemovals, - Mutex& partitionLock) + std::lock_guard const&) { return std::thread([&, this]() { - beast::setCurrentThreadName("sweep-KCache"); - int cacheRemovals = 0; int mapRemovals = 0; - std::lock_guard lock(partitionLock); - // Keep references to all the stuff we sweep // so that we can destroy them outside the lock. { @@ -998,29 +1024,6 @@ TaggedCache< }); } -template < - class Key, - class T, - bool IsKeyCache, - class SharedWeakUnionPointer, - class SharedPointerType, - class Hash, - class KeyEqual, - class Mutex> -inline Mutex& -TaggedCache< - Key, - T, - IsKeyCache, - SharedWeakUnionPointer, - SharedPointerType, - Hash, - KeyEqual, - Mutex>::lockPartition(key_type const& key) const -{ - return partitionLocks_[m_cache.partition_index(key)]; -} - } // namespace ripple #endif diff --git a/include/xrpl/basics/partitioned_unordered_map.h b/include/xrpl/basics/partitioned_unordered_map.h index ecaf16a47e..4e503ad0fa 100644 --- a/include/xrpl/basics/partitioned_unordered_map.h +++ b/include/xrpl/basics/partitioned_unordered_map.h @@ -277,12 +277,6 @@ public: return map_; } - partition_map_type const& - map() const - { - return map_; - } - iterator begin() { @@ -327,12 +321,6 @@ public: return cend(); } - std::size_t - partition_index(key_type const& key) const - { - return partitioner(key); - } - private: template void diff --git a/include/xrpl/protocol/Protocol.h b/include/xrpl/protocol/Protocol.h index bd39233cca..898fd06fbd 100644 --- a/include/xrpl/protocol/Protocol.h +++ b/include/xrpl/protocol/Protocol.h @@ -22,6 +22,7 @@ #include #include +#include #include diff --git a/src/test/basics/TaggedCache_test.cpp b/src/test/basics/TaggedCache_test.cpp index ec450e46dd..3d3dba698d 100644 --- a/src/test/basics/TaggedCache_test.cpp +++ b/src/test/basics/TaggedCache_test.cpp @@ -58,10 +58,10 @@ public: // Insert an item, retrieve it, and age it so it gets purged. { BEAST_EXPECT(c.getCacheSize() == 0); - BEAST_EXPECT(c.size() == 0); + BEAST_EXPECT(c.getTrackSize() == 0); BEAST_EXPECT(!c.insert(1, "one")); BEAST_EXPECT(c.getCacheSize() == 1); - BEAST_EXPECT(c.size() == 1); + BEAST_EXPECT(c.getTrackSize() == 1); { std::string s; @@ -72,7 +72,7 @@ public: ++clock; c.sweep(); BEAST_EXPECT(c.getCacheSize() == 0); - BEAST_EXPECT(c.size() == 0); + BEAST_EXPECT(c.getTrackSize() == 0); } // Insert an item, maintain a strong pointer, age it, and @@ -80,7 +80,7 @@ public: { BEAST_EXPECT(!c.insert(2, "two")); BEAST_EXPECT(c.getCacheSize() == 1); - BEAST_EXPECT(c.size() == 1); + BEAST_EXPECT(c.getTrackSize() == 1); { auto p = c.fetch(2); @@ -88,14 +88,14 @@ public: ++clock; c.sweep(); BEAST_EXPECT(c.getCacheSize() == 0); - BEAST_EXPECT(c.size() == 1); + BEAST_EXPECT(c.getTrackSize() == 1); } // Make sure its gone now that our reference is gone ++clock; c.sweep(); BEAST_EXPECT(c.getCacheSize() == 0); - BEAST_EXPECT(c.size() == 0); + BEAST_EXPECT(c.getTrackSize() == 0); } // Insert the same key/value pair and make sure we get the same result @@ -111,7 +111,7 @@ public: ++clock; c.sweep(); BEAST_EXPECT(c.getCacheSize() == 0); - BEAST_EXPECT(c.size() == 0); + BEAST_EXPECT(c.getTrackSize() == 0); } // Put an object in but keep a strong pointer to it, advance the clock a @@ -121,24 +121,24 @@ public: // Put an object in BEAST_EXPECT(!c.insert(4, "four")); BEAST_EXPECT(c.getCacheSize() == 1); - BEAST_EXPECT(c.size() == 1); + BEAST_EXPECT(c.getTrackSize() == 1); { // Keep a strong pointer to it auto const p1 = c.fetch(4); BEAST_EXPECT(p1 != nullptr); BEAST_EXPECT(c.getCacheSize() == 1); - BEAST_EXPECT(c.size() == 1); + BEAST_EXPECT(c.getTrackSize() == 1); // Advance the clock a lot ++clock; c.sweep(); BEAST_EXPECT(c.getCacheSize() == 0); - BEAST_EXPECT(c.size() == 1); + BEAST_EXPECT(c.getTrackSize() == 1); // Canonicalize a new object with the same key auto p2 = std::make_shared("four"); BEAST_EXPECT(c.canonicalize_replace_client(4, p2)); BEAST_EXPECT(c.getCacheSize() == 1); - BEAST_EXPECT(c.size() == 1); + BEAST_EXPECT(c.getTrackSize() == 1); // Make sure we get the original object BEAST_EXPECT(p1.get() == p2.get()); } @@ -146,7 +146,7 @@ public: ++clock; c.sweep(); BEAST_EXPECT(c.getCacheSize() == 0); - BEAST_EXPECT(c.size() == 0); + BEAST_EXPECT(c.getTrackSize() == 0); } } }; diff --git a/src/xrpld/app/ledger/LedgerHistory.cpp b/src/xrpld/app/ledger/LedgerHistory.cpp index dcbd722120..ccec209bd4 100644 --- a/src/xrpld/app/ledger/LedgerHistory.cpp +++ b/src/xrpld/app/ledger/LedgerHistory.cpp @@ -63,6 +63,8 @@ LedgerHistory::insert( ledger->stateMap().getHash().isNonZero(), "ripple::LedgerHistory::insert : nonzero hash"); + std::unique_lock sl(m_ledgers_by_hash.peekMutex()); + bool const alreadyHad = m_ledgers_by_hash.canonicalize_replace_cache( ledger->info().hash, ledger); if (validated) @@ -74,6 +76,7 @@ LedgerHistory::insert( LedgerHash LedgerHistory::getLedgerHash(LedgerIndex index) { + std::unique_lock sl(m_ledgers_by_hash.peekMutex()); if (auto it = mLedgersByIndex.find(index); it != mLedgersByIndex.end()) return it->second; return {}; @@ -83,11 +86,13 @@ std::shared_ptr LedgerHistory::getLedgerBySeq(LedgerIndex index) { { + std::unique_lock sl(m_ledgers_by_hash.peekMutex()); auto it = mLedgersByIndex.find(index); if (it != mLedgersByIndex.end()) { uint256 hash = it->second; + sl.unlock(); return getLedgerByHash(hash); } } @@ -103,6 +108,7 @@ LedgerHistory::getLedgerBySeq(LedgerIndex index) { // Add this ledger to the local tracking by index + std::unique_lock sl(m_ledgers_by_hash.peekMutex()); XRPL_ASSERT( ret->isImmutable(), @@ -452,6 +458,8 @@ LedgerHistory::builtLedger( XRPL_ASSERT( !hash.isZero(), "ripple::LedgerHistory::builtLedger : nonzero hash"); + std::unique_lock sl(m_consensus_validated.peekMutex()); + auto entry = std::make_shared(); m_consensus_validated.canonicalize_replace_client(index, entry); @@ -492,6 +500,8 @@ LedgerHistory::validatedLedger( !hash.isZero(), "ripple::LedgerHistory::validatedLedger : nonzero hash"); + std::unique_lock sl(m_consensus_validated.peekMutex()); + auto entry = std::make_shared(); m_consensus_validated.canonicalize_replace_client(index, entry); @@ -525,9 +535,10 @@ LedgerHistory::validatedLedger( bool LedgerHistory::fixIndex(LedgerIndex ledgerIndex, LedgerHash const& ledgerHash) { - auto ledger = m_ledgers_by_hash.fetch(ledgerHash); + std::unique_lock sl(m_ledgers_by_hash.peekMutex()); auto it = mLedgersByIndex.find(ledgerIndex); - if (ledger && (it != mLedgersByIndex.end()) && (it->second != ledgerHash)) + + if ((it != mLedgersByIndex.end()) && (it->second != ledgerHash)) { it->second = ledgerHash; return false; diff --git a/src/xrpld/rpc/handlers/GetCounts.cpp b/src/xrpld/rpc/handlers/GetCounts.cpp index 2987da46d5..3c1d8cccdd 100644 --- a/src/xrpld/rpc/handlers/GetCounts.cpp +++ b/src/xrpld/rpc/handlers/GetCounts.cpp @@ -114,7 +114,7 @@ getCountsJson(Application& app, int minObjectCount) ret[jss::treenode_cache_size] = app.getNodeFamily().getTreeNodeCache()->getCacheSize(); ret[jss::treenode_track_size] = - static_cast(app.getNodeFamily().getTreeNodeCache()->size()); + app.getNodeFamily().getTreeNodeCache()->getTrackSize(); std::string uptime; auto s = UptimeClock::now(); From c5fe97064678ff8cdf2762acc69b814142db0757 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 22 Aug 2025 17:32:31 -0400 Subject: [PATCH 2/2] Set version to 2.6.0-rc3 --- src/libxrpl/protocol/BuildInfo.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libxrpl/protocol/BuildInfo.cpp b/src/libxrpl/protocol/BuildInfo.cpp index 0d7ea1a7ca..4b55f82f49 100644 --- a/src/libxrpl/protocol/BuildInfo.cpp +++ b/src/libxrpl/protocol/BuildInfo.cpp @@ -36,7 +36,7 @@ namespace BuildInfo { // and follow the format described at http://semver.org/ //------------------------------------------------------------------------------ // clang-format off -char const* const versionString = "2.6.0-rc2" +char const* const versionString = "2.6.0-rc3" // clang-format on #if defined(DEBUG) || defined(SANITIZER)