relax GUI locks: avoid unnecesarry calls to ::ChainstateActive().IsInitialBlockDownload

jonasschnelli commented at 2:34 pm on May 18, 2020: contributor

Introduced in #12783 / 1e0f3c44992fb82e6bf36c2ef9277b0759c17c4c

Calling m_node.isInitialBlockDownload() during catch-up in the main GUI thread leads to locking the GUI for seconds,… which is avoidable since we fetch that information via a signal anyways.

jonasschnelli added the label GUI on May 18, 2020

jonasschnelli added the label macOS on May 18, 2020

jonasschnelli added the label Needs backport (0.20) on May 18, 2020

MarcoFalke added this to the milestone 0.20.1 on May 18, 2020

in src/qt/bitcoingui.cpp:570 in feecf490c4 outdated

566@@ -567,7 +567,7 @@ void BitcoinGUI::setClientModel(ClientModel *_clientModel)
567         connect(_clientModel, &ClientModel::networkActiveChanged, this, &BitcoinGUI::setNetworkActive);
568 
569         modalOverlay->setKnownBestHeight(_clientModel->getHeaderTipHeight(), QDateTime::fromTime_t(_clientModel->getHeaderTipTime()));
570-        setNumBlocks(m_node.getNumBlocks(), QDateTime::fromTime_t(m_node.getLastBlockTime()), m_node.getVerificationProgress(), false);
571+        setNumBlocks(m_node.getNumBlocks(), QDateTime::fromTime_t(m_node.getLastBlockTime()), m_node.getVerificationProgress(), false, false);

MarcoFalke commented at 2:48 pm on May 18, 2020:

0        setNumBlocks(m_node.getNumBlocks(), QDateTime::fromTime_t(m_node.getLastBlockTime()), m_node.getVerificationProgress(), /* header */ false, m_node.isInitialBlockDownload());

Any reason to not call m_node.isInitialBlockDownload() here? This should only be called during the init/de-init sequence, so the overhead should be minimal.

jonasschnelli commented at 2:55 pm on May 18, 2020:

Your right. I’ll add it there. I’m currently also trying to get rid of this initial sets in the main GUI thread.

in src/qt/rpcconsole.cpp:581 in feecf490c4 outdated

577@@ -578,7 +578,7 @@ void RPCConsole::setClientModel(ClientModel *model)
578         connect(model, &ClientModel::numConnectionsChanged, this, &RPCConsole::setNumConnections);
579 
580         interfaces::Node& node = clientModel->node();
581-        setNumBlocks(node.getNumBlocks(), QDateTime::fromTime_t(node.getLastBlockTime()), node.getVerificationProgress(), false);
582+        setNumBlocks(node.getNumBlocks(), QDateTime::fromTime_t(node.getLastBlockTime()), node.getVerificationProgress(), false, false);

MarcoFalke commented at 2:49 pm on May 18, 2020:

Same

MarcoFalke commented at 2:49 pm on May 18, 2020: member

Concept ACK, just a question

relax GUI locks: avoid unnecesarry calls to ::ChainstateActive().IsInitialBlockDownload()

Introduced in #12783 / 1e0f3c44992fb82e6bf36c2ef9277b0759c17c4c

ec2c94ffea

jonasschnelli force-pushed on May 18, 2020

in src/qt/rpcconsole.h:111 in ec2c94ffea

107@@ -108,7 +108,7 @@ public Q_SLOTS:
108     /** Set network state shown in the UI */
109     void setNetworkActive(bool networkActive);
110     /** Set number of blocks and last block date shown in the UI */
111-    void setNumBlocks(int count, const QDateTime& blockDate, double nVerificationProgress, bool headers);
112+    void setNumBlocks(int count, const QDateTime& blockDate, double nVerificationProgress, bool headers, bool initial_sync);

ryanofsky commented at 4:41 pm on May 18, 2020:

In commit “relax GUI locks: avoid unnecesarry calls to ::ChainstateActive().IsInitialBlockDownload()” (feecf490c418399e9771a5b48e078a4c2c5b6b55)

Could revert all these changes to rpcconsole.h and rpcconsole.cpp. Qt is fine with dropping ununused signal arguments https://doc.qt.io/qt-5/signalsandslots.html “a slot may have a shorter signature than the signal it receives”

ryanofsky commented at 4:58 pm on May 18, 2020: member

Code review ACK feecf490c418399e9771a5b48e078a4c2c5b6b55. It’s good to avoid calls from gui->node in the middle of node->gui callbacks

EDIT: Updated ACK comment in light of fix from other PR #19007 (comment)

ryanofsky approved

ryanofsky commented at 5:00 pm on May 18, 2020: member

Code review ACK ec2c94ffeac8eb5be3549fc617ce142b57863a64. Only change since last review was adding back two isInitialBlockDownload calls to reduce the scope of the PR a little

ryanofsky commented at 9:16 pm on May 18, 2020: member

It turns out hebasto already implemented the same change in 7a2a5edc09986bcbcfd370ece644e7c31fa71b25 from #18152, and IMO that change is cleaner and preferable to this one. I’ve acked both PR’s though, since I think they are both improvements.

DrahtBot commented at 0:01 am on May 19, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19011 (Reduce cs_main lock accumulation during GUI startup by jonasschnelli)
#18152 (qt: Use SynchronizationState enum for signals to GUI by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

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

Thanks @ryanofsky for the review and pointing to #18152. I agree that #18152 (which I wasn’t aware) is the superior solution.

DrahtBot added the label Needs rebase on May 19, 2020

DrahtBot commented at 2:19 pm on May 19, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

MarcoFalke removed this from the milestone 0.20.1 on May 19, 2020

MarcoFalke removed the label Needs backport (0.20) on May 19, 2020

ryanofsky commented at 8:51 pm on May 19, 2020: member

I think this can be closed since OG alternative #18152 was merged

MarcoFalke closed this on May 19, 2020

jonasschnelli referenced this in commit 1052b09031 on Aug 13, 2020

fanquake removed the label Needs rebase on May 31, 2021

DrahtBot locked this on Aug 18, 2022

relax GUI locks: avoid unnecesarry calls to ::ChainstateActive().IsInitialBlockDownload #19007

Conflicts