assumeutxo: keep cache when flushing snapshot (#17487 followup)

jamesob commented at 5:10 pm on January 31, 2023: member

Make use of #17487.

Benchmark results available here: #17487 (comment)

DrahtBot commented at 5:10 pm on January 31, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors

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:

#26045 (rpc: Optimize serialization disk space of dumptxoutset by aureleoules)

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.

in src/validation.cpp:5040 in d6f54dac8d outdated

5032@@ -5033,15 +5033,19 @@ bool ChainstateManager::ActivateSnapshot(
5033     return true;
5034 }
5035 
5036-static void FlushSnapshotToDisk(CCoinsViewCache& coins_cache, bool snapshot_loaded)
5037+static void FlushSnapshotToDisk(CCoinsViewCache& coins_cache, bool snapshot_loaded, bool erase=true)
5038 {
5039     LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE(
5040         strprintf("%s (%.2f MB)",
5041                   snapshot_loaded ? "saving snapshot chainstate" : "flushing coins cache",

Sjors commented at 10:01 am on February 6, 2023:

nit: (erase ? "syncing" : "flushing") + "coins cache"

jamesob commented at 2:23 pm on February 6, 2023:

Good idea, thanks.

Sjors approved

Sjors commented at 10:14 am on February 6, 2023: member

utACK d6f54dac8dee776188fd88d725e2728e0822b33f

Based on testing #17487 I think it’s safe. I’d like to take this for a spin on a rebase of #15606 (the bits that enable loadtxoutset), to see if I can reproduce the performance improvement. But imo that doesn’t have to hold back this PR.

jamesob force-pushed on Feb 6, 2023

jamesob commented at 2:25 pm on February 6, 2023: member

Thanks for the look @Sjors. I also realized that there were two outlier calls where we weren’t asserting that Flush()/Sync() has succeeded, so I’ve fixed those.

jamesob requested review from Sjors on Feb 6, 2023

in src/validation.cpp:5039 in d2a9939c8a outdated

5035@@ -5036,12 +5036,12 @@ bool ChainstateManager::ActivateSnapshot(
5036 static void FlushSnapshotToDisk(CCoinsViewCache& coins_cache, bool snapshot_loaded)
5037 {
5038     LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE(
5039-        strprintf("%s (%.2f MB)",
5040-                  snapshot_loaded ? "saving snapshot chainstate" : "flushing coins cache",
5041+        strprintf("%s coins cache (%.2f MB)",

Sjors commented at 4:28 pm on February 6, 2023:

If you’re touching this, maybe use LogPrintf and add [snapshot].

Maybe make it [[nodiscard]]?

jamesob commented at 5:00 pm on February 22, 2023:

Sounds good, fixed.

in src/validation.cpp:4262 in 2834617be0 outdated

4257@@ -4258,7 +4258,9 @@ bool Chainstate::ReplayBlocks()
4258     }
4259 
4260     cache.SetBestBlock(pindexNew->GetBlockHash());
4261-    cache.Flush();
4262+    if (!cache.Flush()) {
4263+        return AbortNode("Failed to write to coin database");

Sjors commented at 4:40 pm on February 6, 2023:

2834617be0e47842dd015b105d2ddc38237c5e98: earlier calls in this function fail with return error(…). What’s the difference with AbortNode?

jamesob commented at 4:55 pm on February 22, 2023:

Good point! Fixed.

validation: optimization: use Sync() when flushing snapshot to disk

Benchmark results available here:
  https://github.com/bitcoin/bitcoin/pull/17487#issuecomment-557226923

129d8f55ee

validation: AbortNode if coins_cache disk sync fails

We've historically done this check in most places, so make sure we're
doing it uniformly.

937016b191

jamesob commented at 5:05 pm on February 22, 2023: member

Thanks for the feedback @Sjors.

jamesob force-pushed on Feb 22, 2023

Sjors commented at 6:33 pm on February 23, 2023: member

re-utACK 937016b1917a0a07fa579c96fc797237f5027cd1

in src/validation.cpp:5049 in 937016b191

5046 
5047-    snapshot_loaded ? coins_cache.Sync() : coins_cache.Flush();
5048+    if (snapshot_loaded ? coins_cache.Sync() : coins_cache.Flush()) {
5049+        return true;
5050+    } else {
5051+        return AbortNode("Failed to write to coin database");

Sjors commented at 6:39 pm on February 23, 2023:

Same question about AbortNode vs. error though.

jamesob commented at 8:22 pm on February 24, 2023:

AbortNode() is used elsewhere in the function that this function is called from, although I did change another reference that you pointed out was inconsistent.

Sjors commented at 2:29 pm on March 10, 2023: member

From my testing of #15606 - without this PR - I get the impression that flushing isn’t working, specifically that RAM is not freed up. It could just be confusion on my end from rebase hell, but I’d like to make sure we’re not obscuring a bug by merging this. (I initially assumed the main assumeutxo PR already contained this PR because RAM didn’t go down)

jamesob commented at 6:58 pm on April 19, 2023: member

@Sjors how did you detect that RAM wasn’t being freed? Did you verify that the flush did not happen based on an absence of logs?

Measures of memory can be deceptive; I remember bitcoinperf spuriously detecting an increase in memory usage because once claimed, the process’ resident set size (RSS) doesn’t necessarily go down upon free.

Sjors commented at 12:36 pm on April 27, 2023: member

@jamesob using the system monitor desktop app in Ubuntu, and I believe also based on the cache size log messages. I’ll double check this next time I run it.

jamesob commented at 1:18 pm on May 6, 2023: member

Actually this is broken at the moment, since although we don’t flush after snapshot load, we do flush (and not sync) in the MaybeRebalanceCaches() -> FlushStateToDisk() call right after activating the chainstate snapshot. Will close this and think about how to rework it.

jamesob closed this on May 6, 2023

bitcoin locked this on May 5, 2024

assumeutxo: keep cache when flushing snapshot (#17487 followup) #27008

Reviews

Conflicts