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

hebasto · 2021-05-21T19:50:59Z

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:

Fixes #247.

jarolrod · 2021-05-21T21:00:02Z

Some occurrences of a seg fault in tests:

promag

Concept ACK.

promag · 2021-05-24T11:05:23Z

src/qt/guiutil.cpp

    // Workaround for macOS-only Qt bug; see: QTBUG-65750, QTBUG-70357.
    const int margin = TextWidth(dialog->fontMetrics(), ("X"));
    dialog->resize(dialog->width() + 2 * margin, dialog->height());
-    dialog->show();


7585010

Direct call show() breaks that behavior on macos.

By "break" you mean it always shows, doesn't take into account minimumDuration. Only macos?

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.

promag · 2021-05-26T07:57:54Z

src/qt/walletcontroller.cpp

-    m_progress_dialog->setCancelButton(nullptr);
-    m_progress_dialog->setWindowModality(Qt::ApplicationModal);
-    GUIUtil::PolishProgressDialog(m_progress_dialog);
+    auto progress_dialog = new QProgressDialog(m_parent_widget);


dc12410

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.

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

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.

promag · 2021-05-26T08:01:33Z

src/qt/walletcontroller.cpp

    , m_wallet_controller(wallet_controller)
    , m_parent_widget(parent_widget)
 {
+    connect(this, &WalletControllerActivity::finished, this, &QObject::deleteLater);


4517c51

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.

ryanofsky

Light code review ACK 8c9a9a3

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:

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

I did implement something like this previously (but with more features) as an experiment in bitcoin/bitcoin@master...ryanofsky:pr/gsync comment bitcoin/bitcoin#16874 (comment)

promag · 2021-05-31T15:36:04Z

@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.

promag

@hebasto what do you think about splitting this PR?

hebasto · 2021-09-05T11:27:29Z

@promag

@hebasto what do you think about splitting this PR?

Sounds good. In what parts?

shaavan

tACK 8c9a9a3
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.

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 · 2021-09-07T06:32:55Z

Rebased 8c9a9a3 -> e075098 (pr342.02 -> pr342.03).

ryanofsky

Code review ACK e075098. 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 · 2021-09-07T18:45:42Z

src/qt/walletcontroller.cpp

-    m_progress_dialog->setCancelButton(nullptr);
-    m_progress_dialog->setWindowModality(Qt::ApplicationModal);
-    GUIUtil::PolishProgressDialog(m_progress_dialog);
+    auto progress_dialog = new QProgressDialog(m_parent_widget);


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

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.

ryanofsky · 2021-09-07T18:50:22Z

src/qt/bitcoingui.cpp

    GUIUtil::ExceptionSafeConnect(wallet_controller, &WalletController::walletAdded, this, &BitcoinGUI::addWallet);
    connect(wallet_controller, &WalletController::walletRemoved, this, &BitcoinGUI::removeWallet);

-    for (WalletModel* wallet_model : m_wallet_controller->getOpenWallets()) {


In commit "qt, wallet: Move wallets loading out from the main GUI thread" (e075098)

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

ryanofsky · 2021-09-07T19:03:22Z

src/qt/walletcontroller.cpp

+        for (std::unique_ptr<interfaces::Wallet>& wallet : node().walletClient().getWallets()) {
+            m_wallet_controller->getOrCreateWallet(std::move(wallet));
+        }


In commit "qt, wallet: Add LoadWalletsActivity" (6edff4d)

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 · 2021-09-08T12:53:15Z

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

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

hebasto · 2021-09-09T19:44:18Z

@ryanofsky @shaavan @promag

Thank you for your reviews!

Updated e075098 -> 2fe69ef (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 -- bitcoin/bitcoin#22231 and this one.

UPDATE: #342 (comment)

It might also be good to mention in the PR description

Done.

promag · 2021-09-11T21:48:53Z

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 · 2021-09-12T10:23:52Z

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 · 2021-09-12T12:40:45Z

@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 (comment) and I see now you are right.

promag · 2021-09-12T20:12:28Z

src/qt/walletcontroller.cpp

+{
+    showProgressDialog(tr("Loading wallets…"));
+
+    QTimer::singleShot(0, worker(), [this] {


Use GUIUtil::ObjectInvoke here and below?

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

promag · 2021-09-12T20:13:34Z

src/qt/walletcontroller.cpp

+
+    QTimer::singleShot(0, worker(), [this] {
+        for (auto& wallet : node().walletClient().getWallets()) {
+            m_wallet_controller->getOrCreateWallet(std::move(wallet));


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

ryanofsky

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

shaavan

reACK 2fe69ef

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

Code review ACK 2fe69ef.

hebasto requested review from promag and ryanofsky May 21, 2021 19:51

hebasto added the Wallet label May 22, 2021

hebasto marked this pull request as draft May 22, 2021 19:37

hebasto closed this May 22, 2021

hebasto reopened this May 22, 2021

hebasto force-pushed the 210521-wallet branch from 7c1c183 to 8c9a9a3 Compare May 22, 2021 20:10

hebasto added the UX All about "how to get things done" label May 22, 2021

hebasto marked this pull request as ready for review May 22, 2021 20:16

promag reviewed May 26, 2021

View reviewed changes

ryanofsky approved these changes May 27, 2021

View reviewed changes

DrahtBot added the Needs rebase label Jun 5, 2021

This was referenced Jun 12, 2021

Separate AppInitStartClients from AppInitMain bitcoin/bitcoin#22231

Closed

bitcoin-qt stays at "Done loading" for a long time #247

Closed

promag reviewed Sep 5, 2021

View reviewed changes

shaavan reviewed Sep 6, 2021

View reviewed changes

hebasto force-pushed the 210521-wallet branch from 8c9a9a3 to e075098 Compare September 7, 2021 06:31

DrahtBot removed the Needs rebase label Sep 7, 2021

ryanofsky approved these changes Sep 7, 2021

View reviewed changes

hebasto added 4 commits September 9, 2021 21:57

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

f9b633e

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

4a024fc

qt, wallet: Add LoadWalletsActivity class

f6991cb

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

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

2fe69ef

hebasto force-pushed the 210521-wallet branch from e075098 to 2fe69ef Compare September 9, 2021 19:26

promag reviewed Sep 12, 2021

View reviewed changes

ryanofsky approved these changes Sep 16, 2021

View reviewed changes

shaavan approved these changes Sep 29, 2021

View reviewed changes

promag reviewed Sep 30, 2021

View reviewed changes

hebasto merged commit 9013f23 into bitcoin-core:master Sep 30, 2021

hebasto deleted the 210521-wallet branch September 30, 2021 14:37

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 30, 2021

Merge bitcoin-core/gui#342: wallet: Move wallets loading out from the…

f63fcb6

hebasto mentioned this pull request Oct 3, 2021

Windows Gui becomes non responsive after "Done loading" bitcoin/bitcoin#10646

Closed

bitcoin-core locked as resolved and limited conversation to collaborators Sep 30, 2022

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

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

Uh oh!

Conversation

hebasto commented May 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jarolrod commented May 21, 2021

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented May 31, 2021

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Sep 5, 2021

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Sep 7, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Sep 8, 2021

Uh oh!

hebasto commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Sep 11, 2021

Uh oh!

hebasto commented Sep 12, 2021

Uh oh!

hebasto commented Sep 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

hebasto commented May 21, 2021 •

edited

Loading

promag May 26, 2021 •

edited

Loading

hebasto commented Sep 9, 2021 •

edited

Loading

ryanofsky left a comment •

edited

Loading