From 316f9535e3c700b8d0f3d5b72bd17470bd9d082c Mon Sep 17 00:00:00 2001 From: Valentin Balaschenko <13349202+vlntb@users.noreply.github.com> Date: Wed, 11 Jun 2025 14:31:26 +0100 Subject: [PATCH 1/5] wip removing mutex and dependencies --- include/xrpl/basics/TaggedCache.h | 14 ++--- include/xrpl/basics/TaggedCache.ipp | 62 +++++-------------- .../xrpl/basics/partitioned_unordered_map.h | 4 +- src/xrpld/app/ledger/LedgerHistory.cpp | 22 ++++--- 4 files changed, 36 insertions(+), 66 deletions(-) diff --git a/include/xrpl/basics/TaggedCache.h b/include/xrpl/basics/TaggedCache.h index 99c91fe393..192399bc19 100644 --- a/include/xrpl/basics/TaggedCache.h +++ b/include/xrpl/basics/TaggedCache.h @@ -170,9 +170,6 @@ public: bool retrieve(key_type const& key, T& data); - mutex_type& - peekMutex(); - std::vector getKeys() const; @@ -193,7 +190,7 @@ public: private: SharedPointerType - initialFetch(key_type const& key, std::lock_guard const& l); + initialFetch(key_type const& key); void collect_metrics(); @@ -328,10 +325,13 @@ private: clock_type::duration const m_target_age; // Number of items cached - int m_cache_count; + int m_cache_count; // TODO: 1) make atomic, 2) think about mem ordering + // access cache_type m_cache; // Hold strong reference to recent objects - std::uint64_t m_hits; - std::uint64_t m_misses; + std::uint64_t + m_hits; // TODO: 1) make atomic, 2) think about mem ordering access + std::uint64_t + m_misses; // TODO: 1) make atomic, 2) think about mem ordering access }; } // namespace ripple diff --git a/include/xrpl/basics/TaggedCache.ipp b/include/xrpl/basics/TaggedCache.ipp index 16a3f7587a..540b20b864 100644 --- a/include/xrpl/basics/TaggedCache.ipp +++ b/include/xrpl/basics/TaggedCache.ipp @@ -105,7 +105,6 @@ TaggedCache< KeyEqual, Mutex>::size() const { - std::lock_guard lock(m_mutex); return m_cache.size(); } @@ -129,7 +128,6 @@ TaggedCache< KeyEqual, Mutex>::getCacheSize() const { - std::lock_guard lock(m_mutex); return m_cache_count; } @@ -153,7 +151,6 @@ TaggedCache< KeyEqual, Mutex>::getTrackSize() const { - std::lock_guard lock(m_mutex); return m_cache.size(); } @@ -177,7 +174,6 @@ TaggedCache< KeyEqual, Mutex>::getHitRate() { - 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)); } @@ -202,7 +198,6 @@ TaggedCache< KeyEqual, Mutex>::clear() { - std::lock_guard lock(m_mutex); m_cache.clear(); m_cache_count = 0; } @@ -227,7 +222,6 @@ TaggedCache< KeyEqual, Mutex>::reset() { - std::lock_guard lock(m_mutex); m_cache.clear(); m_cache_count = 0; m_hits = 0; @@ -255,7 +249,7 @@ TaggedCache< KeyEqual, Mutex>::touch_if_exists(KeyComparable const& key) { - std::lock_guard lock(m_mutex); + // TOOD: acuiqring iterator should lock the partition auto const iter(m_cache.find(key)); if (iter == m_cache.end()) { @@ -369,8 +363,7 @@ TaggedCache< { // Remove from cache, if !valid, remove from map too. Returns true if // removed from cache - std::lock_guard lock(m_mutex); - + // TODO: acuiqring iterator should lock the partition auto cit = m_cache.find(key); if (cit == m_cache.end()) @@ -420,8 +413,8 @@ TaggedCache< { // Return canonical value, store if needed, refresh in cache // Return values: true=we had the data already - std::lock_guard lock(m_mutex); + // TODO: acuiqring iterator should lock the partition auto cit = m_cache.find(key); if (cit == m_cache.end()) @@ -560,8 +553,8 @@ TaggedCache< KeyEqual, Mutex>::fetch(key_type const& key) { - std::lock_guard l(m_mutex); - auto ret = initialFetch(key, l); + // TODO: do we need any lock here, since we are returing a shared pointer? + auto ret = initialFetch(key); if (!ret) ++m_misses; return ret; @@ -627,9 +620,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()); - auto [it, inserted] = m_cache.emplace( + auto [it, inserted] = m_cache.emplace( // TODO: make sure partition is locked std::piecewise_construct, std::forward_as_tuple(key), std::forward_as_tuple(now)); @@ -668,29 +660,6 @@ 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, @@ -714,9 +683,8 @@ TaggedCache< std::vector v; { - std::lock_guard lock(m_mutex); v.reserve(m_cache.size()); - for (auto const& _ : m_cache) + for (auto const& _ : m_cache) // TODO: make sure partition is locked v.push_back(_.first); } @@ -743,7 +711,6 @@ TaggedCache< KeyEqual, Mutex>::rate() const { - std::lock_guard lock(m_mutex); auto const tot = m_hits + m_misses; if (tot == 0) return 0; @@ -771,9 +738,9 @@ TaggedCache< KeyEqual, Mutex>::fetch(key_type const& digest, Handler const& h) { - { - std::lock_guard l(m_mutex); - if (auto ret = initialFetch(digest, l)) + // TODO: do we need any lock here, since we are returing a shared pointer? + { // TODO: potenially remove this scope + if (auto ret = initialFetch(digest)) return ret; } @@ -781,7 +748,6 @@ TaggedCache< if (!sle) return {}; - std::lock_guard l(m_mutex); ++m_misses; auto const [it, inserted] = m_cache.emplace(digest, Entry(m_clock.now(), std::move(sle))); @@ -809,8 +775,7 @@ TaggedCache< SharedPointerType, Hash, KeyEqual, - Mutex>:: - initialFetch(key_type const& key, std::lock_guard const& l) + Mutex>::initialFetch(key_type const& key) { auto cit = m_cache.find(key); if (cit == m_cache.end()) @@ -823,7 +788,7 @@ TaggedCache< entry.touch(m_clock.now()); return entry.ptr.getStrong(); } - entry.ptr = entry.lock(); + entry.ptr = entry.lock(); // TODO what is this? if (entry.isCached()) { // independent of cache size, so not counted as a hit @@ -832,7 +797,8 @@ TaggedCache< return entry.ptr.getStrong(); } - m_cache.erase(cit); + m_cache.erase(cit); // TODO: if this erase happens on fetch, what is left + // for a sweep? return {}; } diff --git a/include/xrpl/basics/partitioned_unordered_map.h b/include/xrpl/basics/partitioned_unordered_map.h index 4e503ad0fa..7d11b11005 100644 --- a/include/xrpl/basics/partitioned_unordered_map.h +++ b/include/xrpl/basics/partitioned_unordered_map.h @@ -380,7 +380,7 @@ public: clear() { for (auto& p : map_) - p.clear(); + p.clear(); // TODO make sure that it is locked inside } iterator @@ -406,7 +406,7 @@ public: { std::size_t ret = 0; for (auto& p : map_) - ret += p.size(); + ret += p.size(); // TODO make sure that it is locked inside return ret; } diff --git a/src/xrpld/app/ledger/LedgerHistory.cpp b/src/xrpld/app/ledger/LedgerHistory.cpp index ccec209bd4..a59cdcabf5 100644 --- a/src/xrpld/app/ledger/LedgerHistory.cpp +++ b/src/xrpld/app/ledger/LedgerHistory.cpp @@ -63,8 +63,8 @@ LedgerHistory::insert( ledger->stateMap().getHash().isNonZero(), "ripple::LedgerHistory::insert : nonzero hash"); - std::unique_lock sl(m_ledgers_by_hash.peekMutex()); - + // TODOL merge the below into a single call to avoid lock and race + // conditions, i.e. - return alreadyHad on assignment somehow. bool const alreadyHad = m_ledgers_by_hash.canonicalize_replace_cache( ledger->info().hash, ledger); if (validated) @@ -76,7 +76,7 @@ LedgerHistory::insert( LedgerHash LedgerHistory::getLedgerHash(LedgerIndex index) { - std::unique_lock sl(m_ledgers_by_hash.peekMutex()); + // TODO: is it safe to get iterator without lock here? if (auto it = mLedgersByIndex.find(index); it != mLedgersByIndex.end()) return it->second; return {}; @@ -86,13 +86,12 @@ std::shared_ptr LedgerHistory::getLedgerBySeq(LedgerIndex index) { { - std::unique_lock sl(m_ledgers_by_hash.peekMutex()); + // TODO: this lock is not needed auto it = mLedgersByIndex.find(index); if (it != mLedgersByIndex.end()) { uint256 hash = it->second; - sl.unlock(); return getLedgerByHash(hash); } } @@ -108,7 +107,8 @@ LedgerHistory::getLedgerBySeq(LedgerIndex index) { // Add this ledger to the local tracking by index - std::unique_lock sl(m_ledgers_by_hash.peekMutex()); + // std::unique_lock sl(m_ledgers_by_hash.peekMutex()); + // TODO: make sure that canonicalize_replace_client lock the partition XRPL_ASSERT( ret->isImmutable(), @@ -458,7 +458,8 @@ LedgerHistory::builtLedger( XRPL_ASSERT( !hash.isZero(), "ripple::LedgerHistory::builtLedger : nonzero hash"); - std::unique_lock sl(m_consensus_validated.peekMutex()); + // std::unique_lock sl(m_consensus_validated.peekMutex()); + // TODO: make sure that canonicalize_replace_client lock the partition auto entry = std::make_shared(); m_consensus_validated.canonicalize_replace_client(index, entry); @@ -500,7 +501,8 @@ LedgerHistory::validatedLedger( !hash.isZero(), "ripple::LedgerHistory::validatedLedger : nonzero hash"); - std::unique_lock sl(m_consensus_validated.peekMutex()); + // std::unique_lock sl(m_consensus_validated.peekMutex()); + // TODO: make sure that canonicalize_replace_client lock the partition auto entry = std::make_shared(); m_consensus_validated.canonicalize_replace_client(index, entry); @@ -535,7 +537,9 @@ LedgerHistory::validatedLedger( bool LedgerHistory::fixIndex(LedgerIndex ledgerIndex, LedgerHash const& ledgerHash) { - std::unique_lock sl(m_ledgers_by_hash.peekMutex()); + // std::unique_lock sl(m_ledgers_by_hash.peekMutex()); + // TODO: how to ensure that? "Ensure m_ledgers_by_hash doesn't have the + // wrong hash for a particular index" auto it = mLedgersByIndex.find(ledgerIndex); if ((it != mLedgersByIndex.end()) && (it->second != ledgerHash)) From 3effb54e4914ea03216dea09aa2c7d516e7ab3ec Mon Sep 17 00:00:00 2001 From: Valentin Balaschenko <13349202+vlntb@users.noreply.github.com> Date: Wed, 11 Jun 2025 16:31:48 +0100 Subject: [PATCH 2/5] wip atomics for counters --- include/xrpl/basics/TaggedCache.h | 9 ++---- include/xrpl/basics/TaggedCache.ipp | 50 ++++++++++++++++------------- 2 files changed, 31 insertions(+), 28 deletions(-) diff --git a/include/xrpl/basics/TaggedCache.h b/include/xrpl/basics/TaggedCache.h index 192399bc19..46e7654f02 100644 --- a/include/xrpl/basics/TaggedCache.h +++ b/include/xrpl/basics/TaggedCache.h @@ -325,13 +325,10 @@ private: clock_type::duration const m_target_age; // Number of items cached - int m_cache_count; // TODO: 1) make atomic, 2) think about mem ordering - // access + std::atomic m_cache_count; cache_type m_cache; // Hold strong reference to recent objects - std::uint64_t - m_hits; // TODO: 1) make atomic, 2) think about mem ordering access - std::uint64_t - m_misses; // TODO: 1) make atomic, 2) think about mem ordering access + std::atomic m_hits; + std::atomic m_misses; }; } // namespace ripple diff --git a/include/xrpl/basics/TaggedCache.ipp b/include/xrpl/basics/TaggedCache.ipp index 540b20b864..9550dae8f6 100644 --- a/include/xrpl/basics/TaggedCache.ipp +++ b/include/xrpl/basics/TaggedCache.ipp @@ -128,7 +128,7 @@ TaggedCache< KeyEqual, Mutex>::getCacheSize() const { - return m_cache_count; + return m_cache_count.load(std::memory_order_relaxed); } template < @@ -174,8 +174,10 @@ TaggedCache< KeyEqual, Mutex>::getHitRate() { - auto const total = static_cast(m_hits + m_misses); - return m_hits * (100.0f / std::max(1.0f, total)); + auto hits = m_hits.load(std::memory_order_relaxed); + auto misses = m_misses.load(std::memory_order_relaxed); + float total = float(hits + misses); + return hits * (100.0f / std::max(1.0f, total)); } template < @@ -199,7 +201,7 @@ TaggedCache< Mutex>::clear() { m_cache.clear(); - m_cache_count = 0; + m_cache_count.store(0, std::memory_order_relaxed); } template < @@ -223,9 +225,9 @@ TaggedCache< Mutex>::reset() { m_cache.clear(); - m_cache_count = 0; - m_hits = 0; - m_misses = 0; + m_cache_count.store(0, std::memory_order_relaxed); + m_hits.store(0, std::memory_order_relaxed); + m_misses.store(0, std::memory_order_relaxed); } template < @@ -329,7 +331,8 @@ TaggedCache< for (std::thread& worker : workers) worker.join(); - m_cache_count -= allRemovals; + int removals = allRemovals.load(std::memory_order_relaxed); + m_cache_count.fetch_sub(removals, std::memory_order_relaxed); } // At this point allStuffToSweep will go out of scope outside the lock // and decrement the reference count on each strong pointer. @@ -375,7 +378,7 @@ TaggedCache< if (entry.isCached()) { - --m_cache_count; + m_cache_count.fetch_sub(1, std::memory_order_relaxed); entry.ptr.convertToWeak(); ret = true; } @@ -423,7 +426,7 @@ TaggedCache< std::piecewise_construct, std::forward_as_tuple(key), std::forward_as_tuple(m_clock.now(), data)); - ++m_cache_count; + m_cache_count.fetch_add(1, std::memory_order_relaxed); return false; } @@ -472,12 +475,12 @@ TaggedCache< data = cachedData; } - ++m_cache_count; + m_cache_count.fetch_add(1, std::memory_order_relaxed); return true; } entry.ptr = data; - ++m_cache_count; + m_cache_count.fetch_add(1, std::memory_order_relaxed); return false; } @@ -556,7 +559,7 @@ TaggedCache< // TODO: do we need any lock here, since we are returing a shared pointer? auto ret = initialFetch(key); if (!ret) - ++m_misses; + m_misses.fetch_add(1, std::memory_order_relaxed); return ret; } @@ -711,10 +714,12 @@ TaggedCache< KeyEqual, Mutex>::rate() const { - auto const tot = m_hits + m_misses; + auto hits = m_hits.load(std::memory_order_relaxed); + auto misses = m_misses.load(std::memory_order_relaxed); + auto const tot = hits + misses; if (tot == 0) - return 0; - return double(m_hits) / tot; + return 0.0; + return double(hits) / tot; } template < @@ -748,7 +753,7 @@ TaggedCache< if (!sle) return {}; - ++m_misses; + m_misses.fetch_add(1, std::memory_order_relaxed); auto const [it, inserted] = m_cache.emplace(digest, Entry(m_clock.now(), std::move(sle))); if (!inserted) @@ -784,7 +789,7 @@ TaggedCache< Entry& entry = cit->second; if (entry.isCached()) { - ++m_hits; + m_hits.fetch_add(1, std::memory_order_relaxed); entry.touch(m_clock.now()); return entry.ptr.getStrong(); } @@ -792,7 +797,7 @@ TaggedCache< if (entry.isCached()) { // independent of cache size, so not counted as a hit - ++m_cache_count; + m_cache_count.fetch_add(1, std::memory_order_relaxed); entry.touch(m_clock.now()); return entry.ptr.getStrong(); } @@ -827,10 +832,11 @@ TaggedCache< { beast::insight::Gauge::value_type hit_rate(0); { - std::lock_guard lock(m_mutex); - auto const total(m_hits + m_misses); + 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; if (total != 0) - hit_rate = (m_hits * 100) / total; + hit_rate = (hits * 100) / total; } m_stats.hit_rate.set(hit_rate); } From 984c70955a2076b290db31ca714824c6b76dabac Mon Sep 17 00:00:00 2001 From: Valentin Balaschenko <13349202+vlntb@users.noreply.github.com> Date: Wed, 11 Jun 2025 16:33:06 +0100 Subject: [PATCH 3/5] clang --- include/xrpl/basics/TaggedCache.ipp | 9 +++++---- include/xrpl/basics/partitioned_unordered_map.h | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/include/xrpl/basics/TaggedCache.ipp b/include/xrpl/basics/TaggedCache.ipp index 9550dae8f6..8803fd2681 100644 --- a/include/xrpl/basics/TaggedCache.ipp +++ b/include/xrpl/basics/TaggedCache.ipp @@ -624,10 +624,11 @@ TaggedCache< -> std::enable_if_t { clock_type::time_point const now(m_clock.now()); - auto [it, inserted] = m_cache.emplace( // TODO: make sure partition is locked - std::piecewise_construct, - std::forward_as_tuple(key), - std::forward_as_tuple(now)); + auto [it, inserted] = + m_cache.emplace( // TODO: make sure partition is locked + std::piecewise_construct, + std::forward_as_tuple(key), + std::forward_as_tuple(now)); if (!inserted) it->second.last_access = now; return inserted; diff --git a/include/xrpl/basics/partitioned_unordered_map.h b/include/xrpl/basics/partitioned_unordered_map.h index 7d11b11005..ec0fb298b8 100644 --- a/include/xrpl/basics/partitioned_unordered_map.h +++ b/include/xrpl/basics/partitioned_unordered_map.h @@ -380,7 +380,7 @@ public: clear() { for (auto& p : map_) - p.clear(); // TODO make sure that it is locked inside + p.clear(); // TODO make sure that it is locked inside } iterator @@ -406,7 +406,7 @@ public: { std::size_t ret = 0; for (auto& p : map_) - ret += p.size(); // TODO make sure that it is locked inside + ret += p.size(); // TODO make sure that it is locked inside return ret; } From d0f836581b6997591ac744a8672650947e7f0a5f Mon Sep 17 00:00:00 2001 From: Valentin Balaschenko <13349202+vlntb@users.noreply.github.com> Date: Fri, 13 Jun 2025 14:45:27 +0100 Subject: [PATCH 4/5] remove mutex and dead references --- include/xrpl/basics/SHAMapHash.h | 1 - include/xrpl/basics/TaggedCache.h | 8 ++------ include/xrpl/basics/TaggedCache.ipp | 11 ++++------- include/xrpl/protocol/Protocol.h | 1 - 4 files changed, 6 insertions(+), 15 deletions(-) diff --git a/include/xrpl/basics/SHAMapHash.h b/include/xrpl/basics/SHAMapHash.h index 2d2dcdc3ef..1ec326409c 100644 --- a/include/xrpl/basics/SHAMapHash.h +++ b/include/xrpl/basics/SHAMapHash.h @@ -21,7 +21,6 @@ #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 46e7654f02..05c493c186 100644 --- a/include/xrpl/basics/TaggedCache.h +++ b/include/xrpl/basics/TaggedCache.h @@ -297,8 +297,7 @@ private: [[maybe_unused]] clock_type::time_point const& now, typename KeyValueCacheType::map_type& partition, SweptPointersVector& stuffToSweep, - std::atomic& allRemovals, - std::lock_guard const&); + std::atomic& allRemoval); [[nodiscard]] std::thread sweepHelper( @@ -306,15 +305,12 @@ private: clock_type::time_point const& now, typename KeyOnlyCacheType::map_type& partition, SweptPointersVector&, - std::atomic& allRemovals, - std::lock_guard const&); + std::atomic& allRemovals); beast::Journal m_journal; clock_type& m_clock; Stats m_stats; - mutex_type mutable m_mutex; - // Used for logging std::string m_name; diff --git a/include/xrpl/basics/TaggedCache.ipp b/include/xrpl/basics/TaggedCache.ipp index 8803fd2681..02e7ae2203 100644 --- a/include/xrpl/basics/TaggedCache.ipp +++ b/include/xrpl/basics/TaggedCache.ipp @@ -293,7 +293,7 @@ TaggedCache< auto const start = std::chrono::steady_clock::now(); { - std::lock_guard lock(m_mutex); + // TODO: lock the cache partition if (m_target_size == 0 || (static_cast(m_cache.size()) <= m_target_size)) @@ -325,8 +325,7 @@ TaggedCache< now, m_cache.map()[p], allStuffToSweep[p], - allRemovals, - lock)); + allRemovals)); } for (std::thread& worker : workers) worker.join(); @@ -867,8 +866,7 @@ TaggedCache< [[maybe_unused]] clock_type::time_point const& now, typename KeyValueCacheType::map_type& partition, SweptPointersVector& stuffToSweep, - std::atomic& allRemovals, - std::lock_guard const&) + std::atomic& allRemovals) { return std::thread([&, this]() { int cacheRemovals = 0; @@ -956,8 +954,7 @@ TaggedCache< clock_type::time_point const& now, typename KeyOnlyCacheType::map_type& partition, SweptPointersVector&, - std::atomic& allRemovals, - std::lock_guard const&) + std::atomic& allRemovals) { return std::thread([&, this]() { int cacheRemovals = 0; diff --git a/include/xrpl/protocol/Protocol.h b/include/xrpl/protocol/Protocol.h index 898fd06fbd..bd39233cca 100644 --- a/include/xrpl/protocol/Protocol.h +++ b/include/xrpl/protocol/Protocol.h @@ -22,7 +22,6 @@ #include #include -#include #include From da694c830490ed2e81e760df74dd54f342db98ae Mon Sep 17 00:00:00 2001 From: Valentin Balaschenko <13349202+vlntb@users.noreply.github.com> Date: Fri, 13 Jun 2025 18:33:00 +0100 Subject: [PATCH 5/5] wip lock per partition --- include/xrpl/basics/TaggedCache.h | 10 +- include/xrpl/basics/TaggedCache.ipp | 100 +++++++++++++----- .../xrpl/basics/partitioned_unordered_map.h | 12 +++ 3 files changed, 95 insertions(+), 27 deletions(-) diff --git a/include/xrpl/basics/TaggedCache.h b/include/xrpl/basics/TaggedCache.h index 05c493c186..d1e2c46253 100644 --- a/include/xrpl/basics/TaggedCache.h +++ b/include/xrpl/basics/TaggedCache.h @@ -195,6 +195,9 @@ private: void collect_metrics(); + Mutex& + lockPartition(key_type const& key) const; + private: struct Stats { @@ -297,7 +300,8 @@ private: [[maybe_unused]] clock_type::time_point const& now, typename KeyValueCacheType::map_type& partition, SweptPointersVector& stuffToSweep, - std::atomic& allRemoval); + std::atomic& allRemoval, + Mutex& partitionLock); [[nodiscard]] std::thread sweepHelper( @@ -305,7 +309,8 @@ private: clock_type::time_point const& now, typename KeyOnlyCacheType::map_type& partition, SweptPointersVector&, - std::atomic& allRemovals); + std::atomic& allRemovals, + Mutex& partitionLock); beast::Journal m_journal; clock_type& m_clock; @@ -325,6 +330,7 @@ private: cache_type m_cache; // Hold strong reference to recent objects std::atomic m_hits; std::atomic m_misses; + mutable std::vector partitionLocks_; }; } // namespace ripple diff --git a/include/xrpl/basics/TaggedCache.ipp b/include/xrpl/basics/TaggedCache.ipp index 02e7ae2203..41b13297cf 100644 --- a/include/xrpl/basics/TaggedCache.ipp +++ b/include/xrpl/basics/TaggedCache.ipp @@ -60,6 +60,7 @@ inline TaggedCache< , m_hits(0) , m_misses(0) { + partitionLocks_ = std::vector(m_cache.partitions()); } template < @@ -105,7 +106,13 @@ TaggedCache< KeyEqual, Mutex>::size() const { - return m_cache.size(); + 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; } template < @@ -151,7 +158,7 @@ TaggedCache< KeyEqual, Mutex>::getTrackSize() const { - return m_cache.size(); + return size(); } template < @@ -200,7 +207,11 @@ TaggedCache< KeyEqual, Mutex>::clear() { + for (auto& mutex : partitionLocks_) + mutex.lock(); m_cache.clear(); + for (auto& mutex : partitionLocks_) + mutex.unlock(); m_cache_count.store(0, std::memory_order_relaxed); } @@ -224,7 +235,11 @@ TaggedCache< KeyEqual, Mutex>::reset() { + for (auto& mutex : partitionLocks_) + mutex.lock(); m_cache.clear(); + for (auto& mutex : partitionLocks_) + mutex.unlock(); m_cache_count.store(0, std::memory_order_relaxed); m_hits.store(0, std::memory_order_relaxed); m_misses.store(0, std::memory_order_relaxed); @@ -251,7 +266,7 @@ TaggedCache< KeyEqual, Mutex>::touch_if_exists(KeyComparable const& key) { - // TOOD: acuiqring iterator should lock the partition + std::lock_guard lock(lockPartition(key)); auto const iter(m_cache.find(key)); if (iter == m_cache.end()) { @@ -293,8 +308,6 @@ TaggedCache< auto const start = std::chrono::steady_clock::now(); { - // TODO: lock the cache partition - if (m_target_size == 0 || (static_cast(m_cache.size()) <= m_target_size)) { @@ -325,7 +338,8 @@ TaggedCache< now, m_cache.map()[p], allStuffToSweep[p], - allRemovals)); + allRemovals, + partitionLocks_[p])); } for (std::thread& worker : workers) worker.join(); @@ -365,7 +379,9 @@ TaggedCache< { // Remove from cache, if !valid, remove from map too. Returns true if // removed from cache - // TODO: acuiqring iterator should lock the partition + + std::lock_guard lock(lockPartition(key)); + auto cit = m_cache.find(key); if (cit == m_cache.end()) @@ -416,9 +432,8 @@ TaggedCache< // Return canonical value, store if needed, refresh in cache // Return values: true=we had the data already - // TODO: acuiqring iterator should lock the partition + std::lock_guard lock(lockPartition(key)); auto cit = m_cache.find(key); - if (cit == m_cache.end()) { m_cache.emplace( @@ -555,7 +570,8 @@ TaggedCache< KeyEqual, Mutex>::fetch(key_type const& key) { - // TODO: do we need any lock here, since we are returing a shared pointer? + std::lock_guard lock(lockPartition(key)); + auto ret = initialFetch(key); if (!ret) m_misses.fetch_add(1, std::memory_order_relaxed); @@ -623,11 +639,11 @@ TaggedCache< -> std::enable_if_t { clock_type::time_point const now(m_clock.now()); - auto [it, inserted] = - m_cache.emplace( // TODO: make sure partition is locked - std::piecewise_construct, - std::forward_as_tuple(key), - std::forward_as_tuple(now)); + std::lock_guard lock(lockPartition(key)); + auto [it, inserted] = m_cache.emplace( + std::piecewise_construct, + std::forward_as_tuple(key), + std::forward_as_tuple(now)); if (!inserted) it->second.last_access = now; return inserted; @@ -687,8 +703,12 @@ TaggedCache< { v.reserve(m_cache.size()); - for (auto const& _ : m_cache) // TODO: make sure partition is locked - v.push_back(_.first); + 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); + } } return v; @@ -743,11 +763,10 @@ TaggedCache< KeyEqual, Mutex>::fetch(key_type const& digest, Handler const& h) { - // TODO: do we need any lock here, since we are returing a shared pointer? - { // TODO: potenially remove this scope - if (auto ret = initialFetch(digest)) - return ret; - } + std::lock_guard lock(lockPartition(digest)); + + if (auto ret = initialFetch(digest)) + return ret; auto sle = h(); if (!sle) @@ -782,6 +801,8 @@ TaggedCache< KeyEqual, Mutex>::initialFetch(key_type const& key) { + std::lock_guard lock(lockPartition(key)); + auto cit = m_cache.find(key); if (cit == m_cache.end()) return {}; @@ -793,7 +814,7 @@ TaggedCache< entry.touch(m_clock.now()); return entry.ptr.getStrong(); } - entry.ptr = entry.lock(); // TODO what is this? + entry.ptr = entry.lock(); if (entry.isCached()) { // independent of cache size, so not counted as a hit @@ -866,12 +887,15 @@ TaggedCache< [[maybe_unused]] clock_type::time_point const& now, typename KeyValueCacheType::map_type& partition, SweptPointersVector& stuffToSweep, - std::atomic& allRemovals) + std::atomic& allRemovals, + Mutex& partitionLock) { return std::thread([&, this]() { 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,12 +978,15 @@ TaggedCache< clock_type::time_point const& now, typename KeyOnlyCacheType::map_type& partition, SweptPointersVector&, - std::atomic& allRemovals) + std::atomic& allRemovals, + Mutex& partitionLock) { return std::thread([&, this]() { 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. { @@ -994,6 +1021,29 @@ 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 ec0fb298b8..d45221e4fb 100644 --- a/include/xrpl/basics/partitioned_unordered_map.h +++ b/include/xrpl/basics/partitioned_unordered_map.h @@ -277,6 +277,12 @@ public: return map_; } + partition_map_type const& + map() const + { + return map_; + } + iterator begin() { @@ -321,6 +327,12 @@ public: return cend(); } + std::size_t + partition_index(key_type const& key) const + { + return partitioner(key); + } + private: template void