[Qt] reduce cs_main locks during tip update, more fluently update UI #7112

1. 
   jonasschnelli commented at 9:34 am on November 27, 2015: contributor

   At the moment, the UI gets often not updated during initial sync or a catch-up of serval blocks. The UI’s try_lock often can’t acquire the lock which result in a bad user experience (“Baby, it’s not updating!”).

   This PR changes the blocktip-update signal parameter form a bare hash (uint256) to a CBlockIndex*, which allows to safely get it’s height and timestamp without locking anything. In addition, the blocktip-update signal now gets fired also during initial sync, but with an extra boolean flag that indicates the initial sync state. The PR temporarily has a benchmark for the tip update signal. On my standard system it uses 0.03ms for the signal (=reasonable). Keep in mind that this signal is synchronous, but it will emit another asynchronous QT signal to the UI thread that will care about the UI update!

   The timer based update (every 250ms) is still active for bandwidth and mempool UI observation, although there no cs_main lock is required.

2. jonasschnelli added the label GUI on Nov 27, 2015
3. 
   jonasschnelli commented at 9:38 am on November 27, 2015: contributor

   Status bar during IBD looks like: https://bitcoin.jonasschnelli.ch/qt/statusbar.mov Console: https://bitcoin.jonasschnelli.ch/qt/console.mov (sorry the german lang.)
4. 
   laanwj commented at 9:41 am on November 27, 2015: member

   Concept ACK, I’ve been wanting to do this for a long time
5. jonasschnelli force-pushed on Nov 27, 2015
6. 
   jonasschnelli commented at 10:10 am on November 27, 2015: contributor

   Time-profiled this PR on a mac and it looks like that the performance is very similar to current master. I try to now to implement a time delta throttling instead of emitting the signal if nHeight % 10 == 0 (only during initial sync).

   Master:

   PR:

7. jonasschnelli force-pushed on Nov 27, 2015
8. 
   jonasschnelli commented at 10:26 am on November 27, 2015: contributor

   Updated. Much smaller diff/changeset now. Changes to a min time delta update to not over-emit signals to the UI during IBD on a fast computer. Now the UI gets updated if the last update was more then 250ms ago.
9. 
   laanwj commented at 10:59 am on November 27, 2015: member

   This fixes #5664 for me. Even during catching-up and heavy verification, it keeps updating the block number consistently.
10. in src/qt/clientmodel.cpp: in f60a931d91 outdated

    130-        Q_EMIT numBlocksChanged(newNumBlocks, newBlockDate);
    131-    }
    132-
    133+    // no locking required at this point
    134+    // the following calls will aquire the required lock
    135     Q_EMIT mempoolSizeChanged(getMempoolSize(), getMempoolDynamicUsage());
    

    ---

    ---

    

    laanwj commented at 11:31 am on November 27, 2015:

    Hm, spoke too soon. It does hang on long contention of cs_main, I think here. The TRY_LOCK should be re-added here.

    ---

    

    laanwj commented at 11:33 am on November 27, 2015:

    Hm no that can’t be it, none of these require cs_main. Weird.
11. 
    laanwj commented at 11:35 am on November 27, 2015: member

    Looks like it can still hang here:

    0[#5](/bitcoin-bitcoin/5/)  lock (this=<synthetic pointer>) at /store/orion/projects/bitcoin/experiment/boost/include/boost/thread/lock_types.hpp:346
    1[#6](/bitcoin-bitcoin/6/)  Enter (pszName=<optimized out>, pszFile=<optimized out>, nLine=<optimized out>, this=<synthetic pointer>) at ./sync.h:116
    2[#7](/bitcoin-bitcoin/7/)  CMutexLock (fTry=false, nLine=101, pszFile=0x5555559b7648 "qt/clientmodel.cpp", pszName=<optimized out>, mutexIn=..., this=<synthetic pointer>) at ./sync.h:137
    3[#8](/bitcoin-bitcoin/8/)  ClientModel::getVerificationProgress (this=<optimized out>) at qt/clientmodel.cpp:101
    4[#9](/bitcoin-bitcoin/9/)  0x00005555555db479 in BitcoinGUI::setNumBlocks (this=0x5555564ba3c0, count=375314, blockDate=...) at qt/bitcoingui.cpp:752
    5[#10](/bitcoin-bitcoin/10/) 0x000055555564960f in BitcoinGUI::qt_static_metacall (_o=0x5555564ba3c0, _c=<optimized out>, _id=<optimized out>, _a=<optimized out>) at qt/moc_bitcoingui.cpp:189
    

    getVerificationProgress takes a lock on cs_main I suppose because it uses chainActive.Tip()… Maybe we can pass this progress in through a signal as well? After all it is only supposed to change when the block number does.

12. 
    MarcoFalke commented at 11:49 am on November 27, 2015: member

    Concept ACK. f60a931d9137854f3bbcbc0589ab31563662c3c3 Looks a lot smoother on my system.
13. 
    jonasschnelli commented at 12:24 pm on November 27, 2015: contributor

    Added another commit that solves the getVerificationProgress() locking issue. The only reason why getVerificationProgress() needs a cs_main lock is because it accesses chainActive.Tip(). The last commit will pass the signals CBlockIndex* to the GuessVerificationProgress() which make the whole UI update cs_main lock free.
14. jonasschnelli added this to the milestone 0.12.0 on Nov 27, 2015
15. in src/qt/bitcoingui.cpp: in 85487e13a7 outdated

    671@@ -672,7 +672,7 @@ void BitcoinGUI::setNumConnections(int count)
    672     labelConnectionsIcon->setToolTip(tr("%n active connection(s) to Bitcoin network", "", count));
    673 }
    674 
    675-void BitcoinGUI::setNumBlocks(int count, const QDateTime& blockDate)
    676+void BitcoinGUI::setNumBlocks(int count, const QDateTime& blockDate, const CBlockIndex *tip)
    

    ---

    ---

    

    laanwj commented at 12:40 pm on November 27, 2015:

    I don’t like passing the tip through the view code. Let’s just pass the verification progress directly (could even remove ClientModel::getVerificationProgress() after that)

    ---

    

    jonasschnelli commented at 12:44 pm on November 27, 2015:

    Hmm… but i don’t want to execute getVerificationProgress() during the synchronous core signal. I guess it’s better to call this function over the UI thread. But let me have a closer look…

    ---

    

    laanwj commented at 12:48 pm on November 27, 2015:

    Hmm okay you have a point. But ideally these kind of things happen in the model (ClientModel) in this case, not in the view class. But that’d require ‘intercepting’ the signal there to add this information. Don’t like how core’s handles leak all over the place now. But yes maybe better to live with this for now…
16. 
    sipa commented at 2:07 pm on November 27, 2015: member

    @jonasschnelli @laanwj I benchmark GuessVerificationProgress (+random fetch from chainActive) here at around 80ns. I think it’s perfectly acceptable to compute it before passing to the GUI.
17. 
    laanwj commented at 2:51 pm on November 27, 2015: member

    @sipa thanks; absolutely, in that case we should call it in the signal handler directly before sendingto the GUI. I expect GetTimeMillis() takes longer than that.
18. jonasschnelli force-pushed on Nov 27, 2015
19. 
    jonasschnelli commented at 5:28 pm on November 27, 2015: contributor

    @sipa: Thanks for the info.

    Just added another commit that calculate the verification progress during the synchronous core signal, and passes a double through the UI signal. Feels very fast now (testes IBD, short catch-up 10 blocks).

20. in src/qt/clientmodel.cpp: in 1db6f9a308 outdated

    104-    LOCK(cs_main);
    105-    return Checkpoints::GuessVerificationProgress(Params().Checkpoints(), chainActive.Tip());
    106+    CBlockIndex *tip = (CBlockIndex *)tipIn;
    107+    if (!tip)
    108+    {
    109+        LOCK(cs_main);
    

    ---

    ---

    

    jonasschnelli commented at 5:29 pm on November 27, 2015:

    This lock is only required when initially update the UI (first time, outside of the core signal scope).
21. jonasschnelli force-pushed on Nov 27, 2015
22. 
    jonasschnelli commented at 7:15 pm on November 27, 2015: contributor

    Binares are built if someone likes to test this: https://bitcoin.jonasschnelli.ch/pulls/7112/
23. in src/qt/clientmodel.cpp: in e6d07cc581 outdated

     97@@ -99,40 +98,21 @@ size_t ClientModel::getMempoolDynamicUsage() const
     98     return mempool.DynamicMemoryUsage();
     99 }
    100 
    101-double ClientModel::getVerificationProgress() const
    102+double ClientModel::getVerificationProgress(const CBlockIndex *tipIn) const
    103 {
    104-    LOCK(cs_main);
    105-    return Checkpoints::GuessVerificationProgress(Params().Checkpoints(), chainActive.Tip());
    106+    CBlockIndex *tip = (CBlockIndex *)tipIn;
    

    ---

    ---

    

    sipa commented at 12:40 pm on November 28, 2015:

    Don’t recast to non-const, and certainly not with a C-style cast.

    ---

    

    jonasschnelli commented at 3:26 pm on November 28, 2015:

    Better like this (it’s now const_cast<CBlockIndex *>(tipIn))? Or would it make more sense to pass a non-const through the core signal?

    ---

    

    sipa commented at 3:28 pm on November 28, 2015:

    No, I think GuessVerificationProgress should take a const CBlockIndex* :)

    ---

    

    jonasschnelli commented at 3:29 pm on November 28, 2015:

    Yeah. Would be nice.. but i guess this would be a too-broad change for this PR scope.

    ---

    

    sipa commented at 3:31 pm on November 28, 2015:

    Just change the argument to const there. No other changes needed (I tried) :)
24. jonasschnelli force-pushed on Nov 28, 2015
25. NotifyBlockTip signal: switch from hash (uint256) to CBlockIndex*

    - also adds a boolean for indication if the tip update was happening during initial sync
    - emit notification also during initial sync

    012fc91511
26. [Qt] update block tip (height and date) without locking cs_main, update always (each block) e6d50fcdec
27. [Qt] reduce cs_main in getVerificationProgress() 947d20b84a
28. in src/main.cpp: in f87be7157d outdated

    2401@@ -2402,8 +2402,12 @@ bool ActivateBestChain(CValidationState &state, const CBlock *pblock) {
    2402             }
    2403             // Notify external listeners about the new tip.
    2404             GetMainSignals().UpdatedBlockTip(pindexNewTip);
    2405-            uiInterface.NotifyBlockTip(hashNewTip);
    2406         }
    2407+        //TODO: remove tip notification benchmark
    

    ---

    ---

    

    sipa commented at 9:22 pm on November 28, 2015:

    Maybe remove this again :)
29. jonasschnelli force-pushed on Nov 30, 2015
30. [Qt] call GuessVerificationProgress synchronous during core signal, pass double over UI signal 4082e46603
31. in src/main.cpp: in b28c61ede4 outdated

    2635@@ -2636,9 +2636,11 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,
    2636             // Notify external listeners about the new tip.
    2637             if (!vHashes.empty()) {
    2638                 GetMainSignals().UpdatedBlockTip(pindexNewTip);
    2639-                uiInterface.NotifyBlockTip(vHashes.front());
    2640             }
    2641         }
    2642+        if (!vHashes.empty()) {
    2643+            uiInterface.NotifyBlockTip(fInitialDownload, pindexNewTip);
    2644+        }
    

    ---

    ---

    

    jonasschnelli commented at 7:52 am on November 30, 2015:

    @sdaftuar: I’m not familiar with the block announcements with headers, could you check if this rebase makes sense?
32. jonasschnelli force-pushed on Nov 30, 2015
33. in src/main.cpp: in 4082e46603 outdated

    2635@@ -2636,9 +2636,10 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,
    2636             // Notify external listeners about the new tip.
    2637             if (!vHashes.empty()) {
    2638                 GetMainSignals().UpdatedBlockTip(pindexNewTip);
    2639-                uiInterface.NotifyBlockTip(vHashes.front());
    2640             }
    2641         }
    2642+        // Always notify the UI if a new block tip was connected
    2643+        uiInterface.NotifyBlockTip(fInitialDownload, pindexNewTip);
    

    ---

    ---

    

    jonasschnelli commented at 7:54 am on November 30, 2015:

    @sdaftuar: I’m not familiar with the block announcements with headers, could you check if this rebase makes sense? I’m firing off this event even when vHashes is empty now.

    ---

    

    sipa commented at 9:57 am on November 30, 2015:

    No, you should only fire if vHashes is not empty. vHashes contains the block hashes of blocks that were not in the best chain, but now are.

    ---

    

    sipa commented at 10:02 am on November 30, 2015:

    The code line is correct though: if vHashes is not empty, its first entry will be the hash of the new tip anyway, so you can use pindexTip.
34. 
    jonasschnelli commented at 8:01 am on November 30, 2015: contributor

    Rebased. Removed notification benchmark.
35. 
    laanwj commented at 9:54 am on November 30, 2015: member

    ACK. Works great now. The spinner is actually spinning again while syncing instead of blundering and glitching along :)
36. Move uiInterface.NotifyBlockTip signal above the core/wallet signal

    - This will keep getbestblockhash more in sync with blocknotify callbacks

    9af5f9cb87
37. jonasschnelli force-pushed on Nov 30, 2015
38. 
    jonasschnelli commented at 10:39 am on November 30, 2015: contributor

    Added a commit that resolves conflicts with block header announcements (#7129) and now also includes PR #7037 (because it conflicts with it).
39. 
    sipa commented at 11:50 am on November 30, 2015: member

    utACK non-GUI code.
40. laanwj merged this on Nov 30, 2015
41. laanwj closed this on Nov 30, 2015

    ---

42. laanwj referenced this in commit 96b802510d on Nov 30, 2015
43. zkbot referenced this in commit faf3825eed on Sep 30, 2020
44. zkbot referenced this in commit a983344931 on Oct 1, 2020
45. str4d referenced this in commit d95c2239ba on Feb 18, 2021
46. str4d referenced this in commit 5fe551e804 on Feb 18, 2021
47. str4d referenced this in commit 77bcb02cf2 on Feb 19, 2021
48. str4d referenced this in commit 7fb48db1e7 on Feb 19, 2021
49. nuttycom referenced this in commit 752f815a3a on Feb 23, 2021
50. str4d referenced this in commit fd811b1836 on Aug 13, 2021
51. DrahtBot locked this on Sep 8, 2021

Contributors
jonasschnelli laanwj MarcoFalke sipa

Labels
GUI

Milestone
0.12.0