validation, log: improve logging of ChainstateManager snapshot persistance#23738
Merged
maflcko merged 3 commits intobitcoin:masterfrom Dec 13, 2021
Merged
Conversation
This moves the flushing and logging into one method and adds logging of time duration and memory for the snapshot chainstate flushing.
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
jonatack
force-pushed
the
refactor-and-improve-snapshot-persistance-logging
branch
from
61986a5 to
8e37fa8
Compare
maflcko
reviewed
Dec 11, 2021
src/validation.cpp
Outdated
|
|
||
| const int64_t flush_now{GetTimeMillis()}; | ||
|
|
||
| // TODO: if #17487 is merged, add erase=false here if snapshot is loaded, for better performance. |
Member
There was a problem hiding this comment.
unrelated: Wouldn't it be better to remove this comment and just do this in 17487?
Member
Author
There was a problem hiding this comment.
Thanks, done (I had removed it in the first push but wasn't sure). Made a note in that PR: #17487 (comment).
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
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.
Contributor
|
Code review ACK 8e37fa8 |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Dec 13, 2021
…nager snapshot persistance 50209a4 validation, doc: remove TODO comment (Jon Atack) 8e37fa8 validation, log: improve logging in FlushSnapshotToDisk() (Jon Atack) 271252c validation, log: extract FlushSnapshotToDisk() function (Jon Atack) Pull request description: 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 bitcoin#22872 (comment). 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 ``` Top commit has no ACKs. Tree-SHA512: 5d954cd8c7455f8625152a43663a237f04717bb834aed62925a56e17c711fca6ccfc03783970b6b0bde44f64617d804b423a7048287c06ee816db36247acf272
jonatack
deleted the
refactor-and-improve-snapshot-persistance-logging
branch
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Nov 11, 2022
…tance Summary: 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. 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) ``` This is a backport of [[bitcoin/bitcoin#23738 | core#23738]] Depends on D12469 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12470
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use the
LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCEmacro 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 (comment).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 toFlushStateToDisk.before
after
The logging can be observed in the output of