gui: Make polling in ClientModel asynchronous #17135
Conversation
2ca5692 to
c5487d7
Compare
|
Elegant Solution. Will test. |
You mean remove polling? |
c5487d7 to
d3dd119
Compare
d3dd119 to
8f0d8bd
Compare
|
Concept ACK |
|
Put in a gitian build, but I no longer have a windows box for testing |
|
Gitian builds for commit ec3ed5a (master):
Gitian builds for commit 6661358 (master and this pull):
|
|
I can confirm that 8f0d8bd makes the unresponsive behavior go away on macOS 10.15 Catalina (e.g. the file menu now works at launch). |
8f0d8bd to
8cf1c67
Compare
|
@MarcoFalke @Sjors force push with --- a/src/qt/clientmodel.cpp
+++ b/src/qt/clientmodel.cpp
@@ -57,9 +57,9 @@ ClientModel::~ClientModel()
{
unsubscribeFromCoreSignals();
- delete pollTimer;
m_thread->quit();
m_thread->wait();
+ delete pollTimer;
}
int ClientModel::getNumConnections(unsigned int flags) const |
|
"Moved to another thread" isn't really "asynchronous", but Concept ACK |
src/qt/clientmodel.cpp
Outdated
| // move timer to thread so that polling doesn't disturb event loop | ||
| pollTimer->moveToThread(m_thread); | ||
| m_thread->start(); | ||
| connect(pollTimer, &QTimer::timeout, [this] { |
There was a problem hiding this comment.
Can't you leave ClientModel::updateTimer as-is for now? Or at least move the code movement to another commit?
There was a problem hiding this comment.
Yes but I'd have to connect with Qt::DirectConnection, otherwise the slot would be called in the GUI event loop.
There was a problem hiding this comment.
Even after moveToThread? Because of the slot being on a GUI object?
There was a problem hiding this comment.
Yes, with auto connection (the default) the signal checks if the target's thread is the same as the sender or not, if not then it's the same as queued connection.
There was a problem hiding this comment.
I think you should do this connect before moving the timer to the thread?
(or maybe not, I'm not sure here…)
There was a problem hiding this comment.
QObject::connect is thread safe.
src/qt/clientmodel.cpp
Outdated
| pollTimer = new QTimer(this); | ||
| connect(pollTimer, &QTimer::timeout, this, &ClientModel::updateTimer); | ||
|
|
||
| pollTimer = new QTimer; |
There was a problem hiding this comment.
Why can't this use QTimer(m_thread)?
There was a problem hiding this comment.
Because Qt threading model
Being a child of QThread does nothing other than be owned by it. To have it's events processed in the thread then moveToThread must be used. This also works only if the object being moved has no parent.
There was a problem hiding this comment.
I thought it deleted children when being deleted itself?
There was a problem hiding this comment.
You can't have a tree of objects with different thread affinity.
There was a problem hiding this comment.
why can't this be a unique_ptr?
It can, but it requires more changes, which are unnecessary in the scope of this PR.
|
@luke-jr right, conceptually asynchronous from the GUI thread. |
|
tested ACK 8f0d8bd Built, ran tests, some manual testing while running for 24h on macOS. Please add a description. I got the context from the core dev logs but there should be some information here as well. |
src/qt/clientmodel.cpp
Outdated
|
|
||
| m_thread->quit(); | ||
| m_thread->wait(); | ||
| delete pollTimer; |
There was a problem hiding this comment.
As discussed on IRC: I don't think deleting the timer in the main thread, after having moved it to another thread is safe. Qt objects need to be deleted in the thread that owns them. I think that that thread doesn't exist anymore here doesn't change that. You're not even supposed to hold on to the pointer.
I think you can do this before moving it to the thread to make sure it's cleaned up:
connect(&thread, &QThread::finished, pollTimer, &QTimer::deleteLater);
(like in RPCConsole::startExecutor())
With this change polling runs in a different thread to prevent disturbing the event loop.
092d91f to
6b6be41
Compare
With this change polling runs in a different thread to prevent disturbing the event loop. Github-Pull: bitcoin#17135 Rebased-From: 6b6be41
| Q_EMIT mempoolSizeChanged(m_node.getMempoolSize(), m_node.getMempoolDynamicUsage()); | ||
| Q_EMIT bytesChanged(m_node.getTotalBytesRecv(), m_node.getTotalBytesSent()); | ||
| }); | ||
| connect(m_thread, &QThread::finished, timer, &QObject::deleteLater); |
There was a problem hiding this comment.
Ops, I think this is wrong - deleteLater isn't called because the thread's event loop is not running. I'll check this.
There was a problem hiding this comment.
Nope, all good! From https://doc.qt.io/qt-5/qthread.html#finished:
When this signal is emitted, the event loop has already stopped running. No more events will be processed in the thread, except for deferred deletion events. This signal can be connected to QObject::deleteLater(), to free objects in that thread.
There was a problem hiding this comment.
Phew, good to know (I've read that once but forgot)
|
ACK 6b6be41 |
|
Code review re-ACK 6b6be41; only replaced the scary cast with |
6b6be41 gui: Make polling in ClientModel asynchronous (João Barbosa) Pull request description: After #14193 `ClientModel::updateTimer` can take some time, as such the GUI hangs, like #17112. Fixes this by polling in a background thread and updating the GUI asynchronously. ACKs for top commit: laanwj: ACK 6b6be41 Sjors: Code review re-ACK 6b6be41; only replaced the scary cast with `{ timer->start(); }` Tree-SHA512: fd98b0c6535441aee3ee03c48b58b4b1f9bdd172ec6b8150da883022f719df34cabfd4c133412bf410e7f709f7bf1e9ef16dca05ef1f3689d526ceaeee51de38
d5c36ce gui: Make polling in ClientModel asynchronous (João Barbosa) Pull request description: Backport #17135. ACKs for top commit: laanwj: ACK d5c36ce, it is a clean cherry-pick of 6b6be41. Tree-SHA512: 4e514f205866d87bdc19a57dede2214891237d7b663c9c8c9f19a9ab5c5a6e64876065bebb6c16a1799b02e0eb971318866b4e0824155b47063ce379fb0155e2
With this change polling runs in a different thread to prevent disturbing the event loop. Github-Pull: bitcoin#17135 Rebased-From: 6b6be41
With this change polling runs in a different thread to prevent disturbing the event loop. Github-Pull: bitcoin#17135 Rebased-From: 6b6be41
With this change polling runs in a different thread to prevent disturbing the event loop. Github-Pull: bitcoin#17135 Rebased-From: 6b6be41
…ceChanged 0933a37 gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged (João Barbosa) Pull request description: Each 250ms the slot `WalletModel::pollBalanceChanged` is called which, at worst case, calls `Wallet::GetBalance`. This is a waste of resources since most of the time there aren't new transactions or new blocks. Fix this by early checking if cache is dirty or not. The actual balance computation can still hang the GUI thread but that is tracked in #16874 and should be fixed with a solution similar to #17135. ACKs for top commit: hebasto: ACK 0933a37, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged. jonasschnelli: utACK 0933a37 instagibbs: ACK 0933a37 ryanofsky: Code review ACK 0933a37, but I would prefer (not strongly) for #17905 to be merged first. This PR can be simpler if it is based on #17905, so tryGetBalances can just be left alone instead of changing into to a more complicated tryGetBalancesIfNeeded function, and then getting changed back later when we want to optimize it out. jonatack: ACK 0933a37 based primarily on code review, despite a lot of manual testing with a large 177MB wallet. Tree-SHA512: 18db35bf33a7577666658c8cb0b57308c8474baa5ea95bf1468cd8531a69857d8915584f6ac505874717aa6aabeb1b506ac77630f8acdb6651afab89275e38a1
…llBalanceChanged 0933a37 gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged (João Barbosa) Pull request description: Each 250ms the slot `WalletModel::pollBalanceChanged` is called which, at worst case, calls `Wallet::GetBalance`. This is a waste of resources since most of the time there aren't new transactions or new blocks. Fix this by early checking if cache is dirty or not. The actual balance computation can still hang the GUI thread but that is tracked in bitcoin#16874 and should be fixed with a solution similar to bitcoin#17135. ACKs for top commit: hebasto: ACK 0933a37, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged. jonasschnelli: utACK 0933a37 instagibbs: ACK 0933a37 ryanofsky: Code review ACK 0933a37, but I would prefer (not strongly) for bitcoin#17905 to be merged first. This PR can be simpler if it is based on bitcoin#17905, so tryGetBalances can just be left alone instead of changing into to a more complicated tryGetBalancesIfNeeded function, and then getting changed back later when we want to optimize it out. jonatack: ACK 0933a37 based primarily on code review, despite a lot of manual testing with a large 177MB wallet. Tree-SHA512: 18db35bf33a7577666658c8cb0b57308c8474baa5ea95bf1468cd8531a69857d8915584f6ac505874717aa6aabeb1b506ac77630f8acdb6651afab89275e38a1
Summary: > After #14193 ClientModel::updateTimer can take some time, as such the GUI hangs, like #17112. > > Fixes this by polling in a background thread and updating the GUI asynchronously. This is a backport of Core [[bitcoin/bitcoin#17135 | PR17135]] and Core [[bitcoin/bitcoin#17427 | PR17427]] The second PR fixes a bug introduced by the first (`ClientModel::mempoolSizeChanged() signal has unregistered parameter type size_t`) Test Plan: `ninja && ninja check && src/qt/bitcoin-qt` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D8193
After #14193
ClientModel::updateTimercan take some time, as such the GUI hangs, like #17112.Fixes this by polling in a background thread and updating the GUI asynchronously.