gui: Long or blocking calls should be async

promag commented at 9:40 am on September 15, 2019: member

The comment here is not entirely correct: https://github.com/bitcoin/bitcoin/blob/4bfef0daeb9351c200b5bd09e60596a29b4d3523/src/qt/walletmodel.cpp#L71-L74

The problem happens when the locks are held and calculating the balance takes a lot of time - causing the GUI to stuck.

Beside the obvious improvement - drop polling and update only when necessary - the actual balance calculation must be asynchronous otherwise Qt’s event loop isn’t able to make the GUI smooth - noticeable on big wallet. This also applies for anything that takes a lot - whether it requires other locks or not.

promag added the label Bug on Sep 15, 2019

fanquake added the label GUI on Sep 15, 2019

fanquake removed the label Bug on Sep 15, 2019

laanwj referenced this in commit f2880e21ef on Mar 31, 2020

sidhujag referenced this in commit b857234f16 on Mar 31, 2020

ryanofsky commented at 10:48 pm on May 18, 2020: contributor

I think ideally the way for the gui to receive these updates would be with a new wallet callback handleLastBlockChanged analogous to existing handleAddressBookChanged, handleTransactionChanged, etc callbacks. The callback could be invoked whenever CWallet::m_last_block_processed is updated and pass along CWallet::m_last_block_processed_height or other useful state values.

This way no polling would be required at all.

In the meantime, there have been a lot of PRs affecting polling and tip updates. Making a list here to keep track.

Two changes were made to make polling less expensive and block less:

#18160 (merged) avoids expensive wallet balance calls during polling
#18587 (pending) reverts previous change and takes it further, implementing it entirely in the GUI and not calling out to node or wallet

Other polling changes:

#17993 (pending) switches all block change checks in GUI code from height checks to hash checks so the checks are more reliable
#17905 (merged) avoids repeated calls from the gui to the wallet to update transaction statuses as they are displayed, by skipping updates if the last block isn’t changed

Other notification changes:

#18121 (merged) - avoids rapid gui updates in tip changed callback during reindexing
#18152 (pending) - avoids getReindex and isInitialBlockDownload calls during tip changed callback and
#19007 (pending) - subset of #18152, avoid isInitialBlockDownload call during tip changed callback

promag commented at 11:07 pm on May 18, 2020: member

This way no polling would be required at all.

Yes! Title is misleading.

The approach you suggest looks fine to me but it doesn’t prevent going back to node/wallet and request more data which in turn can disturb the GUI responsiveness.

promag renamed this:
~~gui: Poll for balances should be async~~
gui: Long or blocking calls should be async
on May 18, 2020

jonasschnelli commented at 7:07 am on May 19, 2020: contributor

When running with a wallet, I encountered that one of the most annoying blockings I had in mainnet-catchup was SendCoinsDialog::updateSmartFeeLabel(). We call CTxMemPool::GetMinFee() on all tip updates which is fine when in-sync but problematic otherwise.

It seems to be a mempool lock issue. The backtrace where the GUI thread gets stuck for seconds:

0    frame [#8](/bitcoin-bitcoin/8/): 0x000000010fe75136 bitcoin-qt`CTxMemPool::GetMinFee(this=0x0000000110cd75e8, sizelimit=300000000) const at txmempool.cpp:1004:5
1    frame [#9](/bitcoin-bitcoin/9/): 0x000000010fa6c531 bitcoin-qt`interfaces::(anonymous namespace)::ChainImpl::mempoolMinFee(this=0x00007fee41f46730) at chain.cpp:332:26
2    frame [#10](/bitcoin-bitcoin/10/): 0x0000000110194e21 bitcoin-qt`GetMinimumFeeRate(wallet=0x00007fee35804080, coin_control=0x00007fee35b8a880, feeCalc=0x00007ffee04d10f8) at fees.cpp:67:55
3    frame [#11](/bitcoin-bitcoin/11/): 0x0000000110194b06 bitcoin-qt`GetMinimumFee(wallet=0x00007fee35804080, nTxBytes=1000, coin_control=0x00007fee35b8a880, feeCalc=0x00007ffee04d10f8) at fees.cpp:20:12
4    frame [#12](/bitcoin-bitcoin/12/): 0x000000011011fa26 bitcoin-qt`interfaces::(anonymous namespace)::WalletImpl::getMinimumFee(this=0x00007fee367175b0, tx_bytes=1000, coin_control=0x00007fee35b8a880, returned_target=0x00007ffee04d12a4, reason=0x00007ffee04d12a0) at wallet.cpp:428:18
5    frame [#13](/bitcoin-bitcoin/13/): 0x000000010f8d4ace bitcoin-qt`SendCoinsDialog::updateSmartFeeLabel(this=0x00007fee35b89e50) at sendcoinsdialog.cpp:755:49

ryanofsky commented at 11:17 am on May 19, 2020: contributor

I guess the scope of this issue has changed since or was different than I thought when I replied #16874 (comment). At the time the issue title was “gui: Poll for balances should be async” not “gui: Long or blocking calls should be async”. These are different problems. It is relatively easy to move slow polling code off the main event loop thread to a different thread, as opposed to getting slow code which is actually triggered by UI events (like SendCoinsDialog::updateSmartFeeLabel in Jonas’s example) to not block the event loop. In the latter case you need one of the approaches from https://doc.qt.io/archives/qq/qq27-responsive-guis.html or #10504, beyond just a different choice of thread for polling.

promag commented at 8:35 am on May 20, 2020: member

@ryanofsky right, the real problem are the long/blocking calls on the GUI thread - polling one of those just makes it more noticeable. Polling in some thread and sending the results to the GUI thread is what makes more sense to me.

as opposed to getting slow code which is actually triggered by UI events

This is what is done in OpenWalletActivity::open.

jonasschnelli commented at 8:47 am on May 20, 2020: contributor

I generally agree on doing things async, though I think It is not a sliver bullet. Even if calls that “take longer” are ran in the background (a.k.a outside the GUI thread), it might still be “laggy” for the user (he might see a loading... instead of an ugly GUI freeze).

IMO the GUI/node interaction design is not very performance optimised. Things like constant polling the minimal fee on every block update during sync seems to be wasteful. Moving that to another thread will not cure the issue.

What I think would be an efficient performance increase is pushing relevant information to the GUI rather the requesting it from the GUI side either periodical or right after a signal notification. Often we have the information available at the relevant time under the relevant lock,… but fail to pass it to the GUI. What then happens is that the GUI requests the information again (periodically or after a signal) leading to another expansive lock.

ryanofsky commented at 1:12 am on May 22, 2020: contributor

re: @jonasschnelli #16874 (comment)

When running with a wallet, I encountered that one of the most annoying blockings I had in mainnet-catchup was SendCoinsDialog::updateSmartFeeLabel().

I looked into this case as an example, trying to see if there’s an easy way to fix bad GUI hangs without having to rewrite lots of code.

My idea was to write a little helper function AsyncUpdate that can be called anywhere and takes two callback arguments, running the first callback asynchronously on a worker thread and passing whatever that returns as an argument to the second callback, which runs on the UI thread, so it can update GUI elements with the result.

https://github.com/bitcoin/bitcoin/blob/eacd34144c1ee989c9ab43eb439fecca76addc76/src/qt/async_update.h#L46-L50

Example applied to SendCoinsDialog::updateSmartFeeLabel looks like: eacd34144c1ee989c9ab43eb439fecca76addc76 (branch)

I think something like this could be used as a bandaid solution to fix known hangs, and help identify places in the code that should be priorities for rewriting in a more asynchronous style.

hebasto commented at 9:46 am on August 5, 2022: member

Moved to bitcoin-core/gui#642.

hebasto closed this on Aug 5, 2022

knst referenced this in commit 7ac0f0e417 on May 25, 2023

knst referenced this in commit 7897c466b0 on Jun 12, 2023

knst referenced this in commit e9fdb1f684 on Jul 6, 2023

knst referenced this in commit 40214ee4ab on Jul 24, 2023

knst referenced this in commit 63c57df249 on Jul 28, 2023

PastaPastaPasta referenced this in commit 9a84f7ef76 on Aug 3, 2023

bitcoin locked this on Aug 5, 2023

gui: Long or blocking calls should be async #16874