Ditch wallet model juggling

promag commented at 10:41 pm on September 2, 2021: contributor

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

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

promag force-pushed on Sep 2, 2021

promag force-pushed on Sep 3, 2021

hebasto added the label Wallet on Sep 3, 2021

hebasto commented at 10:08 am on September 3, 2021: member

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 commented at 10:12 am on September 3, 2021: contributor

@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 commented at 11:01 am on September 3, 2021: contributor

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

promag force-pushed on Sep 3, 2021

DrahtBot added the label Needs rebase on Sep 9, 2021

promag force-pushed on Nov 8, 2021

DrahtBot removed the label Needs rebase on Nov 8, 2021

promag commented at 10:19 pm on November 8, 2021: contributor

Rebased.

hebasto commented at 7:05 pm on January 12, 2022: member

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

bitcoin/bitcoin#22868 has been merged. Rebase?

promag force-pushed on Feb 23, 2022

in src/qt/walletcontroller.cpp:134 in 458d682aaf outdated

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

promag commented at 1:00 am on February 23, 2022:

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

Gemk83 approved

DrahtBot commented at 4:22 am on April 17, 2022: 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	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.

DrahtBot added the label Needs rebase on Apr 22, 2022

qt: Ditch wallet model juggling f3e7047925

qt: Move wallet preloading out of model constructors e2ffc98e73

promag force-pushed on May 21, 2022

DrahtBot removed the label Needs rebase on May 21, 2022

hebasto commented at 3:56 pm on May 21, 2022: member

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 commented at 3:52 pm on October 26, 2022: member

cc @ryanofsky

ryanofsky commented at 4:12 pm on October 26, 2022: contributor

IIUC, the second commit “qt: Move wallet preloading out of model constructors” (e2ffc98e73df68d0de9755d2ebbb26daa17c92fc) 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” (f3e70479257770bc9f61de694db8704140139548) 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 commented at 10:04 am on October 31, 2022: contributor

@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 (?)

in src/qt/walletcontroller.cpp:129 in f3e7047925 outdated

137-    wallet_model->moveToThread(thread());
138-    // setParent(parent) must be called in the thread which created the parent object. More details in #18948.
139-    QMetaObject::invokeMethod(this, [wallet_model, this] {
140-        wallet_model->setParent(this);
141+    WalletModel* wallet_model;
142+    // Instantiate model in GUI main thread.

ryanofsky commented at 6:18 pm on November 15, 2022:

In commit “qt: Ditch wallet model juggling” (f3e70479257770bc9f61de694db8704140139548)

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.

in src/qt/walletcontroller.cpp:134 in e2ffc98e73

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

ryanofsky commented at 6:48 pm on November 15, 2022:

In commit “qt: Move wallet preloading out of model constructors” (e2ffc98e73df68d0de9755d2ebbb26daa17c92fc)

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.

ryanofsky approved

ryanofsky commented at 7:02 pm on November 15, 2022: contributor

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 e2ffc98e73df68d0de9755d2ebbb26daa17c92fc, 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.

promag commented at 9:33 pm on November 15, 2022: contributor

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

DrahtBot added the label Needs rebase on Apr 11, 2023

DrahtBot commented at 6:46 pm on April 11, 2023: contributor

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

DrahtBot commented at 0:49 am on July 10, 2023: contributor

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 commented at 5:46 pm on September 22, 2023: member

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

Labeling “Up for grabs” as well.

hebasto closed this on Sep 22, 2023

hebasto added the label Up for grabs on Sep 22, 2023

bitcoin-core locked this on Sep 21, 2024

Ditch wallet model juggling #417

Reviews

Conflicts