WIP [bench] CCoinsView(Cache): measure various scenarios #13470

1. 
   Sjors commented at 2:27 pm on June 14, 2018: member

   src/bench/bench_bitcoin -printer=plot -filter=CCoinsView.* > plot.html:

   

   Tested scenarios (plus ideas that could be added later):

   • access a cached coin
   • add 200.000 coins to cache (plot shows average time per coin)
   • flush cache (plot shows average time per coin)
   • load cache from disk
   • access coins that are not cached
   • access coins that are cached but dirty
   • flush dirty coins

   The flush test creates a temporary directory on disk.

   I normalized the cache access benchmark to take about the same speed as the existing benches do on my machine.

   I disabled scaling iterations for the cache addition and flush bench. It doesn’t make sense to have more than 1 iteration per eval, because the speed per coin as part of a large flush is a more relevant metric then how often you can flush 1 coin. At the same time I didn’t want the benches to use up too much RAM if someone sets -scaling to high number.

   To test a bigger cache increase N_CACHE_SCALE=1. The cache is about 40 MB by default, try N_CACHE_SCALE=100 for more realistic scenarios, but note that test needs ~3x RAM.

   I’m doing a few things the bench framework doesn’t seem designed for. Would like some feedback before I refactor it in the wrong direction:

   • share code with test framework (should support --disable-test?)
   • clean up stuff between evals; e.g. I need to reset the coin cache for CCoinsViewCacheAddCoinFresh and CCoinsViewCacheFlush
   • disable scaling iterations
   • add a memory-scaling argument
   • allow pausing the clock between iterations; e.g. to generate test coins on the fly rather in bulk before the run
2. [bench] CCoinsView(Cache): measure various scenarios 8572a38d31
3. fanquake added the label Tests on Jun 14, 2018
4. in src/bench/bench.cpp:119 in 8572a38d31

   115@@ -116,7 +116,7 @@ void benchmark::BenchRunner::RunAll(Printer& printer, uint64_t num_evals, double
   116         if (0 == num_iters) {
   117             num_iters = 1;
   118         }
   119-        State state(p.first, num_evals, num_iters, printer);
   120+        State state(p.first, num_evals, num_iters, scaling, printer);
   

   ---

   ---

   

   MarcoFalke commented at 8:17 pm on June 14, 2018:

   Why is this change required? Should be in a separate commit

   ---

   

   Sjors commented at 8:38 am on June 15, 2018:

   I need to disable the effect of the scaling setting, see above. The current implementation is a bit of a hack though; once refactored it can go in its own commit.
5. MarcoFalke deleted a comment on Jun 15, 2018
6. 
   DrahtBot commented at 11:23 am on June 26, 2018: member

   • #14244 (amount: Move CAmount CENT to unit test header by MarcoFalke)
   • #14224 (Document intentional and unintentional unsigned integer overflows (wraparounds) using annotations by practicalswift)
   • #14156 ([WIP] refactor: Make explicit CMutableTransaction -> CTransaction conversion. by lucash-dev)
   • #13525 (Report reason inputs are nonstandard from AreInputsStandard by Empact)

   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.

7. 
   DrahtBot commented at 12:37 pm on August 10, 2018: member
8. DrahtBot closed this on Aug 10, 2018

   ---

9. DrahtBot reopened this on Aug 10, 2018

   ---

10. in src/bench/ccoins_caching.cpp:78 in 8572a38d31

    111+    
    112+    const std::vector<CKey> keys = SetupDummyKeys(keystoreRet, n_keys);
    113+    
    114+    const std::vector<CTransaction> transactions = SetupDummyTransactions(keys, n_coins);
    115+        
    116+    for (const CTransaction tx : transactions) {
    

    ---

    ---

    

    practicalswift commented at 4:02 pm on September 14, 2018:

    Should be const CTransaction &?
11. in src/bench/ccoins_caching.cpp:163 in 8572a38d31

    196+        CCoinsViewCache coinsViewCache(pcoinsdbview.get());
    197+        FastRandomContext rng(true);
    198+        coinsViewCache.SetBestBlock(rng.rand256());
    199+                
    200+        // Add coins to cache:
    201+        for (const CTransaction tx : transactions) {
    

    ---

    ---

    

    practicalswift commented at 4:02 pm on September 14, 2018:

    Should be const CTransaction &?
12. in src/bench/ccoins_caching.cpp:9 in 8572a38d31

    5@@ -6,82 +6,216 @@
    6 #include <coins.h>
    7 #include <policy/policy.h>
    8 #include <wallet/crypter.h>
    9+#include <validation.h>
    

    ---

    ---

    

    practicalswift commented at 11:25 am on September 18, 2018:

    02018-09-18 10:55:16 clang-tidy(pr=13470): src/bench/ccoins_caching.cpp:6:1: warning: #includes are not sorted properly [llvm-include-order]
    

13. in src/bench/bench.h:70 in 8572a38d31

    67 
    68-    State(std::string name, uint64_t num_evals, double num_iters, Printer& printer) : m_name(name), m_num_iters_left(0), m_num_iters(num_iters), m_num_evals(num_evals)
    69+    State(std::string name, uint64_t num_evals, double num_iters, double scaling, Printer& printer) : m_name(name), m_num_iters_left(num_iters), m_num_iters(num_iters), m_scaling(scaling), m_num_evals(num_evals)
    70     {
    71     }
    72+    
    

    ---

    ---

    

    practicalswift commented at 8:16 am on September 23, 2018:

    02018-09-22 22:13:03 cpplint(pr=13470): src/bench/bench.h:70:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
    12018-09-22 22:13:03 cpplint(pr=13470): src/bench/bench.h:75:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
    22018-09-22 22:13:03 cpplint(pr=13470): src/bench/bench.h:82:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
    32018-09-22 22:13:03 cpplint(pr=13470): src/bench/bench.h:88:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
    42018-09-22 22:13:03 cpplint(pr=13470): src/bench/bench.h:92:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
    52018-09-22 22:13:03 cpplint(pr=13470): src/bench/bench.h:96:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
    62018-09-22 22:13:03 cpplint(pr=13470): src/bench/bench.h:99:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
    

14. in src/bench/bench.h:109 in 8572a38d31

    108+            bool lastEval = !UpdateTimer(clock::now());
    109+            if (!lastEval) {
    110+                m_num_iters_left = m_num_iters;
    111+            }
    112+            return true;
    113+        };
    

    ---

    ---

    

    practicalswift commented at 8:16 am on September 23, 2018:

    02018-09-22 22:13:03 cpplint(pr=13470): src/bench/bench.h:109:  You don't need a ; after a }  [readability/braces] [4]
    

15. in src/bench/ccoins_caching.cpp:38 in 8572a38d31

    48-        keystoreRet.AddKey(key[i]);
    49+    for (int i = 0; i < n_keys; i++) {
    50+        keys[i].MakeNewKey(true);
    51+        keystoreRet.AddKey(keys[i]);
    52     }
    53+    
    

    ---

    ---

    

    practicalswift commented at 8:17 am on September 23, 2018:

    02018-09-22 22:13:03 cpplint(pr=13470): src/bench/ccoins_caching.cpp:38:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
    12018-09-22 22:13:03 cpplint(pr=13470): src/bench/ccoins_caching.cpp:43:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
    22018-09-22 22:13:03 cpplint(pr=13470): src/bench/ccoins_caching.cpp:45:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
    32018-09-22 22:13:03 cpplint(pr=13470): src/bench/ccoins_caching.cpp:47:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
    42018-09-22 22:13:03 cpplint(pr=13470): src/bench/ccoins_caching.cpp:54:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
    52018-09-22 22:13:03 cpplint(pr=13470): src/bench/ccoins_caching.cpp:55:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
    62018-09-22 22:13:03 cpplint(pr=13470): src/bench/ccoins_caching.cpp:58:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
    72018-09-22 22:13:03 cpplint(pr=13470): src/bench/ccoins_caching.cpp:60:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
    82018-09-22 22:13:03 cpplint(pr=13470): src/bench/ccoins_caching.cpp:64:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
    92018-09-22 22:13:03 cpplint(pr=13470): src/bench/ccoins_caching.cpp:73:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
    

    Please fix throughout :-)

16. in src/bench/ccoins_caching.cpp:149 in 8572a38d31

    182+    while (state.IsNewEval()) {
    183+        // TODO: dedup from test suite
    184+        SelectParams(CBaseChainParams::REGTEST);
    185+        // const CChainParams& chainparams = Params();
    186+        ClearDatadirCache();
    187+        fs::path pathTemp = fs::temp_directory_path() / "bench_bitcoin" / strprintf("%lu_%i", (unsigned long)GetTime(), (int)(InsecureRandRange(1 << 30)));
    

    ---

    ---

    

    practicalswift commented at 8:17 am on September 23, 2018:

    02018-09-22 22:13:03 cpplint(pr=13470): src/bench/ccoins_caching.cpp:149:  Use int16/int64/etc, rather than the C type long  [runtime/int] [4]
    

17. in src/bench/bench.h:67 in 8572a38d31

    64     time_point m_start_time;
    65 
    66     bool UpdateTimer(time_point finish_time);
    67 
    68-    State(std::string name, uint64_t num_evals, double num_iters, Printer& printer) : m_name(name), m_num_iters_left(0), m_num_iters(num_iters), m_num_evals(num_evals)
    69+    State(std::string name, uint64_t num_evals, double num_iters, double scaling, Printer& printer) : m_name(name), m_num_iters_left(num_iters), m_num_iters(num_iters), m_scaling(scaling), m_num_evals(num_evals)
    

    ---

    ---

    

    practicalswift commented at 5:30 am on September 26, 2018:

    num_iters should be uint64_t and not double?
18. DrahtBot added the label Needs rebase on Sep 27, 2018
19. 
    DrahtBot commented at 5:36 pm on September 27, 2018: member
20. 
    Sjors commented at 11:40 am on November 30, 2018: member

    Consider this up for grabs. It should be a matter of rebasing and cleaning up.
21. Sjors closed this on Nov 30, 2018

    ---

22. fanquake added the label Up for grabs on Nov 30, 2018
23. laanwj removed the label Needs rebase on Oct 24, 2019
24. DrahtBot locked this on Dec 16, 2021

Contributors
Sjors MarcoFalke DrahtBot practicalswift

Labels
Tests Up for grabs