From 7825b8b9aeceb4ff607650cdc9c49e5de9c7719f Mon Sep 17 00:00:00 2001 From: Andrew Toth Date: Wed, 17 Jul 2024 23:11:48 -0400 Subject: [PATCH] coins: pass linked list of flagged entries to BatchWrite BatchWrite now iterates through the linked list of flagged entries instead of the entire coinsCache map. Co-Authored-By: l0rinc --- src/coins.cpp | 35 ++++++++++++++------------------ src/coins.h | 32 +++++++++++++++++++++++++---- src/test/coins_tests.cpp | 9 ++++---- src/test/fuzz/coins_view.cpp | 5 ++++- src/test/fuzz/coinscache_sim.cpp | 6 +++--- src/txdb.cpp | 6 +++--- src/txdb.h | 2 +- 7 files changed, 59 insertions(+), 36 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 739cc7bc3bd..92444fb0172 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -12,7 +12,7 @@ bool CCoinsView::GetCoin(const COutPoint &outpoint, Coin &coin) const { return false; } uint256 CCoinsView::GetBestBlock() const { return uint256(); } std::vector CCoinsView::GetHeadBlocks() const { return std::vector(); } -bool CCoinsView::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase) { return false; } +bool CCoinsView::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { return false; } std::unique_ptr CCoinsView::Cursor() const { return nullptr; } bool CCoinsView::HaveCoin(const COutPoint &outpoint) const @@ -27,7 +27,7 @@ bool CCoinsViewBacked::HaveCoin(const COutPoint &outpoint) const { return base-> uint256 CCoinsViewBacked::GetBestBlock() const { return base->GetBestBlock(); } std::vector CCoinsViewBacked::GetHeadBlocks() const { return base->GetHeadBlocks(); } void CCoinsViewBacked::SetBackend(CCoinsView &viewIn) { base = &viewIn; } -bool CCoinsViewBacked::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase) { return base->BatchWrite(mapCoins, hashBlock, erase); } +bool CCoinsViewBacked::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { return base->BatchWrite(cursor, hashBlock); } std::unique_ptr CCoinsViewBacked::Cursor() const { return base->Cursor(); } size_t CCoinsViewBacked::EstimateSize() const { return base->EstimateSize(); } @@ -183,10 +183,8 @@ void CCoinsViewCache::SetBestBlock(const uint256 &hashBlockIn) { hashBlock = hashBlockIn; } -bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn, bool erase) { - for (CCoinsMap::iterator it = mapCoins.begin(); - it != mapCoins.end(); - it = erase ? mapCoins.erase(it) : std::next(it)) { +bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlockIn) { + for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) { // Ignore non-dirty entries (optimization). if (!it->second.IsDirty()) { continue; @@ -200,10 +198,9 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn // and mark it as dirty. itUs = cacheCoins.try_emplace(it->first).first; CCoinsCacheEntry& entry{itUs->second}; - if (erase) { - // The `move` call here is purely an optimization; we rely on the - // `mapCoins.erase` call in the `for` expression to actually remove - // the entry from the child map. + if (cursor.WillErase(*it)) { + // Since this entry will be erased, + // we can move the coin into us instead of copying it entry.coin = std::move(it->second.coin); } else { entry.coin = it->second.coin; @@ -235,10 +232,9 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn } else { // A normal modification. cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage(); - if (erase) { - // The `move` call here is purely an optimization; we rely on the - // `mapCoins.erase` call in the `for` expression to actually remove - // the entry from the child map. + if (cursor.WillErase(*it)) { + // Since this entry will be erased, + // we can move the coin into us instead of copying it itUs->second.coin = std::move(it->second.coin); } else { itUs->second.coin = it->second.coin; @@ -257,12 +253,10 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn } bool CCoinsViewCache::Flush() { - bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/true); + auto cursor{CoinsViewCacheCursor(cachedCoinsUsage, m_sentinel, cacheCoins, /*will_erase=*/true)}; + bool fOk = base->BatchWrite(cursor, hashBlock); if (fOk) { - if (!cacheCoins.empty()) { - /* BatchWrite must erase all cacheCoins elements when erase=true. */ - throw std::logic_error("Not all cached coins were erased"); - } + cacheCoins.clear(); ReallocateCache(); } cachedCoinsUsage = 0; @@ -271,7 +265,8 @@ bool CCoinsViewCache::Flush() { bool CCoinsViewCache::Sync() { - bool fOk = base->BatchWrite(cacheCoins, hashBlock, /*erase=*/false); + auto cursor{CoinsViewCacheCursor(cachedCoinsUsage, m_sentinel, cacheCoins, /*will_erase=*/false)}; + bool fOk = base->BatchWrite(cursor, hashBlock); // Instead of clearing `cacheCoins` as we would in Flush(), just clear the // FRESH/DIRTY flags of any coin that isn't spent. for (auto it = cacheCoins.begin(); it != cacheCoins.end(); ) { diff --git a/src/coins.h b/src/coins.h index cf14107ce3e..e1ab6413925 100644 --- a/src/coins.h +++ b/src/coins.h @@ -243,6 +243,30 @@ private: uint256 hashBlock; }; +/** + * Cursor for iterating over the linked list of flagged entries in CCoinsViewCache. + */ +struct CoinsViewCacheCursor +{ + CoinsViewCacheCursor(size_t& usage LIFETIMEBOUND, + CoinsCachePair& sentinel LIFETIMEBOUND, + CCoinsMap& map LIFETIMEBOUND, + bool will_erase) noexcept + : m_usage(usage), m_sentinel(sentinel), m_map(map), m_will_erase(will_erase) {} + + inline CoinsCachePair* Begin() const noexcept { return m_sentinel.second.Next(); } + inline CoinsCachePair* End() const noexcept { return &m_sentinel; } + + inline CoinsCachePair* NextAndMaybeErase(CoinsCachePair& current) noexcept { return current.second.Next(); } + + inline bool WillErase(CoinsCachePair& current) const noexcept { return m_will_erase; } +private: + size_t& m_usage; + CoinsCachePair& m_sentinel; + CCoinsMap& m_map; + bool m_will_erase; +}; + /** Abstract view on the open txout dataset. */ class CCoinsView { @@ -266,8 +290,8 @@ public: virtual std::vector GetHeadBlocks() const; //! Do a bulk modification (multiple Coin changes + BestBlock change). - //! The passed mapCoins can be modified. - virtual bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true); + //! The passed cursor is used to iterate through the coins. + virtual bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock); //! Get a cursor to iterate over the whole state virtual std::unique_ptr Cursor() const; @@ -293,7 +317,7 @@ public: uint256 GetBestBlock() const override; std::vector GetHeadBlocks() const override; void SetBackend(CCoinsView &viewIn); - bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true) override; + bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) override; std::unique_ptr Cursor() const override; size_t EstimateSize() const override; }; @@ -332,7 +356,7 @@ public: bool HaveCoin(const COutPoint &outpoint) const override; uint256 GetBestBlock() const override; void SetBestBlock(const uint256 &hashBlock); - bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true) override; + bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) override; std::unique_ptr Cursor() const override { throw std::logic_error("CCoinsViewCache cursor iteration not supported."); } diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index a8af21c6878..fb929a2b0e0 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -55,9 +55,9 @@ public: uint256 GetBestBlock() const override { return hashBestBlock_; } - bool BatchWrite(CCoinsMap& mapCoins, const uint256& hashBlock, bool erase = true) override + bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256& hashBlock) override { - for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); it = erase ? mapCoins.erase(it) : std::next(it)) { + for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)){ if (it->second.IsDirty()) { // Same optimization used in CCoinsViewDB is to only write dirty entries. map_[it->first] = it->second.coin; @@ -618,8 +618,9 @@ void WriteCoinsViewEntry(CCoinsView& view, CAmount value, char flags) sentinel.second.SelfRef(sentinel); CCoinsMapMemoryResource resource; CCoinsMap map{0, CCoinsMap::hasher{}, CCoinsMap::key_equal{}, &resource}; - InsertCoinsMapEntry(map, sentinel, value, flags); - BOOST_CHECK(view.BatchWrite(map, {})); + auto usage{InsertCoinsMapEntry(map, sentinel, value, flags)}; + auto cursor{CoinsViewCacheCursor(usage, sentinel, map, /*will_erase=*/true)}; + BOOST_CHECK(view.BatchWrite(cursor, {})); } class SingleEntryCacheTest diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 8b7592f28ca..368c69819a0 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -122,6 +122,7 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) [&] { CoinsCachePair sentinel{}; sentinel.second.SelfRef(sentinel); + size_t usage{0}; CCoinsMapMemoryResource resource; CCoinsMap coins_map{0, SaltedOutpointHasher{/*deterministic=*/true}, CCoinsMap::key_equal{}, &resource}; LIMITED_WHILE(good_data && fuzzed_data_provider.ConsumeBool(), 10'000) @@ -140,10 +141,12 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view) } auto it{coins_map.emplace(random_out_point, std::move(coins_cache_entry)).first}; it->second.AddFlags(flags, *it, sentinel); + usage += it->second.coin.DynamicMemoryUsage(); } bool expected_code_path = false; try { - coins_view_cache.BatchWrite(coins_map, fuzzed_data_provider.ConsumeBool() ? ConsumeUInt256(fuzzed_data_provider) : coins_view_cache.GetBestBlock()); + auto cursor{CoinsViewCacheCursor(usage, sentinel, coins_map, /*will_erase=*/true)}; + coins_view_cache.BatchWrite(cursor, fuzzed_data_provider.ConsumeBool() ? ConsumeUInt256(fuzzed_data_provider) : coins_view_cache.GetBestBlock()); expected_code_path = true; } catch (const std::logic_error& e) { if (e.what() == std::string{"FRESH flag misapplied to coin that exists in parent cache"}) { diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp index 8c815e65ab5..8e717e96b4e 100644 --- a/src/test/fuzz/coinscache_sim.cpp +++ b/src/test/fuzz/coinscache_sim.cpp @@ -172,13 +172,13 @@ public: std::unique_ptr Cursor() const final { return {}; } size_t EstimateSize() const final { return m_data.size(); } - bool BatchWrite(CCoinsMap& data, const uint256&, bool erase) final + bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256&) final { - for (auto it = data.begin(); it != data.end(); it = erase ? data.erase(it) : std::next(it)) { + for (auto it{cursor.Begin()}; it != cursor.End(); it = cursor.NextAndMaybeErase(*it)) { if (it->second.IsDirty()) { if (it->second.coin.IsSpent() && (it->first.n % 5) != 4) { m_data.erase(it->first); - } else if (erase) { + } else if (cursor.WillErase(*it)) { m_data[it->first] = std::move(it->second.coin); } else { m_data[it->first] = it->second.coin; diff --git a/src/txdb.cpp b/src/txdb.cpp index d4b0186356f..3865692b6a6 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -88,7 +88,7 @@ std::vector CCoinsViewDB::GetHeadBlocks() const { return vhashHeadBlocks; } -bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase) { +bool CCoinsViewDB::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) { CDBBatch batch(*m_db); size_t count = 0; size_t changed = 0; @@ -114,7 +114,7 @@ bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, boo batch.Erase(DB_BEST_BLOCK); batch.Write(DB_HEAD_BLOCKS, Vector(hashBlock, old_tip)); - for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end();) { + for (auto it{cursor.Begin()}; it != cursor.End();) { if (it->second.IsDirty()) { CoinEntry entry(&it->first); if (it->second.coin.IsSpent()) @@ -124,7 +124,7 @@ bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, boo changed++; } count++; - it = erase ? mapCoins.erase(it) : std::next(it); + it = cursor.NextAndMaybeErase(*it); if (batch.SizeEstimate() > m_options.batch_write_bytes) { LogPrint(BCLog::COINDB, "Writing partial batch of %.2f MiB\n", batch.SizeEstimate() * (1.0 / 1048576.0)); m_db->WriteBatch(batch); diff --git a/src/txdb.h b/src/txdb.h index c9af0a091ec..e0acb09e98a 100644 --- a/src/txdb.h +++ b/src/txdb.h @@ -63,7 +63,7 @@ public: bool HaveCoin(const COutPoint &outpoint) const override; uint256 GetBestBlock() const override; std::vector GetHeadBlocks() const override; - bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock, bool erase = true) override; + bool BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &hashBlock) override; std::unique_ptr Cursor() const override; //! Whether an unsupported database format is used.