From 71fcf5e89a852d24298e5ba3eaabae973fc7db19 Mon Sep 17 00:00:00 2001 From: Prasanna Loganathar Date: Mon, 6 Mar 2023 15:24:43 +0800 Subject: [PATCH] Eliminate the use of CLockFreeGuard with AtomicMutex (#1800) * Make CLockFreeGuard safer * Fix getburninfo consortium and result allocations * Fix CLockFreeGuard, introduce AtomicMutex * Fix missing expected * Use larger thread quantums * Increase default yields since sleep quantums are larger * Fix syntax * Add more comments * Introduce BufferPool * Switch getburninfo to use BufferPool * Eliminate CLockFreeGuard, use AtomicMutex --------- Co-authored-by: Peter John Bushnell --- src/miner.cpp | 4 ++-- src/miner.h | 4 +++- src/rpc/mining.cpp | 2 +- src/rpc/resultcache.cpp | 14 +++++++------- src/rpc/resultcache.h | 4 ++-- src/rpc/stats.cpp | 8 ++++---- src/rpc/stats.h | 2 +- src/sync.h | 26 -------------------------- src/test/sync_tests.cpp | 4 ++-- src/wallet/ismine.cpp | 8 ++++---- 10 files changed, 26 insertions(+), 50 deletions(-) diff --git a/src/miner.cpp b/src/miner.cpp index 267fd76d23..695b3d6c9e 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -717,7 +717,7 @@ namespace pos { //initialize static variables here std::map Staker::mapMNLastBlockCreationAttemptTs; - std::atomic_bool Staker::cs_MNLastBlockCreationAttemptTs(false); + AtomicMutex cs_MNLastBlockCreationAttemptTs; int64_t Staker::nLastCoinStakeSearchTime{0}; int64_t Staker::nFutureTime{0}; uint256 Staker::lastBlockSeen{}; @@ -825,7 +825,7 @@ namespace pos { withSearchInterval([&](const int64_t currentTime, const int64_t lastSearchTime, const int64_t futureTime) { // update last block creation attempt ts for the master node here { - CLockFreeGuard lock(pos::Staker::cs_MNLastBlockCreationAttemptTs); + std::unique_lock l{pos::cs_MNLastBlockCreationAttemptTs}; pos::Staker::mapMNLastBlockCreationAttemptTs[masternodeID] = GetTime(); } CheckContextState ctxState; diff --git a/src/miner.h b/src/miner.h index 244153f57f..2a9a29c5d3 100644 --- a/src/miner.h +++ b/src/miner.h @@ -224,6 +224,9 @@ namespace pos { // The main staking routine. // Creates stakes using CWallet API, creates PoS kernels and mints blocks. // Uses Args.getWallets() to receive and update wallets list. + + extern AtomicMutex cs_MNLastBlockCreationAttemptTs; + class ThreadStaker { public: @@ -258,7 +261,6 @@ namespace pos { // declaration static variables // Map to store [master node id : last block creation attempt timestamp] for local master nodes static std::map mapMNLastBlockCreationAttemptTs; - static std::atomic_bool cs_MNLastBlockCreationAttemptTs; // Variables to manage search time across threads static int64_t nLastCoinStakeSearchTime; diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 6aafed7e44..430d433575 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -293,7 +293,7 @@ static UniValue getmininginfo(const JSONRPCRequest& request) subObj.pushKV("lastblockcreationattempt", "0"); } else { // get the last block creation attempt by the master node - CLockFreeGuard lock(pos::Staker::cs_MNLastBlockCreationAttemptTs); + std::unique_lock l{pos::cs_MNLastBlockCreationAttemptTs}; auto lastBlockCreationAttemptTs = pos::Staker::mapMNLastBlockCreationAttemptTs[mnId.second]; subObj.pushKV("lastblockcreationattempt", (lastBlockCreationAttemptTs != 0) ? FormatISO8601DateTime(lastBlockCreationAttemptTs) : "0"); } diff --git a/src/rpc/resultcache.cpp b/src/rpc/resultcache.cpp index 15cc391c0f..de63c8038b 100644 --- a/src/rpc/resultcache.cpp +++ b/src/rpc/resultcache.cpp @@ -3,7 +3,7 @@ #include void RPCResultCache::Init(RPCCacheMode mode) { - CLockFreeGuard lock{syncFlag}; + std::unique_lock l{aMutex}; this->mode = mode; } @@ -14,7 +14,7 @@ std::string GetKey(const JSONRPCRequest &request) { } bool RPCResultCache::InvalidateCaches() { - CLockFreeGuard lock{syncFlag}; + std::unique_lock l{aMutex}; auto height = GetLastValidatedHeight(); if (cacheHeight != height) { LogPrint(BCLog::RPCCACHE, "RPCCache: clear\n"); @@ -33,7 +33,7 @@ std::optional RPCResultCache::TryGet(const JSONRPCRequest &request) { auto key = GetKey(request); UniValue val; { - CLockFreeGuard lock{syncFlag}; + std::unique_lock l{aMutex}; if (auto res = cacheMap.find(key); res != cacheMap.end()) { if (LogAcceptCategory(BCLog::RPCCACHE)) { LogPrint(BCLog::RPCCACHE, "RPCCache: hit: key: %d/%s, val: %s\n", cacheHeight, key, res->second.write()); @@ -47,7 +47,7 @@ std::optional RPCResultCache::TryGet(const JSONRPCRequest &request) { const UniValue& RPCResultCache::Set(const JSONRPCRequest &request, const UniValue &value) { auto key = GetKey(request); { - CLockFreeGuard lock{syncFlag}; + std::unique_lock l{aMutex}; if (LogAcceptCategory(BCLog::RPCCACHE)) { LogPrint(BCLog::RPCCACHE, "RPCCache: set: key: %d/%s, val: %s\n", cacheHeight, key, value.write()); } @@ -77,7 +77,7 @@ void SetLastValidatedHeight(int height) { } void MemoizedResultCache::Init(RPCResultCache::RPCCacheMode mode) { - CLockFreeGuard lock{syncFlag}; + std::unique_lock l{aMutex}; this->mode = mode; } @@ -85,7 +85,7 @@ CMemoizedResultValue MemoizedResultCache::GetOrDefault(const JSONRPCRequest &req if (mode == RPCResultCache::RPCCacheMode::None) return {}; auto key = GetKey(request); { - CLockFreeGuard lock{syncFlag}; + std::unique_lock l{aMutex}; if (auto res = cacheMap.find(key); res != cacheMap.end()) { if (!::ChainActive().Contains(LookupBlockIndex(res->second.hash))) return {}; @@ -102,7 +102,7 @@ CMemoizedResultValue MemoizedResultCache::GetOrDefault(const JSONRPCRequest &req void MemoizedResultCache::Set(const JSONRPCRequest &request, const CMemoizedResultValue &value) { auto key = GetKey(request); { - CLockFreeGuard lock{syncFlag}; + std::unique_lock l{aMutex}; if (LogAcceptCategory(BCLog::RPCCACHE)) { LogPrint(BCLog::RPCCACHE, "RPCCache: set: key: %d/%s\n", value.height, key); } diff --git a/src/rpc/resultcache.h b/src/rpc/resultcache.h index f1a88c850d..8578da925f 100644 --- a/src/rpc/resultcache.h +++ b/src/rpc/resultcache.h @@ -35,7 +35,7 @@ class RPCResultCache { bool InvalidateCaches(); private: - std::atomic_bool syncFlag{false}; + AtomicMutex aMutex; std::set smartModeList{}; RPCCacheMode mode{RPCCacheMode::None}; std::map cacheMap{}; @@ -60,7 +60,7 @@ class MemoizedResultCache { CMemoizedResultValue GetOrDefault(const JSONRPCRequest &request); void Set(const JSONRPCRequest &request, const CMemoizedResultValue &value); private: - std::atomic_bool syncFlag{false}; + AtomicMutex aMutex; std::map cacheMap{}; RPCResultCache::RPCCacheMode mode{RPCResultCache::RPCCacheMode::None}; }; diff --git a/src/rpc/stats.cpp b/src/rpc/stats.cpp index df26c878c4..e9b8770c2a 100644 --- a/src/rpc/stats.cpp +++ b/src/rpc/stats.cpp @@ -6,7 +6,7 @@ bool CRPCStats::isActive() { return active.load(); } void CRPCStats::setActive(bool isActive) { active.store(isActive); } std::optional CRPCStats::get(const std::string& name) { - CLockFreeGuard lock(lock_stats); + std::unique_lock lock(lock_stats); auto it = map.find(name); if (it == map.end()) { @@ -16,7 +16,7 @@ std::optional CRPCStats::get(const std::string& name) { } std::map CRPCStats::getMap() { - CLockFreeGuard lock(lock_stats); + std::unique_lock lock(lock_stats); return map; } @@ -41,7 +41,7 @@ void CRPCStats::load() { UniValue arr(UniValue::VARR); arr.read((const std::string)line); - CLockFreeGuard lock(lock_stats); + std::unique_lock lock(lock_stats); for (const auto &val : arr.getValues()) { auto name = val["name"].get_str(); map[name] = RPCStats::fromJSON(val); @@ -140,7 +140,7 @@ void CRPCStats::add(const std::string& name, const int64_t latency, const int64_ } stats->history.push_back({ stats->lastUsedTime, latency, payload }); - CLockFreeGuard lock(lock_stats); + std::unique_lock lock(lock_stats); map[name] = *stats; } diff --git a/src/rpc/stats.h b/src/rpc/stats.h index 6a4327c78f..b630ef45fa 100644 --- a/src/rpc/stats.h +++ b/src/rpc/stats.h @@ -55,7 +55,7 @@ struct RPCStats { class CRPCStats { private: - std::atomic_bool lock_stats{false}; + AtomicMutex lock_stats; std::map map; std::atomic_bool active{DEFAULT_RPC_STATS}; diff --git a/src/sync.h b/src/sync.h index a06133a0ca..ce180fc27d 100644 --- a/src/sync.h +++ b/src/sync.h @@ -389,31 +389,5 @@ class AtomicMutex { } }; -class CLockFreeGuard -{ - std::atomic_bool& lock; -public: - CLockFreeGuard(std::atomic_bool& lock) : lock(lock) - { - bool expected = false; - while (std::atomic_compare_exchange_weak_explicit( - &lock, - &expected, true, - std::memory_order_seq_cst, - std::memory_order_relaxed) == false) { - // Could have been a spurious failure or another thread could have taken the - // lock in-between since we're now out of the atomic ops. - // Reset expected to start from scratch again, since we only want - // a singular atomic false -> true transition. - expected = false; - std::this_thread::sleep_for(std::chrono::milliseconds(1)); - } - } - - ~CLockFreeGuard() - { - lock.store(false); - } -}; #endif // DEFI_SYNC_H diff --git a/src/test/sync_tests.cpp b/src/test/sync_tests.cpp index 33c00c49b8..be41742fc4 100644 --- a/src/test/sync_tests.cpp +++ b/src/test/sync_tests.cpp @@ -55,13 +55,13 @@ BOOST_AUTO_TEST_CASE(lock_free) constexpr int num_threads = 10; auto testFunc = []() { - static std::atomic_bool cs_lock; + static AtomicMutex m; static std::atomic_int context(0); static std::atomic_int threads(num_threads); threads--; // every thread decrements count - CLockFreeGuard lock(cs_lock); + std::unique_lock lock{m}; context++; while (threads > 0); // wait all threads to be here BOOST_CHECK_EQUAL(threads.load(), 0); // now they wait for lock diff --git a/src/wallet/ismine.cpp b/src/wallet/ismine.cpp index cebf167152..41ae2754a1 100644 --- a/src/wallet/ismine.cpp +++ b/src/wallet/ismine.cpp @@ -210,19 +210,19 @@ struct CCacheInfo isminetype IsMineCached(const CWallet& keystore, CScript const & script) { - static std::atomic_bool cs_cache(false); + static AtomicMutex cs_cache; static std::unordered_map cache; auto* wallet = &keystore; - CLockFreeGuard lock(cs_cache); + std::unique_lock lock(cs_cache); auto it = cache.find(wallet); if (it == cache.end()) { it = cache.emplace(wallet, CCacheInfo{{}, { wallet->NotifyOwnerChanged.connect([wallet](CScript const & owner) { - CLockFreeGuard lock(cs_cache); + std::unique_lock lock(cs_cache); cache[wallet].mineData.erase(owner); }), wallet->NotifyUnload.connect([wallet]() { - CLockFreeGuard lock(cs_cache); + std::unique_lock lock(cs_cache); cache.erase(wallet); }), }}).first;