💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667680249)
In commit "coins: track flagged cache entries in linked list"

Would it make sense to add code to `CCoinsViewCache`'s destructor to explicitly wipe the map, and then test that `m_sentinel.m_next == &m_sentinel` in an `Assume` or so? To ascertain that no deleted `CCoinsCacheEntrys` remain in the list?

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667679725)
In commit "coins: track flagged cache entries in linked list"

This doesn't matter much, but if you'd swap all prevs/nexts here, the entry would get inserted at the end of the list rather than the beginning, making the list be ordered old-to-new. Unless there is a reason to write things out from new to old?

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667678752)
In commit "refactor: require self and sentinel parameters for AddFlags"

Maybe turn this into an `Assume`, so it doesn't have a runtime cost in non-debug builds?

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667681297)
In commit "coins: pass linked list of flagged entries to BatchWrite"

It's a bit unfortunate that this means iterating through spent entries twice; wouldn't it be possible to let BatchWrite do the deletion too in the same loop? This will probably mean passing both `cacheCoins` and `m_sentinel` in.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667703546)
I see there's also an `assert(!coin.IsSpent());` at the beginning of `AddCoin`, which would also be in the hot path. When should we use `assert` vs `Assume` then?

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667703693)
Yeah it also means passing in the `cachedCoinsUsage` so we can decrement that when we erase.

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667704346)
I guess that predates the introduction of `Assume`.

`assert` runs always; `Assume` only runs when `ABORT_ON_FAILED_ASSUME` is defined, which is the case in debug builds and in fuzz tests.

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667705233)
Hmm, yeah, that's pretty ugly. It's fine to leave exploring this for a follow-up.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667721192)
I ended up doing it. I pass in sentinel and cachedCoinsUsage and delete the spent entries when not erasing.
I think calling erase with the outpoint only is slower than erasing with the iterator itself, so I only erase when calling `Sync`, and clear the map at the end of the loop if `erase=true`.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667721222)
Done.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667721248)
Reordered the insertion.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667721281)
Changed to `Assume`.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667721501)
I moved the iteration down so the logic should be more clear now.

👍 andrewtoth approved a pull request: "optimization: Reduce cache lookups in CCoinsViewCache::FetchCoin"
(https://github.com/bitcoin/bitcoin/pull/30326#pullrequestreview-2161892129)
ACK 00052ecc0402f2c53e3602b901a937b64aa7f7cc

I benchmarked IBD from a local node connection to block 820k, twice on this branch twice on master.
This branch: 22265 seconds mean time
Master: 22523 seconds mean time

Also benchmarked `-reindex-chainstate` up to block 820k, twice on this branch and twice on master.
This branch: 22085 seconds mean time
Master: 22255 seconds mean time

So it appears to shave off ~3 minutes of time.

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667729124)
Hmm, it's not at all obvious that this will be faster, as an erase by outpoint involves hashing and lookup in the hash table. It may be worth benchmarking the two options to make sure.

Another option is switching the coinscache map from `std::unordered_map` to a Boost multiindex, which can simultaneously maintain a hashtable and a linked list transparently without needing to hack our own linked list on top.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667729456)
> Hmm, it's not at all obvious that this will be faster, as an erase by outpoint involves hashing and lookup in the hash table. It may be worth benchmarking the two options to make sure.

Sorry, I may not have been clear. I am *not* erasing by outpoint. I only erase the spent coins if `erase=false` by outpoint, because that's the only way to find those in constant time.
Otherwise, for a normal erasing flush, we just call `mapCoins.clear()` at the end to wipe them all at once.
I think I have
...

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667730042)
Isn't this an erase by outpoint?

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667730311)
Yes, but if `!erase`. This is what `Sync` was doing before I moved this into `BatchWrite`. Only spent outpoint when we don't want to clear the cache. This is what I already benchmarked to be much faster.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667730532)
This is the reason we need to have the linked list be `CoinsCachePair`s instead of `CCoinsCacheEntry`s, as you mentioned [here](https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1296524385).

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667730793)
Ah, thank you! I missed that this was already being done in master.