Share unused mempool memory with coincache #8610

sipa · 2016-08-27T13:24:11Z

A suggestion from IRC: allow the chainstate cache to use unused mempool memory.

If the mempool is not completely full, treat the difference between the maximum size and the actual usage as available for the coin cache.

This also changes the early flush trigger from (usage > 0.9 * space) to (usage > 0.9 * space && usage > space - 100MB). This means we're not permanently leaving 10% of the space unused when the space is large.

jonasschnelli · 2016-08-29T11:40:14Z

This is indeed a nice idea.
utACK.

instagibbs · 2016-08-29T14:38:03Z

At the risk of it being blatantly obvious, this would especially allow nodes with default settings during IBD to sync faster, yes?

concept ACK

sipa · 2016-08-29T15:13:57Z

At the risk of it being blatantly obvious, this would especially allow nodes with default settings during IBD to sync faster, yes?

Indeed!

gmaxwell · 2016-09-03T09:22:26Z

utACK.

rebroad · 2016-09-15T15:02:19Z

At the risk of appearing stupid, how does this help nodes with default settings sync faster during IBD?

...coincache helps to validate blocks..?

sipa · 2016-09-15T15:07:57Z

Because during IBD, the mempool is nearly empty. With this patch, all that unused mempool space (300 Mbyte by default currently) will be used for the chainstate cache, which is the bottleneck for block validation during IBD.

paveljanik · 2016-09-15T15:12:58Z

utACK 8b85ede

morcos · 2016-09-15T16:22:51Z

@sipa do you have any concern that this gives outside peers some control over when your coinsviewcache is flushed? That seems not ideal to me, at least until we are smarter about flushing it.

I had previously imagined doing this by setting the mempool limit low initially and then increasing it to the desired setting after IBD was finished. But I'm not sure I succeeded in having the code architected to make that possible.

sipa · 2016-09-19T10:47:20Z

@morcos I think that's already a concern, but I'm not sure this patch worsens it significantly. External peers can cause an increase in memory usage of the cache or the mempool, but the latter is already harder anyway.

TheBlueMatt · 2016-09-21T19:37:00Z

src/init.cpp

    LogPrintf("Cache configuration:\n");
    LogPrintf("* Using %.1fMiB for block index database\n", nBlockTreeDBCache * (1.0 / 1024 / 1024));
    LogPrintf("* Using %.1fMiB for chain state database\n", nCoinDBCache * (1.0 / 1024 / 1024));
-    LogPrintf("* Using %.1fMiB for in-memory UTXO set\n", nCoinCacheUsage * (1.0 / 1024 / 1024));
+    LogPrintf("* Using %.1fMiB for UTXO set (shared with mempool)\n", nCoinCachePlusMempoolUsage * (1.0 / 1024 / 1024));


This is confusing to me - maybe "Using $cache_only_size for UTXO set (+ up to $mempool_size from mempool)"

Github-Pull: bitcoin#8610 Rebased-From: 27562ef

fanquake · 2016-11-07T08:26:33Z

Seeing a roughly 30% decrease in time to reindex to 270'000 with this change.

Master -reindex-chainstate

08:04:43 Cache configuration:
08:04:43 * Using 2.0MiB for block index database
08:04:43 * Using 8.0MiB for chain state database
08:04:43 * Using 290.0MiB for in-memory UTXO set

08:04:45 UpdateTip: height=0 cache=0.0MiB(0tx)
08:08:09 UpdateTip: height=200000 cache=172.9MiB(553629tx)
08:12:52 UpdateTip: height=230000 cache=274.9MiB(668943tx)
08:19:08 UpdateTip: height=260488 cache=239.4MiB(318281tx)
08:19:08 UpdateTip: height=260489 cache=239.4MiB(318525tx)
08:22:11 UpdateTip: height=270000 cache=31.7MiB(8943tx)

1046s

#8610 -reindex-chainstate

07:50:16 Cache configuration:
07:50:16 * Using 2.0MiB for block index database
07:50:16 * Using 8.0MiB for chain state database
07:50:16 * Using 290.0MiB for UTXO set (plus up to 286.1MiB of unused mempool space)
...
07:50:18 UpdateTip: height=0 cache=0.0MiB(0tx)
07:53:33 UpdateTip: height=200000 cache=356.1MiB(1336689tx)
07:56:58 UpdateTip: height=230000 cache=575.5MiB(1900937tx)
08:01:02 UpdateTip: height=260488 cache=576.1MiB(1324684tx)
08:01:11 UpdateTip: height=260489 cache=16.0MiB(0tx)
08:02:32 UpdateTip: height=270000 cache=486.6MiB(972452tx)

734s

morcos · 2016-11-25T21:30:55Z

src/main.cpp

@@ -2553,6 +2553,7 @@ enum FlushStateMode {
 * or always and in all cases if we're in prune mode and are deleting files.
 */
 bool static FlushStateToDisk(CValidationState &state, FlushStateMode mode) {
+    size_t cacheSize = mempool.DynamicMemoryUsage();


I wonder if it would be better to call LimitMempoolSize right before this? During a reorg the mempool size isn't limited until the end, so theoretically FlushStateToDisk could get called and have its effective cache size reduced below the expected minimum. @sdaftuar Is there any harm in calling an extra LimitMempoolSize in the middle of a reorg other than possible inefficiencies of what you're evicting?

EDIT: Shoot this isn't exactly what I was imagining. I was thinking it would only limit the mempool if it was going to flush anyway, but this suggestion will limit the mempool every time FSTD is called which defeats the whole point of not limiting inside a reorg.

morcos · 2016-11-25T21:31:58Z

utACK modulo suggestion about calling LimitMempoolSize in FSTD.
However if we decide there are downsides to that suggestion, I'm fine with leaving it out.

EDIT: maybe 0dedace? Though I'm not sure it's worth it..

sipa · 2016-11-26T00:22:14Z

@morcos Looks good, I've included it.

morcos · 2016-11-26T01:11:46Z

src/main.cpp

    // The cache is large and close to the limit, but we have time now (not in the middle of a block processing).
-    bool fCacheLarge = mode == FLUSH_STATE_PERIODIC && cacheSize * (10.0/9) > nCoinCacheUsage;
+    bool fCacheLarge = mode == FLUSH_STATE_PERIODIC && cacheSize * (10.0/9) > nCoinCachePlusMempoolUsage;


@sipa sorry, one more issue. this line would be pretty broken if someone sets a large maxmempool. i was thinking maybe we should change it to 20.0/19 so it doesn't make the base case behavior a smaller effective cache size, but then realized if someone sets say a 2GB mempool and doesn't make a larger dbcache, then they could be flushing all the time.

Perhaps this problem and the other one could be avoided by just calculating any extra available space max(0,(maxmempool - usage)) and comparing only the pcoinstip usage to (dbcache limit + any extra available space).

sdaftuar · 2016-11-26T01:15:22Z

@morcos Not sure I'm a fan of 0dedace. I believe this code will work correctly for now, but only because we don't seem to call FlushStateToDisk anywhere the mempool might be in an inconsistent state. I don't think it's safe to call LimitMempoolSize() when the mempool is in an inconsistent state, because CTxMemPool::TrimToSize() and CTxMemPool::Expire rely on CTxMemPool::CalculateDescendants() to do the right thing, and that function assumes that setMemPoolChildren is correct for all entries. If CalculateDescendants() doesn't catch everything it should, then we could wind up with orphans in the mempool.

So in particular, during a reorg, we call AcceptToMemoryPool() for all the transactions in a disconnected block. If someone were to add a FlushStateToDisk() call inside ATMP (which I think seems harmless on the face of it), then that would introduce a bug that could cause orphans in the mempool after a reorg.

So at the very least, we would need to document that FlushStateToDisk() has a new requirement, that the mempool is in a consistent state when invoked. But as this condition is difficult to assert for and test, I think my preference would be to not impose this requirement, just to reduce the chance we introduce a bug in the future.

But also, note that #9208 should solve the problem of the mempool growing beyond its configured limit during a reorg...

sipa · 2016-11-26T03:27:24Z

I removed the commit again.

laanwj · 2016-11-28T05:52:37Z

Concept ACK

sipa · 2016-11-28T06:58:21Z

Going to address @morcos's issue soon.

sipa · 2016-12-22T03:23:04Z

Rebased, and rewritten to take #8610 (comment) into account.

jtimon · 2016-12-22T08:17:04Z

Concept ACK.

morcos · 2016-12-22T16:04:45Z

Looks good @sipa , thanks

Seems like an actual lock inversion, I think you fixed it previously by moving the mempool.DynamicMemoryUsage() to the top of FlustStateToDisk, which makes sense to me.
However I don't understand why CWallet::ReacceptWalletTransactions is locking mempool.cs anyway, so maybe that should also be removed.

I guess you lost the advantage of the extra space in VerifyDB, but that seems no big loss to me.

If the mempool is not completely full, treat the difference between the maximum size and the actual usage as available for the coin cache. This also changes the early flush trigger from (usage > 0.9 * space) to (usage > 0.9 * space && usage > space - 100MB). This means we're not permanently leaving 10% of the space unused when the space is large.

If the mempool is not completely full, treat the difference between the maximum size and the actual usage as available for the coin cache. This also changes the early flush trigger from (usage > 0.9 * space) to (usage > 0.9 * space && usage > space - 100MB). This means we're not permanently leaving 10% of the space unused when the space is large. Github-Pull: bitcoin#8610 Rebased-From: ba3cecf

fanquake · 2017-01-04T00:05:56Z

#8610 on top of master (2a524b8) with -reindex-chainstate -dbcache=2048:

2017-01-03 22:39:59 Cache configuration:
2017-01-03 22:39:59 * Using 2.0MiB for block index database
2017-01-03 22:39:59 * Using 8.0MiB for chain state database
2017-01-03 22:39:59 * Using 2038.0MiB for in-memory UTXO set (plus up to 286.1MiB of unused mempool space)
2017-01-03 22:43:04 height=200000 cache=356.1MiB(1336689tx)
2017-01-03 22:46:20 height=230000 cache=575.5MiB(1900937tx)
2017-01-03 22:51:26 height=270000 cache=915.5MiB(2499199tx)
2017-01-03 22:53:26 height=280000 cache=1050.5MiB(2816546tx)

807s (to 280'000)

Master (2a524b8) -reindex-chainstate -dbcache=2048:

2017-01-03 23:01:24 Cache configuration:
2017-01-03 23:01:24 * Using 2.0MiB for block index database
2017-01-03 23:01:24 * Using 8.0MiB for chain state database
2017-01-03 23:01:24 * Using 2038.0MiB for in-memory UTXO set
2017-01-03 23:04:29 height=200000 cache=356.1MiB(1336689tx)
2017-01-03 23:07:46 height=230000 cache=575.5MiB(1900937tx)
2017-01-03 23:12:48 height=270000 cache=915.5MiB(2499199tx)
2017-01-03 23:14:45 height=280000 cache=1050.5MiB(2816546tx)

801s (to 280'000)

Master (2a524b8) -reindex-chainstate -dbcache=300:

2017-01-03 23:24:34 Cache configuration:
2017-01-03 23:24:34 * Using 2.0MiB for block index database
2017-01-03 23:24:34 * Using 8.0MiB for chain state database
2017-01-03 23:24:34 * Using 290.0MiB for in-memory UTXO set
2017-01-03 23:27:55 height=200000 cache=172.9MiB(553629tx)
2017-01-03 23:32:32 height=230000 cache=274.9MiB(668943tx)
2017-01-03 23:36:41 height=250000 cache=128.6MiB(113010tx)
2017-01-03 23:42:36 height=270000 cache=31.7MiB(8943tx)

1082s (to 270'000)

#8610 on top of master (2a524b8) -reindex-chainstate -dbcache=300

2017-01-03 23:47:30 Cache configuration:
2017-01-03 23:47:30 * Using 2.0MiB for block index database
2017-01-03 23:47:30 * Using 8.0MiB for chain state database
2017-01-03 23:47:30 * Using 290.0MiB for in-memory UTXO set (plus up to 286.1MiB of unused mempool space)
2017-01-03 23:50:37 height=200000 cache=356.1MiB(1336689tx)
2017-01-03 23:53:58 height=230000 cache=575.5MiB(1900937tx)
2017-01-03 23:56:50 height=250000 cache=504.4MiB(1124858tx)
2017-01-03 23:59:30 height=270000 cache=486.6MiB(972452tx)
2017-01-04 00:02:10 height=280000 cache=75.2MiB(48075tx)

720s (to 270'000)

morcos · 2017-01-05T18:57:46Z

utACK ba3cecf (for the avoidance of doubt)

TheBlueMatt · 2017-01-05T18:58:06Z

utACK ba3cecf

ba3cecf Share unused mempool memory with coincache (Pieter Wuille)

rebroad · 2017-01-06T06:46:46Z

utACK - will merge and test

Nodes running in -blocksonly mode do not send and receive transactions outside blocks. They do not have a mempool, so -maxmempool should be set to 0. Unused mempool memory can be used for the UTXO coincache (PR bitcoin#8610) so not setting -maxmempool to 0 can cause to coincache to grow larger than expected. If -blocksonly is set and -maxmempool is set to anything other than 0, error and exit. If -blocksonly is set and -maxmempool is not set, implicitly set -maxmempool to 0.

ba3cecf Share unused mempool memory with coincache (Pieter Wuille)

sipa force-pushed the sharemem branch from beaf574 to 8b85ede Compare August 27, 2016 13:26

jonasschnelli added UTXO Db and Indexes Mempool labels Aug 29, 2016

sipa mentioned this pull request Sep 9, 2016

Mempool memory usage underutilized #8609

Closed

TheBlueMatt reviewed Sep 21, 2016

View reviewed changes

sipa force-pushed the sharemem branch from 8b85ede to 27562ef Compare October 3, 2016 00:23

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016

Share unused mempool memory with coincache

8c6813b

Github-Pull: bitcoin#8610 Rebased-From: 27562ef

morcos reviewed Nov 25, 2016

View reviewed changes

morcos reviewed Nov 26, 2016

View reviewed changes

sipa force-pushed the sharemem branch from 3ec401e to 27562ef Compare November 26, 2016 01:29

sipa force-pushed the sharemem branch from 27562ef to fcb8e03 Compare December 22, 2016 03:20

sipa force-pushed the sharemem branch from fcb8e03 to 586f4f0 Compare December 22, 2016 22:47

sipa force-pushed the sharemem branch from 586f4f0 to ba3cecf Compare December 22, 2016 23:04

sipa merged commit ba3cecf into bitcoin:master Jan 5, 2017

sipa added a commit that referenced this pull request Jan 5, 2017

Merge #8610: Share unused mempool memory with coincache

c252685

ba3cecf Share unused mempool memory with coincache (Pieter Wuille)

sipa mentioned this pull request Jan 10, 2017

TODO for release notes 0.14.0 #8455

Closed

18 tasks

jnewbery mentioned this pull request Jan 17, 2017

Setting -blocksonly sets -maxmempool to zero. #9569

Closed

This was referenced Feb 19, 2017

Suggestion: Increase MAX_OUTBOUND_CONNECTIONS for initial syncing if node accepts incoming connections. #9800

Closed

release: add a few performance-related notes #9787

Merged

codablock pushed a commit to codablock/dash that referenced this pull request Oct 23, 2017

Merge bitcoin#8610: Share unused mempool memory with coincache

ceb64fc

ba3cecf Share unused mempool memory with coincache (Pieter Wuille)

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Share unused mempool memory with coincache #8610

Share unused mempool memory with coincache #8610

Uh oh!

Conversation

sipa commented Aug 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Aug 29, 2016

Uh oh!

instagibbs commented Aug 29, 2016

Uh oh!

sipa commented Aug 29, 2016

Uh oh!

gmaxwell commented Sep 3, 2016

Uh oh!

rebroad commented Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Sep 15, 2016 via email

Uh oh!

paveljanik commented Sep 15, 2016

Uh oh!

morcos commented Sep 15, 2016

Uh oh!

sipa commented Sep 19, 2016

Uh oh!

TheBlueMatt Sep 21, 2016

Choose a reason for hiding this comment

Uh oh!

sipa Oct 3, 2016

Choose a reason for hiding this comment

Uh oh!

fanquake commented Nov 7, 2016

Master -reindex-chainstate

1046s

#8610 -reindex-chainstate

734s

Uh oh!

morcos Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Nov 26, 2016

Uh oh!

morcos Nov 26, 2016

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented Nov 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Nov 26, 2016

Uh oh!

laanwj commented Nov 28, 2016

Uh oh!

sipa commented Nov 28, 2016

Uh oh!

sipa commented Dec 22, 2016

Uh oh!

jtimon commented Dec 22, 2016

Uh oh!

morcos commented Dec 22, 2016

Uh oh!

fanquake commented Jan 4, 2017

807s (to 280'000)

801s (to 280'000)

1082s (to 270'000)

720s (to 270'000)

Uh oh!

morcos commented Jan 5, 2017

Uh oh!

TheBlueMatt commented Jan 5, 2017

Uh oh!

rebroad commented Jan 6, 2017

Uh oh!

Uh oh!

sipa commented Aug 27, 2016 •

edited

Loading

rebroad commented Sep 15, 2016 •

edited

Loading

morcos Nov 25, 2016 •

edited

Loading

morcos commented Nov 25, 2016 •

edited

Loading

sdaftuar commented Nov 26, 2016 •

edited

Loading