Refactor: clean up PeriodicFlush() #19085

pull jnewbery wants to merge 1 commits into bitcoin:master from jnewbery:2020-05-refactor-periodic-flush changing 1 files +22 −31

jnewbery commented at 3:01 AM on May 28, 2020: member

PeriodicFlush() is much more convoluted than it needs to be: it has triple nesting, local variables counting refs and return values, and increments the mapFileUseCount iterator unnecessarily. Removing all of that makes the function much easier to understand.
jnewbery requested review from MarcoFalke on May 28, 2020
fanquake added the label Refactoring on May 28, 2020
DrahtBot commented at 4:57 AM on May 28, 2020: 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:
- #19335 (wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch by achow101)
- #19334 (wallet: Introduce WalletDatabase abstract class by achow101)
- #19325 (wallet: Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class by achow101)
- #19324 (wallet: Move BerkeleyBatch static functions to BerkeleyDatabase by achow101)
- #19137 (wallettool: Add dump and createfromdump commands by achow101)
- #19102 (wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase by achow101)
- #19077 (wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101)
- #18904 (Don't call lsn_reset in periodic flush by bvbfan)
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.
MarcoFalke commented at 10:47 AM on May 28, 2020: member

Concept ⒶⒸⓀ
fanquake commented at 8:27 AM on June 4, 2020: member

Glanced over 2495f8fa9992872f19c8c80180322d2deedd2b87 and it looks like an improvement. Can you rebase now that #18792 is in, and I'll re-review. @achow101 you might also want to take a look here?
jnewbery commented at 3:14 PM on June 4, 2020: member

Thanks @fanquake . I've rebased on master.
jnewbery force-pushed on Jun 4, 2020
fanquake approved
fanquake commented at 11:57 AM on June 5, 2020: member

ACK 9c031049be0a9b2fc9d8459adccdb56f28169ac0 - Thanks for the quick follow up. This is a nice simplification, and it'd seem that functions like BerkeleyDatabase::Backup or BerkeleyEnvironment::Flush could receive similar refactors.

There was one behaviour I noticed while reviewing (unchanged by this PR), which I've opened an issue for in #19175.
DrahtBot added the label Needs rebase on Jun 17, 2020
jnewbery commented at 6:41 PM on June 23, 2020: member

Rebased
jnewbery force-pushed on Jun 23, 2020
DrahtBot removed the label Needs rebase on Jun 23, 2020
in src/wallet/bdb.cpp:626 in 7c10020456 outdated
```
 656 | -            if (mi != env->mapFileUseCount.end())
 657 | -            {
 658 | -                LogPrint(BCLog::WALLETDB, "Flushing %s\n", strFile);
 659 | -                int64_t nStart = GetTimeMillis();
 660 | +    // Don't flush if any databases are in use
 661 | +    for (auto it = env->mapFileUseCount.begin() ; it != env->mapFileUseCount.end(); it++) {
```
jonatack commented at 2:50 AM on July 1, 2020:

nit, while retouching this code, ++i is preferred over i++

for (auto it = env->mapFileUseCount.begin() ; it != env->mapFileUseCount.end(); ++it) {
jnewbery commented at 1:43 PM on July 1, 2020:

It makes no difference to the compiled code since we're not using the return value of the incrementer. I'll change to ++it if I have to retouch this branch, but otherwise it's not worth invalidating ACKs.

promag commented at 9:55 AM on July 9, 2020:

nit, extra space after begin. Better, use range based loop.

jnewbery commented at 10:40 AM on July 9, 2020:

Yes. Even better. Will change if I have to retouch.
in src/wallet/bdb.cpp:640 in 7c10020456 outdated
```
 679 | +    int64_t nStart = GetTimeMillis();
 680 | +
 681 | +    // Flush wallet file so it's self contained
 682 | +    env->CloseDb(strFile);
 683 | +    env->CheckpointLSN(strFile);
 684 | +    env->mapFileUseCount.erase(it);
```
jonatack commented at 2:52 AM on July 1, 2020:

Sanity check: Does this erase the same element as the previous version? env->mapFileUseCount.erase(mi++);

jnewbery commented at 1:41 PM on July 1, 2020:

Yes. The post-incrementer returns (a copy of) the value before incrementing https://en.cppreference.com/w/cpp/language/operator_incdec

in src/wallet/bdb.cpp:637 in 7c10020456 outdated

 676 | -        }
 677 | -    }
 678 | +    LogPrint(BCLog::WALLETDB, "Flushing %s\n", strFile);
 679 | +    int64_t nStart = GetTimeMillis();
 680 | +
 681 | +    // Flush wallet file so it's self contained

jonatack commented at 2:54 AM on July 1, 2020:

nit: self-contained

jnewbery commented at 1:43 PM on July 1, 2020:

Will change if I need to retouch

jonatack commented at 3:02 AM on July 1, 2020: member

ACK 9c031049be0a9b2fc9d8459adccdb56f28169ac0 nice refactoring
jonatack commented at 3:04 AM on July 1, 2020: member

Note to reviewers: I suggest looking at the diff with space changes ignored (git show -w).
DrahtBot added the label Needs rebase on Jul 5, 2020
refactor: clean up PeriodicFlush() e846a2a1d9
jnewbery commented at 6:07 AM on July 9, 2020: member

rebased
jnewbery force-pushed on Jul 9, 2020
jonatack approved
jonatack commented at 7:18 AM on July 9, 2020: member

re-ACK e846a2a per git range-diff f7c19e8 7c10020 e846a2a
DrahtBot removed the label Needs rebase on Jul 9, 2020

MarcoFalke commented at 9:45 AM on July 9, 2020: member

ACK e846a2a1d9e8aa37abfa55bd4d2a0a9f55ba6c8e 🎁

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash d38062803983ae335282eeba787d6c83aeeabfe3699adc77650b02de45c422fc -

</details>

MarcoFalke commented at 9:45 AM on July 9, 2020: member

cc @achow101 any objections to this?
promag commented at 10:01 AM on July 9, 2020: member

ACK e846a2a1d9e8aa37abfa55bd4d2a0a9f55ba6c8e.
jnewbery commented at 10:40 AM on July 9, 2020: member

I think this is ready for merge. It's a simple refactor and has 3 ACKs now.
MarcoFalke merged this on Jul 9, 2020
MarcoFalke closed this on Jul 9, 2020
MarcoFalke commented at 11:09 AM on July 9, 2020: member

Went ahead to merge this and postponed to fix the nits in a related pull that had less ACKs (#18923)
jnewbery deleted the branch on Jul 10, 2020
Fabcien referenced this in commit 24a9346645 on Dec 7, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Review Requested
MarcoFalke

Labels

Refactoring

Linked (view graph)

#19175 wallet: PeriodicFlush called when mapFileUseCount is empty

Refactor: clean up PeriodicFlush() #19085

Conflicts