From c8f5e446dc95712a63e4dd88786e2f7cb697b986 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Mon, 1 Sep 2025 21:29:37 -0700 Subject: [PATCH 1/2] coins: reduce lookups in dbcache layer propagation Previously, when the parent coins cache had no entry and the child did, `BatchWrite` performed a find followed by `try_emplace`, which resulted in multiple `SipHash` computations and bucket traversals on the common insert path. This change uses a single leading `try_emplace` and branches on the returned `inserted` flag. In the `FRESH && SPENT` case (only exercised by tests), we erase the just-inserted placeholder (which is constant time with no rehash anyway). Semantics are unchanged for all valid parent/child state combinations. This change is a minimal version of https://github.com/bitcoin/bitcoin/pull/32128/commits/723c49b63bb10da843fbb6efc6928dca415cc47f and draws simplification ideas https://github.com/bitcoin/bitcoin/pull/30673/commits/ae76ec7bcff0a08a61f294882a71e46d177b009f. Added TODO versions for related pre-existing issues that should be fixed in follow-ups. Co-authored-by: Martin Ankerl Co-authored-by: Andrew Toth Co-authored-by: optout <13562139+optout21@users.noreply.github.com> --- src/coins.cpp | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index 090d36dd041..e650b81fe4b 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -185,18 +185,16 @@ void CCoinsViewCache::SetBestBlock(const uint256 &hashBlockIn) { 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()) { + if (!it->second.IsDirty()) { // TODO a cursor can only contain dirty entries continue; } - CCoinsMap::iterator itUs = cacheCoins.find(it->first); - if (itUs == cacheCoins.end()) { - // The parent cache does not have an entry, while the child cache does. - // We can ignore it if it's both spent and FRESH in the child - if (!(it->second.IsFresh() && it->second.coin.IsSpent())) { - // Create the coin in the parent cache, move the data up - // and mark it as dirty. - itUs = cacheCoins.try_emplace(it->first).first; + auto [itUs, inserted]{cacheCoins.try_emplace(it->first)}; + if (inserted) { + if (it->second.IsFresh() && it->second.coin.IsSpent()) { + cacheCoins.erase(itUs); // TODO fresh coins should have been removed at spend + } else { + // The parent cache does not have an entry, while the child cache does. + // Move the data up and mark it as dirty. CCoinsCacheEntry& entry{itUs->second}; assert(entry.coin.DynamicMemoryUsage() == 0); if (cursor.WillErase(*it)) { From 0ac969cddfdba52f7947e9b140ef36e2b19c2c41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Thu, 4 Sep 2025 17:02:35 -0700 Subject: [PATCH 2/2] validation: don't reallocate cache for short-lived CCoinsViewCache A few temporary `CCoinsViewCache`'s are destructed right after the `Flush()`, therefore it is not necessary to call `ReallocateCache` to recreate them right before they're killed anyway. * `Flush()` - retains existing functionality; * `Flush(/*will_reuse_cache=*/false)` - skips destruction and reallocation of the parent cache since it will soon go out of scope anyway; For the `will_reuse_cache` parameter we want to see exactly which ones will reallocate memory and which won't - since both can be valid usages. This change was based on a subset of https://github.com/bitcoin/bitcoin/pull/28945. Co-authored-by: Martin Ankerl --- src/coins.cpp | 6 ++++-- src/coins.h | 4 +++- src/test/fuzz/coins_view.cpp | 2 +- src/test/fuzz/coinscache_sim.cpp | 10 +--------- src/validation.cpp | 6 +++--- 5 files changed, 12 insertions(+), 16 deletions(-) diff --git a/src/coins.cpp b/src/coins.cpp index e650b81fe4b..b5dd7c62935 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -249,12 +249,14 @@ bool CCoinsViewCache::BatchWrite(CoinsViewCacheCursor& cursor, const uint256 &ha return true; } -bool CCoinsViewCache::Flush() { +bool CCoinsViewCache::Flush(bool will_reuse_cache) { auto cursor{CoinsViewCacheCursor(m_sentinel, cacheCoins, /*will_erase=*/true)}; bool fOk = base->BatchWrite(cursor, hashBlock); if (fOk) { cacheCoins.clear(); - ReallocateCache(); + if (will_reuse_cache) { + ReallocateCache(); + } cachedCoinsUsage = 0; } return fOk; diff --git a/src/coins.h b/src/coins.h index 2fcc764a3fd..8d07f7fe9fd 100644 --- a/src/coins.h +++ b/src/coins.h @@ -439,9 +439,11 @@ public: * Push the modifications applied to this cache to its base and wipe local state. * Failure to call this method or Sync() before destruction will cause the changes * to be forgotten. + * If will_reuse_cache is false, the cache will retain the same memory footprint + * after flushing and should be destroyed to deallocate. * If false is returned, the state of this cache (and its backing view) will be undefined. */ - bool Flush(); + bool Flush(bool will_reuse_cache = true); /** * Push the modifications applied to this cache to its base while retaining diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index c687065423a..f277eadf763 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -74,7 +74,7 @@ void TestCoinsView(FuzzedDataProvider& fuzzed_data_provider, CCoinsView& backend } }, [&] { - (void)coins_view_cache.Flush(); + (void)coins_view_cache.Flush(/*will_reuse_cache=*/fuzzed_data_provider.ConsumeBool()); }, [&] { (void)coins_view_cache.Sync(); diff --git a/src/test/fuzz/coinscache_sim.cpp b/src/test/fuzz/coinscache_sim.cpp index 30845c2ef9c..82e5f1fc437 100644 --- a/src/test/fuzz/coinscache_sim.cpp +++ b/src/test/fuzz/coinscache_sim.cpp @@ -392,7 +392,7 @@ FUZZ_TARGET(coinscache_sim) // Apply to simulation data. flush(); // Apply to real caches. - caches.back()->Flush(); + caches.back()->Flush(/*will_reuse_cache=*/provider.ConsumeBool()); }, [&]() { // Sync. @@ -402,14 +402,6 @@ FUZZ_TARGET(coinscache_sim) caches.back()->Sync(); }, - [&]() { // Flush + ReallocateCache. - // Apply to simulation data. - flush(); - // Apply to real caches. - caches.back()->Flush(); - caches.back()->ReallocateCache(); - }, - [&]() { // GetCacheSize (void)caches.back()->GetCacheSize(); }, diff --git a/src/validation.cpp b/src/validation.cpp index af523b06d74..5906eef8a7b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3036,7 +3036,7 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra LogError("DisconnectTip(): DisconnectBlock %s failed\n", pindexDelete->GetBlockHash().ToString()); return false; } - bool flushed = view.Flush(); + bool flushed = view.Flush(/*will_reuse_cache=*/false); // local CCoinsViewCache goes out of scope assert(flushed); } LogDebug(BCLog::BENCH, "- Disconnect block: %.2fms\n", @@ -3171,7 +3171,7 @@ bool Chainstate::ConnectTip( Ticks(time_3 - time_2), Ticks(m_chainman.time_connect_total), Ticks(m_chainman.time_connect_total) / m_chainman.num_blocks_total); - bool flushed = view.Flush(); + bool flushed = view.Flush(/*will_reuse_cache=*/false); // local CCoinsViewCache goes out of scope assert(flushed); } const auto time_4{SteadyClock::now()}; @@ -4950,7 +4950,7 @@ bool Chainstate::ReplayBlocks() } cache.SetBestBlock(pindexNew->GetBlockHash()); - cache.Flush(); + cache.Flush(/*will_reuse_cache=*/false); // local CCoinsViewCache goes out of scope m_chainman.GetNotifications().progress(bilingual_str{}, 100, false); return true; }