Add option dbfilesize to control LevelDB target ("max") file size #30059
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30059. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
i'm broadly on board with making this an option (seperately from changing the default in #30039). Assuming there are legit reasons for varying this based on the kind of server, operating system, amount of RAM, kind of disk, etc, and there in't a single sweet spot. |
There was a problem hiding this comment.
Concept ACK.
Thank you. Seems reasonable to include this as a debug option, and increases flexibility over the single value chosen for PR #30039. Left an inline comment/question.
Ran a quick sanity check:
With dbfilesize=64 set (along with txindex=1), performed an IBD on Signet from a node on the same LAN, then stopped and restarted bitcoind. Saw chainstate and indexes/txindex files that were close to 64MB (blocks/index for signet was less than 64MB, but still had a file over 2MB).
| @@ -448,6 +449,11 @@ void SetupServerArgs(ArgsManager& argsman) | |||
| argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); | |||
| argsman.AddArg("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS); | |||
| argsman.AddArg("-dbcache=<n>", strprintf("Maximum database cache size <n> MiB (%d to %d, default: %d). In addition, unused mempool memory is shared for this cache (see -maxmempool).", nMinDbCache, nMaxDbCache, nDefaultDbCache), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); | |||
| argsman.AddArg("-dbfilesize", | |||
| strprintf("Target size of files within databases, in MiB (%u to %u, default: %u).", | |||
There was a problem hiding this comment.
The text states a range of 1MiB to 1024MiB, but these values didn't seem to be enforced (e.g. 0.5, -3, and 1025 were not rejected). Is the intent here to provide guidance for the user (i.e. tell the user to choose 1 to 1024 MiB) rather than enforce a specific range of values?
There was a problem hiding this comment.
LevelDB itself enforces it. Would it make sense to check it redundantly on our end to control error behaviour? (But then we have to remember to keep it in sync with LevelDB... but maybe just having the docs necessitates that)
There was a problem hiding this comment.
Yeah, redundant checking of range (beyond LevelDB) seems a bit much for a debug option (which is used by a more niche userbase). The range in the help message seems to be more of an example of a usable range rather than a recommendation of range or a strictly enforced range. The default value would be the defacto recommendation.
If we're set on continuing to have LevelDB handle the range rather than Core, it seems like it would make sense to inform the user in the help message that the range is an example. This could help prevent a developer from wasting time on a PR that misses the intent (e.g. could envision a PR starting with: ...dbfilesize specifies a range of allowable values, but doesn't enforce this range. This PR fixes this...).
It might also help to clarify to the user which files this option impacts.
This is a bit of a nit of a nit, so feel free to disregard it. Example:
diff --git a/src/init.cpp b/src/init.cpp
index 5cc513e18c9..f4be033f1c1 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -450,7 +450,7 @@ void SetupServerArgs(ArgsManager& argsman)
argsman.AddArg("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS);
argsman.AddArg("-dbcache=<n>", strprintf("Maximum database cache size <n> MiB (%d to %d, default: %d). In addition, unused mempool memory is shared for this cache (see -maxmempool).", nMinDbCache, nMaxDbCache, nDefaultDbCache), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
argsman.AddArg("-dbfilesize",
- strprintf("Target size of files within databases, in MiB (%u to %u, default: %u).",
+ strprintf("Target size of files within databases (chainstate, indexes), in MiB (example: %u to %u, default: %u).",
1, 1024,
DEFAULT_DB_FILE_SIZE),
ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS);|
I think it would be good to understand what the trade-offs are for this before considering making it configurable (as in: understand in what situations we'd want to recommend people use lower vs higher values). I assume lower values mean more open files/IO and less outdated data on disk, and higher values mean fewer compactions. Depending on whether those are the only effects, and to what extent these things are affected by the value, maybe we can get away with a default value that's a few % of the expected total db size. FWIW, RocksDB uses a default of 64 MiB, and has an option (default off) to make higher database-level files become larger (e.g.: level L files would target 64 * (2^L) MiB). |
There was a problem hiding this comment.
I've rebased this and ran a few benchmarks with different values.
Will do a few more measurements, but so far I'm rather leaning on finding a better default value instead of making this configurable.
See: #30039 (comment)
9160cc0 to
1c2c840
Compare
1c2c840 to
c283a57
Compare
|
Given that we couldn't find any measurable difference for bigger values, do you still think it makes sense to make this configurable? And if so, what's the intended usecase? |
|
Can't predict the behaviours of all storage devices and filesystems. This would enable users to tweak it and see if another configuration works better for them. |
No strong opinion but i veer toward "no", we should not be adding unneccesary knobs, and beyond this size the comparative overhead of handling files is neglible. Maybe there comes a future time when this size needs to be increased again, but it's not something end-users are going to play with. |
Debug option to control LevelDB file sizes. Since LevelDB seems to always overshoot, "max" didn't seem fitting.
Intended to be followed up with a change to the default in a rebased #30039