validation, log: improve logging of ChainstateManager snapshot persistance #23738

1. 
   jonatack commented at 9:21 pm on December 10, 2021: member

   Use the LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE macro to improve the logging of ChainstateManager snapshot persistance, log task start and completion separately and no longer manually track the duration, as suggested by Marco Falke in #22872 (review).

   Extract the flushing into one function, which clarifies the logic, extends the improved logging to both flushing call sites, and allows logging the prefix FlushSnapshotToDisk, which is similar to FlushStateToDisk.

   before

   0[snapshot] flushing coins cache (0 MB)... done (0.00ms)
   1
   2[snapshot] flushing snapshot chainstate to disk
   

   after

   0FlushSnapshotToDisk: flushing coins cache (0 MB) started
   1...
   2FlushSnapshotToDisk: completed (0.00ms)
   3
   4FlushSnapshotToDisk: saving snapshot chainstate (0 MB) started
   5...
   6FlushSnapshotToDisk: completed (0.00ms)
   

   The logging can be observed in the output of

   0./src/test/test_bitcoin -t validation_chainstate_tests -- DEBUG_LOG_OUT
   

2. validation, log: extract FlushSnapshotToDisk() function

   This moves the flushing and logging into one method and adds logging
   of time duration and memory for the snapshot chainstate flushing.

   271252c0bd
3. validation, log: improve logging in FlushSnapshotToDisk()

   Use the `LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE` macro to improve the
   logging of snapshot persistance and no longer manually track the duration.
   
   before
   
   [snapshot] flushing coins cache (0 MB)... done (0.00ms)
   
   [snapshot] flushing snapshot chainstate to disk (0 MB)... done (0.00ms)
   
   after
   
   FlushSnapshotToDisk: flushing coins cache (0 MB) started
   FlushSnapshotToDisk: completed (0.00ms)
   
   FlushSnapshotToDisk: saving snapshot chainstate (0 MB) started
   FlushSnapshotToDisk: completed (0.00ms)
   
   The logging can be observed in the output of
   
   ./src/test/test_bitcoin -t validation_chainstate_tests -- DEBUG_LOG_OUT

   8e37fa8393
4. jonatack force-pushed on Dec 10, 2021
5. DrahtBot added the label Validation on Dec 10, 2021
6. in src/validation.cpp:4851 in 271252c0bd outdated

   4846+              snapshot_loaded ? "snapshot chainstate to disk" : "coins cache",
   4847+              coins_cache.DynamicMemoryUsage() / (1000 * 1000));
   4848+
   4849+    const int64_t flush_now{GetTimeMillis()};
   4850+
   4851+    // TODO: if #17487 is merged, add erase=false here if snapshot is loaded, for better performance.
   

   ---

   ---

   

   MarcoFalke commented at 8:42 am on December 11, 2021:

   unrelated: Wouldn’t it be better to remove this comment and just do this in 17487?

   ---

   

   jonatack commented at 11:55 am on December 11, 2021:

   Thanks, done (I had removed it in the first push but wasn’t sure). Made a note in that PR: #17487 (review).
7. 
   DrahtBot commented at 11:43 am on December 11, 2021: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #23545 (scripted-diff: Use clang-tidy syntax for C++ named arguments by MarcoFalke)

   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.

8. validation, doc: remove TODO comment

   It would make for sense for the TODO to be done in PR 17487
   (or noted in the review feedback for a follow-up),
   no need to continue maintaining the TODO in the codebase.

   50209a42ad
9. 
   jamesob commented at 1:57 pm on December 13, 2021: member

   Code review ACK https://github.com/bitcoin/bitcoin/pull/23738/commits/8e37fa83934ad47a4a73c326213e2073dfc35e29
10. MarcoFalke merged this on Dec 13, 2021
11. MarcoFalke closed this on Dec 13, 2021

    ---

12. sidhujag referenced this in commit 35eedd1b41 on Dec 13, 2021
13. jonatack deleted the branch on Dec 13, 2021
14. Fabcien referenced this in commit d4bc7d6d31 on Nov 11, 2022
15. DrahtBot locked this on Dec 13, 2022

Contributors
jonatack MarcoFalke DrahtBot jamesob

Labels
Validation