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

jonasschnelli · 2015-11-27T09:34:31Z

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.

jonasschnelli · 2015-11-27T09:38:57Z

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

laanwj · 2015-11-27T09:41:55Z

Concept ACK, I've been wanting to do this for a long time

jonasschnelli · 2015-11-27T10:10:06Z

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:

jonasschnelli · 2015-11-27T10:26:10Z

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.

laanwj · 2015-11-27T10:59:40Z

This fixes #5664 for me. Even during catching-up and heavy verification, it keeps updating the block number consistently.

laanwj · 2015-11-27T11:31:40Z

src/qt/clientmodel.cpp

-    }
-
+    // no locking required at this point
+    // the following calls will aquire the required lock
    Q_EMIT mempoolSizeChanged(getMempoolSize(), getMempoolDynamicUsage());


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

Hm no that can't be it, none of these require cs_main. Weird.

laanwj · 2015-11-27T11:35:38Z

Looks like it can still hang here:

#5  lock (this=<synthetic pointer>) at /store/orion/projects/bitcoin/experiment/boost/include/boost/thread/lock_types.hpp:346
#6  Enter (pszName=<optimized out>, pszFile=<optimized out>, nLine=<optimized out>, this=<synthetic pointer>) at ./sync.h:116
#7  CMutexLock (fTry=false, nLine=101, pszFile=0x5555559b7648 "qt/clientmodel.cpp", pszName=<optimized out>, mutexIn=..., this=<synthetic pointer>) at ./sync.h:137
#8  ClientModel::getVerificationProgress (this=<optimized out>) at qt/clientmodel.cpp:101
#9  0x00005555555db479 in BitcoinGUI::setNumBlocks (this=0x5555564ba3c0, count=375314, blockDate=...) at qt/bitcoingui.cpp:752
#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.

maflcko · 2015-11-27T11:49:48Z

Concept ACK. f60a931d9137854f3bbcbc0589ab31563662c3c3 Looks a lot smoother on my system.

jonasschnelli · 2015-11-27T12:24:50Z

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.

laanwj · 2015-11-27T12:40:59Z

src/qt/bitcoingui.cpp

@@ -672,7 +672,7 @@ void BitcoinGUI::setNumConnections(int count)
    labelConnectionsIcon->setToolTip(tr("%n active connection(s) to Bitcoin network", "", count));
 }

-void BitcoinGUI::setNumBlocks(int count, const QDateTime& blockDate)
+void BitcoinGUI::setNumBlocks(int count, const QDateTime& blockDate, const CBlockIndex *tip)


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)

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

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

sipa · 2015-11-27T14:07:14Z

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

laanwj · 2015-11-27T14:51:47Z

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

jonasschnelli · 2015-11-27T17:28:15Z

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

jonasschnelli · 2015-11-27T17:29:23Z

src/qt/clientmodel.cpp

+    CBlockIndex *tip = (CBlockIndex *)tipIn;
+    if (!tip)
+    {
+        LOCK(cs_main);


This lock is only required when initially update the UI (first time, outside of the core signal scope).

jonasschnelli · 2015-11-27T19:15:31Z

Binares are built if someone likes to test this: https://bitcoin.jonasschnelli.ch/pulls/7112/

sipa · 2015-11-28T12:40:42Z

src/qt/clientmodel.cpp

 {
-    LOCK(cs_main);
-    return Checkpoints::GuessVerificationProgress(Params().Checkpoints(), chainActive.Tip());
+    CBlockIndex *tip = (CBlockIndex *)tipIn;


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

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?

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

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

Just change the argument to const there. No other changes needed (I tried) :)

sipa · 2015-11-28T21:22:03Z

src/main.cpp

        }
+        //TODO: remove tip notification benchmark


Maybe remove this again :)

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

…te always (each block)

jonasschnelli · 2015-11-30T07:52:16Z

src/main.cpp

            }
        }
+        if (!vHashes.empty()) {
+            uiInterface.NotifyBlockTip(fInitialDownload, pindexNewTip);
+        }


@sdaftuar: I'm not familiar with the block announcements with headers, could you check if this rebase makes sense?

…ass double over UI signal

jonasschnelli · 2015-11-30T07:54:37Z

src/main.cpp

            }
        }
+        // Always notify the UI if a new block tip was connected
+        uiInterface.NotifyBlockTip(fInitialDownload, pindexNewTip);


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

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.

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.

jonasschnelli · 2015-11-30T08:01:52Z

Rebased. Removed notification benchmark.

laanwj · 2015-11-30T09:54:44Z

ACK. Works great now. The spinner is actually spinning again while syncing instead of blundering and glitching along :)

- This will keep getbestblockhash more in sync with blocknotify callbacks

jonasschnelli · 2015-11-30T10:39:42Z

Added a commit that resolves conflicts with block header announcements (#7129) and now also includes PR #7037 (because it conflicts with it).

sipa · 2015-11-30T11:50:26Z

utACK non-GUI code.

9af5f9c Move uiInterface.NotifyBlockTip signal above the core/wallet signal - This will keep getbestblockhash more in sync with blocknotify callbacks (Jonas Schnelli) 4082e46 [Qt] call GuessVerificationProgress synchronous during core signal, pass double over UI signal (Jonas Schnelli) 947d20b [Qt] reduce cs_main in getVerificationProgress() (Jonas Schnelli) e6d50fc [Qt] update block tip (height and date) without locking cs_main, update always (each block) (Jonas Schnelli) 012fc91 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 (Jonas Schnelli)

Small httpserver.cpp backports Also includes a change to the `uiInterface.NotifyBlockTip` signal API. These remove merge conflicts from subsequent backports for `sync.h`. Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6859 - bitcoin/bitcoin#7112 - Only the non-QT changes. - bitcoin/bitcoin#7966 - bitcoin/bitcoin#8421 - We already backported the second commit in #2555

@sipa

This replaces using inv messages to announce new blocks, when a peer requests (via the new "sendheaders" message) that blocks be announced with headers instead of inv's. Since headers-first was introduced, peers send getheaders messages in response to an inv, which requires generating a block locator that is large compared to the size of the header being requested, and requires an extra round-trip before a reorg can be relayed. Save time by tracking headers that a peer is likely to know about, and send a headers chain that would connect to a peer's known headers, unless the chain would be too big, in which case we revert to sending an inv instead. Based off of @sipa's commit to announce all blocks in a reorg via inv, which has been squashed into this commit. Rebased-by: Pieter Wuille Zcash: Includes changes from the following subsequent upstream commits from bitcoin/bitcoin#7112 (which we already backported): - 012fc91 - 4082e46 - 9af5f9c

jonasschnelli added the GUI label Nov 27, 2015

jonasschnelli force-pushed the 2015/11/qt_tipupdate branch from 8e163b1 to a976224 Compare November 27, 2015 09:58

jonasschnelli force-pushed the 2015/11/qt_tipupdate branch from a976224 to f60a931 Compare November 27, 2015 10:22

jonasschnelli mentioned this pull request Nov 27, 2015

Bitcoin Core 0.10 freezing and disconnecting during synchronization #5851

Closed

laanwj reviewed Nov 27, 2015
View reviewed changes

jonasschnelli added this to the 0.12.0 milestone Nov 27, 2015

laanwj reviewed Nov 27, 2015
View reviewed changes

jonasschnelli force-pushed the 2015/11/qt_tipupdate branch from cf50f04 to 1db6f9a Compare November 27, 2015 17:25

jonasschnelli reviewed Nov 27, 2015
View reviewed changes

jonasschnelli force-pushed the 2015/11/qt_tipupdate branch from 1db6f9a to e6d07cc Compare November 27, 2015 17:30

sipa reviewed Nov 28, 2015
View reviewed changes

jonasschnelli force-pushed the 2015/11/qt_tipupdate branch from e6d07cc to f87be71 Compare November 28, 2015 15:25

sipa reviewed Nov 28, 2015
View reviewed changes

src/main.cpp

}

//TODO: remove tip notification benchmark

Copy link

Member

sipa Nov 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe remove this again :)

jonasschnelli added 3 commits November 30, 2015 08:50

NotifyBlockTip signal: switch from hash (uint256) to CBlockIndex*

012fc91

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

[Qt] update block tip (height and date) without locking cs_main, upda…

e6d50fc

…te always (each block)

[Qt] reduce cs_main in getVerificationProgress()

947d20b

jonasschnelli force-pushed the 2015/11/qt_tipupdate branch from f87be71 to b28c61e Compare November 30, 2015 07:51

jonasschnelli reviewed Nov 30, 2015
View reviewed changes

[Qt] call GuessVerificationProgress synchronous during core signal, p…

4082e46

…ass double over UI signal

jonasschnelli force-pushed the 2015/11/qt_tipupdate branch from b28c61e to 4082e46 Compare November 30, 2015 07:54

jonasschnelli reviewed Nov 30, 2015
View reviewed changes

Move uiInterface.NotifyBlockTip signal above the core/wallet signal

9af5f9c

- This will keep getbestblockhash more in sync with blocknotify callbacks

jonasschnelli force-pushed the 2015/11/qt_tipupdate branch from fa0ced4 to 9af5f9c Compare November 30, 2015 10:34

laanwj merged commit 9af5f9c into bitcoin:master Nov 30, 2015

laanwj mentioned this pull request Nov 30, 2015

Cosmetic problem: block number not being updated in Windows during start up #5664

Closed

jonasschnelli mentioned this pull request Dec 1, 2015

Error while compiling bitcoin with qt support #7138

Closed

laanwj mentioned this pull request Dec 1, 2015

bug, bitcoin-qt take clicking away from desktop #7139

Closed

TheBlueMatt mentioned this pull request Dec 2, 2015

Move the blocknotify callback ahead of peer announcement. #7037

Closed

dagurval mentioned this pull request May 31, 2018

Use non-atomic flushing with block replay bitcoinxt/bitcoinxt#439

Merged

aistrych mentioned this pull request Mar 16, 2019

Make UI more responsive Pink2Dev/Pink2#28

Draft

str4d mentioned this pull request Sep 30, 2020

Small httpserver.cpp backports zcash/zcash#4752

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

jonasschnelli commented Nov 27, 2015

Uh oh!

jonasschnelli commented Nov 27, 2015

Uh oh!

laanwj commented Nov 27, 2015

Uh oh!

jonasschnelli commented Nov 27, 2015

Uh oh!

jonasschnelli commented Nov 27, 2015

Uh oh!

laanwj commented Nov 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Nov 27, 2015

Uh oh!

maflcko commented Nov 27, 2015

Uh oh!

jonasschnelli commented Nov 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Nov 27, 2015

Uh oh!

laanwj commented Nov 27, 2015

Uh oh!

jonasschnelli commented Nov 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Nov 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa Nov 30, 2015 via email

Choose a reason for hiding this comment

Uh oh!

sipa Nov 30, 2015 via email

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Nov 30, 2015

Uh oh!

laanwj commented Nov 30, 2015

Uh oh!

jonasschnelli commented Nov 30, 2015

Uh oh!

sipa commented Nov 30, 2015

Uh oh!

Uh oh!