kernel: Move kernel-related cache constants to kernel cache #31483

pull TheCharlatan wants to merge 6 commits into bitcoin:master from TheCharlatan:kernel_cache_sizes changing 13 files +100 −70
  1. TheCharlatan commented at 12:17 pm on December 12, 2024: contributor

    Carrying non-kernel related fields in the cache sizes for the indexes is confusing for kernel library users. The cache sizes are set currently with magic numbers in bitcoin-chainstate. The comments for the cache size calculations are not completely clear. The constants for the cache sizes are also currently in txdb.h, which is not an ideal place for holding all cache size related constants.

    Solve these things by moving the kernel-specific cache size fields to their own struct and moving the constants to either the node or the kernel cache sizes.

    This slightly changes the way the cache is allocated if (and only if) the txindex and/or blockfilterindex is used. Since they are now given precedence over the block tree db cache, this results in a bit less cache being allocated to the block tree db, coinsdb and coins caches. The effect is negligible though, i.e. cache sizes with default dbcache reported through the logs are:

    master:

    0Cache configuration:
1* Using 2.0 MiB for block index database
2* Using 56.0 MiB for transaction index database
3* Using 49.0 MiB for basic block filter index database
4* Using 8.0 MiB for chain state database
5* Using 335.0 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)

    this PR:

    0Cache configuration:
1* Using 2.0 MiB for block index database
2* Using 56.2 MiB for transaction index database
3* Using 49.2 MiB for basic block filter index database
4* Using 8.0 MiB for chain state database
5* Using 334.5 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)

    This PR is part of the libbitcoinkernel project.

  2. DrahtBot commented at 12:17 pm on December 12, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31483.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK stickies-v, ryanofsky

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #30965 (kernel: Move block tree db open to block manager by TheCharlatan)
    • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
    • #26022 (Add util::ResultPtr class by ryanofsky)
    • #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
    • #25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)
    • #24539 (Add a “tx output spender” index by sstone)

    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.

  3. DrahtBot added the label Validation on Dec 12, 2024
  4. in src/kernel/caches.h:15 in 62da6a0b1e outdated 
    10+
11+//! min. -dbcache (MiB)
12+static constexpr int64_t MIN_DB_CACHE{4};
13+//! -dbcache default (MiB)
14+static constexpr int64_t DEFAULT_DB_CACHE{450};
15+//! Max memory allocated to block tree DB specific cache
    ajtowns commented at 12:40 pm on December 12, 2024:
    Missing the (MiB) comment.
    TheCharlatan commented at 12:58 pm on December 12, 2024:
    Thanks, fixed.
  5. doc: Correct docstring describing max block tree db cache 
    It always applied in the same way, no matter how the txindex is setup.
    6915932681
  6. TheCharlatan force-pushed on Dec 12, 2024
  7. in src/node/caches.h:22 in 173477b5ef outdated 
    16 namespace node {
17-struct CacheSizes {
18-    int64_t block_tree_db;
19-    int64_t coins_db;
20-    int64_t coins;
21+struct IndexCacheSizes {
    stickies-v commented at 8:18 am on December 13, 2024:

    nit: will we have separate structs for different node cache sizes? I think just keeping it CacheSizes and adding the node:: namespace in ambiguous places makes more sense for now?

     0diff --git a/src/node/caches.cpp b/src/node/caches.cpp
 1index 50e41b8d1e..41bd8c95d0 100644
 2--- a/src/node/caches.cpp
 3+++ b/src/node/caches.cpp
 4@@ -13,11 +13,11 @@
 5 #include <string>
 6 
 7 namespace node {
 8-std::tuple<IndexCacheSizes, kernel::CacheSizes> CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
 9+std::tuple<node::CacheSizes, kernel::CacheSizes> CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
10 {
11     int64_t nTotalCache = (args.GetIntArg("-dbcache", nDefaultDbCache) << 20);
12     nTotalCache = std::max(nTotalCache, nMinDbCache << 20); // total cache cannot be less than nMinDbCache
13-    IndexCacheSizes sizes;
14+    CacheSizes sizes;
15     sizes.tx_index = std::min(nTotalCache / 8, args.GetBoolArg("-txindex", DEFAULT_TXINDEX) ? nMaxTxIndexCache << 20 : 0);
16     nTotalCache -= sizes.tx_index;
17     sizes.filter_index = 0;
18diff --git a/src/node/caches.h b/src/node/caches.h
19index f4cc14bb23..c47134539b 100644
20--- a/src/node/caches.h
21+++ b/src/node/caches.h
22@@ -14,11 +14,11 @@
23 class ArgsManager;
24 
25 namespace node {
26-struct IndexCacheSizes {
27+struct CacheSizes {
28     int64_t tx_index;
29     int64_t filter_index;
30 };
31-std::tuple<IndexCacheSizes, kernel::CacheSizes> CalculateCacheSizes(const ArgsManager& args, size_t n_indexes = 0);
32+std::tuple<node::CacheSizes, kernel::CacheSizes> CalculateCacheSizes(const ArgsManager& args, size_t n_indexes = 0);
33 } // namespace node
34 
35 #endif // BITCOIN_NODE_CACHES_H
36diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
37index 46035772f5..fbea90a20b 100644
38--- a/src/test/util/setup_common.h
39+++ b/src/test/util/setup_common.h
40@@ -104,7 +104,7 @@ struct BasicTestingSetup {
41  * initialization behaviour.
42  */
43 struct ChainTestingSetup : public BasicTestingSetup {
44-    node::IndexCacheSizes m_index_cache_sizes{std::get<0>(node::CalculateCacheSizes(m_args))};
45+    node::CacheSizes m_index_cache_sizes{std::get<0>(node::CalculateCacheSizes(m_args))};
46     kernel::CacheSizes m_kernel_cache_sizes{std::get<1>(node::CalculateCacheSizes(m_args))};
47     bool m_coins_db_in_memory{true};
48     bool m_block_tree_db_in_memory{true};
    TheCharlatan commented at 9:45 am on December 14, 2024:
    My expectation would be that if other non-index caches are added later, they would get their own struct too. This also might make it a bit easier in case the indexes ever get their own process?
    stickies-v commented at 7:33 pm on December 16, 2024:
    Works for me, thanks!
  8. in src/init.cpp:123 in 173477b5ef outdated 
    119@@ -119,9 +120,10 @@ using common::AmountErrMsg;
120 using common::InvalidPortErrMsg;
121 using common::ResolveErrMsg;
122 
123+using kernel::CacheSizes;
    stickies-v commented at 10:01 am on December 13, 2024:

    nit: since it doesn’t meaningfully change the diff, I would much prefer not to use using here to immediately disambiguate with node::CacheSizes (+ in chainstate.cpp)

     0diff --git a/src/init.cpp b/src/init.cpp
 1index dd273cfe61..f52933b658 100644
 2--- a/src/init.cpp
 3+++ b/src/init.cpp
 4@@ -120,8 +120,6 @@ using common::AmountErrMsg;
 5 using common::InvalidPortErrMsg;
 6 using common::ResolveErrMsg;
 7 
 8-using kernel::CacheSizes;
 9-
10 using node::ApplyArgsManOptions;
11 using node::BlockManager;
12 using node::CalculateCacheSizes;
13@@ -1179,7 +1177,7 @@ static ChainstateLoadResult InitAndLoadChainstate(
14     NodeContext& node,
15     bool do_reindex,
16     const bool do_reindex_chainstate,
17-    CacheSizes& cache_sizes,
18+    kernel::CacheSizes& cache_sizes,
19     const ArgsManager& args)
20 {
21     const CChainParams& chainparams = Params();
22diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
23index 660d2c3289..d4d8828f73 100644
24--- a/src/node/chainstate.cpp
25+++ b/src/node/chainstate.cpp
26@@ -29,14 +29,12 @@
27 #include <memory>
28 #include <vector>
29 
30-using kernel::CacheSizes;
31-
32 namespace node {
33 // Complete initialization of chainstates after the initial call has been made
34 // to ChainstateManager::InitializeChainstate().
35 static ChainstateLoadResult CompleteChainstateInitialization(
36     ChainstateManager& chainman,
37-    const CacheSizes& cache_sizes,
38+    const kernel::CacheSizes& cache_sizes,
39     const ChainstateLoadOptions& options) EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
40 {
41     auto& pblocktree{chainman.m_blockman.m_block_tree_db};
42@@ -172,7 +170,7 @@ static ChainstateLoadResult CompleteChainstateInitialization(
43     return {ChainstateLoadStatus::SUCCESS, {}};
44 }
45 
46-ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSizes& cache_sizes,
47+ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const kernel::CacheSizes& cache_sizes,
48                                     const ChainstateLoadOptions& options)
49 {
50     if (!chainman.AssumedValidBlock().IsNull()) {

    edit: oh, that’s less true for the current node::IndexCacheSizes name of course. I personally still prefer keeping the namespace for clarity, but it also fits within the init.cpp coding style.

  9. in src/qt/optionsmodel.cpp:15 in 2e6c43f9c3 outdated 
    11@@ -12,6 +12,7 @@
12 
13 #include <common/args.h>
14 #include <interfaces/node.h>
15+#include <kernel/caches.h>
    stickies-v commented at 10:14 am on December 13, 2024:
    nit: this is in the wrong commit (2e6c43f9c3aed577678a537b5456adaa31a2cb04)
  10. in src/kernel/caches.h:14 in 173477b5ef outdated 
     9+#include <cstdint>
10+
11+//! min. -dbcache (MiB)
12+static constexpr int64_t MIN_DB_CACHE{4};
13+//! -dbcache default (MiB)
14+static constexpr int64_t DEFAULT_DB_CACHE{450};
    stickies-v commented at 2:57 pm on December 13, 2024:
    I’m not sure what the best solution is, but aren’t MIN_DB_CACHE and DEFAULT_DB_CACHE node attributes, instead of kernel? Then again, we do need a default total cache size in kernel, so maybe the best approach is to just rename to e.g. MIN_KERNEL_CACHE and DEFAULT_KERNEL_CACHE - or redefine DEFAULT_DB_CACHE{DEFAULT_KERNEL_CACHE};?
    TheCharlatan commented at 9:43 am on December 14, 2024:
    Mmh, is there a scenario where DEFAULT_DB_CACHE and DEFAULT_KERNEL_CACHE would ever diverge? The current naming does not seem too confusing to me.
    stickies-v commented at 7:31 pm on December 16, 2024:

    Mmh, is there a scenario where DEFAULT_DB_CACHE and DEFAULT_KERNEL_CACHE would ever diverge?

    node could decide to increase the default total cache size (e.g. because it enables certain indexes by default), and that shouldn’t affect the kernel default cache size.

    The current naming does not seem too confusing to me.

    It’s not necessarily confusing, but -dbcache is a node setting, not a kernel setting. So I think DEFAULT_KERNEL_CACHE is both more accurate (in kernel/caches.h) and - as per the previous paragraph - more futureproof.

    Either way, it’s easy to fix in the future. My suggestion feels more logical to me but either is fine, so can be resolved if you disagree.

  11. in src/test/util/setup_common.h:107 in 173477b5ef outdated 
    103@@ -103,7 +104,8 @@ struct BasicTestingSetup {
104  * initialization behaviour.
105  */
106 struct ChainTestingSetup : public BasicTestingSetup {
107-    node::CacheSizes m_cache_sizes{};
108+    node::IndexCacheSizes m_index_cache_sizes{std::get<0>(node::CalculateCacheSizes(m_args))};
    stickies-v commented at 3:09 pm on December 13, 2024:
    This member is not used, best to remove until it is?
  12. stickies-v commented at 3:12 pm on December 13, 2024: contributor
    Concept ACK. Makes sense to separate this.
  13. TheCharlatan force-pushed on Dec 14, 2024
  14. TheCharlatan commented at 10:10 am on December 14, 2024: contributor

    Thanks for the review @stickies-v,

    Updated 173477b5efad4723863b38fe51cd1156620d01df -> f454987be6f140aca6c13ab3abf94c3afa183fdf (kernel_cache_sizes_0 -> kernel_cache_sizes_1, compare)

    • Addressed @stickies-v’s comment, removed unnecassary new index size member in the tests.
    • Addressed @stickies-v’s comment, fixed commit order for include addition and optionsmodel and removed the no longer needed txdb include.
  15. TheCharlatan force-pushed on Dec 16, 2024
  16. TheCharlatan commented at 9:28 pm on December 16, 2024: contributor

    Thanks for the comments @stickies-v,

    Updated f454987be6f140aca6c13ab3abf94c3afa183fdf -> 2c49e0bfd295dc0326d02cd0a80853d7dee885ac (kernel_cache_sizes_1 -> kernel_cache_sizes_2, compare)

    • Addressed @stickies-v’s comment, added a kernel-specific cache size constant and moved the minimum size out of the kernel.
  17. in src/node/caches.cpp:24 in 2c49e0bfd2 outdated 
    27-    CacheSizes sizes;
28-    sizes.block_tree_db = std::min(nTotalCache / 8, nMaxBlockDBCache << 20);
29-    nTotalCache -= sizes.block_tree_db;
30-    sizes.tx_index = std::min(nTotalCache / 8, args.GetBoolArg("-txindex", DEFAULT_TXINDEX) ? nMaxTxIndexCache << 20 : 0);
31+    int64_t nTotalCache = (args.GetIntArg("-dbcache", DEFAULT_DB_CACHE) << 20);
32+    nTotalCache = std::max(nTotalCache, MIN_DB_CACHE << 20); // total cache cannot be less than nMinDbCache
    stickies-v commented at 1:53 pm on December 17, 2024: 
    0    nTotalCache = std::max(nTotalCache, MIN_DB_CACHE << 20); // total cache cannot be less than MIN_DB_CACHE

    edit: actually, the comment is pretty redundant, might as well just remove it too.

    TheCharlatan commented at 3:37 pm on December 17, 2024:
    thanks for the suggestion, I removed it.
  18. stickies-v approved
  19. stickies-v commented at 2:31 pm on December 17, 2024: contributor

    ACK 2c49e0bfd295dc0326d02cd0a80853d7dee885ac

    The only behaviour change is a very slight difference in cache allocation by moving block_tree_db allocation after index allocation. Because block_tree_db is capped at 2MiB, the maximum impact this change can have is very limited. Even at the most extreme case with -dbcache=4 -txindex -blockfilterindex=1, the results are not meaningfully different:

    On master:

    02024-12-17T14:25:35Z Cache configuration:
12024-12-17T14:25:35Z * Using 0.5 MiB for block index database
22024-12-17T14:25:35Z * Using 0.4 MiB for transaction index database
32024-12-17T14:25:35Z * Using 0.4 MiB for basic block filter index database
42024-12-17T14:25:35Z * Using 1.3 MiB for chain state database
52024-12-17T14:25:35Z * Using 1.3 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)

    With this PR:

    02024-12-17T14:27:38Z Cache configuration:
12024-12-17T14:27:38Z * Using 0.4 MiB for block index database
22024-12-17T14:27:38Z * Using 0.5 MiB for transaction index database
32024-12-17T14:27:38Z * Using 0.4 MiB for basic block filter index database
42024-12-17T14:27:38Z * Using 1.3 MiB for chain state database
52024-12-17T14:27:38Z * Using 1.3 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)
  20. TheCharlatan force-pushed on Dec 17, 2024
  21. TheCharlatan commented at 3:37 pm on December 17, 2024: contributor

    Updated 2c49e0bfd295dc0326d02cd0a80853d7dee885ac -> 8e929e343e14b0228f6f4841ecf2d85a73afda3b (kernel_cache_sizes_2 -> kernel_cache_sizes_3, compare)

  22. stickies-v approved
  23. stickies-v commented at 5:34 pm on December 17, 2024: contributor
    re-ACK 8e929e343e14b0228f6f4841ecf2d85a73afda3b - just a small doc change.
  24. in src/kernel/caches.h:29 in 8e929e343e outdated 
    22@@ -18,10 +23,10 @@ struct CacheSizes {
23 
24     CacheSizes(int64_t total_cache)
25     {
26-        block_tree_db = std::min(total_cache / 8, nMaxBlockDBCache << 20);
27+        block_tree_db = std::min(total_cache / 8, MAX_BLOCK_DB_CACHE << 20);
28         total_cache -= block_tree_db;
29         // use 25%-50% of the remainder for disk cache, capped by the maximum.
30-        coins_db = std::min({total_cache / 2, (total_cache / 4) + (1 << 23), nMaxCoinsDBCache << 20});
31+        coins_db = std::min({total_cache / 2, (total_cache / 4) + (1 << 23), MAX_COINS_DB_CACHE << 20});
    ryanofsky commented at 6:09 pm on December 17, 2024:

    re: In commit “kernel: Move kernel-related cache constants to kernel cache” (8e929e343e14b0228f6f4841ecf2d85a73afda3b)

    Precedes this PR, but this code does not seem to make any sense. (1 « 23) is 8MiB which is the same as MAX_COINS_DB_CACHE. So this is effectively min(total / 2, total / 4 + 8MiB, 8Mib) with the middle argument not doing anything. Could be simplified to std::min(total_cache / 2, MAX_COINS_DB_CACHE << 20) with no change in behavior AFAICT.

    TheCharlatan commented at 8:31 pm on December 17, 2024:
    Heh, I guess I should have actually done the math here. Will remove.
  25. ryanofsky approved
  26. ryanofsky commented at 6:24 pm on December 17, 2024: contributor

    Code review ACK 8e929e343e14b0228f6f4841ecf2d85a73afda3b. All these changes make sense and definitely make the kernel API nicer.

    I do also think it would be an improvement to change all the int64_t fields to size_t since they just get casted to size_t in the end anyway, and because size_t is more self-documenting and makes it clearer the sizes are in bytes.

  27. init: Simplify coinsdb cache calculation 
    (total_cache / 4) + (1 << 23) is at least 8 MiB and nMaxCoinsDBCache is
also 8 MiB, so the minimum between the two will always be
nMaxCoinsDBCache.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
    c71fc6ac6e
  28. kernel: Move kernel-specific cache size options to kernel 
    Carrying non-kernel related fields in the cache sizes for the indexes is
confusing for kernel library users. The cache sizes also are set
currently with magic numbers in bitcoin-chainstate. The comments for the
cache size calculations are also not completely clear.

Solve these things by moving the kernel-specific cache size fields to
their own struct.

This slightly changes the way the cache is allocated if the txindex
and/or blockfilterindex is used. Since they are now given precedence
over the block tree db cache, this results in a bit less cache being
allocated to the block tree db, coinsdb and coins caches. The effect is
negligible though, i.e. cache sizes with default dbcache reported
through the logs are:

master:
Cache configuration:
* Using 2.0 MiB for block index database
* Using 56.0 MiB for transaction index database
* Using 49.0 MiB for basic block filter index database
* Using 8.0 MiB for chain state database
* Using 335.0 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)

this branch:
Cache configuration:
* Using 2.0 MiB for block index database
* Using 56.2 MiB for transaction index database
* Using 49.2 MiB for basic block filter index database
* Using 8.0 MiB for chain state database
* Using 334.5 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)
    ed35258c2f
  29. kernel: Move non-kernel db cache size constants 
    These have nothing to do with the txdb, so move them out and into the
node caches.
    9558bd7b76
  30. kernel: Move kernel-related cache constants to kernel cache 
    They are not all related to the txdb, so a better place for them is the
new kernel cache file.
    9f55bd460f
  31. init: Use size_t consistently for cache sizes 
    This avoids having to rely on implicit casts when passing them to the
various functions allocating the caches.

Also take this opportunity to make the total amounts of cache in the
chainstate manager a size_t too
    7517126d9f
  32. TheCharlatan force-pushed on Dec 17, 2024
  33. TheCharlatan commented at 9:14 pm on December 17, 2024: contributor

    Thanks for the review @ryanofsky! Updated 8e929e343e14b0228f6f4841ecf2d85a73afda3b -> 7517126d9fd2c3e50dc95ce831bac6895b950e24 (kernel_cache_sizes_3 -> kernel_cache_sizes_4, compare)

  34. in src/kernel/caches.h:26 in 7517126d9f 
    21+    size_t coins_db;
22+    size_t coins;
23+
24+    CacheSizes(int64_t total_cache)
25+    {
26+        block_tree_db = std::min(total_cache / 8, MAX_BLOCK_DB_CACHE << 20);
    stickies-v commented at 12:12 pm on December 18, 2024:

    Would be prudent to ensure no negative values here

    0        assert(total_cache >= 0);
1        block_tree_db = std::min(total_cache / 8, MAX_BLOCK_DB_CACHE << 20);
  35. in src/kernel/caches.h:22 in 7517126d9f 
    17+
18+namespace kernel {
19+struct CacheSizes {
20+    size_t block_tree_db;
21+    size_t coins_db;
22+    size_t coins;
    stickies-v commented at 12:23 pm on December 18, 2024:
    I think I’m ~0 on this change. I follow @ryanofsky’s reasoning, but it also goes against the work done e.g. in #23962. If kernel::CacheSizes becomes part of the public kernel interface, then keeping these members an int64_t might be more prudent, as per e.g. the link in this comment? But of course, size_t is a pretty natural fit for a size type.
    ryanofsky commented at 5:03 pm on December 18, 2024:

    re: #31483 (review)

    I agree with changes in the issues you linked to, but I don’t think the reasoning in those cases carries over to this one. int32_t is a good choice for transaction sizes and weights, because we know the sizes of transactions are bounded and can’t get very big, so it makes sense to use a small type, and we want the bounds to be platform independent so it makes to use fixed width types, and because they are numbers we do arithmetic with, and signed types tend to be safer than unsigned types for arithmetic.

    I think size_t is a better choice for the cache size variables in this PR, because these values are eventually used to allocate memory so making them size_t from the beginning is clearer and avoids the need for casts and conversions later on. Also size_t is the standard type to use for this purpose so it’s more idiomatic c++. Also maximum cache sizes do depend on the platform, so size_t a safe choice because it is guaranteed to be able to represent the maximum size of any allocated object on a platform. I do think a downside of choosing size_t is that it is unsigned and unsigned types have less intuitive overflow behavior, but if the values are just going to get casted to size_t later anyway, potentially bad overflow behavior is there no matter what.

    stickies-v commented at 6:14 pm on December 18, 2024:

    Yeah, thinking about it more I agree with all of your points. I was a bit alarmed by all the comparison operations we were doing with these (now) size_t types, but they almost all happen inside the constructor. Once the sizes are determined, they don’t seem likely to be used for arithmetic operations or comparisons, just for allocating memory.

    I think the benefits you outline outweigh the potential downsides, so I’m happy with the new approach.

  36. stickies-v approved
  37. stickies-v commented at 12:24 pm on December 18, 2024: contributor

    re-ACK 7517126d9fd2c3e50dc95ce831bac6895b950e24

    I think c71fc6ac6e62e5eea96e8aa4e0f43260afc81723 is an absolute improvement, but I’m ~0 on 7517126d9fd2c3e50dc95ce831bac6895b950e24. (edit: resolved)

  38. DrahtBot requested review from ryanofsky on Dec 18, 2024
  39. in src/node/caches.cpp:28 in c71fc6ac6e outdated 
    23@@ -24,8 +24,7 @@ CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
24         sizes.filter_index = max_cache / n_indexes;
25         nTotalCache -= sizes.filter_index * n_indexes;
26     }
27-    sizes.coins_db = std::min(nTotalCache / 2, (nTotalCache / 4) + (1 << 23)); // use 25%-50% of the remainder for disk cache
28-    sizes.coins_db = std::min(sizes.coins_db, nMaxCoinsDBCache << 20); // cap total coins db cache
    ryanofsky commented at 4:40 pm on December 18, 2024:

    In commit “init: Simplify coinsdb cache calculation” (c71fc6ac6e62e5eea96e8aa4e0f43260afc81723)

    For background on why previous code was written this way, apparently the first line coins_db = min(total / 2, total / 4 + 8MiB) made sense when it was added in b3ed4236beb7f68e1720ceb3da15e0c3682ef629 from #6102. But when the second line coins_db = min(coins_db, 8MiB) was added later in 32cab91278651d07a11132b7636dc3d21144e616 from #8273, the total / 4 term in the first line became redundant.

    It seems like the second change was probably intentional (even though the implementation is confusing) because the commit message says “This avoids that the extra cache memory goes to the much less effective leveldb cache instead of our application-level cache.”

  40. ryanofsky approved
  41. ryanofsky commented at 5:09 pm on December 18, 2024: contributor
    Code review ACK 7517126d9fd2c3e50dc95ce831bac6895b950e24. Just added two new commits since last review simplifying coinsdb cache calculation and switching to size_t to avoid casts. Thanks for the updates!


TheCharlatan DrahtBot ajtowns stickies-v ryanofsky

Labels
Validation

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-21 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me