gui: Make polling in ClientModel asynchronous #17135

pull promag wants to merge 1 commits into bitcoin:master from promag:2019-10-fix-gui-freeze changing 2 files +21 −14
  1. promag commented at 2:24 PM on October 14, 2019: member

    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.

  2. fanquake added the label GUI on Oct 14, 2019
  3. fanquake added the label Needs gitian build on Oct 14, 2019
  4. MarcoFalke removed the label Needs gitian build on Oct 14, 2019
  5. MarcoFalke added the label Needs gitian build on Oct 14, 2019
  6. promag force-pushed on Oct 14, 2019
  7. jonasschnelli commented at 1:29 PM on October 15, 2019: contributor

    Elegant Solution. Concept ACK. In the long run, we should probably also improve the 250ms polling on WalletModel::pollBalanceChanged()

    Will test.

  8. promag commented at 1:34 PM on October 15, 2019: member

    also improve the 250ms polling

    You mean remove polling? :trollface:

  9. promag force-pushed on Oct 15, 2019
  10. promag renamed this:
    wip: gui: It's freezing
    gui: Make polling in ClientModel asynchronous
    on Oct 15, 2019
  11. in src/qt/clientmodel.h:94 in d3dd119fc1 outdated 
      89 | @@ -90,7 +90,9 @@ class ClientModel : public QObject
  90 |      PeerTableModel *peerTableModel;
  91 |      BanTableModel *banTableModel;
  92 |  
  93 | -    QTimer *pollTimer;
  94 | +    //! A thread to interact with m_node asynchronously
  95 | +    QThread* const thread;
    MarcoFalke commented at 2:39 PM on October 15, 2019: 
        QThread* const m_thread;

    :eyes: ?

    promag commented at 2:49 PM on October 15, 2019:

    Sorry, done.

  12. promag force-pushed on Oct 15, 2019
  13. DrahtBot removed the label Needs gitian build on Oct 16, 2019
  14. laanwj added this to the "Blockers" column in a project
  15. MarcoFalke deleted a comment on Oct 17, 2019
  16. MarcoFalke added the label Needs gitian build on Oct 17, 2019
  17. laanwj commented at 7:35 PM on October 17, 2019: member

    Concept ACK

  18. MarcoFalke commented at 7:49 PM on October 17, 2019: member

    Put in a gitian build, but I no longer have a windows box for testing

  19. in src/qt/clientmodel.cpp:99 in 8f0d8bd17a outdated 
     108 | -{
 109 | -    // no locking required at this point
 110 | -    // the following calls will acquire the required lock
 111 | -    Q_EMIT mempoolSizeChanged(m_node.getMempoolSize(), m_node.getMempoolDynamicUsage());
 112 | -    Q_EMIT bytesChanged(m_node.getTotalBytesRecv(), m_node.getTotalBytesSent());
 113 | -}
    MarcoFalke commented at 2:10 PM on October 18, 2019:

    moving this code into a lambda is stylistic only, right?

    promag commented at 2:17 PM on October 18, 2019:

    I could keep this here but the connection from the timer needs to be direct (not auto) so that this would run in the timer's thread.

    Also, this is now unreachable, and it's a small step towards removing polling.

  20. MarcoFalke commented at 2:11 PM on October 18, 2019: member

    ACK 8f0d8bd17a575a4235296d82cda80ba268a01dba

    Didn't test on windows to confirm this actually reduces the "not responding" pop ups, nor that the mempool size is still updated

  21. DrahtBot commented at 7:20 PM on October 18, 2019: member

    <!--a722867cd34abeea1fadc8d60700f111-->

    Gitian builds for commit ec3ed5a4487886f1c2a35fda0a3289be7b280248 (master):

    Gitian builds for commit 6661358a0c082fd50378fe6fc88dddbd0566ebf0 (master and this pull):

  22. DrahtBot removed the label Needs gitian build on Oct 18, 2019
  23. in src/qt/clientmodel.cpp:60 in 8f0d8bd17a outdated 
      55 | @@ -46,6 +56,10 @@ ClientModel::ClientModel(interfaces::Node& node, OptionsModel *_optionsModel, QO
  56 |  ClientModel::~ClientModel()
  57 |  {
  58 |      unsubscribeFromCoreSignals();
  59 | +
  60 | +    delete pollTimer;
    ysangkok commented at 4:06 PM on October 21, 2019:

    why is the timer deleted before the thread stops and not the other way around?

    promag commented at 4:27 PM on October 21, 2019:

    What is the issue?

    ysangkok commented at 4:43 PM on October 21, 2019:

    I don't know if there is an issue or not, I am just wondering if there is any specific reason for this ordering. Mostly because I saw this comment.

    promag commented at 5:07 PM on October 21, 2019:

    Thanks for pointing it, there's indeed an issue here - must be deleteLater of move after joining the other thread.

    promag commented at 5:26 PM on October 21, 2019:

    Fixed.

  24. Sjors commented at 4:45 PM on October 21, 2019: member

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

  25. promag force-pushed on Oct 21, 2019
  26. promag commented at 5:27 PM on October 21, 2019: member

    @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
  27. luke-jr commented at 5:51 PM on October 21, 2019: member

    "Moved to another thread" isn't really "asynchronous", but Concept ACK

  28. in src/qt/clientmodel.cpp:45 in 8cf1c67717 outdated 
      43 | +
  44 | +    pollTimer = new QTimer;
  45 | +    // move timer to thread so that polling doesn't disturb event loop
  46 | +    pollTimer->moveToThread(m_thread);
  47 | +    m_thread->start();
  48 | +    connect(pollTimer, &QTimer::timeout, [this] {
    luke-jr commented at 5:53 PM on October 21, 2019:

    Can't you leave ClientModel::updateTimer as-is for now? Or at least move the code movement to another commit?

    promag commented at 6:09 PM on October 21, 2019:

    Yes but I'd have to connect with Qt::DirectConnection, otherwise the slot would be called in the GUI event loop.

    luke-jr commented at 6:32 PM on October 21, 2019:

    Even after moveToThread? Because of the slot being on a GUI object?

    promag commented at 6:50 PM on October 21, 2019:

    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.

    laanwj commented at 7:54 PM on October 24, 2019:

    I think you should do this connect before moving the timer to the thread? (or maybe not, I'm not sure here…)

    promag commented at 8:20 AM on October 25, 2019:

    QObject::connect is thread safe.

  29. in src/qt/clientmodel.cpp:41 in 8cf1c67717 outdated 
      39 |      peerTableModel = new PeerTableModel(m_node, this);
  40 |      banTableModel = new BanTableModel(m_node, this);
  41 | -    pollTimer = new QTimer(this);
  42 | -    connect(pollTimer, &QTimer::timeout, this, &ClientModel::updateTimer);
  43 | +
  44 | +    pollTimer = new QTimer;
    luke-jr commented at 5:55 PM on October 21, 2019:

    Why can't this use QTimer(m_thread)?

    promag commented at 6:15 PM on October 21, 2019:

    Because Qt threading model :trollface:

    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.

    ysangkok commented at 6:28 PM on October 21, 2019:

    why can't this be a unique_ptr?

    luke-jr commented at 6:31 PM on October 21, 2019:

    I thought it deleted children when being deleted itself?

    promag commented at 6:51 PM on October 21, 2019:

    You can't have a tree of objects with different thread affinity.

    promag commented at 6:54 PM on October 21, 2019:

    why can't this be a unique_ptr?

    It can, but it requires more changes, which are unnecessary in the scope of this PR.

  30. promag commented at 6:12 PM on October 21, 2019: member

    @luke-jr right, conceptually asynchronous from the GUI thread.

  31. fjahr commented at 5:08 PM on October 24, 2019: member

    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.

  32. in src/qt/clientmodel.cpp:62 in 8cf1c67717 outdated 
      55 | @@ -46,6 +56,10 @@ ClientModel::ClientModel(interfaces::Node& node, OptionsModel *_optionsModel, QO
  56 |  ClientModel::~ClientModel()
  57 |  {
  58 |      unsubscribeFromCoreSignals();
  59 | +
  60 | +    m_thread->quit();
  61 | +    m_thread->wait();
  62 | +    delete pollTimer;
    laanwj commented at 7:52 PM on October 24, 2019:

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

    promag commented at 8:20 AM on October 25, 2019:

    Done.

  33. promag commented at 8:21 AM on October 25, 2019: member

    Updated after IRC discussion.

  34. promag force-pushed on Oct 25, 2019
  35. promag force-pushed on Oct 25, 2019
  36. in src/qt/clientmodel.cpp:50 in 092d91f619 outdated 
      49 | +        // the following calls will acquire the required lock
  50 | +        Q_EMIT mempoolSizeChanged(m_node.getMempoolSize(), m_node.getMempoolDynamicUsage());
  51 | +        Q_EMIT bytesChanged(m_node.getTotalBytesRecv(), m_node.getTotalBytesSent());
  52 | +    });
  53 | +    connect(m_thread, &QThread::finished, timer, &QObject::deleteLater);
  54 | +    connect(m_thread, &QThread::started, timer, static_cast<void (QTimer::*)()>(&QTimer::start));
    promag commented at 8:33 AM on October 25, 2019:

    The cast is needed because QTimer::start is overloaded.

    Sjors commented at 9:27 AM on October 25, 2019:

    ^ can you put that in a source code comment?

    laanwj commented at 11:15 AM on October 25, 2019:

    could this work instead of the scary cast ?

    connect(m_thread, &QThread::started, timer, [timer] {
    timer->start();
});
    promag commented at 11:21 AM on October 25, 2019:

    Yes that works, and the following also works:

    connect(m_thread, &QThread::started, [timer] {
    timer->start();
});

    because started is emitted from the associated thread.

    laanwj commented at 11:37 AM on October 25, 2019:

    Much better!

  37. Sjors approved
  38. Sjors commented at 10:15 AM on October 25, 2019: member

    ACK 092d91f6196af497b9618418f84527f552ef1154. Tested without a wallet (bitcoin-qt -nowallet) by calling invalidateblock and later reconsiderblock. Without this commit QT is very unresponsive during sync. The original issue seems less pronounced with a wallet.

    Would it make sense to create a NodePoller object, which handles the timer life cycle? Or is that overkill if the plan is to get rid of polling asap anyway?

  39. laanwj commented at 11:10 AM on October 25, 2019: member

    Would it make sense to create a NodePoller object, which handles the timer life cycle? Or is that overkill if the plan is to get rid of polling asap anyway?

    I think that as long as polling is necessary (and I don't believe we will completely get rid of it any time soon) it would make sense to have one thread to do all polling, and not, say, create a thread per WalletModel too.

    But it's out of scope for this PR.

  40. laanwj added this to the milestone 0.19.0 on Oct 25, 2019
  41. laanwj added the label Needs backport (0.19) on Oct 25, 2019
  42. gui: Make polling in ClientModel asynchronous
    With this change polling runs in a different thread to prevent
disturbing the event loop.
    6b6be41c36
  43. promag force-pushed on Oct 25, 2019
  44. promag referenced this in commit d5c36ce0c4 on Oct 25, 2019
  45. in src/qt/clientmodel.cpp:49 in 6b6be41c36 
      48 | +        // no locking required at this point
  49 | +        // the following calls will acquire the required lock
  50 | +        Q_EMIT mempoolSizeChanged(m_node.getMempoolSize(), m_node.getMempoolDynamicUsage());
  51 | +        Q_EMIT bytesChanged(m_node.getTotalBytesRecv(), m_node.getTotalBytesSent());
  52 | +    });
  53 | +    connect(m_thread, &QThread::finished, timer, &QObject::deleteLater);
    promag commented at 2:02 PM on October 25, 2019:

    Ops, I think this is wrong - deleteLater isn't called because the thread's event loop is not running. I'll check this.

    promag commented at 2:03 PM on October 25, 2019:

    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.

    laanwj commented at 4:26 PM on October 25, 2019:

    Phew, good to know (I've read that once but forgot)

  46. laanwj commented at 4:43 PM on October 25, 2019: member

    ACK 6b6be41c36e4fe9a74bed50e7f0a06532ab1260b

  47. Sjors commented at 6:21 PM on October 25, 2019: member

    Code review re-ACK 6b6be41; only replaced the scary cast with { timer->start(); }

  48. laanwj removed the label Needs backport (0.19) on Oct 26, 2019
  49. laanwj referenced this in commit e9f73b839a on Oct 26, 2019
  50. laanwj merged this on Oct 26, 2019
  51. laanwj closed this on Oct 26, 2019
  52. laanwj referenced this in commit 0f6f7a574a on Oct 26, 2019
  53. promag deleted the branch on Oct 26, 2019
  54. fanquake removed this from the "Blockers" column in a project
  55. HashUnlimited referenced this in commit 7f22c29779 on Nov 17, 2019
  56. fxtc referenced this in commit 1d03515446 on Nov 25, 2019
  57. fxtc referenced this in commit 03bf0f50fc on Nov 25, 2019
  58. laanwj referenced this in commit f2880e21ef on Mar 31, 2020
  59. sidhujag referenced this in commit b857234f16 on Mar 31, 2020
  60. deadalnix referenced this in commit 3bfb5fc3f8 on Oct 31, 2020
  61. kittywhiskers referenced this in commit 743189834b on Nov 1, 2021
  62. kittywhiskers referenced this in commit 8278e8ab0f on Nov 1, 2021
  63. pravblockc referenced this in commit 4013c49d4a on Nov 18, 2021
  64. DrahtBot locked this on Feb 15, 2022
Contributors
promagjonasschnelliMarcoFalkelaanwjDrahtBotysangkokSjorsluke-jrfjahr
Labels
GUI

Milestone
0.19.0

Linked (view graph)
#14193 validation: Add missing mempool locks#17112 v0.19.0rc1 GUI repeatedly not responding#17182 0.19: Revert 14193 to fix 17112#17252 0.19: gui: Make polling in ClientModel asynchronous#18160 gui: Avoid Wallet::GetBalance in WalletModel::pollBalanceChanged
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-20 03:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me