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

pull jonatack wants to merge 3 commits into bitcoin:master from jonatack:refactor-and-improve-snapshot-persistance-logging changing 1 files +15 −11

jonatack commented at 9:21 PM on December 10, 2021: contributor
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
```
[snapshot] flushing coins cache (0 MB)... done (0.00ms)

[snapshot] flushing snapshot chainstate to disk
```
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
```

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

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

jonatack force-pushed on Dec 10, 2021
jonatack cross-referenced this on Dec 10, 2021 from issue log: improve checkaddrman logging with duration in milliseconds by jonatack
DrahtBot added the label Validation on Dec 10, 2021

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).

DrahtBot commented at 11:43 AM on December 11, 2021: contributor


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.

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

jonatack cross-referenced this on Dec 11, 2021 from issue coins: allow write to disk without cache drop by jamesob
DrahtBot cross-referenced this on Dec 11, 2021 from issue scripted-diff: Use clang-tidy syntax for C++ named arguments by MarcoFalke
jamesob commented at 1:57 PM on December 13, 2021: member

Code review ACK https://github.com/bitcoin/bitcoin/pull/23738/commits/8e37fa83934ad47a4a73c326213e2073dfc35e29
MarcoFalke merged this on Dec 13, 2021
MarcoFalke closed this on Dec 13, 2021
sidhujag referenced this in commit 35eedd1b41 on Dec 13, 2021
jonatack deleted the branch on Dec 13, 2021
Fabcien referenced this in commit d4bc7d6d31 on Nov 11, 2022
bitcoin locked this on Dec 13, 2022

Contributors

Labels

Linked (view graph)

#17487 coins: allow write to disk without cache drop #22872 log: improve checkaddrman logging with duration in milliseconds

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

Conflicts