Ditch wallet model juggling #417

promag · 2021-09-02T22:41:48Z

Instantiate WalletModel in the main thread - avoids callingsetParent and moveToThread.

Also ensures loading (fetching transactions, addresses, etc) still occurs in the background thread.

hebasto · 2021-09-03T10:08:01Z

There is a change in behavior because now everything that is loaded in WalletModel instantiation happens in the GUI thread.

Shouldn't we move in the opposite direction?

promag · 2021-09-03T10:12:34Z

@hebasto at the moment loading happens on a separate thread but it loads everything, which is not ideal. For now, I think I'll move the loading code from constructors to separate functions so that only QObject instantiations happen on the GUI thread. But later we need to lazy load wallet data.

promag · 2021-09-03T11:01:48Z

@hebasto added fa425d1 to address the above comment, also updated OP.

promag · 2021-11-08T22:19:26Z

Rebased.

hebasto · 2022-01-12T19:05:45Z

Depends on bitcoin/bitcoin#22868 because otherwise, a deadlock would occur during the wallet loading.

bitcoin/bitcoin#22868 has been merged. Rebase?

promag · 2022-02-23T01:00:27Z

src/qt/walletcontroller.cpp

@@ -130,6 +130,8 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wal
        wallet_model = new WalletModel(std::move(wallet), m_client_model, m_platform_style, this);
    }, GUIUtil::blockingGUIThreadConnection());

+    wallet_model->preload();


In a follow-up, some preload will be replaced by lazy loading.

DrahtBot · 2022-04-17T04:22:03Z

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	ryanofsky

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:

#723 (Add warnings for non-active addresses in receive tab and address book by pinheadmz)
#bitcoin/bitcoin/26699 (wallet, gui: bugfix, getAvailableBalance skips selected coins by furszy)

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.

hebasto · 2022-05-21T15:56:46Z

This change takes advantage of GUIUtil::ObjectInvoke to avoid workarounds around how Qt handles object instantiation and threads.

GUIUtil::ObjectInvoke seems a bit outdated :)

hebasto · 2022-10-26T15:52:36Z

cc @ryanofsky

ryanofsky · 2022-10-26T16:12:11Z

IIUC, the second commit "qt: Move wallet preloading out of model constructors" (e2ffc98) seems like a good change, moving wallet "preloading" tasks off of the GUI thread into timer threads or notification callback threads. But I'm not 100% sure this is what it is doing. Would be helpful if the commit description said what loading behavior was before the change and how it is different afterward.

The first commit "qt: Ditch wallet model juggling" (f3e7047) is more confusing, because it seems like the new code is doing the same thing as the old code, just making slightly different function calls to do the same thing. This is probably ok, but it would be helpful if commit message said whether this is just supposed to be a local code cleanup, or a bigger change that affects other code. Also that commit seems to delete comments which I think are helpful about why particular threads are assigned. The new comment says what thread is assigned but not why.

promag · 2022-10-31T10:04:03Z

@ryanofsky

The first commit "qt: Ditch wallet model juggling" (f3e7047) is more confusing, because it seems like the new code is doing the same thing as the old code, just making slightly different function calls to do the same thing. This is probably ok, but it would be helpful if commit message said whether this is just supposed to be a local code cleanup, or a bigger change that affects other code. Also that commit seems to delete comments which I think are helpful about why particular threads are assigned. The new comment says what thread is assigned but not why.

It doesn't do the same thing as before since WalletModel constructor is executed in the GUI thread whereas before it was executed in the wallet controller background thread or on the RPC handling thread. This change makes loading big wallets block the GUI. But this blocking issue is "fixed" in the following commit.

In terms of Qt calls, it avoids moveToThread and setParent, since now WalletModel is instanced in the GUI thread with the right parent.

IIUC, the second commit "qt: Move wallet preloading out of model constructors" (e2ffc98) seems like a good change, moving wallet "preloading" tasks off of the GUI thread into timer threads or notification callback threads. But I'm not 100% sure this is what it is doing. Would be helpful if the commit description said what loading behavior was before the change and how it is different afterward.

So, the WalletModel constructor instantiated the remaining models (like TransactionTableModel) and those models would load everything from interfaces::Wallet. This happened on the wallet controller background thread or the RPC handling thread, depending on how the wallet was loaded. So, the WalletModel constructor could block for a while for big wallets - that was the reason for not instantiating the model in the GUI thread.

This commit splits instancing models and loading data. Instancing models still happens on the GUI thread while loading data happens on a separate thread. This "fixes" the blocking issue introduced in the 1st commit.

Note that, even though preload happens on a background thread, we can have a blocked GUI because underneath loading locks some mutexes that other parts of the GUI could also try to lock. For instance, WalletImpl::getWalletTxs locks cs_wallet, and the more transactions the worse.

Now that we have the loading split from the instancing, we can improve the loading for big wallets. For instance:

load transactions only when the user opens the transactions list, same for addresses, etc
adopt a lazy loading approach
chunked loading where locks don't get locked for so long.
read-write locks (?)

ryanofsky

Thanks for the explanations. This seems like a pretty good change that just took me a long time to understand. My description would be:

Stop loading wallet data inside the WalletModel constructor, so the WalletModel can now be constructed more straightforwardly on the GUI thread without the need for setParent and moveToThread calls, but still not blocking the GUI thread

Code review ACK e2ffc98, but I left a lot of suggestions to clarify, and really would like to see the order of commits switched so wallet data keeps being loaded on the same thread and does not switch to a different thread and then back again.

ryanofsky · 2022-11-15T18:18:46Z

src/qt/walletcontroller.cpp

-    QMetaObject::invokeMethod(this, [wallet_model, this] {
-        wallet_model->setParent(this);
+    WalletModel* wallet_model;
+    // Instantiate model in GUI main thread.


In commit "qt: Ditch wallet model juggling" (f3e7047)

It seems unfortunate that this commit is deleting previous comment which explained what this code was trying to do, replacing it with a much shorter comment that doesn't provide much explanation.

It would be good to combine two comments with something like:

// Instantiate WalletModel in the thread that created the WalletControllet object (GUI main thread), instead of the current thread, which could be an outside wallet thread or RPC thread sending a LoadWallet notification. This ensures queued signals sent to the WalletModel object will be handled on the GUI event loop.

I also think commit message is unclear about what behavior is changing. Initially I didn't realize that constructing the WalletModel was so expensive and actually loaded transactions, so this at first appeared to be a no-op change that was replacing setParent and moveToThread calls with a constructor call. Would suggest adding an explanation like the following to the commit message:

qt: Ditch wallet model juggling

Construct WalletModel on the GUI thread, not on the outside thread which loaded the wallet. This is a simplification because it avoids the need to make setParent and moveToThread calls after the wallet is constructed. This commit however can cause GUI freezes while loading the wallet because the WalletModel constructor can take a long time to run if the wallet contains a lot of transactions. The GUI freeze issue is addressed by making WalletModel initialization more asynchronous in the next commit."

Or, as an alternative, if you switch the order of the two commits, you could drop "This commit however can cause GUI freezes..." part and everything after.

ryanofsky · 2022-11-15T18:48:23Z

src/qt/walletcontroller.cpp

@@ -131,6 +131,8 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wal
        wallet_model = new WalletModel(std::move(wallet), m_client_model, m_platform_style, this);
    }, GUIUtil::blockingGUIThreadConnection());

+    wallet_model->preload();


In commit "qt: Move wallet preloading out of model constructors" (e2ffc98)

I think it would be good to switch the order of the two commits so the other commit doesn't cause a performance regression, and so wallet preloading doesn't move from the outside thread to the GUI thread than back to the outside thread again within the PR.

Additionally might suggest renaming preload to loadData to give a clearer idea what this method is actually doing and avoid the "pre" prefix, which I'm not sure actually means anything, since it runs separately after the constructor so might more logically be called "postload".

Also would add a rationale to the commit description like:

The allows WalletModel constructor to be called on the GUI thread (so queued signals sent to the wallet model object will execute on the GUI thread), while allowing wallet data to be loaded off the GUI thread, so the GUI will not freeze while the data is loading.

promag · 2022-11-15T21:33:06Z

@ryanofsky cool, I'll address your comments, makes sense to reorder commits.

DrahtBot · 2023-04-11T18:46:52Z

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot · 2023-07-10T00:49:18Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

hebasto · 2023-09-22T17:46:15Z

Closing for now due to lack of progress. Let us know if this should be reopened.

Labeling "Up for grabs" as well.

promag force-pushed the 2021-09-eager-wallet-load-1 branch 4 times, most recently from 5ba0643 to c09fd3a Compare September 3, 2021 07:47

hebasto added the Wallet label Sep 3, 2021

promag force-pushed the 2021-09-eager-wallet-load-1 branch 4 times, most recently from b498cac to a845f63 Compare September 3, 2021 14:16

DrahtBot added the Needs rebase label Sep 9, 2021

promag mentioned this pull request Nov 8, 2021

wallet: Call load handlers without cs_wallet locked bitcoin/bitcoin#22868

Merged

promag force-pushed the 2021-09-eager-wallet-load-1 branch from a845f63 to 0697de7 Compare November 8, 2021 22:04

DrahtBot removed the Needs rebase label Nov 8, 2021

promag force-pushed the 2021-09-eager-wallet-load-1 branch from 0697de7 to 458d682 Compare February 23, 2022 00:55

promag commented Feb 23, 2022

View reviewed changes

Gemk83 approved these changes Feb 26, 2022

View reviewed changes

DrahtBot mentioned this pull request Apr 17, 2022

refactor: Replace GUIUtil::ObjectInvoke() with QMetaObject::invokeMethod() #587

Merged

DrahtBot added the Needs rebase label Apr 22, 2022

promag added 2 commits May 21, 2022 14:49

qt: Ditch wallet model juggling

f3e7047

qt: Move wallet preloading out of model constructors

e2ffc98

promag force-pushed the 2021-09-eager-wallet-load-1 branch from 458d682 to e2ffc98 Compare May 21, 2022 13:53

DrahtBot removed the Needs rebase label May 21, 2022

DrahtBot mentioned this pull request May 27, 2022

refactor: Skip manual signal disconnection in descructors #607

Closed

ryanofsky approved these changes Nov 15, 2022

View reviewed changes

DrahtBot mentioned this pull request Dec 17, 2022

wallet, gui: bugfix, getAvailableBalance skips selected coins bitcoin/bitcoin#26699

Merged

DrahtBot mentioned this pull request Mar 24, 2023

Add warnings for non-active addresses in receive tab and address book #723

Closed

DrahtBot added the Needs rebase label Apr 11, 2023

hebasto closed this Sep 22, 2023

hebasto added the Up for grabs label Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ditch wallet model juggling #417

Ditch wallet model juggling #417

promag commented Sep 2, 2021 •

edited

Loading

hebasto commented Sep 3, 2021

promag commented Sep 3, 2021

promag commented Sep 3, 2021

promag commented Nov 8, 2021

hebasto commented Jan 12, 2022

promag Feb 23, 2022

DrahtBot commented Apr 17, 2022 •

edited

Loading

hebasto commented May 21, 2022

hebasto commented Oct 26, 2022

ryanofsky commented Oct 26, 2022

promag commented Oct 31, 2022 •

edited

Loading

ryanofsky left a comment •

edited

Loading

ryanofsky Nov 15, 2022

ryanofsky Nov 15, 2022

promag commented Nov 15, 2022

DrahtBot commented Apr 11, 2023

DrahtBot commented Jul 10, 2023

hebasto commented Sep 22, 2023

Ditch wallet model juggling #417

Ditch wallet model juggling #417

Conversation

promag commented Sep 2, 2021 • edited Loading

hebasto commented Sep 3, 2021

promag commented Sep 3, 2021

promag commented Sep 3, 2021

promag commented Nov 8, 2021

hebasto commented Jan 12, 2022

promag Feb 23, 2022

Choose a reason for hiding this comment

DrahtBot commented Apr 17, 2022 • edited Loading

Reviews

Conflicts

hebasto commented May 21, 2022

hebasto commented Oct 26, 2022

ryanofsky commented Oct 26, 2022

promag commented Oct 31, 2022 • edited Loading

ryanofsky left a comment • edited Loading

Choose a reason for hiding this comment

ryanofsky Nov 15, 2022

Choose a reason for hiding this comment

ryanofsky Nov 15, 2022

Choose a reason for hiding this comment

promag commented Nov 15, 2022

DrahtBot commented Apr 11, 2023

DrahtBot commented Jul 10, 2023

hebasto commented Sep 22, 2023

promag commented Sep 2, 2021 •

edited

Loading

DrahtBot commented Apr 17, 2022 •

edited

Loading

promag commented Oct 31, 2022 •

edited

Loading

ryanofsky left a comment •

edited

Loading