Some coin database memory usage tweaks. #4683

pull sipa wants to merge 2 commits into bitcoin:master from sipa:memdrops changing 5 files +27 −14

sipa commented at 11:53 AM on August 12, 2014: member
- Make CScript::clear() release memory (indirectly causing transaction outputs being spent to release associated heap memory).
- Make CCoinsView::BatchWrite consume the passed CCoinsMap, rather than copy from it.
  
  As a result, CCoinsViewDb now progressively converts the map to LevelDB entries, rather than copying.
  
  Merging a CCoinsViewCache into a parent cache uses CCoins::swap + release rather than copy + bulk release at the end.
Untested, but should reduce peak memory usage and some CPU time.
in src/coins.cpp:None in b8763384b4 outdated
```
 135 | -        cacheCoins[it->first] = it->second;
 136 | +bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn) {
 137 | +    for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end();) {
 138 | +        cacheCoins[it->first].swap(it->second);
 139 | +        CCoinsMap::iterator itOld = it++;
 140 | +        mapCoins.erase(itOld);
```
laanwj commented at 12:08 PM on August 12, 2014:

As this iterates over the full mapCoins, a clear() at the end may be faster than erasing the entries one by one.

sipa commented at 12:18 PM on August 12, 2014:

map.clear() still has to iterate over all entries, while they're already in the CPU cache when we're doing this. Also, it means more memory being freed early while increasing memory in the destination, in theory reducing peak usage.

laanwj commented at 12:33 PM on August 12, 2014:

That's true.

sipa commented at 12:37 PM on August 12, 2014:

Hmm, I was reasoning in terms of a treemap, but it's a hashmap now. Just erasing all entries will not cause its hash table to be freed, and clear() afterwards will still iterate over all entries (to potentially erase them), so I'm not sure whether this weighs up against the slightly better peak memory usage.

laanwj commented at 12:50 PM on August 12, 2014:

It probably makes a minimal difference, if at all - let's prioritize the peak memory usage and keep it like this.
laanwj added the label Improvement on Aug 12, 2014
laanwj added the label UTXO Db and Indexes on Aug 12, 2014
sipa commented at 12:30 PM on August 17, 2014: member

@laanwj How do you feel about this and its conflicting with #4700?
laanwj commented at 9:59 AM on August 18, 2014: member

The only serious conflict with #4700 is caused by CCoinsViewCache::BatchWrite now taking a mutable CCoinsView.

But I'm not sure about the #4700 yet. In my experiments since, I've noticed that tweaking with the cache doesn't seem to have noticable influence on performance at all in syncing/reindexing. The cache mostly acts as a 'write buffer'. The read cache component is less important. Even though it gets lots of hits it doesn't figure into what actually consumes time.

So - reducing memory usage while keeping the current functionality seems fine to me. ACK.
sipa commented at 6:55 PM on August 18, 2014: member
Benchmarked: did a full testnet reindex using:
```
$ /usr/bin/time -f "user:%U elapsed:%E maxmem:%M" ./bitcoind -testnet -reindex -nodaemon -stopafterblockimport -debug=benchmark -dbcache=3000
```
Resulting peak memory usage: 1041 MiB without this patch, 922 MiB with. No difference in speed.

Note that this peak is during the final batch write to LevelDB.
gmaxwell commented at 11:59 PM on August 23, 2014: contributor

Looks reasonable to me, I hadn't commented because I'd moved most of my testing to 0.9.3rc1.
jgarzik commented at 12:04 AM on August 24, 2014: contributor

ut ACK, though I would have preferred that the script change be in a separate commit.
Allow BatchWrite to destroy its input, reducing copying b0875eb3fe
Make CScript::clear() release its memory fff7455ded
sipa force-pushed on Aug 24, 2014
sipa commented at 12:08 AM on August 24, 2014: member

@jgarzik Done.
BitcoinPullTester commented at 1:25 AM on August 24, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4683_fff7455deda80bf483422dd7d7c2446e93522f2d/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
TheBlueMatt commented at 8:41 AM on August 24, 2014: member

I didnt do a full reindex, but it does continue to sync, ACK.
sipa merged this on Aug 24, 2014
sipa closed this on Aug 24, 2014
sipa referenced this in commit 41abb02122 on Aug 24, 2014
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Refactoring UTXO Db and Indexes