Refactor: clean up PeriodicFlush()

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++

0    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 🎁

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK e846a2a1d9e8aa37abfa55bd4d2a0a9f55ba6c8e 🎁
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgG+wv/SWkvdinhfmWvllvmoyHz6TKFcTwBBsvJFneg6saYLV/A21hlxGfD+iS8
 8xu5f03nFpFsb+bFQvRcm2FtG1McxQTnmEcnNUss0MZ53F8uNCoIG5kfM1eAfBXuw
 9omRgHgMAk2NMZmCUrHT2Qv4ZOKwUZ2N/4TiVvc2apXy6Zbgf0NKxqagGKs7yRXKN
10arajJLM8ua6ewKQFZtPZySRvH03Ed33im0vpae/nsSmluqtJ3VnzVhoCGuxybKzT
11RpbBtjD4ayMC/O8q2E93+VHao/zB/CIEpess1KEWIkyyT7w/JZ86XYiVWcEPdfkD
12yHUPoPTMOhJuPe/1Vvx9sMxoSV07c/aAgodFDP/UHHaT1JIPfZwyMHWmXcx5pIZd
13V9qIdH2uk2mBEp57o5HcJlA/57y4gtDd32WGo23mo/Ef1kqMKwjBs/o2YnE0/f+e
14eoCn9pDih3lnbxg919suNbfCtLpVQAlqHVXMe5XafrYDLSQtXGBkfCk33CTSTEGL
158AJuiXGE
16=0P36
17-----END PGP SIGNATURE-----

Timestamp of file with hash d38062803983ae335282eeba787d6c83aeeabfe3699adc77650b02de45c422fc -

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

Refactor: clean up PeriodicFlush() #19085

Conflicts