wallet: Move wallets loading out from the main GUI thread

hebasto commented at 7:50 pm on May 21, 2021: member

This PR improves the GUI responsiveness during initial wallets loading at startup (especially ones that have tons of txs), and shows a standard progress dialog for long loading:

DeepinScreenshot_select-area_20210522230626

Fixes #247.

hebasto requested review from promag on May 21, 2021

hebasto requested review from ryanofsky on May 21, 2021

jarolrod commented at 9:00 pm on May 21, 2021: member

Some occurrences of a seg fault in tests:

hebasto added the label Wallet on May 22, 2021

hebasto marked this as a draft on May 22, 2021

hebasto closed this on May 22, 2021

hebasto reopened this on May 22, 2021

hebasto force-pushed on May 22, 2021

hebasto added the label UX on May 22, 2021

hebasto marked this as ready for review on May 22, 2021

in src/qt/guiutil.cpp:821 in 75850106ae outdated

817@@ -818,7 +818,6 @@ void PolishProgressDialog(QProgressDialog* dialog)
818     // Workaround for macOS-only Qt bug; see: QTBUG-65750, QTBUG-70357.
819     const int margin = TextWidth(dialog->fontMetrics(), ("X"));
820     dialog->resize(dialog->width() + 2 * margin, dialog->height());
821-    dialog->show();

promag commented at 11:05 am on May 24, 2021:

75850106aeecfed1d2dc16d8a67ec210c5826a47

Direct call show() breaks that behavior on macos.

By “break” you mean it always shows, doesn’t take into account minimumDuration. Only macos?

hebasto commented at 5:55 am on September 7, 2021:

By “break” you mean it always shows, doesn’t take into account minimumDuration.

Exactly.

Only macos?

The removed code is placed within #ifdef Q_OS_MAC.

in src/qt/walletcontroller.cpp:189 in dc124101cc outdated

205-    m_progress_dialog->setLabelText(label_text);
206-    m_progress_dialog->setRange(0, 0);
207-    m_progress_dialog->setCancelButton(nullptr);
208-    m_progress_dialog->setWindowModality(Qt::ApplicationModal);
209-    GUIUtil::PolishProgressDialog(m_progress_dialog);
210+    auto progress_dialog = new QProgressDialog(m_parent_widget);

promag commented at 7:57 am on May 26, 2021:

dc124101cc50ca8dfd26796f26caf87316f3182d

This approach is fine for indeterminate progress dialog. If we want to change the progress or update the label text then having the member m_progress_dialog is better.

Either way, I think this change is unrelated to the goal of this PR.

ryanofsky commented at 6:45 pm on September 7, 2021:

In commit “qt, wallet: Move activity progress dialog from data member to local” (6035536d8875f9cf3561048abd486272756e15dc)

Either way, I think this change is unrelated to the goal of this PR.

People will have different ideas of what’s related/unrelated, but I think this commit fits in with the rest of the PR by making sure activities are constructed/destroyed/used the same way. I can see how keeping m_progress_dialog pointer might make it possible to attach more inputs to the dialog later, but this apparently isn’t needed right now, and even trying to do it seems like it would involve other complications like managing the pointer lifetime. By contrast the approach here of just connecting signals right away and not keeping the pointer seems simple while still being extensible.

in src/qt/walletcontroller.cpp:184 in 4517c51b86 outdated

189@@ -190,6 +190,7 @@ WalletControllerActivity::WalletControllerActivity(WalletController* wallet_cont
190     , m_wallet_controller(wallet_controller)
191     , m_parent_widget(parent_widget)
192 {
193+    connect(this, &WalletControllerActivity::finished, this, &QObject::deleteLater);

promag commented at 8:01 am on May 26, 2021:

4517c51b8605fbf91e2319b16d7d5dcf80cb1565

Originally the idea was to allow the user to see activity details and then the user could dismiss or it could auto dismiss. I also think this is an unrelated change - this PR could also connect LoadWalletsActivity finished signal. But this is also fine and can be improved if later the original idea is implemented.

promag commented at 8:08 am on May 26, 2021: contributor

Concept ACK.

ryanofsky approved

ryanofsky commented at 10:57 pm on May 27, 2021: contributor

Light code review ACK 8c9a9a39876afc650ebb3a54becd652e9b03e66f

I do think it is unfortunate so much boilerplate (WalletControllerActivity class definition, multiple methods, multiple single shot timers and signals) is added just to run 3 lines of code asynchronously.

It should be possible to have reusable utility:

0RunAsynchronously([this] {
1   for (std::unique_ptr<interfaces::Wallet>& wallet : node().walletClient().getWallets()) {
2       m_wallet_controller->getOrCreateWallet(std::move(wallet));
3   }
4})

I did implement something like this previously (but with more features) as an experiment in https://github.com/bitcoin/bitcoin/compare/master...ryanofsky:pr/gsync comment https://github.com/bitcoin/bitcoin/issues/16874#issuecomment-632424392

promag commented at 3:36 pm on May 31, 2021: contributor

@ryanofsky agree, that or QtConcurrent::run or QMetaObject::invokeMethod (requires min Qt 5.10 IIRC) to just run some code async. But if you want to show progress feedback, update something with “loading wallet x”, allow the user to interrupt, maybe pause, at the end ask something else, then wrapping the activity state in some class makes sense to me.

DrahtBot added the label Needs rebase on Jun 5, 2021

promag commented at 11:00 am on September 5, 2021: contributor

@hebasto what do you think about splitting this PR?

hebasto commented at 11:27 am on September 5, 2021: member

@promag

@hebasto what do you think about splitting this PR?

Sounds good. In what parts?

shaavan commented at 3:22 pm on September 6, 2021: contributor

tACK 8c9a9a39876afc650ebb3a54becd652e9b03e66f Tested on Ubuntu 20.04 (Using Qt version 5.12.8)

This PR adds a ‘Loading Wallet’ progress window for the initial loading of opened wallet after opening GUI. This PR also moves wallet loading out of the main GUI thread while simplifying some other aspects of the codebase.

I was able to test the PR successfully. I have added a screenshot for the resultant progress window I observed while opening the GUI. Screenshot from 2021-09-06 20-06-46

The changes made in this PR are pretty extensive and detailed. Thanks for this fantastic work, @hebasto. It took me quite a while to understand everything that has been done in this PR. That’s why I am sharing my notes that discuss the changes in quite detail. So that other reviewers could have an easier time reviewing this PR. If some of my observations are erroneous, please do inform me.

NOTES:

qt, macos: Fix GUIUtil::PolishProgressDialog bug
- Done to prevent misbehaving of this PR in macOS.
qt: Improve GUI responsiveness
- Removed Q_UNUSED because now dialog is being used at least once in any case under this function.
- setMinimumDuration is set through the PolishProgressDialog function. Since this function is used multiple times when setting setMinimumDuration was required, it makes sense to move that in this function. This change simplifies the code while maintaining the same functionality.
- Under WalletControllerActivity::showProgressDialog function, setValue is used for the m_progress_dialog variable. This starts the internal duration estimation.
- The now redundant setMinimumDuration instruction from BitcoinGUI::showProgress and WalletView::showProgress functions.
qt, wallet: Move activity progress dialog from data member to local
- Since the activity progress dialog has been moved from data member to local variable, subsequent changes have been made:
  - WalletControllerActivity destructor has been restored to default destructor.
  - In WalletControllerActivity::showProgressDialog function:
    - m_progress_dialog variable has been renamed to progress_dialog to avoid naming confusion with a data member.
    - The non-existence of m_progress_dialog (or progress_dialog) is not being asserted now.
  - The destroyProgressDialog function is also removed. Because now progress_dialog doesn’t exist out of the function scope and hence no need for a progress_dialog destructor.
qt, wallet, refactor: Move connection to QObject::deleteLater to ctor
- Instead of creating similar connections multiple times for individual objects of class, the connection has been moved to the constructor of the common parent class itself, resulting in simplification of the code.
qt, wallet: Add LoadWalletsActivity
- New LoadWalletsActivity class is declared as a child class of the WalletControllerActivity class.
- A function Load is added to the class that calls the showProgressDialog function and loads the wallets.
- This new class and its functions are appropriately declared under the walletcontroler.h file
qt, wallet: Move wallets loading out from the main GUI thread
- The new LoadWalletActivity::Load function is now used to load wallets at the start of the GUI.
- The redundant code, which is now part of the Load function, has been removed from the WalletController constructor.

I hope these notes will be of some help to fellow reviewers.

hebasto force-pushed on Sep 7, 2021

hebasto commented at 6:32 am on September 7, 2021: member

Rebased 8c9a9a39876afc650ebb3a54becd652e9b03e66f -> e07509867ec14ccb4f2f5bce082f73b77ade7743 (pr342.02 -> pr342.03).

DrahtBot removed the label Needs rebase on Sep 7, 2021

in src/qt/bitcoingui.cpp:665 in e07509867e outdated

658@@ -659,9 +659,8 @@ void BitcoinGUI::setWalletController(WalletController* wallet_controller)
659     GUIUtil::ExceptionSafeConnect(wallet_controller, &WalletController::walletAdded, this, &BitcoinGUI::addWallet);
660     connect(wallet_controller, &WalletController::walletRemoved, this, &BitcoinGUI::removeWallet);
661 
662-    for (WalletModel* wallet_model : m_wallet_controller->getOpenWallets()) {

ryanofsky commented at 6:50 pm on September 7, 2021:

In commit “qt, wallet: Move wallets loading out from the main GUI thread” (e07509867ec14ccb4f2f5bce082f73b77ade7743)

The getOpenWallets wallets method seems unused after this change and could be dropped

in src/qt/walletcontroller.cpp:348 in 6edff4dfa5 outdated

353+    showProgressDialog(tr("Loading wallets…"));
354+
355+    QTimer::singleShot(0, worker(), [this] {
356+        for (std::unique_ptr<interfaces::Wallet>& wallet : node().walletClient().getWallets()) {
357+            m_wallet_controller->getOrCreateWallet(std::move(wallet));
358+        }

ryanofsky commented at 7:03 pm on September 7, 2021:

In commit “qt, wallet: Add LoadWalletsActivity” (6edff4dfa508100564c3152bbccc7b37ac777528)

Note: This is not new code, this is existing code which is just being moved. The original code is deleted in the next commit. I would squash two commits for more clarity here, but it’s not important, and of course the best way to split and group commits is subjective.

ryanofsky approved

ryanofsky commented at 7:08 pm on September 7, 2021: contributor

Code review ACK e07509867ec14ccb4f2f5bce082f73b77ade7743. I still think the need for these activity classes and timer hacks is unfortunate, because it should be possible to have a utility function that takes a lambda, runs it asynchronously and posts a result back to the GUI thread. (The lambda would also be able to capture variables so would not be precluded from receiving input or posting incremental feedback either.) But the activity class here is pretty small and this suggestion isn’t very important, just something to keep in mind if we are looking to reduce boilerplate and improve readability later.

ryanofsky commented at 12:53 pm on September 8, 2021: contributor

It seems like this would fix #247 by making the gui responsive during walletmodel loading. Is this the case? It might also be good to mention in the PR description

qt, wallet: Move activity progress dialog from data member to local f9b633eeab

qt, wallet, refactor: Move connection to QObject::deleteLater to ctor 4a024fc310

qt, wallet: Add LoadWalletsActivity class

Also this commit moves wallets loading out from the main GUI thread.

f6991cb906

qt, wallet: Drop no longer used WalletController::getOpenWallets() 2fe69efbc6

hebasto force-pushed on Sep 9, 2021

hebasto commented at 7:44 pm on September 9, 2021: member

@ryanofsky @shaavan @promag

Thank you for your reviews!

Updated e07509867ec14ccb4f2f5bce082f73b77ade7743 -> 2fe69efbc607fdcc3657637d59a38cc5b4db2d05 (pr342.03 -> pr342.04):

addressed @ryanofsky comment:

The getOpenWallets wallets method seems unused after this change and could be dropped

addressed @ryanofsky comment:

This is not new code, this is existing code which is just being moved. The original code is deleted in the next commit. I would squash two commits for more clarity here

rebased on top of the recent changes in CI

@ryanofsky

It seems like this would fix #247 by making the gui responsive during walletmodel loading. Is this the case?

I don’t think creating WalletModel instances is the most time consuming process. Another part is WalletClient::start(). And I believe to fix #247, both PRs are required – https://github.com/bitcoin/bitcoin/pull/22231 and this one.

UPDATE: #342 (comment)

It might also be good to mention in the PR description

Done.

promag commented at 9:48 pm on September 11, 2021: contributor

I don’t think creating WalletModel instances is the most time consuming process.

This is not true, in fact, it’s the most expensive task, because on WalletModel constructor AddressTableModel and TransactionTableModel are also instanced, and these prefetch wallet addresses and transactions.

hebasto commented at 10:23 am on September 12, 2021: member

I don’t think creating WalletModel instances is the most time consuming process.

This is not true, in fact, it’s the most expensive task, because on WalletModel constructor AddressTableModel and TransactionTableModel are also instanced, and these prefetch wallet addresses and transactions.

Yeah, TransactionTablePriv::refreshWallet could really take much time for big wallets.

hebasto commented at 12:40 pm on September 12, 2021: member

@ryanofsky

It seems like this would fix #247 by making the gui responsive during walletmodel loading. Is this the case? It might also be good to mention in the PR description

I’ve re-read logs provided by @laanwj in #247#issue-830828828 and I see now you are right.

in src/qt/walletcontroller.cpp:345 in 2fe69efbc6

340+
341+void LoadWalletsActivity::load()
342+{
343+    showProgressDialog(tr("Loading wallets…"));
344+
345+    QTimer::singleShot(0, worker(), [this] {

promag commented at 8:12 pm on September 12, 2021:

Use GUIUtil::ObjectInvoke here and below?

hebasto commented at 12:16 pm on September 29, 2021:

I’d rather keep this PR as is, and in a follow up use GUIUtil::ObjectInvoke in a wider scope of all walletcontroller.cpp.

in src/qt/walletcontroller.cpp:347 in 2fe69efbc6

342+{
343+    showProgressDialog(tr("Loading wallets…"));
344+
345+    QTimer::singleShot(0, worker(), [this] {
346+        for (auto& wallet : node().walletClient().getWallets()) {
347+            m_wallet_controller->getOrCreateWallet(std::move(wallet));

promag commented at 8:13 pm on September 12, 2021:

nit, we could show progress and/or show each wallet name.

ryanofsky approved

ryanofsky commented at 6:43 pm on September 16, 2021: contributor

Code review ACK 2fe69efbc607fdcc3657637d59a38cc5b4db2d05. Just suggested changes since last review: squashing commits and dropping unused method (thanks!)

shaavan approved

shaavan commented at 3:43 pm on September 29, 2021: contributor

reACK 2fe69efbc607fdcc3657637d59a38cc5b4db2d05

Updates since my last review:

PR rebased on current master
Two Commits. qt, macOS: Fix GUIUtil::PolishProgressDialog bug and qt: Improve GUI responsiveness are dropped because they are no longer needed.
Made appropriate changes to other commits and removed parts that are no longer needed or used.
qt, wallet: Move wallets loading out from the main GUI thread and qt, wallet: Add LoadWalletsActivity class have been squashed together.
New commit qt, wallet: Drop no longer used WalletController::getOpenWallets() which, as it say so, removed getOpenWallets() function.

promag commented at 2:16 pm on September 30, 2021: contributor

Code review ACK 2fe69efbc607fdcc3657637d59a38cc5b4db2d05.

hebasto merged this on Sep 30, 2021

hebasto closed this on Sep 30, 2021

hebasto deleted the branch on Sep 30, 2021

sidhujag referenced this in commit f63fcb62ba on Sep 30, 2021

bitcoin-core locked this on Sep 30, 2022

wallet: Move wallets loading out from the main GUI thread #342