Share unused mempool memory with coincache #8610

Merged
merged 1 commit into from Jan 5, 2017

Projects

None yet
@sipa
Member
sipa commented Aug 27, 2016 edited

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
Member

This is indeed a nice idea.
utACK.

@instagibbs
Contributor

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
Member
sipa commented Aug 29, 2016

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

Indeed!

@gmaxwell
Member
gmaxwell commented Sep 3, 2016

utACK.

@rebroad
Contributor
rebroad commented Sep 15, 2016 edited

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

...coincache helps to validate blocks..?

@sipa
Member
sipa commented Sep 15, 2016
@paveljanik
Contributor

utACK 8b85ede

@morcos
Contributor
morcos commented Sep 15, 2016

@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
Member
sipa commented Sep 19, 2016

@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.

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));
@TheBlueMatt
TheBlueMatt Sep 21, 2016 Contributor

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

@sipa
sipa Oct 3, 2016 Member

Done.

@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016
@sipa @luke-jr sipa + luke-jr Share unused mempool memory with coincache
Github-Pull: #8610
Rebased-From: 27562ef
8c6813b
@fanquake
Contributor
fanquake commented Nov 7, 2016

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

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();
@morcos
morcos Nov 25, 2016 edited Contributor

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
Contributor
morcos commented Nov 25, 2016 edited

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
Member
sipa commented Nov 26, 2016

@morcos Looks good, I've included it.

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;
@morcos
morcos Nov 26, 2016 Contributor

@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
Contributor
sdaftuar commented Nov 26, 2016 edited

@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
Member
sipa commented Nov 26, 2016

I removed the commit again.

@laanwj
Member
laanwj commented Nov 28, 2016

Concept ACK

@sipa
Member
sipa commented Nov 28, 2016

Going to address @morcos's issue soon.

@sipa
Member
sipa commented Dec 22, 2016

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

@jtimon
Contributor
jtimon commented Dec 22, 2016

Concept ACK.

@morcos
Contributor
morcos commented Dec 22, 2016

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.

@sipa sipa Share unused mempool memory with coincache
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.
ba3cecf
@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 23, 2016
@sipa @luke-jr sipa + luke-jr Share unused mempool memory with coincache
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: #8610
Rebased-From: ba3cecf
dd934d6
@fanquake
Contributor
fanquake commented Jan 4, 2017

#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
Contributor
morcos commented Jan 5, 2017

utACK ba3cecf (for the avoidance of doubt)

@TheBlueMatt
Contributor

utACK ba3cecf

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sipa sipa added a commit that referenced this pull request Jan 5, 2017
@sipa sipa Merge #8610: Share unused mempool memory with coincache
ba3cecf Share unused mempool memory with coincache (Pieter Wuille)
c252685
@rebroad
Contributor
rebroad commented Jan 6, 2017

utACK - will merge and test

@sipa sipa referenced this pull request Jan 10, 2017
Open

TODO for release notes 0.14.0 #8455

1 of 18 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment