Refactor: clean up PeriodicFlush() #19085

jnewbery · 2020-05-28T03:01:07Z

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.

DrahtBot · 2020-05-28T04:57:58Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch #19335 (wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch by achow101)
wallet: Introduce WalletDatabase abstract class #19334 (wallet: Introduce WalletDatabase abstract class by achow101)
wallet: Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class #19325 (wallet: Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class by achow101)
wallet: Move BerkeleyBatch static functions to BerkeleyDatabase #19324 (wallet: Move BerkeleyBatch static functions to BerkeleyDatabase by achow101)
wallettool: Add dump and createfromdump commands #19137 (wallettool: Add dump and createfromdump commands by achow101)
wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase #19102 (wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase by achow101)
wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets #19077 (wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101)
Don't call lsn_reset in periodic flush #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.

maflcko · 2020-05-28T10:47:16Z

Concept ⒶⒸⓀ

fanquake

Glanced over 2495f8f 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 · 2020-06-04T15:14:58Z

Thanks @fanquake . I've rebased on master.

fanquake

ACK 9c03104 - 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.

jnewbery · 2020-06-23T18:41:24Z

Rebased

jonatack

ACK 9c03104 nice refactoring

jonatack · 2020-07-01T02:50:34Z

src/wallet/bdb.cpp

-                LogPrint(BCLog::WALLETDB, "Flushing %s\n", strFile);
-                int64_t nStart = GetTimeMillis();
+    // Don't flush if any databases are in use
+    for (auto it = env->mapFileUseCount.begin() ; it != env->mapFileUseCount.end(); it++) {


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

Suggested change

for (auto it = env->mapFileUseCount.begin() ; it != env->mapFileUseCount.end(); it++) {

for (auto it = env->mapFileUseCount.begin() ; it != env->mapFileUseCount.end(); ++it) {

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.

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

Yes. Even better. Will change if I have to retouch.

src/wallet/bdb.cpp

jonatack · 2020-07-01T02:54:50Z

src/wallet/bdb.cpp

+    LogPrint(BCLog::WALLETDB, "Flushing %s\n", strFile);
+    int64_t nStart = GetTimeMillis();
+
+    // Flush wallet file so it's self contained


nit: self-contained

Will change if I need to retouch

jonatack · 2020-07-01T03:04:05Z

Note to reviewers: I suggest looking at the diff with space changes ignored (git show -w).

jnewbery · 2020-07-09T06:07:42Z

rebased

jonatack

re-ACK e846a2a per git range-diff f7c19e8 7c10020 e846a2a

maflcko · 2020-07-09T09:45:01Z

ACK e846a2a 🎁

Show signature and timestamp

Signature:

Timestamp of file with hash d38062803983ae335282eeba787d6c83aeeabfe3699adc77650b02de45c422fc -

maflcko · 2020-07-09T09:45:22Z

cc @achow101 any objections to this?

promag

ACK e846a2a.

promag · 2020-07-09T09:55:31Z

src/wallet/bdb.cpp

-                LogPrint(BCLog::WALLETDB, "Flushing %s\n", strFile);
-                int64_t nStart = GetTimeMillis();
+    // Don't flush if any databases are in use
+    for (auto it = env->mapFileUseCount.begin() ; it != env->mapFileUseCount.end(); it++) {


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

jnewbery · 2020-07-09T10:40:20Z

I think this is ready for merge. It's a simple refactor and has 3 ACKs now.

maflcko · 2020-07-09T11:09:11Z

Went ahead to merge this and postponed to fix the nits in a related pull that had less ACKs (#18923)

Summary: ``` 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. ``` Backport of core [[bitcoin/bitcoin#19085 | PR19085]]. Depends on D8617. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8618

jnewbery requested a review from maflcko May 28, 2020 03:01

fanquake added the Refactoring label May 28, 2020

This was referenced May 28, 2020

wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets #19077

Merged

wallet: Refactor the classes in wallet/db.{cpp/h} #18971

Closed

Don't call lsn_reset in periodic flush #18904

Closed

This was referenced May 28, 2020

refactor: Make CCheckQueue RAII-styled #18731

Closed

Add local thread pool to CCheckQueue #18710

Merged

wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase #19102

Merged

wallettool: Add dump and createfromdump commands #19137

Merged

fanquake reviewed Jun 4, 2020

View reviewed changes

jnewbery force-pushed the 2020-05-refactor-periodic-flush branch from 2495f8f to 9c03104 Compare June 4, 2020 15:15

fanquake mentioned this pull request Jun 5, 2020

wallet: PeriodicFlush called when mapFileUseCount is empty #19175

Closed

fanquake approved these changes Jun 5, 2020

View reviewed changes

This was referenced Jun 16, 2020

wallet: Refactor BerkeleyBatch Read, Write, Erase, and Exists functions into non-template functions #19292

Merged

wallet: move BDB specific classes to bdb.{cpp/h} #19290

Merged

DrahtBot added the Needs rebase label Jun 17, 2020

jnewbery force-pushed the 2020-05-refactor-periodic-flush branch from 9c03104 to 7c10020 Compare June 23, 2020 18:41

DrahtBot removed the Needs rebase label Jun 23, 2020

jonatack reviewed Jul 1, 2020

View reviewed changes

DrahtBot added the Needs rebase label Jul 5, 2020

refactor: clean up PeriodicFlush()

e846a2a

jnewbery force-pushed the 2020-05-refactor-periodic-flush branch from 7c10020 to e846a2a Compare July 9, 2020 06:07

jonatack approved these changes Jul 9, 2020

View reviewed changes

DrahtBot removed the Needs rebase label Jul 9, 2020

promag reviewed Jul 9, 2020

View reviewed changes

maflcko merged commit c4a4418 into bitcoin:master Jul 9, 2020

jnewbery deleted the 2020-05-refactor-periodic-flush branch July 10, 2020 16:31

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

	for (auto it = env->mapFileUseCount.begin() ; it != env->mapFileUseCount.end(); it++) {
	for (auto it = env->mapFileUseCount.begin() ; it != env->mapFileUseCount.end(); ++it) {

Refactor: clean up PeriodicFlush() #19085

Refactor: clean up PeriodicFlush() #19085

Uh oh!

Conversation

jnewbery commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented May 28, 2020

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jun 4, 2020

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jun 23, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

jnewbery Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

promag Jul 9, 2020

Choose a reason for hiding this comment

Uh oh!

jnewbery Jul 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

jnewbery Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

jonatack commented Jul 1, 2020

Uh oh!

jnewbery commented Jul 9, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 9, 2020

Uh oh!

maflcko commented Jul 9, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Jul 9, 2020

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jul 9, 2020

Uh oh!

maflcko commented Jul 9, 2020

Uh oh!

Uh oh!

jnewbery commented May 28, 2020 •

edited

Loading

DrahtBot commented May 28, 2020 •

edited

Loading