Implement accurate UTXO cache size accounting #6102
Conversation
|
I added a commit which changes the block flushing policy a bit. The block index is still flushed at least once an hour (so that we don't need to redownload hours worth of blocks after a crash), but the coins cache is only guaranteed to be flushed once per day (as the cache size allows). @sdaftuar feel like having a look? |
0aeec88 to
ef15b5f
Compare
|
utACK. It would be really nice to be able to get the approximated in-memory size of various data structures through RPC, such as the UTXO cache size introduced here. |
|
As mentioned on IRC, I think it would be better to still guard the block index writes with CheckDiskSpace. Something like this: a8e9baa. Other than that, utACK. I did a light code review of the first commit and full code review of the last 4, and some light testing of the last commit. |
|
Code review and light testing ACK. |
Make sure we're checking disk space for block index writes and allow for pruning to happen before chainstate writes.
|
Cherry-picked @morcos's commit.
As a test, I reindexed the entire main chain with -dbcache=16000, and this
did not result in a single database flush. Memory usage near the end was
slightly lower (100 MB off on a total of 3.5 GB) than what the coin cache
usage predicted. I can't explain that difference (it should be more), but
at least it errs on the safe side.
|
| @@ -93,6 +99,7 @@ bool CCoinsViewCache::GetCoins(const uint256 &txid, CCoins &coins) const { | |||
| CCoinsModifier CCoinsViewCache::ModifyCoins(const uint256 &txid) { | |||
| assert(!hasModifier); | |||
| std::pair<CCoinsMap::iterator, bool> ret = cacheCoins.insert(std::make_pair(txid, CCoinsCacheEntry())); | |||
| size_t cachedCoinUsage = 0; | |||
There was a problem hiding this comment.
nit: when reading the code, I found it slightly concerning that a one-character typo in this variable name below would result in code that would still compile but be wrong (since cachedCoinsUsage is a class member variable), though thankfully that mistake causes the unit test to fail.
|
I reviewed and did a little testing as well, including replicating those results on my machine, ACK. |
86a5f4b Relocate calls to CheckDiskSpace (Alex Morcos) 67708ac Write block index more frequently than cache flushes (Pieter Wuille) b3ed423 Cache tweak and logging improvements (Pieter Wuille) fc684ad Use accurate memory for flushing decisions (Pieter Wuille) 046392d Keep track of memory usage in CCoinsViewCache (Pieter Wuille) 540629c Add memusage.h (Pieter Wuille)
9880245 Relocate calls to CheckDiskSpace (furszy) 0b69282 Write block index more frequently than cache flushes (furszy) 4338b08 Cache tweak and logging improvements (furszy) 5e505df Use accurate memory for flushing decisions (furszy) e59840a Keep track of memory usage in CCoinsViewCache. (furszy) 34e48a1 Add memusage.h (Pieter Wuille) Pull request description: This adds a basic foundation for accurate memory counting (memusage.h, with some implementation-specific assumptions, but at least the tree node and hashtable node implementations used are very generic and straightforward, so likely accurate for several systems). On the tested system at least, they are exact, ignoring memory fragmentation (tested using a single binary creating and modifying large amounts of different configurations of these data structures, and observing total resident set size afterwards). I expect this to be useful for other resource-limiting subsystems later on. Then, this is used to implement accurate memory usage counting for CCoins objects, and efficiently computed memory usage counting for CCoinsViewCache (using cached totals, and increments/decrements on updates through CCoinsModifier). The existing CCoinsViewCache randomized simultation unit test is extended to also verify the correctness of the cached memory usage totals. Changing any of the cached total update statements in coins.cpp breaks the unit test. Finally, the internal flush triggering mechanism is changed to use the memory usage mechanism rather than transaction count of pcoinsTip. Coming from upstream@[6102](bitcoin#6102) . --- Needs more testing --- ACKs for top commit: random-zebra: ACK 9880245 Fuzzbawls: ACK 9880245 Tree-SHA512: ca9abd18cbcf94825cd216b4a31e38092cba1107be32b94ef8a38bf82fda9ee679591126066bcca50e56f8b727376149f41d4109183c0127b5908f2cf2921003
This adds a basic foundation for accurate memory counting (memusage.h, with some implementation-specific assumptions, but at least the tree node and hashtable node implementations used are very generic and straightforward, so likely accurate for several systems). On the tested system at least, they are exact, ignoring memory fragmentation (tested using a single binary creating and modifying large amounts of different configurations of these data structures, and observing total resident set size afterwards). I expect this to be useful for other resource-limiting subsystems later on.
Then, this is used to implement accurate memory usage counting for CCoins objects, and efficiently computed memory usage counting for CCoinsViewCache (using cached totals, and increments/decrements on updates through CCoinsModifier). The existing CCoinsViewCache randomized simultation unit test is extended to also verify the correctness of the cached memory usage totals. Changing any of the cached total update statements in coins.cpp breaks the unit test.
Finally, the internal flush triggering mechanism is changed to use the memory usage mechanism rather than transaction count of pcoinsTip.