New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump `-dbcache` default to 300MiB #8273

Merged
merged 2 commits into from Jul 6, 2016

Conversation

Projects
None yet
7 participants
@laanwj
Member

laanwj commented Jun 27, 2016

Bump -dbcache default value to 300MiB.

Also cap the allocations for the leveldb-specific caches to 32MiB, the current default for 100MiB. This avoids that the extra cache memory goes to the much less effective leveldb cache instead of our application-level cache.

Before:

-dbcache -txindex block index db (MiB) chain state db (MiB) in-memory UTXO set (MiB)
100 default 0 2.0 32.5 65.5
100 1 12.5 29.9 57.6
300 0 2.0 82.5 215.5
300 1 37.5 73.6 188.9

After:

-dbcache -txindex block index db (MiB) chain state db (MiB) in-memory UTXO set (MiB)
100 0 2.0 8.0 90.0
100 1 12.5 29.9 57.6
300 default 0 2.0 8.0 290.0
300 1 8.0 8.0 284.0

Number in bold is the size allocated to the CCoinsCache in the default case.

See #8245, as well as 2016-06-23 meeting log for discussion.

Note: after this, we also need something like #8138 - currently the dbcache determines how deep undo data is rolled back at start, so otherwise this will result in the start-up verification becoming even slower.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 27, 2016

Member

Specific numbers are of course open for discussion.

Member

laanwj commented Jun 27, 2016

Specific numbers are of course open for discussion.

@laanwj laanwj added this to the 0.13.0 milestone Jun 27, 2016

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jun 27, 2016

Member

Would this require mention in the release notes?

Imagine someone updating on weak hardware, which does not support the new memory requirement.

Member

MarcoFalke commented Jun 27, 2016

Would this require mention in the release notes?

Imagine someone updating on weak hardware, which does not support the new memory requirement.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 27, 2016

Member

This is a good idea and we should def. do this!

Long term I would wish a flexible solution. If you have to catch up a couple of days/week (or during IBD), bitcoind/qt should use a catchup-cache-setting (can be a different, second -dbcacheibd [or similar] settings).
Or we could think about adding "profiles" (set-changes of some CPU/MEM related settings).

Member

jonasschnelli commented Jun 27, 2016

This is a good idea and we should def. do this!

Long term I would wish a flexible solution. If you have to catch up a couple of days/week (or during IBD), bitcoind/qt should use a catchup-cache-setting (can be a different, second -dbcacheibd [or similar] settings).
Or we could think about adding "profiles" (set-changes of some CPU/MEM related settings).

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 27, 2016

Member

utACK fdf085f
ACK for 0.13.
Would need prominent releasenotes part.

Member

jonasschnelli commented Jun 27, 2016

utACK fdf085f
ACK for 0.13.
Would need prominent releasenotes part.

@dcousens

View changes

Show outdated Hide outdated src/txdb.h
@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jun 28, 2016

Contributor

concept ACK

Contributor

dcousens commented Jun 28, 2016

concept ACK

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 28, 2016

Member

Would this require mention in the release notes?

Yes.

Imagine someone updating on weak hardware, which does not support the new memory requirement.

Well at least now that the mempool is capped, that ought to free up some memory for other uses. But yes it could be the final straw, sure...

Long term I would wish a flexible solution. If you have to catch up a couple of days/week (or during IBD), bitcoind/qt should use a catchup-cache-setting (can be a different, second -dbcacheibd [or similar] settings).

Right - during initial sync (and catch-up) the mempool is virtually guaranteed to be empty. This fact could be used to temporarily bump dbcache. But it's too late for that for 0.13.

Member

laanwj commented Jun 28, 2016

Would this require mention in the release notes?

Yes.

Imagine someone updating on weak hardware, which does not support the new memory requirement.

Well at least now that the mempool is capped, that ought to free up some memory for other uses. But yes it could be the final straw, sure...

Long term I would wish a flexible solution. If you have to catch up a couple of days/week (or during IBD), bitcoind/qt should use a catchup-cache-setting (can be a different, second -dbcacheibd [or similar] settings).

Right - during initial sync (and catch-up) the mempool is virtually guaranteed to be empty. This fact could be used to temporarily bump dbcache. But it's too late for that for 0.13.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jun 28, 2016

Member

Concept ACK for .13

Member

MarcoFalke commented Jun 28, 2016

Concept ACK for .13

@MarcoFalke

View changes

Show outdated Hide outdated src/txdb.h
@MarcoFalke

View changes

Show outdated Hide outdated src/txdb.h
@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jun 28, 2016

Member

I am (slowly) trying various leveldb cache sizes to see how big it really needs to be.

Member

gmaxwell commented Jun 28, 2016

I am (slowly) trying various leveldb cache sizes to see how big it really needs to be.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jun 28, 2016

Member

On a Xeon-D 1541 with 64GB ram, 7200 rpm disk, with no txindex, and all signature checks disabled, I found that:

Capping GetOptions ncachesize to 1MB (holding all else constant) resulted in a 1% performance loss and capping at 4MB to a 0.1% loss (31072.23 to reindex instead of 31021.76). (2MB was very close to the 1MB numbers). On this basis I recommend clamping at leveldb 4MB or 8MB (just in case future growth or other workloads makes it matter more), and handing the rest of the memory to the coins cache.

Member

gmaxwell commented Jun 28, 2016

On a Xeon-D 1541 with 64GB ram, 7200 rpm disk, with no txindex, and all signature checks disabled, I found that:

Capping GetOptions ncachesize to 1MB (holding all else constant) resulted in a 1% performance loss and capping at 4MB to a 0.1% loss (31072.23 to reindex instead of 31021.76). (2MB was very close to the 1MB numbers). On this basis I recommend clamping at leveldb 4MB or 8MB (just in case future growth or other workloads makes it matter more), and handing the rest of the memory to the coins cache.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 29, 2016

Member

Capping GetOptions ncachesize to 1MB (holding all else constant) resulted in a 1% performance loss and capping at 4MB to a 0.1% loss (31072.23 to reindex instead of 31021.76). (2MB was very close to the 1MB numbers). On this basis I recommend clamping at leveldb 4MB or 8MB (just in case future growth or other workloads makes it matter more), and handing the rest of the memory to the coins cache.

Thanks for testing! I'll change the maximums to 8 MiB.

Member

laanwj commented Jun 29, 2016

Capping GetOptions ncachesize to 1MB (holding all else constant) resulted in a 1% performance loss and capping at 4MB to a 0.1% loss (31072.23 to reindex instead of 31021.76). (2MB was very close to the 1MB numbers). On this basis I recommend clamping at leveldb 4MB or 8MB (just in case future growth or other workloads makes it matter more), and handing the rest of the memory to the coins cache.

Thanks for testing! I'll change the maximums to 8 MiB.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jun 29, 2016

Member

Benchmarked this pull req on the above system with reindex (and no signature checking at all), went from 31021.76 seconds to 21303.22.

It's still a lot slower than the 'infinite' dbcache size results; but a huge improvement over the old results.

ACK.

Member

gmaxwell commented Jun 29, 2016

Benchmarked this pull req on the above system with reindex (and no signature checking at all), went from 31021.76 seconds to 21303.22.

It's still a lot slower than the 'infinite' dbcache size results; but a huge improvement over the old results.

ACK.

@roques

This comment has been minimized.

Show comment
Hide comment
@roques

roques Jun 29, 2016

Contributor

@gmaxwell I recently enabled txindex and reindexed. Reindexing was IO-bound by LevelDB compacting. When setting -dbcache=16384 I observed that LevelDB's Level-0 files became much larger (~350MB) than with the default settings. This resulted in top LevelDB compactions of 4@0 + 0@1 files => 1458007320 bytes, resulting in several hundred new level-1 files of which the great majority can simply be moved into higher levels instead of requiring to be compacted into higher levels. This saved me quite a few compactions and thus sped-up the whole reindex.

Unfortunately, I'm unfamiliar with the exact memory management of LevelDB and what memory is used to build these large Level-0 files. However, if this patch clamps that memory of LevelDB to 8MB it will not be able to create such large Level-0 files and thus will have to do quite a few more lower-level compactions. — @maxwell, it would be nice if you could time an 'infinite dbcache' reindex with txindex=1 with this patch and see if the patch hurts its performance or if my guess is wrong.

Contributor

roques commented Jun 29, 2016

@gmaxwell I recently enabled txindex and reindexed. Reindexing was IO-bound by LevelDB compacting. When setting -dbcache=16384 I observed that LevelDB's Level-0 files became much larger (~350MB) than with the default settings. This resulted in top LevelDB compactions of 4@0 + 0@1 files => 1458007320 bytes, resulting in several hundred new level-1 files of which the great majority can simply be moved into higher levels instead of requiring to be compacted into higher levels. This saved me quite a few compactions and thus sped-up the whole reindex.

Unfortunately, I'm unfamiliar with the exact memory management of LevelDB and what memory is used to build these large Level-0 files. However, if this patch clamps that memory of LevelDB to 8MB it will not be able to create such large Level-0 files and thus will have to do quite a few more lower-level compactions. — @maxwell, it would be nice if you could time an 'infinite dbcache' reindex with txindex=1 with this patch and see if the patch hurts its performance or if my guess is wrong.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jun 29, 2016

Member

@roques I started that test before commenting above. Will update when it's done.

Member

gmaxwell commented Jun 29, 2016

@roques I started that test before commenting above. Will update when it's done.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jun 30, 2016

Member

With dbcache set to 8192, reindex with this patch and no signature checks took 8071.65 seconds, and without 8071.65 seconds. Test with txindex isn't done yet (it was later in my testqueue).

Member

gmaxwell commented Jun 30, 2016

With dbcache set to 8192, reindex with this patch and no signature checks took 8071.65 seconds, and without 8071.65 seconds. Test with txindex isn't done yet (it was later in my testqueue).

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 30, 2016

Member

Could set static const int64_t nMaxBlockDBAndTxIndexCache = 8; back to 32, that would be safer. It's possible that it makes more of a difference there then for the UTXO cache, as the access pattern is different, and I wasn't really planning to make any changes for -txindex here anyway.

Member

laanwj commented Jun 30, 2016

Could set static const int64_t nMaxBlockDBAndTxIndexCache = 8; back to 32, that would be safer. It's possible that it makes more of a difference there then for the UTXO cache, as the access pattern is different, and I wasn't really planning to make any changes for -txindex here anyway.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jun 30, 2016

Member

With txindex, no signature checks, and 8GB dbcache, before this PR 10131.43 and after 11682.22. So it makes a meaningful difference. I'm starting a test with the txindex case at 32 and seeing how that goes.

Member

gmaxwell commented Jun 30, 2016

With txindex, no signature checks, and 8GB dbcache, before this PR 10131.43 and after 11682.22. So it makes a meaningful difference. I'm starting a test with the txindex case at 32 and seeing how that goes.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 30, 2016

Member

Added mention in release notes.

Member

laanwj commented Jun 30, 2016

Added mention in release notes.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Jun 30, 2016

Contributor

Concept ACK

Contributor

petertodd commented Jun 30, 2016

Concept ACK

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jun 30, 2016

Member

10721.93 with txindex, infinite dbcache, and the 32MB clamp.

Member

gmaxwell commented Jun 30, 2016

10721.93 with txindex, infinite dbcache, and the 32MB clamp.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 1, 2016

Member

Ok, thanks for measuring, so that still suffers.
Removing the ceiling for txindex cache completely (well, bump it up to some unlikely number,1 GiB) in this PR. The exact performance compromise for txindex can be figured out later, this PR just aims to improve the common case.

Member

laanwj commented Jul 1, 2016

Ok, thanks for measuring, so that still suffers.
Removing the ceiling for txindex cache completely (well, bump it up to some unlikely number,1 GiB) in this PR. The exact performance compromise for txindex can be figured out later, this PR just aims to improve the common case.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 1, 2016

Member

Positing some slightly off topic results here:

Settings:

  • Current master da50997
  • --disable-debug
  • -dbcache=8000
  • SSD

Results:

Member

jonasschnelli commented Jul 1, 2016

Positing some slightly off topic results here:

Settings:

  • Current master da50997
  • --disable-debug
  • -dbcache=8000
  • SSD

Results:

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 2, 2016

Member

More data:

  • ~5h22min default DB cache -reindex with master (da50997) debug.log
  • ~4h06min default DB cache -reindex this PR (f2bdbb7) debug.log
Member

jonasschnelli commented Jul 2, 2016

More data:

  • ~5h22min default DB cache -reindex with master (da50997) debug.log
  • ~4h06min default DB cache -reindex this PR (f2bdbb7) debug.log
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 4, 2016

Member

Tested ACK 1009626d308ff70e3c0457559b87aacd510d0c42.

Member

jonasschnelli commented Jul 4, 2016

Tested ACK 1009626d308ff70e3c0457559b87aacd510d0c42.

laanwj added some commits Jun 27, 2016

Bump `-dbcache` default to 300MiB
Also cap the allocation for the leveldb-specific cache for the UTXO set
to 8MiB.
This avoids that the extra cache memory goes to the much less effective
leveldb cache instead of our application-level cache.
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 6, 2016

Member

Squashed and updated the commit messages for the new values.

Member

laanwj commented Jul 6, 2016

Squashed and updated the commit messages for the new values.

@laanwj laanwj merged commit efd1d83 into bitcoin:master Jul 6, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

laanwj added a commit that referenced this pull request Jul 6, 2016

Merge #8273: Bump `-dbcache` default to 300MiB
efd1d83 doc: Mention dbcache increase in release notes (Wladimir J. van der Laan)
32cab91 Bump `-dbcache` default to 300MiB (Wladimir J. van der Laan)

@dagurval dagurval referenced this pull request Nov 10, 2016

Merged

Bump default dbcache #168

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #8273: Bump `-dbcache` default to 300MiB
efd1d83 doc: Mention dbcache increase in release notes (Wladimir J. van der Laan)
32cab91 Bump `-dbcache` default to 300MiB (Wladimir J. van der Laan)

codablock added a commit to codablock/dash that referenced this pull request Sep 27, 2017

Merge #8273: Bump `-dbcache` default to 300MiB
efd1d83 doc: Mention dbcache increase in release notes (Wladimir J. van der Laan)
32cab91 Bump `-dbcache` default to 300MiB (Wladimir J. van der Laan)

codablock added a commit to codablock/dash that referenced this pull request Oct 12, 2017

Merge #8273: Bump `-dbcache` default to 300MiB
efd1d83 doc: Mention dbcache increase in release notes (Wladimir J. van der Laan)
32cab91 Bump `-dbcache` default to 300MiB (Wladimir J. van der Laan)

codablock added a commit to codablock/dash that referenced this pull request Oct 19, 2017

Merge #8273: Bump `-dbcache` default to 300MiB
efd1d83 doc: Mention dbcache increase in release notes (Wladimir J. van der Laan)
32cab91 Bump `-dbcache` default to 300MiB (Wladimir J. van der Laan)

UdjinM6 added a commit to UdjinM6/dash that referenced this pull request Nov 8, 2017

Merge #8273: Bump `-dbcache` default to 300MiB
efd1d83 doc: Mention dbcache increase in release notes (Wladimir J. van der Laan)
32cab91 Bump `-dbcache` default to 300MiB (Wladimir J. van der Laan)

AmirAbrams added a commit to duality-solutions/Dynamic that referenced this pull request Aug 3, 2018

Update default txdb cache parameters
- reduces wallet memory usage
- inline with Dash parameters

see bitcoin/bitcoin#8273

AmirAbrams added a commit to duality-solutions/Dynamic that referenced this pull request Aug 4, 2018

Update default txdb cache parameters
- reduces wallet memory usage
- inline with Dash parameters
- reduce DEFAULT_MAX_MEMPOOL_SIZE

see bitcoin/bitcoin#8273
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment